Bug 71413 - Check parameters to biquad filters
Summary: Check parameters to biquad filters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on: 76659
Blocks: 71391
  Show dependency treegraph
 
Reported: 2011-11-02 15:56 PDT by Raymond Toy
Modified: 2012-02-02 20:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (15.41 KB, patch)
2011-11-07 11:38 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (50.05 KB, patch)
2012-01-17 18:46 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (50.98 KB, patch)
2012-01-18 15:54 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (51.11 KB, patch)
2012-01-18 20:14 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (50.98 KB, patch)
2012-02-01 11:19 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (50.97 KB, patch)
2012-02-01 13:31 PST, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond Toy 2011-11-02 15:56:00 PDT
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.
Comment 1 Raymond Toy 2011-11-07 11:38:31 PST
Created attachment 113901 [details]
Patch
Comment 2 Raymond Toy 2011-11-07 11:44:04 PST
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>.
Comment 3 Raymond Toy 2012-01-17 18:46:48 PST
Created attachment 122861 [details]
Patch
Comment 4 Chris Rogers 2012-01-18 12:46:00 PST
(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 5 Chris Rogers 2012-01-18 13:26:22 PST
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 6 Raymond Toy 2012-01-18 14:28:46 PST
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.)
Comment 7 Raymond Toy 2012-01-18 15:54:58 PST
Created attachment 123028 [details]
Patch
Comment 8 Raymond Toy 2012-01-18 16:06:22 PST
All comments should be addressed now.
Comment 9 Chris Rogers 2012-01-18 17:09:24 PST
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 10 WebKit Review Bot 2012-01-18 17:59:21 PST
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
Comment 11 Raymond Toy 2012-01-18 20:14:18 PST
Created attachment 123059 [details]
Patch
Comment 12 Raymond Toy 2012-01-18 20:15:52 PST
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.
Comment 13 Raymond Toy 2012-01-23 09:23:01 PST
Wait for bug 76659 to land before landing this patch.
Comment 14 Raymond Toy 2012-02-01 11:19:04 PST
Created attachment 124977 [details]
Patch
Comment 15 Chris Rogers 2012-02-01 13:14:26 PST
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"
Comment 16 Raymond Toy 2012-02-01 13:31:18 PST
Created attachment 125004 [details]
Patch
Comment 17 Raymond Toy 2012-02-01 13:31:55 PST
Fixed the two nits.
Comment 18 WebKit Review Bot 2012-02-01 13:45:29 PST
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.
Comment 19 Chris Rogers 2012-02-01 14:16:11 PST
Looks good.

Sorry, I'm not a reviewer yet.  Ken could you please have a look?
Comment 20 Kenneth Russell 2012-02-02 17:53:14 PST
Comment on attachment 125004 [details]
Patch

Nice tests. rs=me
Comment 21 WebKit Review Bot 2012-02-02 20:09:07 PST
Comment on attachment 125004 [details]
Patch

Clearing flags on attachment: 125004

Committed r106621: <http://trac.webkit.org/changeset/106621>
Comment 22 WebKit Review Bot 2012-02-02 20:09:14 PST
All reviewed patches have been landed.  Closing bug.