1100349 - Implement StereoPannerNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

There is a new node in the Web Audio API, that is a simple equal-power stereo panner, for musical applications rather than games/simulation.

Spec: http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface

Comment 1

[Paul Adenot (:padenot)]

Attachment: Implement StereoPannerNode. r=

Ehsan, this is a new node that's been added to the spec to try to make the
panning situation in Web Audio API better. It's basically like the PannerNode
with equal-power, but with a [-1; +1] AudioParam (full-left -> full-right), that
is better for musical applications (PannerNode is better for 3d games).

A bit more background on why it's needed can be found on the intent-to-implement
and ship on blink-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/IVDmh6ubHZM,
where I outlined the why and how we are fixing this.

The spec is there: http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface

I clarified the spec a bit (it was enough to implement, but could have use some
clarification, especially that pan is an a-rate AudioParam), and it might not
have landed in the spec when you look at this patch, so here the PR that fixes
it: https://github.com/WebAudio/web-audio-api/pull/440

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8524692 [details] [diff] [review]
Implement StereoPannerNode. r=

smaug, I need a DOM reviewer for this. This is adding a new interface, StereoPannerNode. The spec is here: http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface

Comment 3

[Paul Adenot (:padenot)]

Attachment: Tests for the StereoPannerNode. r=

Some tests, testing the DOM bits and the different branches that existe:
stereo/mono input, automated/non-automated.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8524692 [details] [diff] [review]
Implement StereoPannerNode. r=

(I believe ehsan will become a DOM peer real soon ;) )

Comment 5

[Paul Adenot (:padenot)]

Attachment: Implement StereoPannerNode. r=

This fixes an issue caused by the small refactoring on the panning function.

Comment 6

[Paul Adenot (:padenot)]

Attachment: imported patch stereo-panner-test-interfaces

It appears this is necessary when adding a new interface.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8525303 [details] [diff] [review]
imported patch stereo-panner-test-interfaces

Yup, to indicate patch authors that a new thing is exposed in the global scope.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Implement StereoPannerNode. r=

Apparently, the previous version busted non-unified builds.

Comment 9

[Paul Adenot (:padenot)]

Attachment: Tests for the StereoPannerNode. r=

And this removes a issue with the test on try.

Comment 10

[(no longer active)]

Comment on attachment 8525404 [details] [diff] [review]
Implement StereoPannerNode. r=

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

Please send an intent to ship email to dev-platform about this if you haven't already (sorry, I'm way behind my email!)

::: dom/media/webaudio/AudioNodeEngine.cpp
@@ +246,5 @@
> +                            float aOutputL[WEBAUDIO_BLOCK_SIZE],
> +                            float aOutputR[WEBAUDIO_BLOCK_SIZE])
> +{
> +#ifdef BUILD_ARM_NEON
> +  // No NEON version yet

Please file a follow-up for this and include the bug# here.

@@ +256,5 @@
> +      *aOutputL++ = *aInputL++ + *aInputR * *aGainL++;
> +      *aOutputR++ = *aInputR++ * *aGainR++;
> +    } else {
> +      *aOutputL++ = *aInputL * *aGainL++;
> +      *aOutputR++ = *aInputR++ + *aInputL++ * *aGainR++;

Can you please index all of these arrays using i instead of incrementing the pointers?  This is super error prone.  :-)

::: dom/media/webaudio/AudioNodeEngine.h
@@ -158,5 @@
> - * Vector copy-scaled operation.
> - */
> -void AudioBlockCopyChannelWithScale(const float aInput[WEBAUDIO_BLOCK_SIZE],
> -                                    const float aScale[WEBAUDIO_BLOCK_SIZE],
> -                                    float aOutput[WEBAUDIO_BLOCK_SIZE]);

Couldn't you keep using the array version?

::: dom/media/webaudio/PanningUtils.h
@@ +16,5 @@
> +template<typename T>
> +void
> +GainMonoToStereo(const AudioChunk& aInput, AudioChunk* aOutput,
> +                 T aGainL, T aGainR)
> +{

Nit: please MOZ_ASSERT the expected channel counts on aInput and aOutput.

@@ +27,5 @@
> +
> +template<typename T, typename U>
> +void
> +GainStereoToStereo(const AudioChunk& aInput, AudioChunk* aOutput,
> +                   T aGainL, T aGainR, U aOnLeft)

Please document that U can be expected to be either a bool or an array of bools here and further down.

@@ +28,5 @@
> +template<typename T, typename U>
> +void
> +GainStereoToStereo(const AudioChunk& aInput, AudioChunk* aOutput,
> +                   T aGainL, T aGainR, U aOnLeft)
> +{

Ditto.

::: dom/media/webaudio/StereoPannerNode.cpp
@@ +29,5 @@
> +
> +class StereoPannerNodeEngine : public AudioNodeEngine
> +{
> +public:
> +  explicit StereoPannerNodeEngine(AudioNode* aNode,

Nit: explicit only makes sense if the ctor can be called with one argument.

@@ +37,5 @@
> +    , mDestination(static_cast<AudioNodeStream*>(aDestination->Stream()))
> +    // Keep the default value in sync with the default value in
> +    // StereoPannerNode::StereoPannerNode.
> +    , mPan(0.f)
> +

Nit: please remove the empty line!

@@ +70,5 @@
> +                               float& aLeftGain,
> +                               float& aRightGain)
> +  {
> +    // Clamp and normalize the panning in [0; 1]
> +    aPanning = std::min(std::max(aPanning, -1.f), 1.f);

Please make sure that this normalization is spec'ed.  I couldn't find the spec text for this.

@@ +76,5 @@
> +    if (aMonoToStereo) {
> +      aPanning += 1;
> +      aPanning /= 2;
> +    } else {
> +      if (aPanning <= 0) {

Nit: else if.

@@ +94,5 @@
> +    MOZ_ASSERT(mSource == aStream, "Invalid source stream");
> +
> +    // The output of this node is always stereo, no matter what the inputs are.
> +    AllocateAudioBlock(2, aOutput);
> +    bool monoToStereo = aInput.ChannelCount() == 1;

Please MOZ_ASSERT that aInput's channels cannot be greater than 2.

@@ +98,5 @@
> +    bool monoToStereo = aInput.ChannelCount() == 1;
> +
> +    if (aInput.IsNull()) {
> +      // If input is silent, so is the output
> +      aOutput->SetNull(WEBAUDIO_BLOCK_SIZE);

If the output is always meant to be stereo, then this is incorrect, since AudioChunk::SetNull creates a silent mono chunk.  Please test the desired behavior here too.

@@ +104,5 @@
> +      float panning = mPan.GetValue();
> +      // If the panning is 0.0, we can simply copy the input to the
> +      // output: this node is a no-op
> +      if (panning == 0.0f) {
> +        *aOutput = aInput;

This will only be correct if aInput is stereo, right?  Please add a test to cover the case when it's not.

@@ +110,5 @@
> +        // Optimize the case where the panning is constant for this processing
> +        // block: we can just apply a constant gain on the left and right
> +        // channel
> +        float gainL, gainR;
> +        float value = mPan.GetValue();

Use |panning| instead?

@@ +112,5 @@
> +        // channel
> +        float gainL, gainR;
> +        float value = mPan.GetValue();
> +        GetGainValuesForPanning(value, monoToStereo, gainL, gainR);
> +        ApplyStereoPanning(aInput, aOutput, gainL, gainR, value <= 0);

Hmm, you're losing aInput.mVolume here, aren't you?  Please add a test that would have caught this.

@@ +119,5 @@
> +      float computedGain[2][WEBAUDIO_BLOCK_SIZE];
> +      bool onLeft[WEBAUDIO_BLOCK_SIZE];
> +
> +      for (size_t counter = 0; counter < WEBAUDIO_BLOCK_SIZE; ++counter) {
> +        TrackTicks tick = aStream->GetCurrentPosition();

Nit: StreamTime.

@@ +137,5 @@
> +
> +  virtual size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const MOZ_OVERRIDE
> +  {
> +    return AudioNodeEngine::SizeOfExcludingThis(aMallocSizeOf);
> +  }

Is there any reason to override this function?

@@ +171,5 @@
> +size_t
> +StereoPannerNode::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  size_t amount = AudioNode::SizeOfExcludingThis(aMallocSizeOf);
> +  amount += mPan->SizeOfExcludingThis(aMallocSizeOf);

Did you mean to use SizeOfIncludingThis here?

::: dom/webidl/AudioContext.webidl
@@ +41,5 @@
>                                                optional unsigned long numberOfInputChannels = 2,
>                                                optional unsigned long numberOfOutputChannels = 2);
>  
>      [NewObject]
> +    StereoPannerNode createStereoPanner();

Sigh...  I really wish we'd start to look into using constructors for new audio nodes at least.

(Not minusing because of this!)

::: dom/webidl/StereoPannerNode.webidl
@@ +9,5 @@
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +interface StereoPannerNode : AudioNode {

Please make this node implement AudioNodePassThrough, and add a test to that effect.  See bug 1007778 where I implemented that.

Comment 11

[(no longer active)]

Comment on attachment 8525407 [details] [diff] [review]
Tests for the StereoPannerNode. r=

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

(Note that the test needs to be extended to cover the issues I found in the patch...)

::: dom/media/webaudio/test/test_stereoPannerNode.html
@@ +55,5 @@
> +is(stereoPanner.channelCount, 2, "StereoPannerNode node has 2 input channels by default");
> +is(stereoPanner.channelCountMode, "clamped-max", "Correct channelCountMode for the StereoPannerNode");
> +is(stereoPanner.channelInterpretation, "speakers", "Correct channelCountInterpretation for the StereoPannerNode");
> +expectException(function() {
> +  stereoPanner.channelCount = 3;

So, what happens if you set the channelCount to 1 instead?  Shouldn't we be throwing for all values expect 2?

@@ +166,5 @@
> +  var panner = ac.createStereoPanner();
> +  panner.connect(ac.destination);
> +  var expected = f(ac, panner);
> +  ac.oncomplete = function(e) {
> +    ok(true, f.name);

Nit: use info() instead.

Comment 12

[Paul Adenot (:padenot)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> @@ +70,5 @@
> > +                               float& aLeftGain,
> > +                               float& aRightGain)
> > +  {
> > +    // Clamp and normalize the panning in [0; 1]
> > +    aPanning = std::min(std::max(aPanning, -1.f), 1.f);
> 
> Please make sure that this normalization is spec'ed.  I couldn't find the
> spec text for this.

We are addressing this at a global level in the spec: each AudioParam will have a "nominal range", and values will be clamped to this nominal range (I think the PR has not landed yet). In practice, this is what we do (note that the range can be [-Infinity; +Infinity], for example for the GainNode.gain AudioParam) most of the time, I'll audit and fix Gecko if it's not the case.

Comment 13

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #12)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > @@ +70,5 @@
> > > +                               float& aLeftGain,
> > > +                               float& aRightGain)
> > > +  {
> > > +    // Clamp and normalize the panning in [0; 1]
> > > +    aPanning = std::min(std::max(aPanning, -1.f), 1.f);
> > 
> > Please make sure that this normalization is spec'ed.  I couldn't find the
> > spec text for this.
> 
> We are addressing this at a global level in the spec: each AudioParam will
> have a "nominal range", and values will be clamped to this nominal range (I
> think the PR has not landed yet).

Fantastic!

> In practice, this is what we do (note that
> the range can be [-Infinity; +Infinity], for example for the GainNode.gain
> AudioParam) most of the time, I'll audit and fix Gecko if it's not the case.

Thanks for doing this.  This is indeed one of the old issues with the spec, so it's very nice to see some traction here!

Comment 14

[Paul Adenot (:padenot)]

Attachment: Implement StereoPannerNode. r=

Addressed review comments.

Comment 15

[Paul Adenot (:padenot)]

Attachment: Tests for the StereoPannerNode. r=

Added tests for the bugs fixed in this iteration of the patch.

I'm not entirely sure how to test the mono/stereo silence issue in a mochitest,
it would get upmixed at input, so it's not really observable, right?

Comment 16

[(no longer active)]

Comment on attachment 8530932 [details] [diff] [review]
Implement StereoPannerNode. r=

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

r=me with the below fixed.

::: dom/media/webaudio/AudioNodeEngine.cpp
@@ +253,5 @@
> +  uint32_t i;
> +  for (i = 0; i < WEBAUDIO_BLOCK_SIZE; i++) {
> +    if (aIsOnTheLeft[i]) {
> +      aOutputL[i] = aInputL[i] + *aInputR * *aGainL++;
> +      aOutputR[i] = aInputR[i] * *aGainR++;

Perhaps do the same for aGainL and aGainR referenced above too?

::: dom/media/webaudio/StereoPannerNode.cpp
@@ +84,5 @@
> +  }
> +
> +  void SetToSilentStereoBlock(AudioChunk* aChunk)
> +  {
> +    AllocateAudioBlock(2, aChunk);

The buffer is already allocated here, so you can remove this.

@@ +134,5 @@
> +        // Optimize the case where the panning is constant for this processing
> +        // block: we can just apply a constant gain on the left and right
> +        // channel
> +        float gainL, gainR;
> +        float panning = mPan.GetValue();

There is a panning variable in the outer scope :)  That's what I meant, sorry for not being specific.

::: dom/webidl/StereoPannerNode.webidl
@@ +17,5 @@
> +// Mozilla extension
> +partial interface StereoPannerNode {
> +  [ChromeOnly]
> +  attribute boolean passThrough;
> +};

Instead of this, you can just say:

StereoPannerNode implements AudioNodePassThrough;

Comment 17

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #15)
> Created attachment 8530941 [details] [diff] [review]
> Tests for the StereoPannerNode. r=
> 
> Added tests for the bugs fixed in this iteration of the patch.
> 
> I'm not entirely sure how to test the mono/stereo silence issue in a
> mochitest,
> it would get upmixed at input, so it's not really observable, right?

Perhaps use a ChannelSplitterNode?  (That part can be done in a separate bug if you want.)

Comment 18

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61277c6b999e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d112575f39f9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d112575f39f9
https://hg.mozilla.org/mozilla-central/rev/61277c6b999e

Comment 20

[Chris Mills [:cmills]]

I've documented this feature:

https://developer.mozilla.org/en-US/docs/Web/API/StereoPannerNode
https://developer.mozilla.org/en-US/docs/Web/API/StereoPannerNode.pan
https://developer.mozilla.org/en-US/docs/Web/API/AudioContext.createStereoPanner

A quick tech review would be great when you get the chance. Thanks!

Comment 21

[Jean-Yves Perrier [:teoli]]

Also there is a note in:
https://developer.mozilla.org/en-US/Firefox/Releases/37#Interfaces.2FAPIs.2FDOM