[webkit-reviews] review requested: [Bug 74273] Constant values to set "distanceModel" are undefined : [Attachment 122089] Patch

[bugzilla-daemon at webkit.org]

Raymond Toy <rtoy at chromium.org> has asked  for review:
Bug 74273: Constant values to set "distanceModel" are undefined
https://bugs.webkit.org/show_bug.cgi?id=74273

Attachment 122089: Patch
https://bugs.webkit.org/attachment.cgi?id=122089&action=review

------- Additional Comments from Raymond Toy <rtoy at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122089&action=review


>> LayoutTests/webaudio/distance-exponential.html:28
>> +	      // Threshold experimentally determined.
> 
> Can you use a more descriptive variable name and comment than simply
threshold.  What type of value is being compared, etc...

Done.

>>>>> LayoutTests/webaudio/distance-inverse.html:29
>>>>> + 	 var threshold = 1.3e-7;
>>>> 
>>>> Now that I look at this, let's just settle on a single threshold value for
all three tests (I guess the largest of the values).  And then get rid of this
code in all the .html files and remove the "threshold" parameter
>>>> to createTestAndRun()
>>> 
>>> They vary a bit (from 1.3e-7 to 2.3e-6).  Do you want to use just one?
>> 
>> Yes, I was hoping you could pick a threshold which is the least stringent of
them all and go with that, and then just define that threshold inside the .js
file instead of separately for each .html
> 
> I'll update this once the bug 75767 has landed due to the change in
audio-testing.js.

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:3
>> +var renderLengthSeconds = 8;
> 
> Why do we need to render 8 seconds?  That seems like a really long time and
wasteful.  For the convolution test, it makes more sense, but this could be
fractions of a second, test could complete more quickly, use less memory, etc.

Too much copying and pasting from the convolution test.  The time is much
smaller now.

>> LayoutTests/webaudio/resources/distance-model-testing.js:5
>> +var pulseLengthFrames = pulseLengthSeconds * sampleRate;
> 
> lines 4:5 look like they can be removed

Line 4 deleted, but 5 is used to set the length of the impulse.

>> LayoutTests/webaudio/resources/distance-model-testing.js:21
>> +function createImpulseBuffer(context, sampleFrameLength) {
> 
> Seems like this function would be nice to put in audio-testing.js since it
seems to be useful for several different tests.

Already done in the equalpower panner test.

>> LayoutTests/webaudio/resources/distance-model-testing.js:35
>> +// spec, not the code.
> 
> Instead, can we say that the Web Audio spec follows the OpenAL formulas and
provide a link?

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:54
>> +function inverseDistance(panner, x, y, z) {
> 
> nit: the order these functions are defined is different than the
distanceModelFunction array below -- slightly confusing

That was the order of the implementation of the tests.	Order is changed now.

>> LayoutTests/webaudio/resources/distance-model-testing.js:88
>> +  for (var k = 0; k < nodeCount; ++k) {
> 
> Please consolidate code from loop on line 70 into this loop.	There's no need
to have two separate loops.  Then your comments 76:85 can move down to just
above 89

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:93
>> +	// distance.
> 
> strange line wrapping - nearby line 94 is long enough to not wrap this
comment

Fixed.

>> LayoutTests/webaudio/resources/distance-model-testing.js:100
>> +  for (var k = 0; k < nodeCount; ++k) {
> 
> Once again following my comment on line 88, please consolidate this third
loop into the above loop

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:111
>> +	bufferSource[k].noteOn(time[k]);
> 
> Please just consolidate startSources() function into the above createGraph()
function, and you can use the same loop above.
> 
> This will read more smoothly having a single loop will all of the graph
connection, and the noteOn() one line after the next instead of split out into
separate functions and loops

Done

>> LayoutTests/webaudio/resources/distance-model-testing.js:120
>> +  startSources();
> 
> As per comment above, please just consolidate startSources() into the
createGraph() function

Done

>> LayoutTests/webaudio/resources/distance-model-testing.js:132
>> +function checkDistanceResult(model, expectedModel, threshold, debug) {
> 
> Please remove "debug" param

Done

>> LayoutTests/webaudio/resources/distance-model-testing.js:142
>> +	  testPassed("Distance model value matched expected value.");
> 
> Please update text for failure case -- it's the same text as the passed case

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:154
>> +	// expected location.  (For debugging.)
> 
> remove (for debugging) comment

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:163
>> +	    expected *= equalPowerGain();
> 
> Please add comment explaining that this compensates for the "center-panning"
value using the EQUALPOWERPANNING model

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:165
>> +	    var error = Math.abs(renderedData[k] -
expected)/Math.abs(expected);
> 
> nit: spacing with /

Fixed.

>> LayoutTests/webaudio/resources/distance-model-testing.js:168
>> +	    }
> 
> Let's get rid of this debug code for code we actually commit

Removed.

>> LayoutTests/webaudio/resources/distance-model-testing.js:179
>> +	  testPassed("Number of nodes is correct.");
> 
> Can we be a bit more specific what this test is, something like:
> 
> 'Number of impulses found matches number of panner nodes"

Done.

>> LayoutTests/webaudio/resources/distance-model-testing.js:196
>> +	// FIXME:  File a bug about this.
> 
> We actually have a specific bug for this now, so we can provide the exact
link

The noteOn bug has been commited so this comment and the following code has
been updated appropriately.

>> LayoutTests/webaudio/resources/distance-model-testing.js:198
>> +	  console.log(timeErrors.length + " timing errors found");
> 
> Please convert from console.log() format to testPassed/testFailed format, get
rid of the "debug" parameter to this function and simply comment out these
specific tests
> 
> Then once the bug is fixed, these lines can simply be uncommented and the
expected results .txt files updated

Done