Bug 46527 – Add BiquadProcessor files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add BiquadProcessor files

[Chris Rogers]

Created attachment 68769 [details]
Patch

[Chris Rogers]

BiquadProcessor is used by the various filter AudioNode objects such as LowPass2FilterNode

[Kenneth Russell]

Comment on attachment 68769 [details]
Patch

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

Several issues. Also please update the license text per offline discussion.

> WebCore/webaudio/BiquadProcessor.cpp:58
> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

These are the same parameters as LowPass2. Something looks wrong.

> WebCore/webaudio/BiquadProcessor.cpp:68
> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

These parameters look the same as those for Peaking aside from m_parameter3 being "unused" instead of "gain". I don't know what the right values should be but please double-check both.

> WebCore/webaudio/BiquadProcessor.cpp:73
> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

Two assignments to m_parameter3 and none to m_parameter2.

> WebCore/webaudio/BiquadProcessor.cpp:78
> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);

Two assignments to m_parameter3 and none to m_parameter2.

> WebCore/webaudio/BiquadProcessor.h:65
> +    AudioParam& parameter3() { return *m_parameter3; }

Especially since subclasses take the addresses of these references, I think you should return pointers here. You can indicate in the documentation that the BiquadProcessor owns the storage for the parameters.

> WebCore/webaudio/BiquadProcessor.h:70
> +    FilterType m_type; // lowpass, highpass, etc.

As many of these protected fields should be made private as possible. Also, this comment is unnecessary because the FilterType enum is self-documenting.

[Chris Rogers]

Created attachment 69707 [details]
Patch

[Chris Rogers]

Comment on attachment 68769 [details]
Patch

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

Also updated LICENSE

>> WebCore/webaudio/BiquadProcessor.cpp:58
>> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);
> 
> These are the same parameters as LowPass2. Something looks wrong.

This should be OK.  They do have the same parameters.

>> WebCore/webaudio/BiquadProcessor.cpp:68
>> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);
> 
> These parameters look the same as those for Peaking aside from m_parameter3 being "unused" instead of "gain". I don't know what the right values should be but please double-check both.

Yes, these ones were not quite right.  I've fixed peaking to use the standard three parameters: frequency, gain, Q
and for Allpass: frequency, Q

>> WebCore/webaudio/BiquadProcessor.cpp:73
>> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);
> 
> Two assignments to m_parameter3 and none to m_parameter2.

FIXED

>> WebCore/webaudio/BiquadProcessor.cpp:78
>> +        m_parameter3 = AudioParam::create("unused", 0.0, 0.0, 1.0);
> 
> Two assignments to m_parameter3 and none to m_parameter2.

FIXED

>> WebCore/webaudio/BiquadProcessor.h:65
>> +    AudioParam& parameter3() { return *m_parameter3; }
> 
> Especially since subclasses take the addresses of these references, I think you should return pointers here. You can indicate in the documentation that the BiquadProcessor owns the storage for the parameters.

FIXED

>> WebCore/webaudio/BiquadProcessor.h:70
>> +    FilterType m_type; // lowpass, highpass, etc.
> 
> As many of these protected fields should be made private as possible. Also, this comment is unnecessary because the FilterType enum is self-documenting.

All made private.

[Kenneth Russell]

Comment on attachment 69707 [details]
Patch

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

Thanks for addressing the earlier review feedback. One more issue.

> WebCore/webaudio/BiquadProcessor.cpp:51
> +    case HighPass2:

Since it sounds like these share the same coefficients and how the biquad processor is set up (for a low-pass vs. high-pass filter) is handled at a lower level, please fold these two switch arms together and consider adding a comment that they share the same parameter values, only differing in type.

[Chris Rogers]

Created attachment 69720 [details]
Patch

[Chris Rogers]

FIXED last nit

[Kenneth Russell]

Comment on attachment 69720 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 69720 [details]
Patch

Clearing flags on attachment: 69720

Committed r69091: <http://trac.webkit.org/changeset/69091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.