IIRFilterNode interface is not supported: - https://www.w3.org/TR/webaudio/#iirfilternode
Created attachment 407224 [details] Patch
*** Bug 181195 has been marked as a duplicate of this bug. ***
Created attachment 407228 [details] Patch
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?
Created attachment 407264 [details] Patch
(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 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 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);
Created attachment 407302 [details] Patch
Created attachment 407303 [details] Patch
Created attachment 407307 [details] Patch
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.
(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.
(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.
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.
(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.
Created attachment 407311 [details] Patch
(In reply to Chris Dumez from comment #17) > Created attachment 407311 [details] > Patch Trying to get build fixed on Windows.
Created attachment 407315 [details] Patch
Committed r266186: <https://trac.webkit.org/changeset/266186> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407315 [details].
<rdar://problem/67826590>