Add normalize attribute to ConvolverNode to disable normalization.
Created attachment 120381 [details] Patch
Not ready for review; tests are being written.
Created attachment 120406 [details] Patch
Tests added. To test the tail which should be zero but is not due to round-off, we break the tail into two pieces and check that the max is less than some threshold.
Comment on attachment 120406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120406&action=review Ray, thanks for this patch, especially the layout test! This particular test is important and will cover a lot of currently untested code relating to convolution and the FFTs. > Source/WebCore/ChangeLog:8 > + Tests added I would give details of test file added (convolution-mono-mono.html) > Source/WebCore/platform/audio/Reverb.cpp:97 > + // Only scale if we want normalization (and scale is ok). WebKit style is to avoid comments which are obvious. In this case line 98 is very clear > Source/WebCore/platform/audio/Reverb.cpp:104 > + // FIXME: What about roundoff? I might add a little more detail in the FIXME: // FIXME: consider if we should make a temporary scaled copy of impulseResponse instead of scaling it in-place due to loss of precision. > Source/WebCore/webaudio/ConvolverNode.cpp:50 > + m_normalize(true) WebKit style is to put the comma "," on line 50 (lined up with ":") instead of at the end of line 49 > Source/WebCore/webaudio/ConvolverNode.h:70 > + // for backward compatibility. I wouldn't mention about backward compatibility. It's by design > LayoutTests/webaudio/convolution-mono-mono-expected.txt:6 > +PASS Test signal was correctly delayed. "correctly delayed" should be something like "correctly convolved" (see comments below) > LayoutTests/webaudio/resources/convolution-testing.js:54 > + var diff = renderedData[i + 128] - referenceData[i]; The constant 128 is a "magic" constant representing the current latency of the convolution implementation. We should define a constant for this at the top of the file with a comment describing why we're "fudging" this. We should also write a WebKit bug tracking this issue - something like "ConvolverNode should not incur processing latency" > LayoutTests/webaudio/resources/convolution-testing.js:64 > + // silent. But round-off prevents this from being completely Instead of round-off, I would say "phase errors" in the final FFT due to precision limitations. Also, if we go farther out it should be "exactly zero" and not "even closer" > LayoutTests/webaudio/resources/convolution-testing.js:73 > + var breakpoint = 12800; Can you explain in more detail what you mean by "two tail parts"? > LayoutTests/webaudio/resources/convolution-testing.js:77 > + var threshold2 = 1e-7; shouldn't we be seeing precisely zero for threshold2? In other words, after we've gone past the influence of the signal and the last FFT (phase errors) we should be generating exactly zero. This would be my intuition - interested in what we're actually seeing here? > LayoutTests/webaudio/resources/convolution-testing.js:78 > + for (var i = referenceData.length + 128; i < referenceData.length + breakpoint; ++i) { see comment about "magic constant" 128 above > LayoutTests/webaudio/resources/convolution-testing.js:79 > + var mag = Math.abs(renderedData[i]); "mag" may be a poor choice of name here since it seems to be short for "magnitude" which has connotations of frequency-domain. But, we're dealing with a time-domain sequence. > LayoutTests/webaudio/resources/convolution-testing.js:80 > + if (mag > tailMax1) { no brackets on single-line if(), here and in similar places > LayoutTests/webaudio/resources/convolution-testing.js:88 > + //console.log("Max1 = " + tailMax1); please remove commented-out line > LayoutTests/webaudio/resources/convolution-testing.js:97 > + // alert(i + ": renderedData[i] = " + renderedData[i]); please remove commented-out line > LayoutTests/webaudio/resources/convolution-testing.js:100 > + //console.log("Max2 = " + tailMax2); please remove commented-out line > LayoutTests/webaudio/resources/convolution-testing.js:108 > + var maxDeviationFraction = maxDelta / valueAtMaxDelta; Although we calculate this value, it doesn't look like we're checking it against a threshold to make sure the triangular portion is "within the zone" This is probably the most important part of the test. Similar to lines 119:123 we should have either say "Triangular portion correctly convolved." Also, see line 119 where "success" is never changed to false anywhere in this code > LayoutTests/webaudio/resources/convolution-testing.js:117 > + // } please remove commented-out lines > LayoutTests/webaudio/resources/convolution-testing.js:120 > + testPassed("Test signal was correctly delayed."); Do we want the message to say "correctly delayed" or "correctly convolved"????
Created attachment 121037 [details] Patch
Comment on attachment 120406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120406&action=review >> Source/WebCore/platform/audio/Reverb.cpp:97 >> + // Only scale if we want normalization (and scale is ok). > > WebKit style is to avoid comments which are obvious. In this case line 98 is very clear Deleted. >> Source/WebCore/platform/audio/Reverb.cpp:104 >> + // FIXME: What about roundoff? > > I might add a little more detail in the FIXME: > > // FIXME: consider if we should make a temporary scaled copy of impulseResponse instead of scaling it in-place due to loss of precision. Comment updated. >> Source/WebCore/webaudio/ConvolverNode.cpp:50 >> + m_normalize(true) > > WebKit style is to put the comma "," on line 50 (lined up with ":") instead of at the end of line 49 Oops. Done. >> Source/WebCore/webaudio/ConvolverNode.h:70 >> + // for backward compatibility. > > I wouldn't mention about backward compatibility. It's by design Deleted. >> LayoutTests/webaudio/convolution-mono-mono-expected.txt:6 >> +PASS Test signal was correctly delayed. > > "correctly delayed" should be something like "correctly convolved" (see comments below) Changed to correctly convolved. >> LayoutTests/webaudio/resources/convolution-testing.js:54 >> + var diff = renderedData[i + 128] - referenceData[i]; > > The constant 128 is a "magic" constant representing the current latency of the convolution implementation. > We should define a constant for this at the top of the file with a comment describing why we're "fudging" this. > We should also write a WebKit bug tracking this issue - something like "ConvolverNode should not incur processing latency" Constant defined and used as needed. But tracking bug not entered yet. >> LayoutTests/webaudio/resources/convolution-testing.js:64 >> + // silent. But round-off prevents this from being completely > > Instead of round-off, I would say "phase errors" in the final FFT due to precision limitations. > Also, if we go farther out it should be "exactly zero" and not "even closer" Why just phase errors? Surely there are magnitude errors as well. >> LayoutTests/webaudio/resources/convolution-testing.js:73 >> + var breakpoint = 12800; > > Can you explain in more detail what you mean by "two tail parts"? Comments added that explain this better, I hope. >> LayoutTests/webaudio/resources/convolution-testing.js:77 >> + var threshold2 = 1e-7; > > shouldn't we be seeing precisely zero for threshold2? In other words, after we've gone past the influence of the signal and the last FFT (phase errors) we > should be generating exactly zero. This would be my intuition - interested in what we're actually seeing here? My mistake. The first version had the breakpoint low enough that the samples after the breakpoint were not zero. But in this version, I actually selected the breakpoint so that they are zero. Threshold updated to 0. >> LayoutTests/webaudio/resources/convolution-testing.js:79 >> + var mag = Math.abs(renderedData[i]); > > "mag" may be a poor choice of name here since it seems to be short for "magnitude" which has connotations of frequency-domain. But, we're dealing with a time-domain sequence. I thought the context (Math.abs) made it clear this is the magnitude of the number. >> LayoutTests/webaudio/resources/convolution-testing.js:88 >> + //console.log("Max1 = " + tailMax1); > > please remove commented-out line Done. >> LayoutTests/webaudio/resources/convolution-testing.js:97 >> + // alert(i + ": renderedData[i] = " + renderedData[i]); > > please remove commented-out line Done. >> LayoutTests/webaudio/resources/convolution-testing.js:100 >> + //console.log("Max2 = " + tailMax2); > > please remove commented-out line Done. >> LayoutTests/webaudio/resources/convolution-testing.js:108 >> + var maxDeviationFraction = maxDelta / valueAtMaxDelta; > > Although we calculate this value, it doesn't look like we're checking it against a threshold to make sure the triangular portion is "within the zone" > This is probably the most important part of the test. Similar to lines 119:123 we should have either say "Triangular portion correctly convolved." > > Also, see line 119 where "success" is never changed to false anywhere in this code Fixed. >> LayoutTests/webaudio/resources/convolution-testing.js:117 >> + // } > > please remove commented-out lines Done. >> LayoutTests/webaudio/resources/convolution-testing.js:120 >> + testPassed("Test signal was correctly delayed."); > > Do we want the message to say "correctly delayed" or "correctly convolved"???? Changed to "correctly convolved"
Created attachment 121039 [details] Patch
Comment on attachment 121039 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121039&action=review Ray, this looks really good! I've added a couple of nits and some comments about re-arranging the order of a couple of tests (and adding one new one). > Source/WebCore/platform/audio/Reverb.cpp:97 > + if (normalize && scale) I would move this second if() up into the body of the first if() and just make this: if (scale) > Source/WebCore/platform/audio/Reverb.cpp:105 > + // place. typo: "isntead" nit: I'd also consider re-wrapping these comment lines to be more similar in length to the other lines in this file, some of which are quite long. > Source/WebCore/webaudio/ConvolverNode.h:57 > + // Normalize or not. Comment seems a little obvious and perhaps not needed. > LayoutTests/webaudio/convolution-mono-mono-expected.txt:7 > +PASS Triangular portion of convolution is correct The order of these tests seems a little odd. I would expect it would scan through the signal from start to finish and output test results in time order. There's an extra test would should be added to verify that the leading 128 sample-frames (due to latency) are zero. This test is important since garbage could potentially be there and we're not checking for it right now. I'd suggest tests in this order: 1. Initial part of signal is zero due to expected latency in processing. 2. Main convolution result is triangular. 3. First part of tail of convolution after triangular portion is very close to zero. 4. Rendered signal at end of convolution is silent. > LayoutTests/webaudio/convolution-mono-mono.html:25 > + This is a *really* great test. It would be nice to have just a few lines of comments here describing at a very high level what we're doing in this test. For example, it would be nice to say that we're convolving a square-pulse with another square-pulse and expect the result to be triangular. > LayoutTests/webaudio/resources/convolution-testing.js:9 > +// convolver. Please add: // https://bugs.webkit.org/show_bug.cgi?id=75564 > LayoutTests/webaudio/resources/convolution-testing.js:55 > + Please add test that initial (latency) 128 sample-frames are zero. > LayoutTests/webaudio/resources/convolution-testing.js:67 > + Please move line 91 and the actual 123:128 to this point in the file. Right now "allowedDeviationFraction" is lumped in with the other constants although it's part of a completely different test which can be done right here (for proper time order of tests). the "allowedDeviationFraction" declaration should have an explanatory comment about what it is and how the value was determined. > LayoutTests/webaudio/resources/convolution-testing.js:128 > + } Please see comments above about re-arranging time order of the tests.
Created attachment 121170 [details] Patch
Comment on attachment 121039 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121039&action=review >> Source/WebCore/platform/audio/Reverb.cpp:105 >> + // place. > > typo: "isntead" > > nit: I'd also consider re-wrapping these comment lines to be more similar in length to the other lines in this file, some of which are quite long. Typo fixed and these lines are approximately the same length as the first line instead of being much shorter. >> Source/WebCore/webaudio/ConvolverNode.h:57 >> + // Normalize or not. > > Comment seems a little obvious and perhaps not needed. Deleted. >> LayoutTests/webaudio/convolution-mono-mono-expected.txt:7 >> +PASS Triangular portion of convolution is correct > > The order of these tests seems a little odd. I would expect it would scan through the signal from start to finish and output test results in time order. > > There's an extra test would should be added to verify that the leading 128 sample-frames (due to latency) are zero. This test is important since garbage > could potentially be there and we're not checking for it right now. I'd suggest tests in this order: > > 1. Initial part of signal is zero due to expected latency in processing. > 2. Main convolution result is triangular. > 3. First part of tail of convolution after triangular portion is very close to zero. > 4. Rendered signal at end of convolution is silent. Tests have been rearranged to output the results in the order suggested. Test for the initial latency also added. >> LayoutTests/webaudio/convolution-mono-mono.html:25 >> + > > This is a *really* great test. It would be nice to have just a few lines of comments here describing at a very high level what we're doing in this test. > For example, it would be nice to say that we're convolving a square-pulse with another square-pulse and expect the result to be triangular. Done. >> LayoutTests/webaudio/resources/convolution-testing.js:9 >> +// convolver. > > Please add: > > // https://bugs.webkit.org/show_bug.cgi?id=75564 Done. >> LayoutTests/webaudio/resources/convolution-testing.js:55 >> + > > Please add test that initial (latency) 128 sample-frames are zero. Done. >> LayoutTests/webaudio/resources/convolution-testing.js:67 >> + > > Please move line 91 and the actual 123:128 to this point in the file. Right now "allowedDeviationFraction" is lumped in with the other constants > although it's part of a completely different test which can be done right here (for proper time order of tests). > > the "allowedDeviationFraction" declaration should have an explanatory comment about what it is and how the value was determined. Done. >> LayoutTests/webaudio/resources/convolution-testing.js:128 >> + } > > Please see comments above about re-arranging time order of the tests. Tests reordered as suggested.
Comment on attachment 121170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121170&action=review Looks good, but please fix indentation nit. > Source/WebCore/platform/audio/Reverb.cpp:95 > + scale = calculateNormalizationScale(impulseResponse); nit: indentation should be four spaces here instead of two
Created attachment 121182 [details] Patch
Comment on attachment 121170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121170&action=review >> Source/WebCore/platform/audio/Reverb.cpp:95 >> + scale = calculateNormalizationScale(impulseResponse); > > nit: indentation should be four spaces here instead of two Done.
Looks good.
Comment on attachment 121182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121182&action=review The code and test look good overall, but I think the test should undergo one more revision. Feel free to upload another patch. r=me > LayoutTests/webaudio/convolution-mono-mono.html:19 > +// produce a triangular pulse. We verify the result is correct we We verifiy -> To verify > LayoutTests/webaudio/resources/convolution-testing.js:152 > + } It may be worth factoring all of the code above this point into a separate function which returns a boolean indicating its success. That way you can use early returns to abort the test if any step goes wrong rather than setting the "success" variable in several places throughout the test (and executing code that's going to fail anyway if earlier portions of the test failed).
Comment on attachment 121182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121182&action=review >> LayoutTests/webaudio/resources/convolution-testing.js:152 >> + } > > It may be worth factoring all of the code above this point into a separate function which returns a boolean indicating its success. That way you can use early returns to abort the test if any step goes wrong rather than setting the "success" variable in several places throughout the test (and executing code that's going to fail anyway if earlier portions of the test failed). Putting these in a separate function is a good idea. I will do that. However, I think the only test worth exiting early is the initial latency test. If that's wrong, everything after will be wrong. The other tests have experimentally determined thresholds, so I think it's worth continuing since it's not much more work. Failing the triangle doesn't imply the tail will fail, and vice versa.
I think it's better to *not* exit early for any of the tests, since different combinations of failures might provide clues as to what the regression is. Failure in one test does not necessarily mean another test will fail.
(In reply to comment #18) > I think it's better to *not* exit early for any of the tests, since different combinations of failures might provide clues as to what the regression is. Failure in one test does not necessarily mean another test will fail. I leave this to your best judgment.
Created attachment 121348 [details] Patch
Comment on attachment 121182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121182&action=review >> LayoutTests/webaudio/convolution-mono-mono.html:19 >> +// produce a triangular pulse. We verify the result is correct we > > We verifiy -> To verify Fixed. >>> LayoutTests/webaudio/resources/convolution-testing.js:152 >>> + } >> >> It may be worth factoring all of the code above this point into a separate function which returns a boolean indicating its success. That way you can use early returns to abort the test if any step goes wrong rather than setting the "success" variable in several places throughout the test (and executing code that's going to fail anyway if earlier portions of the test failed). > > Putting these in a separate function is a good idea. I will do that. However, I think the only test worth exiting early is the initial latency test. If that's wrong, everything after will be wrong. The other tests have experimentally determined thresholds, so I think it's worth continuing since it's not much more work. Failing the triangle doesn't imply the tail will fail, and vice versa. Each test is now in its own function.
Hi Ray, sorry to pile on a last minute change, but can you have a look at my last comment in the IPP patch: https://bugs.webkit.org/show_bug.cgi?id=75522
Comment on attachment 121348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121348&action=review > LayoutTests/webaudio/resources/convolution-testing.js:93 > + testFailed("Triangular portion of convolution is not incorrect. Max deviation = " + maxDeviationFraction + " at " + maxDeltaIndex); "is not incorrect" -> "is not correct"
Created attachment 121447 [details] Patch
Comment on attachment 121348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121348&action=review >> LayoutTests/webaudio/resources/convolution-testing.js:93 >> + testFailed("Triangular portion of convolution is not incorrect. Max deviation = " + maxDeviationFraction + " at " + maxDeltaIndex); > > "is not incorrect" -> "is not correct" Fixed. Also, I've changed the thresholds so that the tests will pass on OSX. The thresholds and such are now in "dB" units, relative to the max of the triangular pulse.
(In reply to comment #25) > (From update of attachment 121348 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121348&action=review > > >> LayoutTests/webaudio/resources/convolution-testing.js:93 > >> + testFailed("Triangular portion of convolution is not incorrect. Max deviation = " + maxDeviationFraction + " at " + maxDeltaIndex); > > > > "is not incorrect" -> "is not correct" > > Fixed. > > Also, I've changed the thresholds so that the tests will pass on OSX. The thresholds and such are now in "dB" units, relative to the max of the triangular pulse. This test is failing on windows. According to the test, the triangular pulse is way off. Need to investigate why....
(In reply to comment #26) > > Also, I've changed the thresholds so that the tests will pass on OSX. The thresholds and such are now in "dB" units, relative to the max of the triangular pulse. > > This test is failing on windows. According to the test, the triangular pulse is way off. Need to investigate why.... That's strange. It should be going through the same FFmpeg FFT, running on same/similar hardware as the Linux one
Comment on attachment 121447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121447&action=review Looks good to me if these comments are addressed. I suspect the problems you're seeing on Windows may be related to a local build issue, and think we should get this patch landed. If you still have your doubts about Windows, then you can also an appropriate line to LayoutTests/platform/chromium/test_expectations.txt and include that as part of this patch. > LayoutTests/webaudio/resources/convolution-testing.js:69 > +function db20(x) { This name is quite odd. Please use something like "linearToDecibels()" which is the name we use in platform/audio/AudioUtilities.h > LayoutTests/webaudio/resources/convolution-testing.js:96 > + var maxDeviationFraction = db20(maxDelta / valueAtMaxDelta); These variables should now be named "allowedDeviationDecibels" and "maxDeviationDecibels" since you have changed the units of comparison
Comment on attachment 121447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121447&action=review > LayoutTests/webaudio/resources/convolution-testing.js:95 > + var allowedDeviationFraction = db20(2.1e-7); Please see comment below about "express empirical constant directly in decibels" > LayoutTests/webaudio/resources/convolution-testing.js:130 > + var threshold1 = db20(0.002/refMax); .002 needs to be adjusted slightly higher so that the IPP code will pass. Also, I was hoping you would define this empirical constant directly in decibels, instead of converting from a scalar value. That way we can tell at a glance "how far down in the noise" in dB we can tolerate. In other words, express this as a hard-coded constant in dB. As it now stands the constant 0.002 is only in relation to refMax, so it's hard to say what this means.
Created attachment 121503 [details] Patch
Comment on attachment 121447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121447&action=review >> LayoutTests/webaudio/resources/convolution-testing.js:69 >> +function db20(x) { > > This name is quite odd. Please use something like "linearToDecibels()" which is the name we use in platform/audio/AudioUtilities.h Changed. >> LayoutTests/webaudio/resources/convolution-testing.js:96 >> + var maxDeviationFraction = db20(maxDelta / valueAtMaxDelta); > > These variables should now be named "allowedDeviationDecibels" and "maxDeviationDecibels" since you have changed the units of comparison Done. >> LayoutTests/webaudio/resources/convolution-testing.js:130 >> + var threshold1 = db20(0.002/refMax); > > .002 needs to be adjusted slightly higher so that the IPP code will pass. Also, I was hoping you would define this empirical constant directly in decibels, instead of converting from a scalar value. > That way we can tell at a glance "how far down in the noise" in dB we can tolerate. In other words, express this as a hard-coded constant in dB. As it now stands the constant 0.002 is only in relation to refMax, > so it's hard to say what this means. I prefer to have the IPP patch update this with a new value instead of doing it now. But the new thresholds are now just numbers instead of conversions to dB.
Comment on attachment 121503 [details] Patch Fine with me if it's fine with Chris, but this patch must be rebaselined against TOT; note the EWS failures.
Created attachment 121517 [details] Patch
(In reply to comment #33) > (From update of attachment 121503 [details]) > Fine with me if it's fine with Chris, but this patch must be rebaselined against TOT; note the EWS failures. Rebaseline done. I also removed the change to test_expectations.txt as recommended by Chris. We're hoping for the best on Windows.
Comment on attachment 121517 [details] Patch EWS results look good. rs=me
Comment on attachment 121517 [details] Patch Clearing flags on attachment: 121517 Committed r104476: <http://trac.webkit.org/changeset/104476>
All reviewed patches have been landed. Closing bug.
Comment on attachment 121182 [details] Patch Cleared review? from obsolete attachment 121182 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment on attachment 121348 [details] Patch Cleared review? from obsolete attachment 121348 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
(In reply to comment #37) > (From update of attachment 121517 [details]) > Clearing flags on attachment: 121517 > > Committed r104476: <http://trac.webkit.org/changeset/104476> Just a note, WebAudio tests need to override the WebKitWebAudioEnabled LayoutTestController preference, it has to be set to 1. It's easy to forget when writing new tests it seems :(
(In reply to comment #41) > (In reply to comment #37) > > (From update of attachment 121517 [details] [details]) > > Clearing flags on attachment: 121517 > > > > Committed r104476: <http://trac.webkit.org/changeset/104476> > > Just a note, WebAudio tests need to override the WebKitWebAudioEnabled LayoutTestController preference, it has to be set to 1. It's easy to forget when writing new tests it seems :( Thanks for the info. I didn't know that. See https://bugs.webkit.org/show_bug.cgi?id=76066.