Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcast Wav Extension Metadata #49

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rusty-jules
Copy link

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 the BwavExtMeta fields that are parsed if the file in question has a bext chunk. Not only that, they are also cloned so that BwavExtMeta can be returned by ChunksReader::next and
    still be accessed by WavReader. 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 a bext chunk after the data chunk if it imported a wav file without a bext 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 the BwavExtMeta 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 the ReadExt trait.

Cheers!

@ruuda
Copy link
Owner

ruuda commented Jan 27, 2021

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:

  • A "raw" part, with parsers and writers for the various chunks and structures that occur in wav files. It would be more of a "build your own reader/writer" toolkit, and require some understanding of the wav format to use.
  • A 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?

@rusty-jules
Copy link
Author

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 kinds 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 kinds 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.

@rusty-jules
Copy link
Author

rusty-jules commented Jan 31, 2021

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.

@spluta
Copy link

spluta commented Jun 9, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants