Closed Bug 1100349 Opened 10 years ago Closed 9 years ago

Implement StereoPannerNode

Categories

(Core :: Web Audio, defect)

32 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: padenot, Assigned: padenot)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

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
Attached patch Implement StereoPannerNode. r= (obsolete) — Splinter Review
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
Attachment #8524692 - Flags: review?(ehsan.akhgari)
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
Attachment #8524692 - Flags: review?(bugs)
Some tests, testing the DOM bits and the different branches that existe:
stereo/mono input, automated/non-automated.
Attachment #8524694 - Flags: review?(ehsan.akhgari)
Comment on attachment 8524692 [details] [diff] [review]
Implement StereoPannerNode. r=

(I believe ehsan will become a DOM peer real soon ;) )
Attachment #8524692 - Flags: review?(bugs) → review+
Attached patch Implement StereoPannerNode. r= (obsolete) — Splinter Review
This fixes an issue caused by the small refactoring on the panning function.
Attachment #8525301 - Flags: review?(ehsan.akhgari)
Attachment #8524692 - Attachment is obsolete: true
Attachment #8524692 - Flags: review?(ehsan.akhgari)
It appears this is necessary when adding a new interface.
Attachment #8525303 - Flags: review?(bugs)
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.
Attachment #8525303 - Flags: review?(bugs) → review+
Attached patch Implement StereoPannerNode. r= (obsolete) — Splinter Review
Apparently, the previous version busted non-unified builds.
Attachment #8525404 - Flags: review?(ehsan.akhgari)
Attachment #8525301 - Attachment is obsolete: true
Attachment #8525301 - Flags: review?(ehsan.akhgari)
And this removes a issue with the test on try.
Attachment #8525407 - Flags: review?(ehsan.akhgari)
Attachment #8524694 - Attachment is obsolete: true
Attachment #8524694 - Flags: review?(ehsan.akhgari)
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.
Attachment #8525404 - Flags: review?(ehsan.akhgari) → review-
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.
Attachment #8525407 - Flags: review?(ehsan.akhgari) → review+
(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.
(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!
Addressed review comments.
Attachment #8530932 - Flags: review?(ehsan.akhgari)
Attachment #8525404 - Attachment is obsolete: true
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?
Attachment #8530941 - Flags: review?(ehsan.akhgari)
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;
Attachment #8530932 - Flags: review?(ehsan.akhgari) → review+
(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.)
Attachment #8530941 - Flags: review?(ehsan.akhgari) → review+
https://hg.mozilla.org/mozilla-central/rev/d112575f39f9
https://hg.mozilla.org/mozilla-central/rev/61277c6b999e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Also there is a note in:
https://developer.mozilla.org/en-US/Firefox/Releases/37#Interfaces.2FAPIs.2FDOM
Summary: Implement StereoPanner → Implement StereoPannerNode
Flags: qe-verify-
Depends on: 1194549
Depends on: 1214239
Depends on: 1500238
Depends on: 1500303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: