1265394 - Implement AudioParam cartesian coordinates for PannerNode

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

This will allow performing smooth off-main-thread scheduling of listener and source position.

Spec changes at https://github.com/webaudio/web-audio-api/commit/3ed6f405e3c2b0c34f0fb78f0ab6ce2685b854f9.

Comment 1

[clement.schwartz]

Hello,
I want to work on this implementation but i have some trouble understanding all the work to be done.

--> if we change ThreeDPoint attributes to 3 AudioParam attributes in PannerNode, it seems that a lot of code has to be changed in PannerNode.cpp ? 
I don't get it because it seems convenient to use the ThreeDPoint type to calculate vectors for example.

Or am I supposed to change attributes only and then adapt it to the rest of the code ?

Thanks for help !

Comment 2

[Paul Adenot (:padenot)]

Clément, we should continue using ThreeDPoint internally, and just construct new ThreeDPoints with the AudioParam's value when we need to compute the azimuth and elevation.

Comment 3

[clement.schwartz]

Attachment: First try

Comment 4

[clement.schwartz]

Attachment: bug-1265394-fix2.patch

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8755954 [details] [diff] [review]
First try

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

You should expose this to JavaScript by modifying `dom/webidl/PannerNode.webidl`.

WebIDL is an interface definition language that is used to automatically create binding between C++ and JavaScript. To declare an AudioParam that should be on PannerNode, it goes like this:

> readonly attribute AudioParam myAudioParam;

You need to add a number of members like this, because in this patch, we're adding a bunch of AudioParams. You'll need to add a number of getters to `PannerNode.h`, all returning the correct `AudioParam*`.

Also, exactly the same approach need to be taken for AudioListener (check the spec, there are also a number of AudioParam that have been added). This is a bit harder, because our AudioParam code assumes the AudioParam will be attributes of an AudioNode. Feel free to only do the part about PannerNode here, and we can discuss how to approach the second part about AudioListener later.

The rest of the code looks good, keep it up! This will need a test of course, ping me on IRC and I can tell you how do write one.

::: dom/media/webaudio/PannerNode.cpp
@@ +400,5 @@
>  }
>  
> +ThreeDPoint ConvertTo3DP(AudioParamTimeline aX, AudioParamTimeline aY, AudioParamTimeline aZ)
> +{
> +  ThreeDPoint* resultat = new ThreeDPoint(aX.GetValue(),aY.GetValue(),aZ.GetValue());

No french ! :-)

Also, why allocate this on the heap ? Can you just allocate it on the stack and return it by value ? This is cheap anyways because of the fact that compilers do what is called "return value optimization".

https://en.wikipedia.org/wiki/Return_value_optimization

@@ +413,5 @@
>    int inputChannels = aInput.ChannelCount();
>  
>    // If both the listener are in the same spot, and no cone gain is specified,
>    // this node is noop.
> +  if (mListenerPosition == ConvertTo3DP(mPositionX,mPositionY,mPositionZ) &&

Mozilla style is to put spaces after commas in arguments.

@@ +556,5 @@
>  
>  float
>  PannerNodeEngine::ComputeDistanceGain()
>  {
> +  ThreeDPoint distanceVec = ConvertTo3DP(mPositionX,mPositionY,mPositionZ) - mListenerPosition;

Same, put spaces after commas.

Comment 6

[clement.schwartz]

Attachment: bug-1265394-fix2.patch

Second try

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8756785 [details] [diff] [review]
bug-1265394-fix2.patch

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

::: dom/media/webaudio/PannerNode.cpp
@@ +412,5 @@
>    int inputChannels = aInput.ChannelCount();
>  
>    // If both the listener are in the same spot, and no cone gain is specified,
>    // this node is noop.
> +  if (mListenerPosition == ConvertTo3DP(mPositionX, mPositionY, mPositionZ) &&

Those parameters are spec-ced as being a-rate, that is, you need to fetch the values for a block of audio to be rendered (128 values), and apply the algorithm for each value. See for example how the `StereoPannerNode` handles this:

https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/StereoPannerNode.cpp#141

This allows developers to have very very smooth transition in the audio, when panning objects around in space.

See the spec about a-rate vs. k-rate parameters here: http://webaudio.github.io/web-audio-api/#a-rate

Of course, in our case, we need to fetch the 128 values for a number of AudioParams to be able to compute the output. This and other functions where we use mPositionX and friends have to be changed.

::: dom/media/webaudio/PannerNode.h
@@ +214,5 @@
> +
> +  AudioParam* OrientationZ()
> +  {
> +    return mOrientationZ;
> +  }

Note that those methods still have to be capitalized, because that's what the style guide says, and the webidl compiler respects the style guide.

::: dom/media/webaudio/test/test_bug1265394.html
@@ +36,5 @@
> +    near(panner.PositionY.value, 8.3, "Correct set value for Position Y");
> +    near(panner.PositionZ.value, 0, "Correct set value for Position Z");
> +    near(panner.OrientationX.value, 4, "Correct set value for Orientation X");
> +    near(panner.OrientationY.value, -3.1, "Correct set value for Orientation Y");
> +    near(panner.OrientationZ.value, 6.7, "Correct set value for Orientation Z");

This is a good start. I think you should try to make sure those AudioParam are working as intended as well, for example by panning a mono source left or right and looking at the resulting wave form, which should be panned accordingly. You could modify existing PannerNode tests for this, using the new AudioParam instead of the setPosition and setOrientation methods.

::: dom/webidl/PannerNode.webidl
@@ +39,5 @@
> +
> +    // Cartesian coordinate for orientation
> +    readonly attribute AudioParam OrientationX;
> +    readonly attribute AudioParam OrientationY;
> +    readonly attribute AudioParam OrientationZ;

All those are supposed to not be capitalized: PositionX vs. positionX.

Comment 8

[clement.schwartz]

Attachment: I'm trying to verify the panning

So this time i checked that we can set without using the setPosition method and same for orientation...

I'm now trying to check the panning is working but i dont know how to get the buffers from the Listener.
I see in some other tests they use "CreateExpectedBuffers", but i dont understand how it works, is it the only way to do ?

Comment 9

[clement.schwartz]

Attachment: bug-1265394-fix3.patch

Since last review : 

I modified 1 test to check if it works when we set AudioParam attributes
I added 1 test to check the panning works

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8758292 [details] [diff] [review]
bug-1265394-fix3.patch

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

Looks good, keep it up !

::: dom/media/webaudio/test/test_bug1265394.html
@@ +49,5 @@
> +  near(panner.orientationY.value, -6.7, "Correct set value for as an AudioParam.");
> +  near(panner.orientationZ.value, -12, "Correct set value for as an AudioParam.");
> +
> +
> +

Don't add to much blank lines.

@@ +55,5 @@
> +
> +  SimpleTest.executeSoon(function() {
> +    source.stop(0);
> +    source.disconnect();
> +    panner.disconnect();

This test does not test any audio processing capabilities, just DOM APIs. You can drop the `SimpleTest.waitForExplicitFinish();`, the `SimpleTest.finish();`, and node setup and tear-down.

@@ +64,5 @@
> +
> +</script>
> +</pre>
> +</body>
> +</html>

Can you use a better file name for this test, that would describe what the text do better ?

::: dom/media/webaudio/test/test_bug1265394_bis.html
@@ +37,5 @@
> +    var expectedBuffer = context.createBuffer(2, 2048, context.sampleRate);
> +    for (var i = 0; i < 2048; ++i) {
> +      // Different signals in left and right buffers
> +      expectedBuffer.getChannelData(0)[i] = 1;
> +      expectedBuffer.getChannelData(1)[i] = 0;

Is this actually passing? The attenuation should be less than 100% on the right channel.

@@ +49,5 @@
> +
> +</script>
> +</pre>
> +</body>
> +</html>
\ No newline at end of file

Find a better name for this test.

Comment 11

[clement.schwartz]

Attachment: bug-1265394-fix4.patch

Corrected the tests according to your last review.

So if tests are ok, the only thing remaining is the a-rate attributes thing !

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8758682 [details] [diff] [review]
bug-1265394-fix4.patch

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

Dan, want to have a final look at this? This is implementing AudioParam for cartesian coordinate for the PannerNode. The part about the listener will be implemented in a separate bug.

Comment 13

[clement.schwartz]

Attachment: try_a_rate.patch

This is the code I changed for the a-rate attributes.

My test pass but it should not (since my expected buffers are constant), so it seems that my code is wrong

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8758740 [details] [diff] [review]
try_a_rate.patch

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

Not quite right time time. Step through the code with a debugger to understand what's up.

::: dom/media/webaudio/PannerNode.cpp
@@ +285,5 @@
> +  float fetchPositionY[WEBAUDIO_BLOCK_SIZE];
> +  float fetchPositionZ[WEBAUDIO_BLOCK_SIZE];
> +  float fetchOrientationX[WEBAUDIO_BLOCK_SIZE];
> +  float fetchOrientationY[WEBAUDIO_BLOCK_SIZE];
> +  float fetchOrientationZ[WEBAUDIO_BLOCK_SIZE];

You don't need to have that as a attribute. Just put it on the stack.

@@ +402,5 @@
>  PannerNodeEngine::HRTFPanningFunction(const AudioBlock& aInput,
> +                                      AudioBlock* aOutput) {
> +  for (size_t counter = 0; counter < WEBAUDIO_BLOCK_SIZE; ++counter) {
> +    // The output of this node is always stereo, no matter what the inputs are.
> +    aOutput->AllocateChannels(2);

This is wrong, you don't want to allocate a stereo channel 128 times.

@@ +414,2 @@
>  
> +    mHRTFPanner->pan(azimuth, elevation, &input, aOutput);

This is also wrong, this computes 128 frames, I think, so you're computing 128 * 128 frames here.

@@ +444,3 @@
>  
> +    // The output of this node is always stereo, no matter what the inputs are.
> +    aOutput->AllocateChannels(2);

Same mistake, this has to be outside the loop.

@@ +478,3 @@
>  
> +    // Compute the output.
> +    ApplyStereoPanning(aInput, aOutput, gainL, gainR, azimuth <= 0);

This also computes 128 frames.

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8758682 [details] [diff] [review]
bug-1265394-fix4.patch

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

This looks good, just a some small fixes. I didn't look at the PannerNodeEngine code closely as I noticed you are making changes to it in your next patch.

::: dom/media/webaudio/PannerNode.cpp
@@ +398,5 @@
>  
>    mHRTFPanner->pan(azimuth, elevation, &input, aOutput);
>  }
>  
> +ThreeDPoint ConvertTo3DP(AudioParamTimeline aX, AudioParamTimeline aY, AudioParamTimeline aZ)

Please remove this duplicate definition and use PannerNode::ConvertTo3DP in the calls below.

::: dom/media/webaudio/PannerNode.h
@@ +85,5 @@
>      ThreeDPoint orientation(aX, aY, aZ);
>      if (!orientation.IsZero()) {
>        orientation.Normalize();
>      }
> +    if (ConvertTo3DP(mOrientationX,mOrientationY,mOrientationZ).FuzzyEqual(orientation)){

Please add a space after each comma.

@@ +90,5 @@
>        return;
>      }
> +    mOrientationX->SetValue(aX);
> +    mOrientationY->SetValue(aY);
> +    mOrientationZ->SetValue(aZ);

Please set these values from orientation rather than aX, aY, aZ. Otherwise you will miss out on the normalization done above.

@@ +91,5 @@
>      }
> +    mOrientationX->SetValue(aX);
> +    mOrientationY->SetValue(aY);
> +    mOrientationZ->SetValue(aZ);
> +    SendThreeDPointParameterToStream(ORIENTATION, ConvertTo3DP(mOrientationX,mOrientationY,mOrientationZ));

Please add a space after each comma.

@@ +262,5 @@
>      CONE_OUTER_ANGLE,
>      CONE_OUTER_GAIN
>    };
>  
> +public:

I think you can make this private. It appears you only use it inside of PannerNode itself. PannerNodeEngine is a friend class so it can access private parts of PannerNode anyway.

@@ +263,5 @@
>      CONE_OUTER_GAIN
>    };
>  
> +public:
> +  ThreeDPoint ConvertTo3DP(RefPtr<AudioParam> aX, RefPtr<AudioParam> aY, RefPtr<AudioParam> aZ)

If you make this static, then you will be able to access it from inside of PannerNodeEngine, e.g. PannerNode::ConvertTo3DP(...). Then you can remove the equivalent definition of this function from PannerNode.cpp.

::: dom/media/webaudio/test/test_pannerNode_AudioParam.html
@@ +14,5 @@
> +}
> +
> +var context = new AudioContext();
> +var destination = context.destination;
> +var source = context.createBufferSource();

I don't believe you need to create a source or connect the panner to a destination for your test. Please remove these unless they are necessary.

::: dom/media/webaudio/test/test_pannerNode_Listener.html
@@ +37,5 @@
> +    var expectedBuffer = context.createBuffer(2, 2048, context.sampleRate);
> +    for (var i = 0; i < 2048; ++i) {
> +      // Different signals in left and right buffers
> +      expectedBuffer.getChannelData(0)[i] = 0.65;
> +      expectedBuffer.getChannelData(1)[i] = 0.27;

Please explain where these constants come from.

::: dom/webidl/PannerNode.webidl
@@ +31,5 @@
>      void setOrientation(double x, double y, double z);
>      [Deprecated="PannerNodeDoppler"]
>      void setVelocity(double x, double y, double z);
>  
> +    // Cartesian coordinate for position

You need review from a DOM peer on WebIDL changes. Please move these changes (plus whatever changes you need to PannerNode for the code to still compile) to a separate patch and r?smaug on it, and then rebase the rest of your changes on top of that patch.

Comment 16

[clement.schwartz]

Attachment: PannerNodeWebidl.patch

Comment 17

[clement.schwartz]

Attachment: bug-1265394-fix5.patch

Corrected the patch following your directions !

Comment 18

[Dan Minor [:dminor]]

(In reply to clement.schwartz from comment #17)
> Created attachment 8759633 [details] [diff] [review]
> bug-1265394-fix5.patch
> 
> Corrected the patch following your directions !

Sorry I didn't get to this today. I'll review Monday morning EDT if not before.

Comment 19

[Dan Minor [:dminor]]

Comment on attachment 8759633 [details] [diff] [review]
bug-1265394-fix5.patch

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

Thanks for making the changes, this is looking good.

One other nit - I realize we're not fully consistent about this, but most of the tests are named without underscores. Please rename yours to test_pannerNodeAudioParam.html and test_pannerNodeListener.html.

::: dom/media/webaudio/test/test_pannerNode_AudioParam.html
@@ +23,5 @@
> +near(panner.positionY.value, 8.3, "Correct set value for Position Y with setPosition.");
> +near(panner.positionZ.value, 0, "Correct set value for Position Z with setPosition.");
> +near(panner.orientationX.value, 4, "Correct set value for Orientation X with setOrientation.");
> +near(panner.orientationY.value, -3.1, "Correct set value for Orientation Y with setOrientation.");
> +near(panner.orientationZ.value, 6.7, "Correct set value for Orientation Z with setOrientation.");

These three orientation tests are failing for me locally. Please double check that you didn't miss something while uploading your patch. If it's working for you, please ping me and we can figure out why it doesn't work for me :)

@@ +38,5 @@
> +near(panner.positionZ.value, 0, "Correct set value for as an AudioParam.");
> +near(panner.orientationX.value, 4.7, "Correct set value for as an AudioParam.");
> +near(panner.orientationY.value, -6.7, "Correct set value for as an AudioParam.");
> +near(panner.orientationZ.value, -12, "Correct set value for as an AudioParam.");
> +

Please add a test that verifies the automation is working properly. I think you should be able to set a current value and a value for a short period of time in the future, render to an offline buffer, and then verify that the panner is in the new position in the oncomplete callback.

Comment 20

[Dan Minor [:dminor]]

The setOrientation test is failing because we are now storing the normalized orientation rather than the original orientation passed in (so the test fails with e.g. 4 != 0.476383). Since this is what the original version of the code did, I think we should keep this behaviour. Please update the test to expect normalized values from panner.orientation[XYZ].value.

Comment 21

[clement.schwartz]

Attachment: bug-1265394-fix6.patch

Add corrections following your last review.

But : I tried in test_pannerNodeListener to use a setValueAtTime and this is not working

Comment 22

[Dan Minor [:dminor]]

Hi Clement,

I tried running your test this morning and hit an assertion:

Assertion failure: HasSimpleValue(), at /home/dminor/src/firefox-webaudio/dom/media/webaudio/AudioEventTimeline.h:302
#01: mozilla::dom::ConvertAudioParamTimelineTo3DP(mozilla::dom::AudioParamTimeline, mozilla::dom::AudioParamTimeline, mozilla::dom::AudioParamTimeline) (/home/dminor/src/firefox-webaudio/dom/media/webaudio/PannerNode.cpp:404)
#02: mozilla::dom::PannerNodeEngine::EqualPowerPanningFunction(mozilla::AudioBlock const&, mozilla::AudioBlock*) (/home/dminor/src/firefox-webaudio/dom/media/webaudio/PannerNode.cpp:416)
#03: mozilla::dom::PannerNodeEngine::ProcessBlock(mozilla::AudioNodeStream*, long, mozilla::AudioBlock const&, mozilla::AudioBlock*, bool*) (/home/dminor/src/firefox-webaudio/dom/media/webaudio/PannerNode.cpp:225)
...

Assertions only show up for debug builds. Please add

ac_add_options --enable-debug

to your mozconfig and rebuild (do a ./mach clobber to be safe.) Hopefully fixing this assertion will also fix your test :)

Comment 23

[Dan Minor [:dminor]]

Comment on attachment 8760901 [details] [diff] [review]
bug-1265394-fix6.patch

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

I'll have another look when the automation is fixed as discussed on irc.

Comment 24

[clement.schwartz]

Hi Dan,

I will not be able to continue my work on this bug since I now have an internship. Sorry about that.

Thanks for the help you gave me anyway, and good luck for everything.

Comment 25

[Dan Minor [:dminor]]

Thanks for letting me know! That's a lot better than just disappearing which many people do. Good luck with your internship :)

I'll plan on finishing this one off.

Comment 26

[Dan Minor [:dminor]]

Attachment: Bug 1265394 - Add new PannerNode AudioParams to webidl;

Review commit: https://reviewboard.mozilla.org/r/61022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61022/

Comment 27

[Dan Minor [:dminor]]

Attachment: Bug 1265394 - Use new PannerNode AudioParams;

Review commit: https://reviewboard.mozilla.org/r/61024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61024/

Comment 28

[Dan Minor [:dminor]]

Attachment: Bug 1265394 - Update tests;

Review commit: https://reviewboard.mozilla.org/r/61026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61026/

Comment 29

[Dan Minor [:dminor]]

Attachment: Bug 1265394 - Add test for PannerNode automation;

Review commit: https://reviewboard.mozilla.org/r/61030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61030/

Comment 30

[Dan Minor [:dminor]]

I'm open to suggestions for additional PannerNode automation tests.

Comment 31

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

Comment on attachment 8765885 [details]
Bug 1265394 - Add new PannerNode AudioParams to webidl;

https://reviewboard.mozilla.org/r/61022/#review57882

Comment 32

[Paul Adenot (:padenot)]

Comment on attachment 8765886 [details]
Bug 1265394 - Use new PannerNode AudioParams;

https://reviewboard.mozilla.org/r/61024/#review58104

::: dom/media/webaudio/PannerNode.cpp
(Diff revision 1)
>  
> -NS_IMPL_CYCLE_COLLECTION_CLASS(PannerNode)
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(PannerNode, AudioNode, mPositionX, mPositionY, mPositionZ, mOrientationX, mOrientationY, mOrientationZ)
> -
> -NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(PannerNode)
> -  if (tmp->Context()) {
> -    tmp->Context()->UnregisterPannerNode(tmp);

I think this is still needed until we remove the doppler effect altogether.

Comment 33

[Paul Adenot (:padenot)]

Comment on attachment 8765887 [details]
Bug 1265394 - Update tests;

https://reviewboard.mozilla.org/r/61026/#review58106

::: dom/media/webaudio/test/test_pannerNode.html:54
(Diff revision 1)
> +  near(panner.positionX.value, 1, "setPosition sets AudioParam properly");
> +  near(panner.positionY.value, 1, "setPosition sets AudioParam properly");
> +  near(panner.positionZ.value, 1, "setPosition sets AudioParam properly");
> +
>    panner.setOrientation(1, 1, 1);
> +  near(panner.orientationX.value, 1, "setOrientation sets AudioParam properly");

Maybe it's better to use something else than 1 for this one, because it's the default.

Comment 34

[Paul Adenot (:padenot)]

Comment on attachment 8765888 [details]
Bug 1265394 - Add test for PannerNode automation;

https://reviewboard.mozilla.org/r/61030/#review58108

I suppose a more thorough test would test every property, but this is probably good enough for now.

Comment 35

[Dan Minor [:dminor]]

Comment on attachment 8765886 [details]
Bug 1265394 - Use new PannerNode AudioParams;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61024/diff/1-2/

Comment 36

[Dan Minor [:dminor]]

Comment on attachment 8765887 [details]
Bug 1265394 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61026/diff/1-2/

Comment 37

[Dan Minor [:dminor]]

Comment on attachment 8765888 [details]
Bug 1265394 - Add test for PannerNode automation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61030/diff/1-2/

Comment 38

[Paul Adenot (:padenot)]

Comment on attachment 8765886 [details]
Bug 1265394 - Use new PannerNode AudioParams;

https://reviewboard.mozilla.org/r/61024/#review58816

Comment 39

[Dan Minor [:dminor]]

Attachment: Bug 1265394 - Handle infinite values in SetPosition and SetOrientation;

Passing infinite values into an AudioParam will trigger an assert in debug
builds. Returning early here mimics the non-debug behaviour of refusing to
insert an event with an infinite value.

Review commit: https://reviewboard.mozilla.org/r/62436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62436/

Comment 40

[Dan Minor [:dminor]]

Comment on attachment 8765885 [details]
Bug 1265394 - Add new PannerNode AudioParams to webidl;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61022/diff/1-2/

Comment 41

[Dan Minor [:dminor]]

Comment on attachment 8765886 [details]
Bug 1265394 - Use new PannerNode AudioParams;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61024/diff/2-3/

Comment 42

[Dan Minor [:dminor]]

Comment on attachment 8765887 [details]
Bug 1265394 - Update tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61026/diff/2-3/

Comment 43

[Dan Minor [:dminor]]

Comment on attachment 8765888 [details]
Bug 1265394 - Add test for PannerNode automation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61030/diff/2-3/

Comment 44

[Dan Minor [:dminor]]

Switching to AudioParams caused an assert in a reftest [1]. I'm not sure if the best approach is to handle the infinite values in SetPosition or to modify the test (or both.)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=af50327ebdadc73abad346d4a15c6aba7f09e68f

Comment 45

[Paul Adenot (:padenot)]

Comment on attachment 8768124 [details]
Bug 1265394 - Handle infinite values in SetPosition and SetOrientation;

https://reviewboard.mozilla.org/r/62436/#review59368

Comment 46

[Dan Minor [:dminor]]

Comment on attachment 8768124 [details]
Bug 1265394 - Handle infinite values in SetPosition and SetOrientation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62436/diff/1-2/

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08afe66f63b
Add new PannerNode AudioParams to webidl; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3ad0eb3246
Use new PannerNode AudioParams; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/43b4bc7fcc51
Update tests; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59689e0524e
Add test for PannerNode automation; r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8799895ca858
Handle infinite values in SetPosition and SetOrientation; r=padenot

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a08afe66f63b
https://hg.mozilla.org/mozilla-central/rev/1d3ad0eb3246
https://hg.mozilla.org/mozilla-central/rev/43b4bc7fcc51
https://hg.mozilla.org/mozilla-central/rev/c59689e0524e
https://hg.mozilla.org/mozilla-central/rev/8799895ca858

Comment 49

[Eric Shepherd [:sheppy]]

These pages have been added:

  https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/orientationX

  https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/orientationY

  https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/orientationZ

  https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/positionX

  https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/positionY

  https://developer.mozilla.org/en-US/docs/Web/API/PannerNode/positionZ

These pages have been updated:

  https://developer.mozilla.org/en-US/docs/Web/API/AudioParam

  https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/createPanner

  https://developer.mozilla.org/en-US/Firefox/Releases/50

This documentation should now be complete. Please let me know if you see any issues!