Closed Bug 1436096 Opened 2 years ago Closed 2 years ago

Panner node equal power should have different output for mono and stereo

Categories

(Core :: Web Audio, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: toy.raymond, Assigned: achronop)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36

Steps to reproduce:

Run the test from https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/Panner/panner-equalpower.html

(Soon to be in wpt/webaudio/the-audio-api/the-pannernode-interface)


Actual results:

The output (in part) is:

PASS > [mono source=listener] Source and listener at the same position
PASS   Left and right channels is identical to the array [0,0.06264832615852356,0.12505052983760834,0.18696144223213196,0.24813786149024963,0.308339387178421,0.36732956767082214,0.4248766303062439,0.4807544946670532,0.5347436666488647,0.5866319537162781,0.6362155675888062,0.683299720287323,0.7276993989944458,0.7692402005195618,0.8077588677406311...].
PASS < [mono source=listener] All assertions passed. (total 1 assertions)
PASS > [stereo source=listener] Source and listener at the same position
PASS   Left and right channels is identical to the array [0,0.06264832615852356,0.12505052983760834,0.18696144223213196,0.24813786149024963,0.308339387178421,0.36732956767082214,0.4248766303062439,0.4807544946670532,0.5347436666488647,0.5866319537162781,0.6362155675888062,0.683299720287323,0.7276993989944458,0.7692402005195618,0.8077588677406311...].
PASS < [stereo source=listener] All assertions passed. (total 1 assertions)

Note that the output for the mono test is identical to the stereo case, and in fact looks like the stereo case.


Expected results:

The mono test and the stereo test should have different results.  See https://webaudio.github.io/web-audio-api/#Spatialzation-equal-power-panning

The formulas are different depending on whether the input is mono or stereo.
Component: Untriaged → Audio/Video
Product: Firefox → Core
Alex, can you take a look please, or pass this to someone who can? Thanks!
Component: Audio/Video → Web Audio
Flags: needinfo?(achronop)
Priority: -- → P3
I will rerun the test to see what is going on.
Flags: needinfo?(achronop)
Can you please tell me how to run the test locally?
Flags: needinfo?(toy.raymond)
Ok, here's a self-contained test.  Visit hoch.github.io/canopy and paste the following code into the code window and press the run button (the button above the eye icon on the left of the code window). Look at the output, which is displayed as 4 waveforms above the code window.

I see, now, a couple of issues here.  First, channel 0 is a full amplitude sine wave and channel 1 is silent.  The panner node is specified to produce stereo output with a mono source, as specified in https://webaudio.github.io/web-audio-api/#Spatialzation-equal-power-panning

Second, compare the waveforms in channels 2 and 3.  These are full amplitude sine waves which look like the signal in channel 0.  I think this is not right.  The waveforms should have different amplitudes because the output of the panner node has different formulas depending on whether the input is mono or stereo.

The code:

// @channels 4
// @duration 0.25
// @sampleRate 48000

// Turn off any upmixing in the destination; we want each channel as
// is.
context.destination.channelInterpretation = 'discrete';

let s = new OscillatorNode(context);
let merger = new ChannelMergerNode(context, {numberOfInputs: context.destination.channelCount});
merger.connect(context.destination);

// p1 is the PannerNode with a mono source; p2, with a stereo source
let p1 = new PannerNode(context);
let p2 = new PannerNode(context);

// Split the stereo output of each panner into individual channels
let p1splitter = new ChannelSplitterNode(context, {numberOfOutputs: 2});
let p2splitter = new ChannelSplitterNode(context, {numberOfOutputs: 2});

p1.connect(p1splitter);
p2.connect(p2splitter);

// The output from p1 goes to destination channels 0 and 1
p1splitter.connect(merger, 0, 0);
p1splitter.connect(merger, 1, 1);
// The output from p2 goest to channesl 2 and 3
p2splitter.connect(merger, 0, 2);
p2splitter.connect(merger, 1, 3);

// Convert the mono oscillator to a stereo source using a merger.
let upmix = new ChannelMergerNode(context, {numberOfInputs: 2});
s.connect(upmix, 0, 0);
s.connect(upmix, 0, 1);

// Connect up the sources to the panners
s.connect(p1);
upmix.connect(p2);

// Go!
s.start();
Flags: needinfo?(toy.raymond)
You can also visit https://w3c-test.org/webaudio/the-audio-api/the-pannernode-interface/panner-equalpower.html and look at the output arrays for the mono and stereo tests.  The arrays are the same, but should be different. (The test should have tested for this.)
Thanks Raymond, we'll have a look.
Status: UNCONFIRMED → NEW
Ever confirmed: true
The code takes the following exit path which simple copy the input to the output in every case, that's why panning is not correct:

https://searchfox.org/mozilla-central/source/dom/media/webaudio/PannerNode.cpp#485

I am looking further to understand the intention of that if check.
When I remove that if-check (hard coded) I get from both panners the expected output.
Assignee: nobody → achronop
Comment on attachment 8952307 [details]
Bug 1436096 - PannerNode noop optimization for equal power is valid on stereo source.

https://reviewboard.mozilla.org/r/221550/#review228606

::: commit-message-cb9b9:1
(Diff revision 1)
> +Bug 1436096 - Noop optimization does not apply in mono source. r?padenot

Write a clearer commit message. Out of context, we don't know what's going on here.

::: dom/media/webaudio/PannerNode.cpp:480
(Diff revision 1)
>      ThreeDPoint orientation = ConvertAudioParamTimelineTo3DP(mOrientationX, mOrientationY, mOrientationZ, tick);
>      if (!orientation.IsZero()) {
>        orientation.Normalize();
>      }
>  
> -    // If both the listener are in the same spot, and no cone gain is specified,
> +    // In stereo source, when both the listener and the panner are in

"For a stereo source"

::: dom/media/webaudio/PannerNode.cpp:482
(Diff revision 1)
>        orientation.Normalize();
>      }
>  
> -    // If both the listener are in the same spot, and no cone gain is specified,
> -    // this node is noop.
> -    if (mListenerPosition ==  position &&
> +    // In stereo source, when both the listener and the panner are in
> +    // the same spot, and no cone gain is specified, this node is noop.
> +    if (inputChannels > 1 &&

`input channel == 2` is clearer. This node either has stereo or mono input.
Attachment #8952307 - Flags: review?(padenot) → review+
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d200d50fb66f
In PannerNode noop optimization for equal power is valid on stereo source. r=padenot
Comment on attachment 8953556 [details]
Bug 1436096 - In PannerNode mochitest correct mono input and add test for stereo input.

https://reviewboard.mozilla.org/r/222768/#review228978
Attachment #8953556 - Flags: review?(padenot)
Comment on attachment 8953556 [details]
Bug 1436096 - In PannerNode mochitest correct mono input and add test for stereo input.

https://reviewboard.mozilla.org/r/222768/#review230324

::: dom/media/webaudio/test/test_pannerNodeAtZeroDistance.html:74
(Diff revision 2)
>    panner3.connect(ac.destination);
>  
> +  // Use the input buffer to compare the output. According to the spec,
> +  // mono input at zero distance will apply gain = cos(0.5 * Math.PI / 2)
> +  // https://webaudio.github.io/web-audio-api/#Spatialzation-equal-power-panning
> +  const gain = 0.70710678118;

Why not write `Math.cos(0.5 * Math.PI / 2)` here then ?
Attachment #8953556 - Flags: review?(padenot) → review+
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5dcbba40dd0c
PannerNode noop optimization for equal power is valid on stereo source. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e9d7a3f92bcc
In PannerNode mochitest correct mono input and add test for stereo input. r=padenot
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/af9b9ffd4750
PannerNode noop optimization for equal power is valid on stereo source. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c3f5ebf5eda6
In PannerNode mochitest correct mono input and add test for stereo input. r=padenot
https://hg.mozilla.org/mozilla-central/rev/af9b9ffd4750
https://hg.mozilla.org/mozilla-central/rev/c3f5ebf5eda6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Clearing NI
Flags: needinfo?(achronop)
You need to log in before you can comment on or make changes to this bug.