Bug 82200 – Reset paramters when BiquadFilterNode.type get new value.

• Component: Web Audio
• Status: RESOLVED INVALID

[Gao Chun]

When BiquadFilterNode.type changed, the Q and gain paramters should be set with new names and ranges.

[Gao Chun]

Created attachment 133835 [details]
Patch

[Chris Rogers]

Comment on attachment 133835 [details]
Patch

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

Hi Chun, thanks for the patch.  I don't think we should change the AudioParams in this way, but a good change would be to remove the deprecated filter nodes.

> Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:-48
> -    m_parameter3 = AudioParam::create("gain", 0.0, -40, 40);

The attributes for BiquadFilterNode are defined in the spec as "frequency", "Q", "gain", so I think we shouldn't remove this code here, even though for some types "Q" is unused.

> Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:-76
> -    }

I think it may be a good time to think about removing these old/deprecated AudioNodes (LowPass2FilterNode and HighPass2FilterNode).  There have been warning messages for many months.  But that should probably be in a different patch called "Remove deprecated LowPass2FilterNode and HighPass2FilterNode"

> Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:157
> +}

I don't think we should re-create these AudioParams -- instead just create in the normal constructor and leave named the same always

[Gao Chun]

(In reply to comment #2)
> (From update of attachment 133835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133835&action=review
> 
> Hi Chun, thanks for the patch.  I don't think we should change the AudioParams in this way, but a good change would be to remove the deprecated filter nodes.
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:-48
> > -    m_parameter3 = AudioParam::create("gain", 0.0, -40, 40);
> 
> The attributes for BiquadFilterNode are defined in the spec as "frequency", "Q", "gain", so I think we shouldn't remove this code here, even though for some types "Q" is unused.
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:-76
> > -    }
> 
> I think it may be a good time to think about removing these old/deprecated AudioNodes (LowPass2FilterNode and HighPass2FilterNode).  There have been warning messages for many months.  But that should probably be in a different patch called "Remove deprecated LowPass2FilterNode and HighPass2FilterNode"
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:157
> > +}
> 
> I don't think we should re-create these AudioParams -- instead just create in the normal constructor and leave named the same always


(In reply to comment #2)
> (From update of attachment 133835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133835&action=review
> 
> Hi Chun, thanks for the patch.  I don't think we should change the AudioParams in this way, but a good change would be to remove the deprecated filter nodes.
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:-48
> > -    m_parameter3 = AudioParam::create("gain", 0.0, -40, 40);
> 
> The attributes for BiquadFilterNode are defined in the spec as "frequency", "Q", "gain", so I think we shouldn't remove this code here, even though for some types "Q" is unused.
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:-76
> > -    }
> 
> I think it may be a good time to think about removing these old/deprecated AudioNodes (LowPass2FilterNode and HighPass2FilterNode).  There have been warning messages for many months.  But that should probably be in a different patch called "Remove deprecated LowPass2FilterNode and HighPass2FilterNode"
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:157
> > +}
> 
> I don't think we should re-create these AudioParams -- instead just create in the normal constructor and leave named the same always

Hi Chris, thanks for your suggestion, I'll mark this bug as invalid and fire a new one to remove LowPass2FilterNode and HighPass2FilterNode.

[Eric Seidel (no email)]

Comment on attachment 133835 [details]
Patch

Cleared review? from attachment 133835 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).