875277 - Implement WaveShaperNode.oversample

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: draft - waveShaperOverSample.patch

The idea is to use libspeex for up/down sampling per chunk. However, speex_resampler_process_float() gives weird outputs in Resampler::UpSample/DownSample. Is there anything missing to use libspeex correctly?

Comment 3

[Karl Tomlinson (:karlt)]

Pass 1 for channel count to _init and 0 for channel number in _process_float because the channels are already individual streams.

Comment 4

[Karl Tomlinson (:karlt)]

Actually, please ignore comment 3, sorry.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part1 - add webidl enums

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part2 - add up/down samplers from Blink

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part3 - implement with Blink up/down samplers

Comment 8

[(no longer active)]

Comment on attachment 782480 [details] [diff] [review]
Part2 - add up/down samplers from Blink

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

Why should we not use libspeex here?  I don't think this is the right thing to do.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part2 - implement based on libspeex

Comment 10

[(no longer active)]

Comment on attachment 782972 [details] [diff] [review]
Part2 - implement based on libspeex

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

Looks great, thanks!

::: content/media/webaudio/WaveShaperNode.cpp
@@ +67,5 @@
> +  void Reset(uint32_t aChannels, uint32_t aSampleRate, OverSampleType aType)
> +  {
> +    if (aChannels == mChannels
> +        && aSampleRate == mSampleRate
> +        && aType == mType)

Nit: please put the ands at the end of lines.  Also, please wrap if's in curly brackets.

@@ +90,5 @@
> +    mDownSampler = speex_resampler_init(aChannels,
> +                                        aSampleRate * ValueOf(aType),
> +                                        aSampleRate,
> +                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
> +                                        nullptr);

Hmm, I think it would be slightly better to lazily allocate the resamplers, since I believe in most cases we will only end up using one of them.

@@ +104,5 @@
> +    MOZ_ASSERT(mBuffer.Length() == outSamples);
> +
> +    speex_resampler_process_float(mUpSampler, aChannel,
> +                                  aInputData, &inSamples,
> +                                  outputData, &outSamples);

After this line, please MOZ_ASSERT(inSamples == WEBAUDIO_BLOCK_SIZE && outSamples == WEBAUDIO_BLOCK_SIZE * aBlocks);.  Same in DownSample() below.

@@ +139,5 @@
> +  OverSampleType mType;
> +  SpeexResamplerState* mUpSampler;
> +  SpeexResamplerState* mDownSampler;
> +  uint32_t mChannels;
> +  uint32_t mSampleRate;

Nit: please use TrackRate as the type of the sample rate here and in the above functions.  (TrackRate is int32_t.)

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 782972 [details] [diff] [review]
Part2 - implement based on libspeex

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

::: content/media/webaudio/WaveShaperNode.cpp
@@ +90,5 @@
> +    mDownSampler = speex_resampler_init(aChannels,
> +                                        aSampleRate * ValueOf(aType),
> +                                        aSampleRate,
> +                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
> +                                        nullptr);

I don't get it. Do you mean we will only use either upsampler or downsampler in most cases? I think we will need both of them to complete the process (upsample -> apply curve -> downsample).

Comment 12

[(no longer active)]

(In reply to jwwang from comment #11)
> Comment on attachment 782972 [details] [diff] [review]
> Part2 - implement based on libspeex
> 
> Review of attachment 782972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/WaveShaperNode.cpp
> @@ +90,5 @@
> > +    mDownSampler = speex_resampler_init(aChannels,
> > +                                        aSampleRate * ValueOf(aType),
> > +                                        aSampleRate,
> > +                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
> > +                                        nullptr);
> 
> I don't get it. Do you mean we will only use either upsampler or downsampler
> in most cases? I think we will need both of them to complete the process
> (upsample -> apply curve -> downsample).

No, sorry, this comment was a mistake of mine, and I meant to remove it before submitting the review!  Please ignore it.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part2 - implement based on libspeex

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a633e4d67d31
https://hg.mozilla.org/integration/mozilla-inbound/rev/aee6b1a6c400

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0dd80f800e2

https://tbpl.mozilla.org/php/getParsedLog.php?id=26184561&tree=Mozilla-Inbound

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

"OverSampleType mType;" is missing from WaveShaperNode.h after merge and results in build fail. Does the merge fail?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Maybe. Please make sure your patches apply against m-c tip and rebase as necessary.

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part1 - add webidl enums

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Part2 - implement based on libspeex

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/802ef003bb08
https://hg.mozilla.org/integration/mozilla-inbound/rev/1406e31167e2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/802ef003bb08
https://hg.mozilla.org/mozilla-central/rev/1406e31167e2

Comment 22

[(no longer active)]

Comment on attachment 786152 [details] [diff] [review]
Part1 - add webidl enums

This is needed for Web Audio in Firefox 25.

Comment 23

[(no longer active)]

checkin-needed for Aurora, a=webaudio.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/109bf9967b3d
https://hg.mozilla.org/releases/mozilla-aurora/rev/32aab56d0aa5

Comment 25

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/4d652113720b because something in https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=2d110609f4d3 caused test_mediaStreamAudioSourceNodeResampling.html to time out (once https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=918a847c0018 let things get that far).

Comment 26

[Jean-Marc Valin]

Clear the needinfo

Comment 27

[Jean-Yves Perrier [:teoli]]

Doc is up-to-date:
https://developer.mozilla.org/en-US/docs/Web/API/WaveShaperNode.oversample
https://developer.mozilla.org/en-US/docs/Web/API/WaveShaperNode
https://developer.mozilla.org/en-US/docs/Web/API/WaveShaperNode

Comment 28

[Jean-Yves Perrier [:teoli]]

Ooops, last link should have been: 
https://developer.mozilla.org/en-US/Firefox/Releases/26