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

Support for multiplication instead of shifts inside the encoder? #29

Open
insane-rabenauge opened this issue Nov 10, 2024 · 3 comments
Open

Comments

@insane-rabenauge
Copy link

Certain players (f.e. ffmpeg) use multiplication instead of shifts inside the adpcm decode loop.
f.e.:
sign = nibble & 8; delta = nibble & 7;
diff = ((2 * delta + 1) * step) >> shift;
if (sign) predictor -= diff; else predictor += diff;
This leads to sample differences during decoding.
What do you think about supporting this inside the encoder?
To be honest I can't really hear the difference but I'd like to know your opinion on this.
Cheers!

@dbry
Copy link
Owner

dbry commented Nov 11, 2024

Yes, I am familiar with this. They describe the motivation for this deviation from the reference code here but neglect to mention (or are not aware) that this alters the audio data.

Normally the difference is negligible, but when I recently implemented 5-bit ADPCM I found that this difference is audible in quiet passages as a buzzing, especially with long frames. The problem is that the audio drifts away from the nominal values during the frame and then suddenly jumps back to the correct place with the next frame (generating an audible blip).

This is definitely incorrect behavior because everyone else does it the way the reference code shows (I've looked at libsndfile, SoX, Adobe Audition, Foobar2000 and Rockbox). I'm confident that hardware implementations do it this way also because they long predated FFmpeg's implemention.

One funny thing is that in the Multimedia Wiki article on IMA ADPCM someone commented that using multiplies vs. shifts+jumps produces different results and that this would be bad, but that's as far as it goes.

I suggest that it makes a lot more sense to FFmpeg to correct their code than for other implementations to start adopting this error (even as an option). Unfortunately I'm not sure how likely that is.

@insane-rabenauge
Copy link
Author

Thank you for you input! I'll close this issue then :)

@dbry
Copy link
Owner

dbry commented Nov 13, 2024

Thanks! I'll leave this issue open as a poor man's FAQ entry because I can see this coming up again, especially as people try the new 5-bit ADPCM variation.

@dbry dbry reopened this Nov 13, 2024
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

No branches or pull requests

2 participants