Bug 62078 - Implement BiquadFilterNode for filter types: LOWPASS, HIGHPASS, BANDPASS, LOWSHELF, HIGHSHELF, PEAKING, NOTCH, ALLPASS
Summary: Implement BiquadFilterNode for filter types: LOWPASS, HIGHPASS, BANDPASS, LOW...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-03 17:53 PDT by Chris Rogers
Modified: 2011-06-08 13:30 PDT (History)
2 users (show)

See Also:

Attachments
Patch (36.10 KB, patch)
2011-06-03 18:06 PDT, Chris Rogers 		kbr: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-06-03 17:53:54 PDT 
Implement BiquadFilterNode for filter types: LOWPASS, HIGHPASS, BANDPASS, LOWSHELF, HIGHSHELF, PEAKING, NOTCH, ALLPASS
Comment 1 Chris Rogers 2011-06-03 18:06:39 PDT 
Created attachment 96002 [details]
Patch
Comment 2 Chris Rogers 2011-06-03 18:11:01 PDT 
See filter cookbook reference:
http://www.musicdsp.org/files/Audio-EQ-Cookbook.txt

For newly implemented filter types.
Comment 3 Kenneth Russell 2011-06-08 12:00:19 PDT 
Comment on attachment 96002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96002&action=review

I'm not qualified to review the math for the coefficients but have a couple of minor comments.

Also, when are you going to delete the LowPass2FilterNode and HighPass2FilterNode, or at least implement them in terms of the Biquad2FilterNode? If they're deprecated then their construction should probably log a warning message to the console that they will be removed soon. There are not enough users of the Web Audio API at this point to warrant keeping them around permanently.

> Source/WebCore/platform/audio/Biquad.cpp:230
> +    double a0Inverse = 1.0 / a0;

All of the .0's in this file seem unnecessary and so should be removed per WebKit style.

> Source/WebCore/platform/audio/Biquad.cpp:250
> +    // FIXME: optimize the common terms

Why not at least define aPlusOne and aMinusOne now rather than adding this FIXME?

> Source/WebCore/platform/audio/Biquad.cpp:272
> +    // FIXME: optimize the common terms

Same question about common terms.
Comment 4 Chris Rogers 2011-06-08 13:29:23 PDT 
Committed r88380: <http://trac.webkit.org/changeset/88380>
Comment 5 Chris Rogers 2011-06-08 13:30:52 PDT 
Comment on attachment 96002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96002&action=review

I also added console warnings for deprecated APIs

>> Source/WebCore/platform/audio/Biquad.cpp:230
>> +    double a0Inverse = 1.0 / a0;
> 
> All of the .0's in this file seem unnecessary and so should be removed per WebKit style.

FIXED

>> Source/WebCore/platform/audio/Biquad.cpp:250
>> +    // FIXME: optimize the common terms
> 
> Why not at least define aPlusOne and aMinusOne now rather than adding this FIXME?

FIXED

>> Source/WebCore/platform/audio/Biquad.cpp:272
>> +    // FIXME: optimize the common terms
> 
> Same question about common terms.

FIXED