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.)
Created attachment 121684 [details] Patch
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.
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.
(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 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)
Created attachment 121864 [details] Patch
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.)
Created attachment 122082 [details] Patch
Minor update. Need to include audio-testing.js in test, and the changelog for the tests was missing.
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
Created attachment 122318 [details] Patch
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 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 /
Created attachment 122332 [details] Patch
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.
Created attachment 122335 [details] Patch
Forgot to update the expected results with the changes from the last patch.
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 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 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 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
Created attachment 122439 [details] Patch
(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 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
Created attachment 122464 [details] Patch
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.
I'm assuming you've tested that the test fails without the fix in EqualPowerPanner.cpp (and now passes with it). Looks Good!
Comment on attachment 122464 [details] Patch Nice test! rs=me
(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 on attachment 122464 [details] Patch Clearing flags on attachment: 122464 Committed r104987: <http://trac.webkit.org/changeset/104987>
All reviewed patches have been landed. Closing bug.