Bug 215810 – IIRFilterNode interface is not supported

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

IIRFilterNode interface is not supported:
- https://www.w3.org/TR/webaudio/#iirfilternode

[Chris Dumez]

Created attachment 407224 [details]
Patch

[Chris Dumez]

*** Bug 181195 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 407228 [details]
Patch

[Darin Adler]

Comment on attachment 407228 [details]
Patch

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

> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:37
> +    m_tailTime = m_iirFilter.tailTime(processor.sampleRate(), processor.isFilterStable());

Why not use construction syntax for this?

> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
> +    Vector<float> frequency(length);

Inline capacity for better performance? How long are these likely to be?

> Source/WebCore/Modules/webaudio/IIRDSPKernel.h:45
> +    // AudioDSPKernel
> +    void process(const float* source, float* destination, size_t framesToProcess) final;
> +    double tailTime() const final { return m_tailTime; }
> +    double latencyTime() const final { return 0; }
> +    void reset() final;

Make these private?

> Source/WebCore/platform/audio/IIRFilter.cpp:37
> +const size_t IIRFilter::maxOrder = 20;
> +const unsigned iirRenderingQuantum = 128;

constexpr

> Source/WebCore/platform/audio/IIRFilter.cpp:41
> +const int bufferLength = 32;

constexpr

> Source/WebCore/platform/audio/IIRFilter.cpp:148
> +            std::complex<double> zRecip = std::complex<double>(cos(omega), sin(omega));

auto

> Source/WebCore/platform/audio/IIRFilter.cpp:152
> +            std::complex<double> numerator = evaluatePolynomial(m_feedforward.data(), zRecip, m_feedforward.size() - 1);
> +            std::complex<double> denominator = evaluatePolynomial(m_feedback.data(), zRecip, m_feedback.size() - 1);
> +            std::complex<double> response = numerator / denominator;

auto

> Source/WebCore/platform/audio/IIRFilter.cpp:164
> +    const double maxTailTime = 10;

constexpr

> Source/WebCore/platform/audio/IIRFilter.cpp:169
> +    const float maxTailAmplitude = 1 / 32768.0;

constexpr

> Source/WebCore/platform/audio/IIRFilter.h:65
> +    const Vector<double>& m_feedforward;
> +    const Vector<double>& m_feedback;

Seems like a dangerous design. What guarantees the lifetime fo the vectors?

[Chris Dumez]

Created attachment 407264 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 407228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407228&action=review
> 
> > Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:37
> > +    m_tailTime = m_iirFilter.tailTime(processor.sampleRate(), processor.isFilterStable());
> 
> Why not use construction syntax for this?
> 
> > Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
> > +    Vector<float> frequency(length);
> 
> Inline capacity for better performance? How long are these likely to be?
> 
> > Source/WebCore/Modules/webaudio/IIRDSPKernel.h:45
> > +    // AudioDSPKernel
> > +    void process(const float* source, float* destination, size_t framesToProcess) final;
> > +    double tailTime() const final { return m_tailTime; }
> > +    double latencyTime() const final { return 0; }
> > +    void reset() final;
> 
> Make these private?
> 
> > Source/WebCore/platform/audio/IIRFilter.cpp:37
> > +const size_t IIRFilter::maxOrder = 20;
> > +const unsigned iirRenderingQuantum = 128;
> 
> constexpr
> 
> > Source/WebCore/platform/audio/IIRFilter.cpp:41
> > +const int bufferLength = 32;
> 
> constexpr
> 
> > Source/WebCore/platform/audio/IIRFilter.cpp:148
> > +            std::complex<double> zRecip = std::complex<double>(cos(omega), sin(omega));
> 
> auto
> 
> > Source/WebCore/platform/audio/IIRFilter.cpp:152
> > +            std::complex<double> numerator = evaluatePolynomial(m_feedforward.data(), zRecip, m_feedforward.size() - 1);
> > +            std::complex<double> denominator = evaluatePolynomial(m_feedback.data(), zRecip, m_feedback.size() - 1);
> > +            std::complex<double> response = numerator / denominator;
> 
> auto
> 
> > Source/WebCore/platform/audio/IIRFilter.cpp:164
> > +    const double maxTailTime = 10;
> 
> constexpr
> 
> > Source/WebCore/platform/audio/IIRFilter.cpp:169
> > +    const float maxTailAmplitude = 1 / 32768.0;
> 
> constexpr
> 
> > Source/WebCore/platform/audio/IIRFilter.h:65
> > +    const Vector<double>& m_feedforward;
> > +    const Vector<double>& m_feedback;
> 
> Seems like a dangerous design. What guarantees the lifetime fo the vectors?

I agree it is a little dangerous. It is a memory optimization. The IIRProcessor owns those vectors. The IIRProcessor also owns the IIRDSPKernel objects, each of which has an internal IIRFilter. Those objects are not refcounted. Therefore, the IIRFilter cannot outlive its IIRDSPKernel, which cannot outlive its IIRFilter. The patch is safe as it is and avoids having multiple copies of the Vector in each kernel/filter.

[Chris Dumez]

Comment on attachment 407228 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
>> +    Vector<float> frequency(length);
> 
> Inline capacity for better performance? How long are these likely to be?

anywhere between 1 and 20 entries per vector.

[Chris Dumez]

Comment on attachment 407228 [details]
Patch

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

>>> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
>>> +    Vector<float> frequency(length);
>> 
>> Inline capacity for better performance? How long are these likely to be?
> 
> anywhere between 1 and 20 entries per vector.

Do you think the following would be better?
Vector<float, 20> frequency(length);

[Chris Dumez]

Created attachment 407302 [details]
Patch

[Chris Dumez]

Created attachment 407303 [details]
Patch

[Chris Dumez]

Created attachment 407307 [details]
Patch

[Darin Adler]

Comment on attachment 407228 [details]
Patch

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

>>>> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
>>>> +    Vector<float> frequency(length);
>>> 
>>> Inline capacity for better performance? How long are these likely to be?
>> 
>> anywhere between 1 and 20 entries per vector.
> 
> Do you think the following would be better?
> Vector<float, 20> frequency(length);

Yes, much better.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> Comment on attachment 407228 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407228&action=review
> 
> >>>> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
> >>>> +    Vector<float> frequency(length);
> >>> 
> >>> Inline capacity for better performance? How long are these likely to be?
> >> 
> >> anywhere between 1 and 20 entries per vector.
> > 
> > Do you think the following would be better?
> > Vector<float, 20> frequency(length);
> 
> Yes, much better.

Actually, my bad. I got confused with feedforward and feedback array. This one is provided by script and may be of ANY side. It is the input array and we need to compute the frequency response for each value in the array. As a result, I don't think we should use inline capacity here.

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Darin Adler from comment #12)
> > Comment on attachment 407228 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=407228&action=review
> > 
> > >>>> Source/WebCore/Modules/webaudio/IIRDSPKernel.cpp:46
> > >>>> +    Vector<float> frequency(length);
> > >>> 
> > >>> Inline capacity for better performance? How long are these likely to be?
> > >> 
> > >> anywhere between 1 and 20 entries per vector.
> > > 
> > > Do you think the following would be better?
> > > Vector<float, 20> frequency(length);
> > 
> > Yes, much better.
> 
> Actually, my bad. I got confused with feedforward and feedback array. This
> one is provided by script and may be of ANY side. It is the input array and

any *size*.

> we need to compute the frequency response for each value in the array. As a
> result, I don't think we should use inline capacity here.

[Darin Adler]

Sure, I assumed it might be of any size. I was asking what it is *likely* to be.

Inline capacity is designed so that it can be used even when the input can be of any size. It saves allocation/deallocation any time the size is small enough, and heap is still used any time the size is higher. It does cost greater stack depth, and the memory use is greater when the stack is wasted, but that’s really the only tradeoff other than it being costly to move such vectors around.

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> Sure, I assumed it might be of any size. I was asking what it is *likely* to
> be.
> 
> Inline capacity is designed so that it can be used even when the input can
> be of any size. It saves allocation/deallocation any time the size is small
> enough, and heap is still used any time the size is higher. It does cost
> greater stack depth, and the memory use is greater when the stack is wasted,
> but that’s really the only tradeoff other than it being costly to move such
> vectors around.

Understood. However, I am more convinced it is more likely to be any given size, at least not to my knowledge.

[Chris Dumez]

Created attachment 407311 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #17)
> Created attachment 407311 [details]
> Patch

Trying to get build fixed on Windows.

[Chris Dumez]

Created attachment 407315 [details]
Patch

[EWS]

Committed r266186: <https://trac.webkit.org/changeset/266186>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407315 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67826590>