Bug 75767 – EQUALPOWER panner incorrectly computes gain

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

The formula to compute the left gain for the EQUALPOWER panner is incorrect.  When the azimuth is 0, the gain is 0.5.  It should be 1/sqrt(2).  It should be a cosine curve between -90 and 90 deg, with a gain of 1/sqrt(2) at an azimuth of 0.

This needs to be fixed before 75126 can be committed with its tests.  (That will also indirectly verify that the EQUALPOWER panner works in at least one case.)

[Raymond Toy]

Created attachment 121684 [details]
Patch

[Raymond Toy]

I think the patch implements the correct formula.   The desiredGainL is 1 when azimuth is fully left, 0.707 when centered and 0 when azimuth is fully right.

[Chris Rogers]

Now's a good time to add a layout test for the equal-power panner in this patch.  You should be able to use a similar approach to the distance-model layout test.

[Raymond Toy]

(In reply to comment #3)
> Now's a good time to add a layout test for the equal-power panner in this patch.  You should be able to use a similar approach to the distance-model layout test.

Ok.  Is the formula correct?

[Chris Rogers]

Comment on attachment 121684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121684&action=review

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:87
> +    double desiredGainL = cos(piDouble * desiredPanPosition / 2);

The formula looks good.  Maybe better as:

cos(0.5 * piDouble * desiredPanPosition)

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:88
>      double desiredGainR = sqrt(1.0 - desiredGainL*desiredGainL);

The right gain can be more symmetric by using sin():

sin(0.5 * piDouble * desiredPanPosition)

[Raymond Toy]

Created attachment 121864 [details]
Patch

[Raymond Toy]

Comment on attachment 121684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121684&action=review

>> Source/WebCore/platform/audio/EqualPowerPanner.cpp:87
>> +    double desiredGainL = cos(piDouble * desiredPanPosition / 2);
> 
> The formula looks good.  Maybe better as:
> 
> cos(0.5 * piDouble * desiredPanPosition)

Done.

>> Source/WebCore/platform/audio/EqualPowerPanner.cpp:88
>>      double desiredGainR = sqrt(1.0 - desiredGainL*desiredGainL);
> 
> The right gain can be more symmetric by using sin():
> 
> sin(0.5 * piDouble * desiredPanPosition)

Done.  (I had thought about that, but left it as is, assuming you had a reason for doing the sqrt form.)

[Raymond Toy]

Created attachment 122082 [details]
Patch

[Raymond Toy]

Minor update.  Need to include audio-testing.js in test, and the changelog for the tests was missing.

[Chris Rogers]

Comment on attachment 122082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122082&action=review

This test is testing quite a lot more than it needs to.  I think some of these tests could later be valuable to check the dot(), cross(), etc. are all working correctly.  So, let's save all that stuff aside for later more specific tests.

This test can be extremely simple (vastly more simple than now).  Just sample an angle (azimuth) between -90 and +90 degrees.  Use the cos()/sin() to convert to x,y coordinates (and make z be zero).  And then validate that the scaled impulse is rendered according to the equal-power formula.

> LayoutTests/webaudio/panner-equalpower.html:29
> +          var threshold = .00019;

Similar to the distance tests - I would just make the threshold internal to the .js file - but also give the threshold a more descriptive name and/or comment

> LayoutTests/webaudio/resources/panner-model-testing.js:3
> +var renderLengthSeconds = 8;

8 seconds is very long for this test - could a fraction of a second

> LayoutTests/webaudio/resources/panner-model-testing.js:5
> +var pulseLengthFrames = pulseLengthSeconds * sampleRate;

pulseLengthSeconds is unused - please remove

> LayoutTests/webaudio/resources/panner-model-testing.js:25
> +function createImpulseBuffer(context, sampleFrameLength) {

See comment in distance-model tests -- this function can be moved into audio-testing.js

> LayoutTests/webaudio/resources/panner-model-testing.js:69
> +var distanceModelFunction = [linearDistance, inverseDistance, exponentialDistance];

The distance functions 38:69 are not used - please remove here and all other places with unused code...

> LayoutTests/webaudio/resources/panner-model-testing.js:105
> +  for (var k = 0; k < nodesToCreate; ++k) {

Please simplify loops into a single loop (so that startSource() is not a separate function) similar to distance-model

[Raymond Toy]

Created attachment 122318 [details]
Patch

[Raymond Toy]

Comment on attachment 122082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122082&action=review

>> LayoutTests/webaudio/panner-equalpower.html:29
>> +          var threshold = .00019;
> 
> Similar to the distance tests - I would just make the threshold internal to the .js file - but also give the threshold a more descriptive name and/or comment

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:3
>> +var renderLengthSeconds = 8;
> 
> 8 seconds is very long for this test - could a fraction of a second

Arranged to be just long enough for all of the nodes that are created.

>> LayoutTests/webaudio/resources/panner-model-testing.js:5
>> +var pulseLengthFrames = pulseLengthSeconds * sampleRate;
> 
> pulseLengthSeconds is unused - please remove

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:25
>> +function createImpulseBuffer(context, sampleFrameLength) {
> 
> See comment in distance-model tests -- this function can be moved into audio-testing.js

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:69
>> +var distanceModelFunction = [linearDistance, inverseDistance, exponentialDistance];
> 
> The distance functions 38:69 are not used - please remove here and all other places with unused code...

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:105
>> +  for (var k = 0; k < nodesToCreate; ++k) {
> 
> Please simplify loops into a single loop (so that startSource() is not a separate function) similar to distance-model

Done.

[Chris Rogers]

Comment on attachment 122318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122318&action=review

> Source/WebCore/webaudio/AudioPannerNode.h:63
> +    

It looks like you accidentally included the changes for the distance constants in this patch:
AudioPannerNode.h
AudioPannerNode.idl

please remove

> LayoutTests/webaudio/panner-equalpower.html:29
> +          var tempPanner = context.createPanner();

I wouldn't bother passing tempPanner.EQUALPOWER panner model in as a constant.  The test itself in the .js file is ignoring this value anyway, and it's
unlikely that a similar approach will be useful for testing, for example, HRTF (there are other approaches for that).

So easiest to delete 28:29, and remove 2nd argument to createTestAndRun() below

> LayoutTests/webaudio/resources/panner-model-testing.js:10
> +var timeStep = 0.001;

Should lines 9:10 be defined before line 4, and then use "timeStep" instead of hard-coded value in line 4?

> LayoutTests/webaudio/resources/panner-model-testing.js:76
> +  var maxRelativeError = .00019;

Better to define this threshold inside checkResult() instead of passing it as an argument.

> LayoutTests/webaudio/resources/panner-model-testing.js:78
> +  context.oncomplete = checkResult(pannerModel, maxRelativeError);

I'm not sure that we can leverage this code for other panning models (like HRTF), so easier to remove pannerModel.  And then we don't need the "closures" (inner function)

and can simplify to:

context.oncomplete = checkResult;

> LayoutTests/webaudio/resources/panner-model-testing.js:87
> +  return 90 - angle * 180 / Math.PI;

nit: Indentation here and in the whole file seems to be 2, but it should be 4

> LayoutTests/webaudio/resources/panner-model-testing.js:94
> +  var panPosition = (azimuth + 90)/180;

nit: spaces around /

> LayoutTests/webaudio/resources/panner-model-testing.js:103
> +  return function(event) {

see comments about model and inner function above

> LayoutTests/webaudio/resources/panner-model-testing.js:111
> +    // Max (relative) error and the index of the maxima for the left

Can we have a blank line before lines 111, 117, 119, etc.  I'm not sure if it's a formal WebKit style guideline, but it would definitely be more in-line with other code in WebKit (in general)

> LayoutTests/webaudio/resources/panner-model-testing.js:128
> +                                          position[impulseCount].z);

This distance gain is always going to be 1.0 (at a distance of 1) for all distance models, isn't it?

If so, then let's just get rid of the linearDistance() function and this variable.

> LayoutTests/webaudio/resources/panner-model-testing.js:137
> +        var errorR = Math.abs(renderedRight[k] - expectedR)/Math.abs(expectedR);

nit: spaces around /

[Raymond Toy]

Created attachment 122332 [details]
Patch

[Raymond Toy]

Comment on attachment 122318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122318&action=review

>> Source/WebCore/webaudio/AudioPannerNode.h:63
>> +    
> 
> It looks like you accidentally included the changes for the distance constants in this patch:
> AudioPannerNode.h
> AudioPannerNode.idl
> 
> please remove

Oops.  Done.

>> LayoutTests/webaudio/panner-equalpower.html:29
>> +          var tempPanner = context.createPanner();
> 
> I wouldn't bother passing tempPanner.EQUALPOWER panner model in as a constant.  The test itself in the .js file is ignoring this value anyway, and it's
> unlikely that a similar approach will be useful for testing, for example, HRTF (there are other approaches for that).
> 
> So easiest to delete 28:29, and remove 2nd argument to createTestAndRun() below

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:10
>> +var timeStep = 0.001;
> 
> Should lines 9:10 be defined before line 4, and then use "timeStep" instead of hard-coded value in line 4?

They were actually unrelated but coincidentally the same, but I updated as you suggested.

>> LayoutTests/webaudio/resources/panner-model-testing.js:76
>> +  var maxRelativeError = .00019;
> 
> Better to define this threshold inside checkResult() instead of passing it as an argument.

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:78
>> +  context.oncomplete = checkResult(pannerModel, maxRelativeError);
> 
> I'm not sure that we can leverage this code for other panning models (like HRTF), so easier to remove pannerModel.  And then we don't need the "closures" (inner function)
> 
> and can simplify to:
> 
> context.oncomplete = checkResult;

Done.  (With some simplification of checkResult not to return a function, but be the actual function.)

>> LayoutTests/webaudio/resources/panner-model-testing.js:87
>> +  return 90 - angle * 180 / Math.PI;
> 
> nit: Indentation here and in the whole file seems to be 2, but it should be 4

Fixed.

>> LayoutTests/webaudio/resources/panner-model-testing.js:94
>> +  var panPosition = (azimuth + 90)/180;
> 
> nit: spaces around /

Fixed.

>> LayoutTests/webaudio/resources/panner-model-testing.js:103
>> +  return function(event) {
> 
> see comments about model and inner function above

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:111
>> +    // Max (relative) error and the index of the maxima for the left
> 
> Can we have a blank line before lines 111, 117, 119, etc.  I'm not sure if it's a formal WebKit style guideline, but it would definitely be more in-line with other code in WebKit (in general)

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:128
>> +                                          position[impulseCount].z);
> 
> This distance gain is always going to be 1.0 (at a distance of 1) for all distance models, isn't it?
> 
> If so, then let's just get rid of the linearDistance() function and this variable.

Removed.

[Raymond Toy]

Created attachment 122335 [details]
Patch

[Raymond Toy]

Forgot to update the expected results with the changes from the last patch.

[Chris Rogers]

Comment on attachment 122335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122335&action=review

Looks great overall - thanks Ray!  Just a couple of comment nits and question about 0 value

> LayoutTests/webaudio/resources/panner-model-testing.js:32
> +    var angleStep = Math.PI / (nodeCount + 1);  // nodeCount + 1 so we don't go past 180 deg.

nit: single space before comment

> LayoutTests/webaudio/resources/panner-model-testing.js:42
> +        panner[k].distanceModel = 0;    // Linear distance model.

nit: single space before comment

> LayoutTests/webaudio/resources/panner-model-testing.js:46
> +        // produce 0 which messes up our check routine.

I don't understand why we avoid 0.  -- oh I see it's because of:
// Relative error in the gain.

Can't we simply use absolute error checking instead of relative so we can handle the 0 and not have to worry about this type of work-around.
We *do* actually want to check hard-pan left in the test (and hard-pan right)

> LayoutTests/webaudio/resources/panner-model-testing.js:70
> +// is -90 deg.

Seems like there's plenty of room to not wrap this comment (based on length of other lines in this file)

[Raymond Toy]

Comment on attachment 122335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122335&action=review

>> LayoutTests/webaudio/resources/panner-model-testing.js:46
>> +        // produce 0 which messes up our check routine.
> 
> I don't understand why we avoid 0.  -- oh I see it's because of:
> // Relative error in the gain.
> 
> Can't we simply use absolute error checking instead of relative so we can handle the 0 and not have to worry about this type of work-around.
> We *do* actually want to check hard-pan left in the test (and hard-pan right)

We avoid zero because we search for non-zero values due to the noteOn bug.   Originally, I was only searching the left channel, but now that both channels are searched, this restriction is gone since both channels are never simultaneously zero (where the impulses should be).   And then we do need to get rid of the relative error computation.

[Chris Rogers]

Comment on attachment 122335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122335&action=review

>>> LayoutTests/webaudio/resources/panner-model-testing.js:46
>>> +        // produce 0 which messes up our check routine.
>> 
>> I don't understand why we avoid 0.  -- oh I see it's because of:
>> // Relative error in the gain.
>> 
>> Can't we simply use absolute error checking instead of relative so we can handle the 0 and not have to worry about this type of work-around.
>> We *do* actually want to check hard-pan left in the test (and hard-pan right)
> 
> We avoid zero because we search for non-zero values due to the noteOn bug.   Originally, I was only searching the left channel, but now that both channels are searched, this restriction is gone since both channels are never simultaneously zero (where the impulses should be).   And then we do need to get rid of the relative error computation.

I see - that makes sense.  Then since the restriction is gone (can check if either left or right is non-zero) we can switch to absolute thresholds.  
Seems like an easy fix, and makes this test easier to understand and also tests the edges (which we should)

[Chris Rogers]

Comment on attachment 122335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122335&action=review

> LayoutTests/webaudio/resources/panner-model-testing.js:155
> +        testPassed("Left channel panner values are correct.");

"panner values" -> "gain values"

"gain" seems more descriptive of what we're actually testing

> LayoutTests/webaudio/resources/panner-model-testing.js:157
> +        testFailed("Left channel panner values are incorrect.  Max rel error = " + maxErrorL + " at time " + maxErrorIndexL / sampleRate + " (threshold = " + maxAllowedRelativeError + ")");

ditto

> LayoutTests/webaudio/resources/panner-model-testing.js:162
> +        testPassed("Right channel panner values are correct.");

ditto

> LayoutTests/webaudio/resources/panner-model-testing.js:164
> +        testFailed("Right channel panner values are incorrect.  Max rel error = " + maxErrorR + " at time " + maxErrorIndexR / sampleRate + " (threshold = " + maxAllowedRelativeError + ")");

ditto

[Raymond Toy]

Created attachment 122439 [details]
Patch

[Raymond Toy]

(In reply to comment #22)
> Created an attachment (id=122439) [details]
> Patch

All comments are fixed.  We now test for panning full left and right.

[Chris Rogers]

Comment on attachment 122439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122439&action=review

Thanks Ray, this looks fine if my two comments are addressed.

> LayoutTests/webaudio/panner-equalpower.html:17
> +      function runTest() {

Can we add a few sentences of high-level comment describing the testing approach: move from azimuth -90 -> +90 tracing a semi-circle at a distance of 1, etc.

> LayoutTests/webaudio/resources/panner-model-testing.js:114
> +        // We assume that the left and right channels start at the same instance.

instance -> instant

[Raymond Toy]

Created attachment 122464 [details]
Patch

[Raymond Toy]

Comment on attachment 122439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122439&action=review

>> LayoutTests/webaudio/panner-equalpower.html:17
>> +      function runTest() {
> 
> Can we add a few sentences of high-level comment describing the testing approach: move from azimuth -90 -> +90 tracing a semi-circle at a distance of 1, etc.

Done.

>> LayoutTests/webaudio/resources/panner-model-testing.js:114
>> +        // We assume that the left and right channels start at the same instance.
> 
> instance -> instant

Done.

[Chris Rogers]

I'm assuming you've tested that the test fails without the fix in EqualPowerPanner.cpp (and now passes with it).

Looks Good!

[Kenneth Russell]

Comment on attachment 122464 [details]
Patch

Nice test! rs=me

[Raymond Toy]

(In reply to comment #27)
> I'm assuming you've tested that the test fails without the fix in EqualPowerPanner.cpp (and now passes with it).

Not recently, so I tested again just now.  Test fails as expected.

[WebKit Review Bot]

Comment on attachment 122464 [details]
Patch

Clearing flags on attachment: 122464

Committed r104987: <http://trac.webkit.org/changeset/104987>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.