Open rusty-jules opened 3 years ago
Thank you for taking the time to open a pull request, and writing a detailed description! The implementation looks clean. Thanks for adding tests and test samples as well, that is very useful.
Supporting Broadcast Wav Extension Metadata sounds useful, I would like to merge this, but I’ve also come to realise that the current approach of having one WavReader
or WavWriter
that reads everything is not flexible enough. In the first place because it forces to heap-allocate all the auxiliary non-audio data, even for users who are not interested in it, but also because it’s difficult to support some cases, for example, a bext
chunk after the data
chunk in this case. So I’m thinking about restructuring the crate a bit, and splitting it in two layers:
WavReader
/ WavWriter
layer on top, like currently, which can read or write the samples with minimal effort, but with no support for more advanced features.I think Broadcast Wav Extension Metadata would be a good fit for this "raw" layer. It would also make it a bit easier to deal with the "bext
after data
"-case by exposing something like the chunks iterator directly.
What do you think?
That's great news! Thanks for asking for my opinion!
I agree that the WavReader
may be a bit too inflexible for users who don't care about metadata. With a restructure, it makes sense to me to keep using ChunksReader
as your "raw" layer if you add an api to choose which chunks you would like to parse.
My thinking would be to add a parsed_state
field or similar to ChunksReader
so that it can handle skipping chunks by their len
. The call to next
would check if the chunk had been parsed and skip it if not, similar to the way ChunkReadingState
works now.
You could then bolt on a ChunksReaderBuilder
where you choose which chunk kind
s you want parsed. In this way you have WavReader
build a ChunksReader
that only parses the Fmt
and Data
chunks to avoid all of the metadata allocations entirely. WavReader
would get WavSpecEx
from the call to ChunksReader::next
and hold onto it itself to pass to iter_next
for samples when it gets to the Data
chunk. This frees ChunksReader
from needing to care if it's wrapped by WavReader
or not, and when using ChunksReader
directly you could, for example, skip Fmt
entirely for your "build your own reader/writer" toolkit.
Another idea that might be outside of the scope of a restructure would be giving this builder the option to accept a custom Chunk
parsing closure to use for a specified chunk kind
, and just pass EmbeddedReader
to the closure for each Chunk::Unknown
that has that corresponding kind
(which would require a more sophisticated parsed_state
data structure to store the kind
s in). The return type of the closure will be tricky, but could become a nice api for access to chunks like LIST
and iXML
. Some kind of Chunk::Custom(Result<Box<?>, Error>)
enum variant might work for next
's return type.
I haven't used the write side of the crate as much, so I can't really say what it might look like for writing arbitrary chunks too.
Thinking more about the case for Chunk::Custom
, just a generic T
which is made concrete by the user's parsing closure return type could work. If the user wants to parse multiple custom chunks, their closures would return an enum with the variants being each chunk type. This way ChunksReader::next
can return Chunk::Custom(Result<UserEnum, Error>)
, just like futures 0.1 with the Either
enum.
I just came here to ask for this feature. I am porting an extreme time stretch:
https://github.com/spluta/TimeStretch
to rust. I got a preliminary version working using your crate and rustfft. The file exports top out at around 2h45min (at least that is what the OS sees), but the nice thing is WavWriter actually writes all the data. Reaper opens the file just fine and Audacity will if I tell it to import Raw Data. Anyhow, it would be great to be able to just export an rf64 file, which I believe the BWF does.
Thanks for the great work!
Sam
First off, thank you for this lovely, lovely crate.
This PR adds Broadcast Wav Extension Metadata parsing support. It should not be merged lightly, as it is a breaking change due to the additional
Chunks::Bext
enum variant. More than anything, I am opening this PR for visibility to others who may be looking for this feature and are willing to use this branch. Of course I would be happy if we can perfect the branch and merge it in the future, too.Some things of note:
There are multiple
String
allocations for theBwavExtMeta
fields that are parsed if the file in question has abext
chunk. Not only that, they are also cloned so thatBwavExtMeta
can be returned byChunksReader::next
and still be accessed byWavReader
. This is the most opinionated part that could be optimized away.Pro Tools, Reaper, and SoundDevice's WavAgent
bext
chunks are tested. WavAgent is most notable because it can add abext
chunk after thedata
chunk if it imported a wav file without abext
initially. This was tested by using Audacity to create the wav file initially.The
coding_history
is not parsed because I could not find or create any test files that use this field. It has been commented in theBwavExtMeta
struct declaration for clarity.A version 2
bext
is not tested also due to lack of test files.A
read_le_u64
method was added to theReadExt
trait.Cheers!