Bug 71413

Created attachment 113901 [details] Patch

Check that the parameters to the biquads don't cause invalid biquad settings. The parameters are clipped to the limits. If the limit values cause problems, the limit of the z-transform is used to determine the biquad coefficients. Not all problems can be handled consistently though because the z-transform is not continuous in those cases. We choose some possible limits. The best way to see the effects is to run the mag-phase demo <http://www.corp.google.com/~rtoy/mag-phase.html>.

Created attachment 122861 [details] Patch

(In reply to comment #2) > Check that the parameters to the biquads don't cause invalid biquad settings. The parameters are clipped to the limits. If the limit values cause problems, the limit of the z-transform is used to determine the biquad coefficients. Not all problems can be handled consistently though because the z-transform is not continuous in those cases. We choose some possible limits. > > The best way to see the effects is to run the mag-phase demo <http://www.corp.google.com/~rtoy/mag-phase.html>. Nice demo! We need to host this somewhere where non-Google people can access. One feature request I have is that the frequency axis be on a log-scale, with marks at 20Hz, 40, 80, 160, ...

Comment on attachment 122861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122861&action=review Tests are looking good. I think that later we'll want to add some more which generate actual .wav files we can listen to -- as a different and complementary testing approach. But this should hold us for a long time... > Source/WebCore/platform/audio/Biquad.cpp:197 > + if (cutoff == 1.0) { nit: 1.0 -> 1 > Source/WebCore/platform/audio/Biquad.cpp:198 > + // When cutoff is 1, the z-transform is 1. nit: indentation here is 2 but should be 4 - please correct here and other places > Source/WebCore/platform/audio/Biquad.cpp:199 > + m_b0 = 1.0; nit: 1.0 -> 1 please correct here and other similar places > Source/WebCore/platform/audio/Biquad.cpp:292 > + setNormalizedCoefficients(A*A, 0.0, 0.0, nit: A*A -> A * A , 0.0 -> 0, etc. > Source/WebCore/platform/audio/Biquad.cpp:369 > + if ((frequency > 0) && (frequency < 1)) { nit: extra layer of parenthesis unnecessary - please correct here and in similar places > Source/WebCore/platform/audio/Biquad.cpp:406 > + if ((frequency > 0) && (frequency < 1)) { nit: extra layer of parenthesis > LayoutTests/webaudio/biquad-allpass.html:31 > + var maxAllowedError = 4.0e-8; Please don't define separate threshold values in each .html file. Pick one single value for all the tests and manage it in the privacy of the .js file Otherwise, it's going to be a maintenance nightmare to have to manage the eight (or so) different .html thresholds as separate values when adjusting for different ports, etc. > LayoutTests/webaudio/biquad-test-expected.txt:6 > +FAIL Lowpass filter response is incorrect. Max err = 0.8619335505499499 Test is FAIL? Did you intend to include this file "biquad-test-expected.txt" or is it something old which should be removed? > LayoutTests/webaudio/resources/biquad-testing.js:12 > +// essentially zero. I understand what you mean, but can you re-word a little with some more detail, explaining that this value was chosen such that the "tail-time" of each filtered impulse is effectively contained within this time? You might need to explain that we're rendering end-end several different impulse responses of a few test-cases, and want to have "enough" space between them. > LayoutTests/webaudio/resources/biquad-testing.js:17 > +var maxFilters = 5; It seems like we should do a sanity check (down below) to make sure that the numbers of requested filters fits within this range. > LayoutTests/webaudio/resources/biquad-testing.js:26 > +// H(z) = (b0 + b1/z + b2/z^2)/(1 + a1/z+a2/z^2) nit: spacing > LayoutTests/webaudio/resources/biquad-testing.js:27 > + Please add a comment with URL for the reference *where* we're getting all the equations for the different filter types. I think it's this link: http://www.musicdsp.org/files/Audio-EQ-Cookbook.txt > LayoutTests/webaudio/resources/biquad-testing.js:396 > +function createTestAndRun(context, filterType, filterParameter, threshold) { "filterParameter" -> "filterParameters" (with an 's') since this is a list Please change here and everywhere else in the file > LayoutTests/webaudio/resources/biquad-testing.js:450 > + // Accumulate this filtered data into the final output. nit: can you say "into the final output" -> "into the final output at the desired offset" > LayoutTests/webaudio/resources/biquad-testing.js:460 > + renderedData = renderedBuffer.getChannelData(0); Is there anyway you can run through the renderedData and check for NAN and INF as an additional test?

Comment on attachment 122861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122861&action=review >> LayoutTests/webaudio/resources/biquad-testing.js:27 >> + > > Please add a comment with URL for the reference *where* we're getting all the equations for the different filter types. > > I think it's this link: > http://www.musicdsp.org/files/Audio-EQ-Cookbook.txt Can you provide the reference for your book? Recall that not all of the filters are from the audio eq cookbook. (I have a patch for that as well, but it's out of date and we never decided on exactly how to roll out that change.)

Created attachment 123028 [details] Patch

All comments should be addressed now.

Comment on attachment 123028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123028&action=review Ray, thanks for adding the NaN and INF checks. Just a couple of nits. > Source/WebCore/platform/audio/Biquad.cpp:240 > + // The z-transform is 0. nit: indentation is off - please fix here and verify whole patch > LayoutTests/webaudio/resources/biquad-testing.js:56 > + var d = Math.sqrt((4 - Math.sqrt(16 - 16 / g / g)) / 2); 16 / g / g ? This is confusing when comparing with implementation which is 16 / (g*g) > LayoutTests/webaudio/resources/biquad-testing.js:59 > + var beta = 1/2 * (1 - sn) / (1 + sn); Confusing when comparing equations to C++ versions -- here we use 1/2, but use 0.5* in the C++ code -- can't we make the JS consistent (as much as possible) for people's sanity when trying to track down differences > LayoutTests/webaudio/resources/biquad-testing.js:222 > + var Am1 = A - 1; Can we have consistent spacing. For example, above on line 187 there's a space in almost identical code > LayoutTests/webaudio/resources/biquad-testing.js:328 > + b1 = -2*k; nit: spacing > LayoutTests/webaudio/resources/biquad-testing.js:424 > + nit: extra blank line > LayoutTests/webaudio/resources/biquad-testing.js:504 > + nit: extra blank line

Comment on attachment 123028 [details] Patch Attachment 123028 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11150432 New failing tests: webaudio/biquad-peaking.html

Created attachment 123059 [details] Patch

Fixed the comments for the previous patch and also fixed a failure of the tests on Linux (threshold was a little bit too small). Tests on windows fail because of an issue with noteOn.

Wait for bug 76659 to land before landing this patch.

Created attachment 124977 [details] Patch

Comment on attachment 124977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124977&action=review Looks good overall - just a couple of final nits. > Source/WebCore/platform/audio/Biquad.cpp:204 > + nit: extra blank line > Source/WebCore/platform/audio/Biquad.cpp:247 > + // Compute biquad coefficients for highpass filter nit: period at end of sentence > LayoutTests/webaudio/resources/biquad-testing.js:184 > + var alpha = 1 / 2 * Math.sin(w0) * Math.sqrt((A + 1 / A) * (1 / S - 1)+2); nit: need spaces ")+2" -> ") + 2"

Created attachment 125004 [details] Patch

Fixed the two nits.

Comment on attachment 125004 [details] Patch Rejecting attachment 125004 [details] from review queue. crogers@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.

Looks good. Sorry, I'm not a reviewer yet. Ken could you please have a look?

Comment on attachment 125004 [details] Patch Nice tests. rs=me

Comment on attachment 125004 [details] Patch Clearing flags on attachment: 125004 Committed r106621: <http://trac.webkit.org/changeset/106621>

All reviewed patches have been landed. Closing bug.