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

ADPCM support #41

Closed
thomcc opened this issue Jul 9, 2021 · 1 comment · Fixed by #160
Closed

ADPCM support #41

thomcc opened this issue Jul 9, 2021 · 1 comment · Fixed by #160
Assignees
Labels
enhancement New feature or request

Comments

@thomcc
Copy link
Contributor

thomcc commented Jul 9, 2021

I'd like to use Symphonia with some WAV files containing ADPCM data. Specifically, I'd like to support the "Microsoft ADPCM" (RIFF format 0x021) and "IMA/DVI ADPCM" (RIFF format 0x11) variants. There are also possibly a couple extensions2 that I need to support.

Just these two account for most of the ADPCM I've encounted in the wild, and to some extent rounds out the set of codecs that you most commonly find in WAV files, IME. The various other variants use can of course be added later when/if needed — I think there are a couple that likely will be useful (like QuickTime's "IMA4 ADPCM").

Anyway, I'd be willing to provide the code for those two (I have some parsing code lying around for them, and encoding is easy enough as well).

The reason I'm filing an issue first (rather than just a PR) is:

  1. To make sure you'd accept it.

  2. I am unsure where they should live. Or rather, I suspect the answer is "A new symphonia-codec-adpcm crate, but it seems worth asking about this first.

    I think there's a strong possibility you considered the existence of ADPCM when writing the WAV code, and possibly have thoughts on where it should live, but I could be wrong

  3. So I have somewhere to ask questions when I inevitably get lost due to my vague-at-best understanding of how Symphonia is structured 😅

One final note is that while libavcodec (codec library behind ffmpeg)'s support for ADPCM is pretty thorough, ffmpeg's usage of it used to introduce many strange bugs. So, I'm a little concerned about testing against it as with symphonia-check... I guess we'll cross that it it becomes an issue, though, since it may have been fixed in the 3 years since I tried last.


(footnotes)

  1. I'm specifying the format ID because pretty much every name you could use to describe the different flavors of ADPCM is somewhat ambiguous. There are at least 3 codecs reasonably called "DVI ADPCM" and two reasonably called "Microsoft ADPCM", and these have overlap (it's kind of a disaster).

  2. Specifically, the two important extensions are:

    Note that I'm unsure that these are actually extensions.

@pdeljanov
Copy link
Owner

  1. To make sure you'd accept it.

Yep.

Generally speaking, even if a format or codec is not on the "roadmap" I'll accept it as long as it works and the code is maintainable.

I think ADPCM support would be a great addition.

  1. I am unsure where they should live. Or rather, I suspect the answer is "A new symphonia-codec-adpcm crate, but it seems worth asking about this first.
    I think there's a strong possibility you considered the existence of ADPCM when writing the WAV code, and possibly have thoughts on where it should live, but I could be wrong

You're correct.

I think we should keep ADPCM and PCM separate. To that end, I think you should just copy symphonia-codec-pcm and use it as a starting point for symphonia-codec-adpcm. PcmDecoder is a very good example of a single Decoder that can decode multiple codecs (you'll likely want to consider the different variants to be different codecs).

You'll also need to extend the WavReader to support the ADPCM format. This would involve a few things:

  1. Add support for the ADPCM format in the ext and fmt chunks.
  2. As per the Microsoft docs, it looks like the sample data is stored in the data chunk in discrete blocks with a known (calculatable) size. This lends itself well to packetization. Currently, the WavReader only supports PCM formats which is just a continuous stream of audio frames with no packet boundaries. Therefore, the reader simulates packets by chunking the PCM data into blocks of 1152 audio frames (same as MP3) each. So you'll have to find a way to support both the PCM case where we simulate packets, and the ADPCM case where we have actual packets (blocks).
  3. Seeking would also need to updated to support the ADPCM case.

I actually think most of your work will end up being in WavReader rather than the codec.

Other than that, let me brain dump a few other things you'll have to do to tie everything together:

  1. Update the Cargo.toml for thesymphonia crate to add a adpcm feature. This can be enabled by default if you're reasonably certain the decoder will not infringe on any patents. I imagine it won't.
  2. In the symphonia crate, re-export the AdpcmDecoder here.
  3. Also in the symphonia crate, register the AdpcmDecoder into the default CodecRegistry here.
  4. In symphonia-core add new CODEC_TYPES for all of your ADPCM variants (1 per variant) here. Perhaps start it at 0x200 to allow the uncompressed PCM codecs room to grow.
  5. As mentioned above, symphonia-codec-pcm is a good example of a single Decoder supporting multiple codecs. Each of the CODEC_TYPES you declared in 4 can be supported by your one AdpcmDecoder. Then, you can adjust the behaviour of AdpcmDecoder based on the CODEC_TYPE provided in the CodecParameters on instantiation.

Once that's done, everything should just work ™️ .

One final thought. As per the Microsoft doc, the blocks can be as small as 32 samples. That's pretty small considering the work involved to get a parse a new block, allocate a packet, decode the packet, write to audio output, etc. Since the block size can be calculated, it may make sense to provide > 1 block per Packet, then the AdpcmDecoder can divide the packet buffer length by the block length to know how many blocks were provided. In this way we can reduce the number of next_packet -> decode iterations and amortize the costs over a greater number of playable samples. This is an optimization and comes with some caveats, but might be something to think about later.

One final note is that while libavcodec (codec library behind ffmpeg)'s support for ADPCM is pretty thorough, ffmpeg's usage of it used to introduce many strange bugs. So, I'm a little concerned about testing against it as with symphonia-check... I guess we'll cross that it it becomes an issue, though, since it may have been fixed in the 3 years since I tried last.

I think symphonia-check should be able to use other reference decoders. FFMpeg was just the gold standard in my mind when I wrote it. Any decoder that can output a wave file to standard out can be used with minimal changes.

Probably the best thing to do is just try to implement it, submit a PR, and we can iterate on it.

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

Successfully merging a pull request may close this issue.

2 participants