Implement resampling of AudioBuffers

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla22
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

We currently only support ArrayBuffer's that have the same sampling rate as that of the AudioContext (i.e., 48KHz).  We need to resample other AudioBuffers to make them playable.
Posted patch Patch (v1)Splinter Review
Attachment #722517 - Flags: review?(paul)
Comment on attachment 722517 [details] [diff] [review]
Patch (v1)

Review of attachment 722517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like it would work, but I'm concerned about some values coming from script being used without checks.

In general, I'd prefer to warn the author (say, a message in the console) and abort than to blow up, silently abort, or slow down the browser or the user's machine.

::: content/media/webaudio/AudioBuffer.cpp
@@ +188,4 @@
>    if (size != uint32_t(size)) {
>      return mResampledChannels;
>    }
>    float* outputData = static_cast<float*>(malloc(uint32_t(size)));

So, this can do a 4GB malloc and OOM pretty easily on a 32bit machine (considering we are likely to iterate on the whole array right after, we are not going to be saved by paging), by having |mSamplerate| being super low. Also, it will blow up it one is passing 0.0 or NaN or (-)Infinity in the constructor (divide by zero error).

Maybe we could add limit the mSamplerate/aRate ratio and warn the author he is doing something that is probably not what he wants (and has probably a typo in the code), or in general, sanitizing the inputs, especially because they come straight from JS.

@@ +206,5 @@
> +
> +    speex_resampler_process_float(resampler, i,
> +                                  inputData, &inSamples,
> +                                  outputData, &outSamples);
> +

Maybe we could |assert(inSamples == mLength && outSamples == newLength);| right after this call. We have been bitten in the past by this. Or maybe it is fine if we don't change the rate dynamically.
(In reply to Paul Adenot (:padenot) from comment #3)
> ::: content/media/webaudio/AudioBuffer.cpp
> @@ +188,4 @@
> >    if (size != uint32_t(size)) {
> >      return mResampledChannels;
> >    }
> >    float* outputData = static_cast<float*>(malloc(uint32_t(size)));
> 
> So, this can do a 4GB malloc and OOM pretty easily on a 32bit machine
> (considering we are likely to iterate on the whole array right after, we are
> not going to be saved by paging), by having |mSamplerate| being super low.
> Also, it will blow up it one is passing 0.0 or NaN or (-)Infinity in the
> constructor (divide by zero error).

Great catch.  Bug 849230 will take care of insane sampling rate values, and that's what WebKit does, and I would like to change the spec to mandate this instead of just recommend it.  But we should still make sure that the allocation here just doesn't fail silently.  I'll do that in a separate patch though (right in this bug).

> Maybe we could add limit the mSamplerate/aRate ratio and warn the author he
> is doing something that is probably not what he wants (and has probably a
> typo in the code), or in general, sanitizing the inputs, especially because
> they come straight from JS.

I think given bug 849230 doing that check here will not be necessary.  I'll write a patch for that bug today.

> @@ +206,5 @@
> > +
> > +    speex_resampler_process_float(resampler, i,
> > +                                  inputData, &inSamples,
> > +                                  outputData, &outSamples);
> > +
> 
> Maybe we could |assert(inSamples == mLength && outSamples == newLength);|
> right after this call. We have been bitten in the past by this. Or maybe it
> is fine if we don't change the rate dynamically.

You suggested this for the resampling that we do in decodeAudioData as well, but it turned out to not be a good idea (see bug 792263 comment 24), so we didn't assert there either.



So looks like given the other patch that I will attach here soon, this should still be reviewable standalone.
Attachment #722828 - Flags: review?(paul) → review+
Comment on attachment 722517 [details] [diff] [review]
Patch (v1)

Review of attachment 722517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like it would work, but I'm concerned about some values coming from script being used without checks.

In general, I'd prefer to warn the author (say, a message in the console) and abort than to blow up, silently abort, or slow down the browser or the user's machine.

::: content/media/webaudio/AudioBuffer.cpp
@@ +188,4 @@
>    if (size != uint32_t(size)) {
>      return mResampledChannels;
>    }
>    float* outputData = static_cast<float*>(malloc(uint32_t(size)));

So, this can do a 4GB malloc and OOM pretty easily on a 32bit machine (considering we are likely to iterate on the whole array right after, we are not going to be saved by paging), by having |mSamplerate| being super low. Also, it will blow up it one is passing 0.0 or NaN or (-)Infinity in the constructor (divide by zero error).

Maybe we could add limit the mSamplerate/aRate ratio and warn the author he is doing something that is probably not what he wants (and has probably a typo in the code), or in general, sanitizing the inputs, especially because they come straight from JS.

@@ +206,5 @@
> +
> +    speex_resampler_process_float(resampler, i,
> +                                  inputData, &inSamples,
> +                                  outputData, &outSamples);
> +

Maybe we could |assert(inSamples == mLength && outSamples == newLength);| right after this call. We have been bitten in the past by this. Or maybe it is fine if we don't change the rate dynamically.
Attachment #722517 - Flags: review?(paul) → review+
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.