Add BiquadProcessor files
Created attachment 68769 [details] Patch
BiquadProcessor is used by the various filter AudioNode objects such as LowPass2FilterNode
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.
Created attachment 69707 [details] Patch
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.
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.
Created attachment 69720 [details] Patch
FIXED last nit
Comment on attachment 69720 [details] Patch Looks good.
Comment on attachment 69720 [details] Patch Clearing flags on attachment: 69720 Committed r69091: <http://trac.webkit.org/changeset/69091>
All reviewed patches have been landed. Closing bug.