849918 - Initial support for PannerNode's 3D positional audio (equalpower panning model)

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Paul Adenot (:padenot)]

Attachment: Initial support for PannerNode's 3D positional audio (equalpower panning model). r=

This is a WIP. It works, but I screwed up the azimut computation, so we either
get a null gain on the left or on the right, depending on the orientation. It
should be trivial to fix, though.

Comment 2

[Paul Adenot (:padenot)]

And a little test page: http://paul.cx/public/audiopanner_noise.html

Comment 3

[(no longer active)]

Comment on attachment 726261 [details] [diff] [review]
Initial support for PannerNode's 3D positional audio (equalpower panning model). r=

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

Some drive-by nits/comments.  I'd also like roc to review the final version of the patch here, and I'd be happy to take a look at it as well!

::: content/media/webaudio/PannerNode.cpp
@@ +86,5 @@
>    virtual void ProduceAudioBlock(AudioNodeStream* aStream,
>                                   const AudioChunk& aInput,
>                                   AudioChunk* aOutput,
>                                   bool *aFinished) MOZ_OVERRIDE
>    {

Can you optimize the case where aInput->IsNull() here?  That could be a fairly common case.

@@ +170,5 @@
>  
> +float
> +PannerNodeEngine::LinearGainFunction(float aDistance)
> +{
> +  return 1 - mRolloffFactor * (aDistance - mRefDistance) / (mMaxDistance - mRefDistance);

Not sure who the right person to review the math here is?

@@ +204,5 @@
> +
> +void
> +PannerNodeEngine::EqualPowerPanningFunction(const AudioChunk& aInput,
> +                                            AudioChunk* aOutput)
> +{

Are there any special set of values for the parameters of this node which would make this function return the same output buffer as its input?  If yes, we should optimize that either here or in a follow-up.

@@ +212,5 @@
> +
> +  if (inputChannels == 0) {
> +    *aOutput = aInput;
> +    return;
> +  }

I think this optimization belongs in the ProduceAudioBlock function.

@@ +250,5 @@
> +  if (inputChannels == 1) {
> +    GainMonoToStereo(aInput, aOutput, gainL, gainR);
> +  } else {
> +    GainStereoToStereo(aInput, aOutput, gainL, gainR, azimuth);
> +  }

Do we need to do any special kind of down-mixing here for other channel counts?  It's ok if you don't want to do that in this patch, but you should make this conditional on inputChannels and add a comment or TODO note of some sort.

@@ +256,5 @@
> +
> +void
> +PannerNodeEngine::GainMonoToStereo(const AudioChunk& aInput, AudioChunk* aOutput,
> +                                   float aGainL, float aGainR)
> +{

Please rewrite this and the stereo friend with a style similar to AudioBlockAddChannelWithScale (with an array input/output) and put it in AudioNodeEngine.cpp, so that we can look into getting an SSE implementation of it at some point.  You're welcome to keep these functions as a wrapper if needed, of course.

@@ +259,5 @@
> +                                   float aGainL, float aGainR)
> +{
> +  float* output(static_cast<float*>(const_cast<void*>(aOutput->mChannelData[0])));
> +  const float* input(static_cast<float*>(const_cast<void*>(aInput.mChannelData[0])));
> +  const float* input_bck(input);

Nit: I prefer using = for initializations.

@@ +297,5 @@
> +  }
> +}
> +
> +void
> +PannerNodeEngine::ComputeAzimuthAndElevation(float& aAzimuth, float& aElevation)

For the benefit of mere mortals like me, can you please link to a page which explains the calculations here at a high level (or add a few lines of comments at the top of this file somewhere)?

::: content/media/webaudio/ThreeDPoint.cpp
@@ +22,5 @@
> +{
> +  return ThreeDPoint(lhs.x * rhs.x, lhs.y * rhs.y, lhs.z * rhs.z);
> +}
> +
> +ThreeDPoint operator*(const ThreeDPoint& lhs, const double& rhs)

No need to pass rhs by reference here.

@@ +40,5 @@
> +  y *= invDistance;
> +  z *= invDistance;
> +}
> +
> +ThreeDPoint ThreeDPoint::Cross(const ThreeDPoint& rhs)

Nit: CrossProduct.

@@ +47,5 @@
> +                     z * rhs.x - x * rhs.z,
> +                     x * rhs.y - y * rhs.x);
> +}
> +
> +double ThreeDPoint::Dot(const ThreeDPoint& rhs)

Nit: DotProduct.

::: content/media/webaudio/ThreeDPoint.h
@@ +29,5 @@
> +  double Dot(const ThreeDPoint& rhs);
> +  void Normalize();
> +  ThreeDPoint Cross(const ThreeDPoint& rhs);
> +  double Distance(const ThreeDPoint& rhs);
> +  bool IsZero();

Is there any good reason why you're not inlining all of this stuff?  I don't see why ThreeDPoint.cpp needs to exist at all!

Also, nit: please mark the methods as const where possible.

@@ +36,5 @@
>  };
>  
> +ThreeDPoint operator-(const ThreeDPoint& lhs, const ThreeDPoint& rhs);
> +ThreeDPoint operator*(const ThreeDPoint& lhs, const ThreeDPoint& rhs);
> +ThreeDPoint operator*(const ThreeDPoint& lhs, const double& rhs);

Please add a comment saying that other similar methods can be added as needed.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Initial support for PannerNode's 3D positional audio (equalpower panning model). r=

This patch needs the patch from bug 853076 and headphones for testing.

Test page: http://paul.cx/public/panner.html

Click the Start button, and move the mouse around in the canvas, to move the
listener.

The canvas is the x/z plane, x is vertical axis.

Since the equal power panning algorithm does not use the elevation value, the
demo is only 2d.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 727625 [details] [diff] [review]
Initial support for PannerNode's 3D positional audio (equalpower panning model). r=

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

I don't really want to review the math here but I will do it tomorrow I guess when I'm awake

::: content/media/AudioNodeEngine.cpp
@@ +93,5 @@
> +
> +void
> +AudioBlockPanMonoToStereo(const float aInput[WEBAUDIO_BLOCK_SIZE],
> +                          float aGainL, float aGainR,
> +                          float aOutput[WEBAUDIO_BLOCK_SIZE])

Shouldn't this be 2*WEBAUDIO_BLOCK_SIZE? Or better still, two separate output buffers.

::: content/media/AudioNodeEngine.h
@@ +123,5 @@
> + */
> +void
> +AudioBlockPanMonoToStereo(const float aInput[WEBAUDIO_BLOCK_SIZE],
> +                          float aGainL, float aGainR,
> +                          float aOutput[WEBAUDIO_BLOCK_SIZE]);

Can you be more specific here about what computation is actually performed?

@@ +133,5 @@
> +AudioBlockPanStereoToStereo(const float aInputL[WEBAUDIO_BLOCK_SIZE],
> +                            const float aInputR[WEBAUDIO_BLOCK_SIZE],
> +                            float aGainL, float aGainR, bool aIsBehind,
> +                            float aOutputL[WEBAUDIO_BLOCK_SIZE],
> +                            float aOutputR[WEBAUDIO_BLOCK_SIZE]);

Here too.

::: content/media/webaudio/PannerNode.cpp
@@ +24,2 @@
>      , mDistanceModel(DistanceModelTypeValues::Inverse)
> +    , mDistanceModelFunction(&mozilla::dom::PannerNodeEngine::InverseGainFunction)

Drop redundant "mozilla::dom::" everywhere in this patch

@@ +43,5 @@
>    {
>      switch (aIndex) {
>      case PannerNode::PANNING_MODEL:
>        mPanningModel = PanningModelType(aParam);
> +      switch(mPanningModel) {

Space before (

@@ +58,4 @@
>        break;
>      case PannerNode::DISTANCE_MODEL:
>        mDistanceModel = DistanceModelType(aParam);
> +      switch(mDistanceModel) {

Space before (

::: content/media/webaudio/ThreeDPoint.cpp
@@ +1,3 @@
> +/**
> + * Other similar methods can be added if needed.
> + */

Fix license boilerplate

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please attach a new patch with those fixed though.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Initial support for PannerNode's 3D positional audio (equalpower panning model). r=

Addressed your comments and added comments to the math part of the patch.
Hopefully they make the code easier to review.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 727737 [details] [diff] [review]
Initial support for PannerNode's 3D positional audio (equalpower panning model). r=

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

great!

Comment 9

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8385a54ab93d

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2bb88125e2 for not building on Windows.

Comment 11

[(no longer active)]

<cstdio> is too fancy for us!  Plus, I guess it was a remnant of your debugging code.  So I took it out and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/315671a0bb40

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/315671a0bb40

Comment 13

[(no longer active)]

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