1344785 - Clamp the resampling ratio sent to speex's resampler to something reasonnable, and drop the associated patch on upstream

Status: NEW

(Core :: Web Audio, enhancement, P3)

Description

[Paul Adenot (:padenot)]

Our patch in bug 1274083 that tries to recover from the error, when setting a crazy ratio breaks resampling completely [0] when the code path is hit.

Fortunately, this is a problem in exactly one location in the code base:

For everything but the AudioBufferSourceNode, this is unlikely to be a problem, since the rate is set when creating the resampler, and never changed. A failure to create the resampler (because the resampling ratio is crazy) is handled properly already.

For the AudioBufferSourceNode, we need to be quite liberal, but we still should do something. The spec states a nominal range for the parameter of [−∞,∞] [1], but we can cheat and clamp nonetheless ahead of time, in a way that ensure that the ratio won't overflow. I don't think that this will be an issue, and, iirc, other Web Audio API implementations clamp already.

[0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1341254#c25
[1]: https://webaudio.github.io/web-audio-api/#widl-AudioBufferSourceNode-playbackRate, minus meaning playing backward.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Doing triage. Paul, feel free to set more accurate priority.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1344785 - Clamp the resampling ratio sent to speex's resampler to something reasonnable, and drop the associated patch on upstream.

Review commit: https://reviewboard.mozilla.org/r/120104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120104/

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8847069 [details]
Bug 1344785 - Clamp the resampling ratio sent to speex's resampler to something reasonnable, and drop the associated patch on upstream.

https://reviewboard.mozilla.org/r/120104/#review123284

I agree that the maximum rate should be limited, but not out of concern for
overflow.  Because resampler input rate is set to the buffer nominal rate, and output rate is what varies with buffer playback rate, resampler output rate reduces as the ABSN playbackRate increases.
The output rate for high playbackRates is low and so integer arithmetic is typically less likely to overflow for high playbackRates.

Applying a maximum playbackRate is still a good idea though, because high
playbackRates require reading many buffer samples to play back a single audio
block and there is a point at which this will cause deadlines to be missed.

::: dom/media/webaudio/AudioBufferSourceNode.cpp:36
(Diff revision 1)
> +// but high, to allow creative uses of the playback-rate. This is
> +// guaranteed to not overflow during the various calculations performed
> +// inside the resampler, which are performed using 32bit unsigned integers.

If you are going to claim a guarantee here, then please be specific about which limits combine with this limit to prevent overflow in which calculations in the resampler.

::: dom/media/webaudio/AudioBufferSourceNode.cpp:195
(Diff revision 1)
>        if (mResamplerOutRate == aOutRate) {
>          return;
>        }
> -      if (speex_resampler_set_rate(mResampler, mBufferSampleRate, aOutRate) != RESAMPLER_ERR_SUCCESS) {
> -        NS_ASSERTION(false, "speex_resampler_set_rate failed");
> -        return;
> +      MOZ_ASSERT(mBufferSampleRate >= WebAudioUtils::MinSampleRate);
> +      float resamplingRatio = static_cast<float>(aOutRate) / mBufferSampleRate;
> +      if (resamplingRatio > MaxPlaybackRate) {

Could this limit be applied in ComputeFinalOutSampleRate(), please?

Otherwise, the rate is not the "final" rate.

ComputeFinalOutSampleRate() may also be better because that is where floating point math is already being performed.  Here everything is integer.

::: dom/media/webaudio/AudioBufferSourceNode.cpp:202
(Diff revision 1)
> +
> +        aOutRate = resamplingRatio * mBufferSampleRate;
> +      }
> +      int rv = speex_resampler_set_rate(mResampler, mBufferSampleRate, aOutRate);
> +      MOZ_ASSERT(rv == RESAMPLER_ERR_SUCCESS);
> +      if (rv != RESAMPLER_ERR_SUCCESS) {

I wonder whether it is still necessary to handle this special case if speex_resampler_set_rate() doesn't fail on overflow.

::: media/libspeex_resampler/src/resample.c:1150
(Diff revision 1)
>     if (old_den > 0)
>     {
>        for (i=0;i<st->nb_channels;i++)
>        {
> -         if (_muldiv(&st->samp_frac_num[i],st->samp_frac_num[i],st->den_rate,old_den) != RESAMPLER_ERR_SUCCESS) {
> -           st->samp_frac_num[i] = st->den_rate-1;
> +         if (_muldiv(&st->samp_frac_num[i],st->samp_frac_num[i],st->den_rate,old_den) != RESAMPLER_ERR_SUCCESS)
> +            return RESAMPLER_ERR_OVERFLOW;

(In reply to Paul Adenot (:padenot) from comment #0)
> Our patch in bug 1274083 that tries to recover from the error, when setting
> a crazy ratio breaks resampling completely [0] when the code path is hit.

It was this exact change for bug 1266260 that broke resampling completely.
The change for bug 1274083 restored the behavior so that overflow would lead
only to a tiny glitch.  When the patch for bug 1266260 was accepted upstream,
the bug was not fixed, so we should keep the fix for bug 1274083, which occurs
at high output rates, until that is fixed upstream.

https://bugzilla.mozilla.org/show_bug.cgi?id=1274083#c15

> For everything but the AudioBufferSourceNode, this is unlikely to be a
> problem, since the rate is set when creating the resampler, and never
> changed. A failure to create the resampler (because the resampling ratio is
> crazy) is handled properly already.

Failure in speex_resampler_set_rate_frac() is not handled by
speex_resampler_init_frac().

http://searchfox.org/mozilla-central/rev/006005beff40d377cfd2f69d3400633c5ff09127/media/libspeex_resampler/src/resample.c#860

This is not a problem while speex_resampler_set_rate_frac() does not fail.

Comment 4

[Paul Adenot (:padenot)]

yeah. this is very very bad, I'll start over.

Comment 5

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 6

[Paul Adenot (:padenot)]

uh, looks like I completely forgot about this one. I'll try to get to it.

Comment 7

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information