Bug 216027

Created attachment 407653 [details]
Patch

Created attachment 407655 [details]
Patch

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

(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.

Created attachment 407682 [details]
Patch

Created attachment 407683 [details]
Patch

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

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

<rdar://problem/68149072>