Bug 215810 - IIRFilterNode interface is not supported
Summary: IIRFilterNode interface is not supported
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://www.w3.org/TR/webaudio/#iirfi...
Keywords: InRadar
: 181195 (view as bug list)
Depends on:
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-08-25 08:48 PDT by Chris Dumez
Modified: 2020-08-26 13:40 PDT (History)
29 users (show)

See Also:


Attachments
Patch (121.85 KB, patch)
2020-08-25 13:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (122.76 KB, patch)
2020-08-25 13:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (217.03 KB, patch)
2020-08-25 20:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (123.03 KB, patch)
2020-08-26 09:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (123.39 KB, patch)
2020-08-26 09:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (123.42 KB, patch)
2020-08-26 09:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (123.44 KB, patch)
2020-08-26 10:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (123.49 KB, patch)
2020-08-26 11:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-08-25 08:48:41 PDT
IIRFilterNode interface is not supported:
- https://www.w3.org/TR/webaudio/#iirfilternode
Comment 1 Chris Dumez 2020-08-25 13:34:52 PDT
Created attachment 407224 [details]
Patch
Comment 2 Chris Dumez 2020-08-25 13:38:48 PDT
*** Bug 181195 has been marked as a duplicate of this bug. ***
Comment 3 Chris Dumez 2020-08-25 13:56:35 PDT
Created attachment 407228 [details]
Patch
Comment 4 Darin Adler 2020-08-25 14:22:10 PDT
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?
Comment 5 Chris Dumez 2020-08-25 20:00:49 PDT
Created attachment 407264 [details]
Patch
Comment 6 Chris Dumez 2020-08-26 09:01:17 PDT
(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.
Comment 7 Chris Dumez 2020-08-26 09:03:35 PDT
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.
Comment 8 Chris Dumez 2020-08-26 09:14:31 PDT
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);
Comment 9 Chris Dumez 2020-08-26 09:16:13 PDT
Created attachment 407302 [details]
Patch
Comment 10 Chris Dumez 2020-08-26 09:23:36 PDT
Created attachment 407303 [details]
Patch
Comment 11 Chris Dumez 2020-08-26 09:52:43 PDT
Created attachment 407307 [details]
Patch
Comment 12 Darin Adler 2020-08-26 10:20:16 PDT
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.
Comment 13 Chris Dumez 2020-08-26 10:29:25 PDT
(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.
Comment 14 Chris Dumez 2020-08-26 10:29:59 PDT
(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.
Comment 15 Darin Adler 2020-08-26 10:40:55 PDT
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.
Comment 16 Chris Dumez 2020-08-26 10:42:25 PDT
(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.
Comment 17 Chris Dumez 2020-08-26 10:52:11 PDT
Created attachment 407311 [details]
Patch
Comment 18 Chris Dumez 2020-08-26 10:53:14 PDT
(In reply to Chris Dumez from comment #17)
> Created attachment 407311 [details]
> Patch

Trying to get build fixed on Windows.
Comment 19 Chris Dumez 2020-08-26 11:43:37 PDT
Created attachment 407315 [details]
Patch
Comment 20 EWS 2020-08-26 13:09:44 PDT
Committed r266186: <https://trac.webkit.org/changeset/266186>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407315 [details].
Comment 21 Radar WebKit Bug Importer 2020-08-26 13:40:34 PDT
<rdar://problem/67826590>