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

Switch to libsamplerate's callback API in Sample #7361

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

Conversation

sakertooth
Copy link
Contributor

@sakertooth sakertooth commented Jul 2, 2024

This PR switches the use of libsamplerate in Sample to the callback API instead of the full API. This PR is great for many reasons:

  1. Massive simplification of the audio processing in Sample. The Sample::advance function was removed as well as the heap allocation in Sample::play.
  2. Since the heap allocation in Sample::play was removed, Sample::play should be more safe to call in real-time.
  3. Don't know if anybody can notice this, but in current master, it sounds like some high frequency content in the playback was not being added compared to this PR. Using a marginSize of 64 fixed this in current master, but I wanted to try and improve this code further.

Other changes:

  • I also added some fixes here and there (e.g., libsamplerate takes in the resampling ratio as a double, but it was originally being passed in as a float).
  • I removed AudioResampler since it was no longer being used by anything, and it does not seem like a good idea for libsamplerate's usage in the codebase to be limited by the API of AudioResampler. The code should be allowed to choose which API from libsamplerate it wants to use.
  • s_interpolationMargins is only used by GigInstrument currently, so I moved the array in there.

@sakertooth
Copy link
Contributor Author

sakertooth commented Jul 2, 2024

Forgot to apply amplification, fixing that now (as well as merge conflicts).

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Jul 2, 2024

Since the heap allocation in Sample::play was removed, Sample::play is now safe to call in a real-time.

Does this got anything to do with the crashes I experience on #5990? Looks like it.

@sakertooth
Copy link
Contributor Author

Does this got anything to do with the crashes I experience on #5990? Looks like it.

I don't think so. 5990 uses SampleRecordHandle's, which do not involve calling Sample::play. This is for processing samples and is used in the AFP, Patman, sample tracks, etc.

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

I tested it and it seems to work pretty well

Comment on lines +162 to +166
const auto state = static_cast<PlaybackState*>(callbackData);
const auto& loop = *state->loop;
const auto& sample = state->sample;
auto& index = state->frameIndex;
auto& backwards = state->backwards;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const auto state = static_cast<PlaybackState*>(callbackData);
const auto& loop = *state->loop;
const auto& sample = state->sample;
auto& index = state->frameIndex;
auto& backwards = state->backwards;
const auto state = static_cast<PlaybackState*>(callbackData);
const Loop loop = *state->loop;
const Sample* sample = state->sample;
int& index = state->frameIndex;
bool& backwards = state->backwards;

loop is just an enum and sample is a pointer, so they don't need a const reference

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.

None yet

3 participants