Closed Bug 1472550 Opened 6 years ago Closed 6 years ago

Using linearRampToValueAtTime() with PannerNode produces wrong spatial sound

Categories

(Core :: Web Audio, defect, P3)

Product:
Core
Component:
Web Audio
Version:
61 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: michael.herzog, Assigned: padenot)

References

Details

Attachments

(5 files)

Bug 1472550 - Consistently apply the distance, input and cone gain on all channels when computing the effect of a PannerNode when its AudioParams have been set.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1472550 - Add Audio{Buffer,Block}InPlaceScale that takes an array of gain values, with SSE and NEON variants.
59 bytes, text/x-review-board-request
dminor
: review+
Details
Bug 1472550 - Adjust a web platform test slightly to test that the parameter is a-rate.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1472550 - Add a little regression test.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1472550 - Update wpt expectations.
59 bytes, text/x-review-board-request
jya
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

- Go to: https://jsfiddle.net/4yefu08L/3/
- The example shows a dev version of three.js using linearRampToValueAtTime() in order to change the transformation of the PannerNode (see https://github.com/mrdoob/three.js/pull/14393).


Actual results:

When you move the camera, you hear wrong spatial ping-pong sounds in Firefox on macOS 10.13.5 and Firefox 61. For example, it does not matter how far or close the camera (listener) is to the ping-pong balls, the produces sound is always equal. When using Firefox 60 on Ubuntu 16.04, there is no spatiality at all.


Expected results:

When you move the camera, you should always hear correct spatial ping-pong sounds.
BTW: The demo does work in Firefox when using PannerNode.setPosition() and PannerNode.setOrientation().
Component: Untriaged → Web Audio
Product: Firefox → Core
I need to check if wpt has test for this, and update expectations, or just write one.
Assignee: nobody → padenot
Blocks: 1265394
Comment on attachment 8990039 [details]
Bug 1472550 - Consistently apply the distance, input and cone gain on all channels when computing the effect of a PannerNode when its AudioParams have been set.

https://reviewboard.mozilla.org/r/255066/#review262110

::: dom/media/webaudio/PannerNode.cpp:611
(Diff revision 1)
>        distanceGain = ComputeDistanceGain(position);
>  
>        // Actually compute the left and right gain.
> -      float gainL = cos(0.5 * M_PI * normalizedAzimuth) * aInput.mVolume * distanceGain * coneGain;
> -      float gainR = sin(0.5 * M_PI * normalizedAzimuth) * aInput.mVolume * distanceGain * coneGain;
> +      float gainL = cos(0.5 * M_PI * normalizedAzimuth);
> +      float gainR = sin(0.5 * M_PI * normalizedAzimuth);
>  
>        alignedComputedGain[counter] = gainL;
>        alignedComputedGain[WEBAUDIO_BLOCK_SIZE + counter] = gainR;
>        onLeft[counter] = azimuth <= 0;
>      }
>  
> -    // Apply the gain to the output buffer
> +    // Apply the panning to the output buffer
>      ApplyStereoPanning(aInput, aOutput, alignedComputedGain, &alignedComputedGain[WEBAUDIO_BLOCK_SIZE], onLeft);
>  
> +    aOutput->mVolume = aInput.mVolume * distanceGain * coneGain;

This is better than what was before for StereoToStereo, but it is making
distance and cone gains some kind of k-rate.  The distance and cone gains are
calculated inside the block loop; they should be applied to each sample.
MonoToStereo was good and a-rate, but k-rate would be a regression there.

The current design would mean building up a gain array, or an array
of inputs with the gain applied.  The latter approach would have slightly
better memory locality if the arrays of input with gain are modified in place
to become the output.  I haven't checked whether the SIMD functions would
handle this appropriately.

I wonder how much win there is from the AudioBlockPanStereoToStereo_NEON
version with aIsOnTheLeft an array.
https://bugzilla.mozilla.org/show_bug.cgi?id=1105513#c5 says 10-20ms change
but gives no indication of total time.  I wonder how much of that could be
reclaimed by making a sample-wise version of AudioBlockPanStereoToStereo()
inline.

I would have expected the calculation to be faster if performed
sample-by-sample than if building arrays of the parameters.
With all the trigonometry and conditionals, it's hard to imagine how SIMD
makes much difference for the simple algebra.
Attachment #8990039 - Flags: review?(karlt)
Status: UNCONFIRMED → NEW
Rank: 25
Ever confirmed: true
Priority: -- → P3
Indeed this was incorrect. It's now fixed. I chose to simply make another array and implement a-rate in-place gain (with NEON and SSE variants).


I had a look about testing. This is tested in Web platform tests, but the test itself requires AudioListener AudioParams, that we don't have right now. I'm going to write a simple regression test for this as a mochitest, to run on android and exercise the NEON code.
Comment on attachment 8990383 [details]
Bug 1472550 - Adjust a web platform test slightly to test that the parameter is a-rate.

https://reviewboard.mozilla.org/r/255466/#review262416
Attachment #8990383 - Flags: review?(karlt) → review+
Comment on attachment 8990384 [details]
Bug 1472550 - Add a little regression test.

https://reviewboard.mozilla.org/r/255468/#review262418
Attachment #8990384 - Flags: review?(karlt) → review+
Comment on attachment 8990039 [details]
Bug 1472550 - Consistently apply the distance, input and cone gain on all channels when computing the effect of a PannerNode when its AudioParams have been set.

https://reviewboard.mozilla.org/r/255066/#review262444

::: dom/media/webaudio/PannerNode.cpp:571
(Diff revision 2)
>      } else {
>        orientationZ[0] = mOrientationZ.GetValueAtTime(tick);
>      }
>  
> +    float computedPanning[2*WEBAUDIO_BLOCK_SIZE + 4];
>      float computedGain[2*WEBAUDIO_BLOCK_SIZE + 4];

Only one block of of computedGain is used, and so this array can now be smaller.

What would be most compact and clearest, I think would be something like

float buffer[3*WEB_AUDIO_BLOCK_SIZE+4];
float* alignedPanL = ALIGNED16(buffer);
float* alignedPanR = alignedPanL + WEBAUDIO_BLOCK_SIZE;
float* alignedGain = alignedPanR + WEBAUDIO_BLOCK_SIZE;
ASSERT_ALIGNED16...
Attachment #8990039 - Flags: review?(karlt) → review+
Attachment #8990382 - Flags: review?(karlt) → review?(dminor)
Comment on attachment 8990382 [details]
Bug 1472550 - Add Audio{Buffer,Block}InPlaceScale that takes an array of gain values, with SSE and NEON variants.

https://reviewboard.mozilla.org/r/255464/#review262446

Dan, would you be able to look at this part, please?  I'm not so familiar with these.
Comment on attachment 8990382 [details]
Bug 1472550 - Add Audio{Buffer,Block}InPlaceScale that takes an array of gain values, with SSE and NEON variants.

https://reviewboard.mozilla.org/r/255464/#review262484

::: dom/media/webaudio/AudioNodeEngineSSE2.cpp:161
(Diff revision 1)
> +  __m128 vout0, vout1, vout2, vout3,
> +         vgain0, vgain1, vgain2, vgain3,
> +         vin0, vin1, vin2, vin3;
> +
> +  ASSERT_ALIGNED16(aBlock);
> +  ASSERT_MULTIPLE16(aSize);

It seems strange to me that the NEON version works with values of aSize that are not a multiple of 16, but the SSE version doesn't.

That could lead to surprises in the future. I think it would be best to make them consistent with each other, either by adding this assert to AudioBufferInPlaceScale and the NEON version or dealing with non-multiple of 16 values of aSize here.
Attachment #8990382 - Flags: review?(dminor) → review+
(In reply to Dan Minor [:dminor] from comment #14)
> It seems strange to me that the NEON version works with values of aSize that
> are not a multiple of 16, but the SSE version doesn't.
> 
> That could lead to surprises in the future. I think it would be best to make
> them consistent with each other, either by adding this assert to
> AudioBufferInPlaceScale and the NEON version or dealing with non-multiple of
> 16 values of aSize here.

I found it weird as well, but decided to copy the current style, and address it in a followup if we felt it was important.
Just writing down another possible option for this in case we ever want to
come back to it, but no need to change things now that we have something
working.

EqualPowerPanningFunction() could calculate a matrix of gains to map inputs to
outputs.  For stereo to stereo, that would be four floats per sample instead
of three floats and one bool.  For mono to stereo, it would be two floats per
sample.  There would be more multiplications for stereo to stereo,
but fewer conditional branches.
Comment on attachment 8991890 [details]
Bug 1472550 - Update wpt expectations.

https://reviewboard.mozilla.org/r/256806/#review263662
Attachment #8991890 - Flags: review?(jyavenard) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/bed4190ce013
Add Audio{Buffer,Block}InPlaceScale that takes an array of gain values, with SSE and NEON variants. r=dminor
https://hg.mozilla.org/integration/autoland/rev/ab307640537f
Consistently apply the distance, input and cone gain on all channels when computing the effect of a PannerNode when its AudioParams have been set. r=karlt
https://hg.mozilla.org/integration/autoland/rev/5b8172ac07f6
Adjust a web platform test slightly to test that the parameter is a-rate. r=karlt
https://hg.mozilla.org/integration/autoland/rev/70b5a9fca55b
Add a little regression test. r=karlt
https://hg.mozilla.org/integration/autoland/rev/32557eba9675
Update wpt expectations. r=jya
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11987 for changes under testing/web-platform/tests
I can confirm the three.js demo works now fine with Firefox Nightly. Yay! Thanks for fixing this! :)
Thanks Michael, glad it is working well for three.js. I'm now looking at implementing the AudioParam for the AudioListener.
See Also: → 1500303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: