Bug 216027 – Add proper k-rate automation support for BiquadFilterNode

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

Add proper k-rate automation support for BiquadFilterNode.

[Chris Dumez]

Created attachment 407653 [details]
Patch

[Chris Dumez]

Created attachment 407655 [details]
Patch

[youenn fablet]

Comment on attachment 407655 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        based on Chromium's implementation.

Do they have API/Unit tests we could also run, for instance to cover Biquad implementation?

> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:62
> +        float detune[AudioNode::ProcessingSizeInFrames]; // in Cents

We are allocating on the stack arrays of float but this is only really used in the if below, not the else.
Could we move these arrays to inside the if statement?

> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:87
> +            updateCoefficients(1, cutoffFrequency, q, gain, detune);

Could we write it as the following?
auto cutoffFrequency = biquadProcessor()->parameter1().finalValue();
...
updateCoefficients(1, &cutoffFrequency, &q, &gain, &detune);

> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:145
> +    updateTailTime(numberOfFrames - 1);

We could ASSERT that numberOfFrames is not null.

> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:177
>      for (int k = 0; k < nFrequencies; ++k)

Preexisting, but it seems odd nFrequencies is an int.

> Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:128
> +    // |response_kernel| with these values.

s/|response_kernel|/responseKernel.
Comment on one line.

> Source/WebCore/platform/audio/Biquad.cpp:71
> +    setNormalizedCoefficients(0, 1, 0, 0, 1, 0, 0);

This seems a bit mysterious, especially the first 0 is an index, not a coefficient.
Is there a way we could make that clearer? Rename setNormalizedCoefficients, name the first value or group parameters maybe.

> Source/WebCore/platform/audio/Biquad.cpp:81
> +        int n = framesToProcess;

Why int?

> Source/WebCore/platform/audio/Biquad.cpp:89
> +        const double* b0 = m_b0.data();

auto?

> Source/WebCore/platform/audio/Biquad.cpp:-102
> -    }

Can we return early here?

> Source/WebCore/platform/audio/Biquad.cpp:128
> +        double* outputP = m_outputBuffer.data();

auto*?

> Source/WebCore/platform/audio/Biquad.cpp:150
> +        m_y2 = destP[framesToProcess - 2];

Should we assert framesToProcess is above 2?

> Source/WebCore/platform/audio/Biquad.cpp:814
> +        ASSERT(std::isfinite(tailFrame));

return early?

> Source/WebCore/platform/audio/Biquad.cpp:846
> +        }

Ditto

[Chris Dumez]

(In reply to youenn fablet from comment #3)
> Comment on attachment 407655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407655&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        based on Chromium's implementation.
> 
> Do they have API/Unit tests we could also run, for instance to cover Biquad
> implementation?

I did not find any such API/unit tests by grepping. Biquad is tested by layout tests though. 

> 
> > Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:62
> > +        float detune[AudioNode::ProcessingSizeInFrames]; // in Cents
> 
> We are allocating on the stack arrays of float but this is only really used
> in the if below, not the else.
> Could we move these arrays to inside the if statement?
> 
> > Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:87
> > +            updateCoefficients(1, cutoffFrequency, q, gain, detune);
> 
> Could we write it as the following?
> auto cutoffFrequency = biquadProcessor()->parameter1().finalValue();
> ...
> updateCoefficients(1, &cutoffFrequency, &q, &gain, &detune);
> 
> > Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:145
> > +    updateTailTime(numberOfFrames - 1);
> 
> We could ASSERT that numberOfFrames is not null.
> 
> > Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:177
> >      for (int k = 0; k < nFrequencies; ++k)
> 
> Preexisting, but it seems odd nFrequencies is an int.
> 
> > Source/WebCore/Modules/webaudio/BiquadProcessor.cpp:128
> > +    // |response_kernel| with these values.
> 
> s/|response_kernel|/responseKernel.
> Comment on one line.
> 
> > Source/WebCore/platform/audio/Biquad.cpp:71
> > +    setNormalizedCoefficients(0, 1, 0, 0, 1, 0, 0);
> 
> This seems a bit mysterious, especially the first 0 is an index, not a
> coefficient.
> Is there a way we could make that clearer? Rename setNormalizedCoefficients,
> name the first value or group parameters maybe.
> 
> > Source/WebCore/platform/audio/Biquad.cpp:81
> > +        int n = framesToProcess;
> 
> Why int?
> 
> > Source/WebCore/platform/audio/Biquad.cpp:89
> > +        const double* b0 = m_b0.data();
> 
> auto?
> 
> > Source/WebCore/platform/audio/Biquad.cpp:-102
> > -    }
> 
> Can we return early here?
> 
> > Source/WebCore/platform/audio/Biquad.cpp:128
> > +        double* outputP = m_outputBuffer.data();
> 
> auto*?
> 
> > Source/WebCore/platform/audio/Biquad.cpp:150
> > +        m_y2 = destP[framesToProcess - 2];
> 
> Should we assert framesToProcess is above 2?
> 
> > Source/WebCore/platform/audio/Biquad.cpp:814
> > +        ASSERT(std::isfinite(tailFrame));
> 
> return early?
> 
> > Source/WebCore/platform/audio/Biquad.cpp:846
> > +        }
> 
> Ditto

Applied all those changes.

[Chris Dumez]

Created attachment 407682 [details]
Patch

[Chris Dumez]

Created attachment 407683 [details]
Patch

[EWS]

Committed r266398: <https://trac.webkit.org/changeset/266398>

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

[Radar WebKit Bug Importer]

<rdar://problem/68149072>