[webkit-reviews] review granted: [Bug 216027] Add proper k-rate automation support for BiquadFilterNode : [Attachment 407655] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 216027: Add proper k-rate automation support for BiquadFilterNode
https://bugs.webkit.org/show_bug.cgi?id=216027

Attachment 407655: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 407655
  --> https://bugs.webkit.org/attachment.cgi?id=407655
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