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

fix overflow when calculating number of frames in a window for dsd512… #130

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

Conversation

bcoburn3
Copy link

@bcoburn3 bcoburn3 commented Aug 23, 2024

This fixes an integer overflow that results in libebur128 calculating 0.0 for the loudness range for DSD512 files on Windows.

The sample rate for these files (2822400 samples/sec) multiplied by the window size (3000) overflows a 32 bit integer, resulting in a value for audio_data_frames being half what it should be. This PR re-orders the arithmetic to reduce the size of the intermediate values.

This doesn't happen on Linux/Mac because on those platforms unsigned long is 64 bits, but for Windows unsigned long is 32 bits on 64 bit systems.

@jiixyj
Copy link
Owner

jiixyj commented Aug 23, 2024

Thanks a lot for the fix! If possible I'd avoid reordering the operations though, because it results in different values. What do you think about just casting the samplerate to unsigned long long before doing the arithmetic? This should work as well and the resulting values stay the same:

  st->d->audio_data_frames = (unsigned long long) st->samplerate * st->d->window / 1000;

@bcoburn3
Copy link
Author

I was sure I'd tried that cast and seen it not work, but I tried it and you're correct that re-ordering changes the result and that cast fixes the problem. Thanks for catching the change in results and suggesting this :)

for reference, here's what I did to check: https://godbolt.org/z/T35hGvGPW

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.

2 participants