RustAudio / ogg

Ogg container decoder and encoder written in pure Rust
Other
113 stars 21 forks source link

Add async PacketWriter #32

Closed AlexTMjugador closed 2 years ago

AlexTMjugador commented 2 years ago

Given that this crate already supports async reads, it makes sense to complete its async support for writes for API parity.

To achieve this addition the packet writing logic was extracted to a new private struct, BasePacketWriter, that is largely the same as before, with the exception of some refactors to delete data from ended streams of the map, which was a TODO comment, and getting rid of byteorder and fallible I/O operations. This new struct is not responsible for doing any I/O: it just invokes a callback, which can return false to indicate a failure and stop the writing process.

The blocking PacketWriter now uses BasePacketWriter with a callback that writes everything to the sink as before. Any error that might happen is stored and returned with the same user-visible behavior as before.

The new async PacketWriter is pretty trivial thanks to Tokio encoders and the usage of callbacks by BasePacketWriter, as it is possible to simply copy any written pages to a Tokio-provided buffer, which always succeeds. Tokio takes care of all the dirty details of actual I/O.

AlexTMjugador commented 2 years ago

Also, I'm thinking that another neat refactor could be getting rid of the byteorder dependency. The MSRV has methods in its standard library that replicate most of its functionality, so I don't think it's really needed, and doing such a refactor would turn this crate dependency-less when the async feature is not enabled, which can only reduce build times and effort in keeping dependencies updated.

Let me know if you'd like to see that in another PR!

est31 commented 2 years ago

I'm thinking that another neat refactor could be getting rid of the byteorder dependency.

Yeah I've been wondering about that as well, especially because it might be possible to get this library to be no_std (at least optionally). The only place std is really needed is the Read trait, which byteorder currently relies on. One probably needs some abstraction like the Read trait, I'm not sure which alternative would be good. Feel free to open an issue where we can discuss various proposals!

AlexTMjugador commented 2 years ago

Sorry for the delay, I've been busy with other things. I've just opened an issue to discuss that! :smile: