Bug 75767 - EQUALPOWER panner incorrectly computes gain
Summary: EQUALPOWER panner incorrectly computes gain
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 21:55 PST by Raymond Toy
Modified: 2012-01-23 09:23 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2012-01-09 10:37 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (14.28 KB, patch)
2012-01-10 10:18 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (15.08 KB, patch)
2012-01-11 13:06 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (14.14 KB, patch)
2012-01-12 15:13 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (12.13 KB, patch)
2012-01-12 16:11 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2012-01-12 16:20 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2012-01-13 09:23 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2012-01-13 11:05 PST, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond Toy 2012-01-06 21:55:09 PST
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.)
Comment 1 Raymond Toy 2012-01-09 10:37:52 PST
Created attachment 121684 [details]
Patch
Comment 2 Raymond Toy 2012-01-09 10:39:20 PST
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.
Comment 3 Chris Rogers 2012-01-09 11:18:56 PST
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.
Comment 4 Raymond Toy 2012-01-09 11:22:59 PST
(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?
Comment 5 Chris Rogers 2012-01-09 11:45:52 PST
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)
Comment 6 Raymond Toy 2012-01-10 10:18:02 PST
Created attachment 121864 [details]
Patch
Comment 7 Raymond Toy 2012-01-10 10:22:17 PST
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.)
Comment 8 Raymond Toy 2012-01-11 13:06:56 PST
Created attachment 122082 [details]
Patch
Comment 9 Raymond Toy 2012-01-11 13:08:21 PST
Minor update.  Need to include audio-testing.js in test, and the changelog for the tests was missing.
Comment 10 Chris Rogers 2012-01-12 12:48:13 PST
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
Comment 11 Raymond Toy 2012-01-12 15:13:15 PST
Created attachment 122318 [details]
Patch
Comment 12 Raymond Toy 2012-01-12 15:14:43 PST
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.
Comment 13 Chris Rogers 2012-01-12 15:43:44 PST
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 /
Comment 14 Raymond Toy 2012-01-12 16:11:16 PST
Created attachment 122332 [details]
Patch
Comment 15 Raymond Toy 2012-01-12 16:15:17 PST
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.
Comment 16 Raymond Toy 2012-01-12 16:20:26 PST
Created attachment 122335 [details]
Patch
Comment 17 Raymond Toy 2012-01-12 16:21:16 PST
Forgot to update the expected results with the changes from the last patch.
Comment 18 Chris Rogers 2012-01-12 16:48:23 PST
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)
Comment 19 Raymond Toy 2012-01-12 19:21:03 PST
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.
Comment 20 Chris Rogers 2012-01-12 19:52:31 PST
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)
Comment 21 Chris Rogers 2012-01-12 20:44:26 PST
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
Comment 22 Raymond Toy 2012-01-13 09:23:22 PST
Created attachment 122439 [details]
Patch
Comment 23 Raymond Toy 2012-01-13 09:25:40 PST
(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.
Comment 24 Chris Rogers 2012-01-13 10:18:17 PST
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
Comment 25 Raymond Toy 2012-01-13 11:05:33 PST
Created attachment 122464 [details]
Patch
Comment 26 Raymond Toy 2012-01-13 11:07:17 PST
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.
Comment 27 Chris Rogers 2012-01-13 13:00:38 PST
I'm assuming you've tested that the test fails without the fix in EqualPowerPanner.cpp (and now passes with it).

Looks Good!
Comment 28 Kenneth Russell 2012-01-13 13:05:33 PST
Comment on attachment 122464 [details]
Patch

Nice test! rs=me
Comment 29 Raymond Toy 2012-01-13 13:17:42 PST
(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.
Comment 30 WebKit Review Bot 2012-01-13 14:18:54 PST
Comment on attachment 122464 [details]
Patch

Clearing flags on attachment: 122464

Committed r104987: <http://trac.webkit.org/changeset/104987>
Comment 31 WebKit Review Bot 2012-01-13 14:19:00 PST
All reviewed patches have been landed.  Closing bug.