Skip to content

Fix audio resampling functionality #7858

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

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

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Apr 22, 2025

This PR improves the usage of libsamplerate when resampling.

Changes:

  • Redirect all libsamplerate usage to AudioResampler. This ensures we are always using libsamplerate with the same resampling logic, preventing bugs.

  • Call src_process as many times as necessary to resample all of the source audio we need to fill up the destination buffer. Before, LMMS was only calling src_process once, and assumed that libsamplerate always read all of the input data it gave it, which isn't necessarily true and can cause input frames to be dropped. Any input frames not read in the current iteration are stored within a small array in AudioResampler and will be used on the next call to src_process.

  • Remove the use of buffer margins. This was needed to accommodate for libsamplerate's transport delay, but this involved expensive copies and allocations, and the margin added may not be adequate depending on how long the transport delay needs to be, which wasn't accounted for. To fix this, we allow the transport delay to occur on the onset of when AudioResampler is first used, which then the delay will be removed on subsequent resampling.

@sakertooth sakertooth marked this pull request as draft April 22, 2025 17:31
@sakertooth sakertooth marked this pull request as ready for review April 23, 2025 00:24
@sakertooth sakertooth changed the title Improve correctness of AudioResampler Fix audio resampling logic Apr 23, 2025
@sakertooth sakertooth marked this pull request as draft April 24, 2025 13:39
@sakertooth sakertooth marked this pull request as ready for review April 27, 2025 10:54
@sakertooth
Copy link
Contributor Author

Appreciate the review 👍

@sakertooth
Copy link
Contributor Author

Keeping some of the review unresolved because I consider them to bring up outstanding issues we should eventually resolve, maybe before merge.

@sakertooth
Copy link
Contributor Author

Pretty much done

The function srccpy is used for different audio streams, so the resampler sttate has to be reset on each call to it.
Might be useful in the future, for e.g. exporting audio while resampling
Let me not get ahead of myself. It can be added when its actually going to be used, if ever.
@sakertooth
Copy link
Contributor Author

Hey @messmerd, I added a subspan function for InterleavedBufferView because this PR needed it. I didn't do it for PlanarBufferView because I didn't need it for that view. Feel free to take a look when you can.

Co-authored-by: Dalton Messmer <[email protected]>
Copy link
Member

@bratpeki bratpeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great!

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.

4 participants