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 input_needed_for_output to not return one less than needed, causing draining down the line #811

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

Conversation

padenot
Copy link
Collaborator

@padenot padenot commented Jan 23, 2025

The calculation was incorrect. This meant that in some circumstances,
e.g. playing a 44.1Hz stream on a 96kHz interface, with a block of 441
frames on Windows, rounding was eventually returning 440 instead of 441
in the callback, leading to the stream entering the draining phase.

@padenot
Copy link
Collaborator Author

padenot commented Jan 23, 2025

This fixes BMO#1940319.

test/test_resampler.cpp Outdated Show resolved Hide resolved
test/test_resampler.cpp Show resolved Hide resolved
Copy link
Contributor

@karlt karlt left a comment

Choose a reason for hiding this comment

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

I see now that github interprets review of a commit as review of the entire pull request.

Thank you for testing thoroughly.
I expect that this function should be able to be simple without special cases. The special cases make me suspect that something else may not be quite right.

Comment on lines -291 to -292
int32_t resampled_frames_left =
samples_to_frames(resampling_out_buffer.length());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain in the commit message that resampling_out_buffer.length() is removed from the equation because resampled frames are not carried over from one output() operation to the next.

src/cubeb_resampler_internal.h Show resolved Hide resolved
Comment on lines 293 to 294
if (input_frames_needed_frac < 0) {
return 0;
Copy link
Contributor

@karlt karlt Jan 27, 2025

Choose a reason for hiding this comment

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

resampling_ratio is always non-negative, as is length(), so float arithmetic will always produce a non-negative input_frames_needed_frac, and so this path is never taken.

I'm uncomfortable with using float arithmetic because I don't know that we can prove that n / m * (i * m) >= n * i over the ranges of rates that we care about. Replacing resampling_ratio with a fraction, with integer arithmetic, would provide assurance of this. We'd then want some checks on the ranges for the integer arithmetic.

src/cubeb_resampler_internal.h Outdated Show resolved Hide resolved
Copy link
Contributor

@karlt karlt left a comment

Choose a reason for hiding this comment

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

I spent several hours trying to prove that a certain calculation would produce enough input for an output, but I have not yet come up with something concrete.

The resampler advance can be understood from the code in resampler_basic_zero() (or any of the other resampler_basic_funcs).

The resampler can be considered to count at num_rate * den_rate quantums per
second, where den_rate = out_rate / GCF(in_rate, out_rate) and num_rate = in_rate / GCF(in_rate, out_rate).

samp_frac_num counts in these quantums.
Each last_sample unit represents den_rate quantums.
Adding the two together gives a position.

Each resampler iteration advances by num_rate quantums (1/den_rate seconds), and produces one output frame.

Initially the first input and output frames correspond to 0 quantums.

Each speex_resample() call leaves the resampler at a position exactly aligned with an output frame, so output_frame_count output frames requires an advance of output_frame_count * num_rate quantums.

Input frames intervals are den_rate quantums, but the first input frame is aligned differently on different speex_resample() calls, and there is an additional complicating factor that the speex resampler will sometimes consume input frames that it doesn't actually use.

last_sample values corresponds to input frame positions, but last_sample can get ahead of the number of input frames consumed. In the last iteration of the resampler_basic_func, last_sample can advance beyond *in_len. The input frames stepped over are not needed yet. Input frames are consumed up to last_sample, even the frames that weren't needed.

The consuming of some unused input frames aligns better with the expected ratio of input to output frames given the alignment at 0 quantums. When last_sample advances beyond *in_len, those extra frames are pulled on the next speex_resample() call, pulling more frames than expected.

src/cubeb_resampler_internal.h Outdated Show resolved Hide resolved
src/cubeb_resampler_internal.h Outdated Show resolved Hide resolved
@padenot padenot force-pushed the resampler-draining branch from 8f7e192 to 1b52d0f Compare January 28, 2025 17:40
@padenot
Copy link
Collaborator Author

padenot commented Jan 28, 2025

I'll probably add some test that checks that the output is sensible, iirc we have an utility for that somewhere (feed a sine wave at a certain freq, check that we get that at the output).

@padenot padenot force-pushed the resampler-draining branch from 1b52d0f to 3b1bcae Compare January 28, 2025 17:51
Copy link
Contributor

@karlt karlt left a comment

Choose a reason for hiding this comment

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

Thanks!

src/cubeb_resampler_internal.h Outdated Show resolved Hide resolved
src/cubeb_resampler_internal.h Outdated Show resolved Hide resolved
src/cubeb_resampler_internal.h Outdated Show resolved Hide resolved
test/test_resampler.cpp Outdated Show resolved Hide resolved
…ng draining down the line

The calculation was incorrect. This meant that in some circumstances,
e.g. playing a 44.1Hz stream on a 96kHz interface, with a block of 441
frames on Windows, rounding was eventually returning 440 instead of 441
in the callback, leading to the stream entering the draining phase.
…ck sizes and sample-rates

This might be a bit much, runtime is 25s debug ASAN, 6s opt, but should
be thorough.
@padenot padenot force-pushed the resampler-draining branch 2 times, most recently from 5ca5a4e to 56d90bd Compare February 3, 2025 15:47
@padenot padenot force-pushed the resampler-draining branch from 56d90bd to 8ea49e1 Compare February 4, 2025 13:50
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