Bug 215810

Summary: IIRFilterNode interface is not supported
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, benjamin, calvaris, changseok, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, hta, jer.noble, jsbell, kangil.han, kondapallykalyan, mifenton, mkwst, philipj, ryuan.choi, sergio, tommyw, toyoshim, webkit-bug-importer, webkit.org, youennf, yutak
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://www.w3.org/TR/webaudio/#iirfilternode
Bug Depends on:    
Bug Blocks: 212611    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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>