Check parameters to biquad filters

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

The parameters to the biquad filters should be checked more carefully.  In particular, if the cutoff frequency becomes 0 or if the Q factor becomes zero, the filter can become unstable.  This can cause audio output to stop.  We should also check if the frequency exceeds the Nyquist frequency and clip it to that.

[Raymond Toy]

Created attachment 113901 [details]
Patch

[Raymond Toy]

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>.

[Raymond Toy]

Created attachment 122861 [details]
Patch

[Chris Rogers]

(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, ...

[Chris Rogers]

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?

[Raymond Toy]

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.)

[Raymond Toy]

Created attachment 123028 [details]
Patch

[Raymond Toy]

All comments should be addressed now.

[Chris Rogers]

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

[WebKit Review Bot]

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

[Raymond Toy]

Created attachment 123059 [details]
Patch

[Raymond Toy]

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.

[Raymond Toy]

Wait for bug 76659 to land before landing this patch.

[Raymond Toy]

Created attachment 124977 [details]
Patch

[Chris Rogers]

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"

[Raymond Toy]

Created attachment 125004 [details]
Patch

[Raymond Toy]

Fixed the two nits.

[WebKit Review Bot]

Comment on attachment 125004 [details]
Patch

Rejecting attachment 125004 [details] from review queue.

[e-mail] 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.

[Chris Rogers]

Looks good.

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

[Kenneth Russell]

Comment on attachment 125004 [details]
Patch

Nice tests. rs=me

[WebKit Review Bot]

Comment on attachment 125004 [details]
Patch

Clearing flags on attachment: 125004

Committed r106621: <http://trac.webkit.org/changeset/106621>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.