Bug 71055 – Add methods to compute magnitude and phase response for biquads

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Raymond Toy]

Add methods to compute magnitude and phase response for biquads

[Raymond Toy]

Created attachment 112734 [details]
Patch

[Raymond Toy]

Add methods to compute the magnitude and phase response of a biquad filter.

If you apply the patch, you can test out the results at

http://www.corp.google.com/~rtoy/mag-phase.html

This produces a plot of the magnitude and phase response for any of the possible biquad filters.

[Raymond Toy]

Created attachment 113242 [details]
Patch

[Raymond Toy]

We are changing the API in this patch.  The previous version with magnitude and phase functions at one frequency has problems if the filter is changing.  Each call to magnitude() and phase() will get the response for a *different* filter.

The API is changed to getFrequencyResponse which takes an array of frequencies where the response is desired.  The magnitude and phase at these frequencies is returned.  This makes the result a consistent snapshot of the filter response.  

Also, if the filter is changing via dezippering, the snapshot is taken at the final values of the filter, not at the intermediate dezippered value.  This might need further consideration?  Perhaps we really do want the intermediate response?

[Raymond Toy]

Comment on attachment 113242 [details]
Patch

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

> Source/WebCore/platform/audio/Biquad.cpp:395
> +

To speed things up, the computation should not do complex division with z.  It should multiply by 1/z = exp(-j*pi*freq).

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:115
> +        frequency[k] = frequencyHz[k] / nyquist;

Maybe add comment that we don't need to worry if the user passes in frequencies that are negative or above Nyquist?  (This ought to be handled correctly when computing the response from computing exp(j*pi*f).)

[Raymond Toy]

Comment on attachment 113242 [details]
Patch

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

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:127
> +    maybeUpdateCoefficients(biquad, true);

This doesn't quite work.  If we're asked to compute the response of a filter again, without changing the filter, the coefficients are not dirty and biquad is not initialized to the desired filter.  We need to be able to force the coefficients to be updated in this case.  Maybe add a forceUpdate parameter to maybeUpdateCoefficients to force the update.

[Chris Rogers]

Comment on attachment 113242 [details]
Patch

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

Ray, thanks for doing the work for this patch.  It looks pretty good, but I have a nice simplification that we should use.  Please see rest of comments for details.

> Source/WebCore/platform/audio/AudioDSPKernel.h:64
> +

Please remove getFrequencyResponse() from the abstract base class AudioDSPKernel.  It's not needed there, and doesn't really make sense generally.

> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:127
> +

Please remove getFrequencyResponse() from the abstract base class AudioDSPKernel.  It's not needed there, and doesn't really make sense generally.

> Source/WebCore/platform/audio/Biquad.cpp:382
> +    // frequency from -0.5 to 0.5.

normalized frequency should be from 0 -> 1

Please change comment

> Source/WebCore/platform/audio/Biquad.cpp:400
> +    // atomically copy all of the coefficients.

This WARNING should no longer be needed if you make the other changes I suggest below.  For future reference, we would use a "FIXME" here instead of "WARNING"

Please remove warning comment.

> Source/WebCore/platform/audio/Biquad.cpp:410
> +        Complex top = b0 + (b1 + b2 / z) / z;

nit: I'd change the name of "top" to "numerator" and "bottom" to "denominator"

> Source/WebCore/platform/audio/Biquad.h:75
> +    // The phase response is on radians.

"on" -> "in"

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:32
> +#include <vector>

WebKit style is to not use std::vector, but instead a special Vector class.

please include as follows:
#include <wtf/Vector.h>

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:36
> +void BiquadDSPKernel::maybeUpdateCoefficients(Biquad& biquad, bool finalValue)

I'd highly recommend changing the name "maybeUpdateCoefficients" -> "updateCoefficientsIfNecessary"

I have a way for simplifying the logic in BiquadProcessor (in comments below) so that you'll no longer need the first "Biquad& biquad" parameter.
Please remove this first parameter and use m_biquad (as it was before)

Please change the name of "finalValue" to "useSmoothing"  (and then its bool value will be the opposite), since I think "useSmoothing" is more descriptive

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:109
> +{

You'll need to ASSERT that the three pointer values are not NULL and return early if any of them are:

bool isGood = nFrequencies > 0 && frequencyHz && magResponse && phaseResponse;
ASSERT(isGood);
if (!isGood)
    return;

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:110
> +    std::vector<float> frequency(nFrequencies);

Because of WebKit style, this will have to change to:

Vector<float>

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:113
> +

Please add comment describing that you're taking frequencies in Hertz and normalizing them to the range 0 -> 1  (corresponding to 0 -> nyquist)

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:115
>> +        frequency[k] = frequencyHz[k] / nyquist;
> 
> Maybe add comment that we don't need to worry if the user passes in frequencies that are negative or above Nyquist?  (This ought to be handled correctly when computing the response from computing exp(j*pi*f).)

You can add the comment if you like - seems fine also without that comment.

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:117
> +    maybeUpdateCoefficients(m_biquad, false);

This line should is unnecessary and should be removed

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:125
> +    Biquad biquad;

There's a simplification for BiquadProcessor (see comments below) which will make this temporary Biquad object unnecessary - please remove.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:127
>> +    maybeUpdateCoefficients(biquad, true);
> 
> This doesn't quite work.  If we're asked to compute the response of a filter again, without changing the filter, the coefficients are not dirty and biquad is not initialized to the desired filter.  We need to be able to force the coefficients to be updated in this case.  Maybe add a forceUpdate parameter to maybeUpdateCoefficients to force the update.

Using the simplification in BiquadProcessor, this should simply be:

updateCoefficientsIfNecessary(false);    // false means no smoothing - just get the actual parameter value

> Source/WebCore/webaudio/BiquadDSPKernel.h:57
> +    void maybeUpdateCoefficients(Biquad&, bool finalValue);

Please see name change and other comments in the .cpp file

> Source/WebCore/webaudio/BiquadFilterNode.cpp:60
> +{

You'll need to do sanity checking here and return early if things are bad:

if (!frequencyHz || !magResponse || !phaseResponse)
    return;

> Source/WebCore/webaudio/BiquadFilterNode.cpp:63
> +    

probably worth checking that (n > 0) and return early if not

> Source/WebCore/webaudio/BiquadProcessor.cpp:96
> +    // parameters are not changing.

These comment changes look to be unnecessary.  WebKit style is to not separate lines like this, but to have longer lines (unlike chromium style)

Please revert unrelated comment changes

> Source/WebCore/webaudio/BiquadProcessor.cpp:104
> +        // for subsequent changes.

Please revert unrelated comment changes.

> Source/WebCore/webaudio/BiquadProcessor.cpp:113
> +        // dirty.

Please revert unrelated comment changes.

> Source/WebCore/webaudio/BiquadProcessor.cpp:151
> +        initialize();

Checking for initialization will no longer be necessary with simplification below.

Please remove comment and check

> Source/WebCore/webaudio/BiquadProcessor.cpp:153
> +    checkForDirtyCoefficients();

checkForDirtyCoefficients() should not be called here because it can interfere with audio thread processing (and is not necessary with simplification below since "responseKernel" will now
directly get the *actual* parameter value and not worry about smoothing at all)

> Source/WebCore/webaudio/BiquadProcessor.cpp:165
> +        }

A simplification is to replace lines 155 - 165 with the following:


// Compute the frequency response on a separate temporary kernel to avoid interfering with the processing running in the audio thread on the main kernels.
OwnPtr<BiquadDSPKernel> responseKernel = adoptPtr(new BiquadDSPKernel(this));

responseKernel->getFrequencyResponse(nFrequencies, frequencyHz, magResponse, phaseResponse);



This simplification goes hand-in-hand with my other comments in the BiquadDSPKernel files

> Source/WebCore/webaudio/BiquadProcessor.h:65
> +    virtual void getFrequencyResponse(int nFrequencies, float* frequencyHz,

method should not be virtual

> Source/WebCore/webaudio/BiquadProcessor.h:66
> +                                      float* magResponse, float* phaseResponse);

strange line breaking - I'd recommend either putting all arguments on the same line (preferred) or split each argument onto a separate line

> Source/WebCore/webaudio/BiquadProcessor.h:68
> +    virtual void checkForDirtyCoefficients();

method should not be virtual

[Chris Rogers]

Sorry, one last detail I got slightly wrong.  I think you'll need to add an extra param to updateCoefficientsIfNecessary():

BiquadDSPKernel::updateCoefficientsIfNecessary(bool useSmoothing, bool forceUpdate);

The "forceUpdate" param would be set to "true" inside BiquadDSPKernel::getFrequencyResponse()

(but would be "false" otherwise)

Hopefully the param name makes sense and you see where I'm headed...

[Raymond Toy]

Created attachment 113591 [details]
Patch

[Raymond Toy]

Comment on attachment 113242 [details]
Patch

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

>> Source/WebCore/platform/audio/AudioDSPKernel.h:64
>> +
> 
> Please remove getFrequencyResponse() from the abstract base class AudioDSPKernel.  It's not needed there, and doesn't really make sense generally.

Done.

>> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:127
>> +
> 
> Please remove getFrequencyResponse() from the abstract base class AudioDSPKernel.  It's not needed there, and doesn't really make sense generally.

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:382
>> +    // frequency from -0.5 to 0.5.
> 
> normalized frequency should be from 0 -> 1
> 
> Please change comment

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:395
>> +
> 
> To speed things up, the computation should not do complex division with z.  It should multiply by 1/z = exp(-j*pi*freq).

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:400
>> +    // atomically copy all of the coefficients.
> 
> This WARNING should no longer be needed if you make the other changes I suggest below.  For future reference, we would use a "FIXME" here instead of "WARNING"
> 
> Please remove warning comment.

Done.

>> Source/WebCore/platform/audio/Biquad.h:75
>> +    // The phase response is on radians.
> 
> "on" -> "in"

Done.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:32
>> +#include <vector>
> 
> WebKit style is to not use std::vector, but instead a special Vector class.
> 
> please include as follows:
> #include <wtf/Vector.h>

Done.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:36
>> +void BiquadDSPKernel::maybeUpdateCoefficients(Biquad& biquad, bool finalValue)
> 
> I'd highly recommend changing the name "maybeUpdateCoefficients" -> "updateCoefficientsIfNecessary"
> 
> I have a way for simplifying the logic in BiquadProcessor (in comments below) so that you'll no longer need the first "Biquad& biquad" parameter.
> Please remove this first parameter and use m_biquad (as it was before)
> 
> Please change the name of "finalValue" to "useSmoothing"  (and then its bool value will be the opposite), since I think "useSmoothing" is more descriptive

Done.  maybeUpdateCoefficients is less typing. :-)

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:109
>> +{
> 
> You'll need to ASSERT that the three pointer values are not NULL and return early if any of them are:
> 
> bool isGood = nFrequencies > 0 && frequencyHz && magResponse && phaseResponse;
> ASSERT(isGood);
> if (!isGood)
>     return;

Done.   But should this be done at a higher layer?  By the time we're here, we don't even know if there is enough space in magResponse and phaseResponse to hold all nFrequencies values.  Or even if frequencyHz has nFrequencies values.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:110
>> +    std::vector<float> frequency(nFrequencies);
> 
> Because of WebKit style, this will have to change to:
> 
> Vector<float>

Done.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:113
>> +
> 
> Please add comment describing that you're taking frequencies in Hertz and normalizing them to the range 0 -> 1  (corresponding to 0 -> nyquist)

Done.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:125
>> +    Biquad biquad;
> 
> There's a simplification for BiquadProcessor (see comments below) which will make this temporary Biquad object unnecessary - please remove.

Done.

>> Source/WebCore/webaudio/BiquadDSPKernel.h:57
>> +    void maybeUpdateCoefficients(Biquad&, bool finalValue);
> 
> Please see name change and other comments in the .cpp file

Done.

>> Source/WebCore/webaudio/BiquadFilterNode.cpp:60
>> +{
> 
> You'll need to do sanity checking here and return early if things are bad:
> 
> if (!frequencyHz || !magResponse || !phaseResponse)
>     return;

Done.

>> Source/WebCore/webaudio/BiquadProcessor.cpp:96
>> +    // parameters are not changing.
> 
> These comment changes look to be unnecessary.  WebKit style is to not separate lines like this, but to have longer lines (unlike chromium style)
> 
> Please revert unrelated comment changes

Done.  If I hadn't made a new function, I would have left them alone.  There's a reason why newspapers are printed in columns instead of 12-15 inch lines. :-)

>> Source/WebCore/webaudio/BiquadProcessor.cpp:104
>> +        // for subsequent changes.
> 
> Please revert unrelated comment changes.

Done.

>> Source/WebCore/webaudio/BiquadProcessor.cpp:151
>> +        initialize();
> 
> Checking for initialization will no longer be necessary with simplification below.
> 
> Please remove comment and check

Done.

>> Source/WebCore/webaudio/BiquadProcessor.cpp:153
>> +    checkForDirtyCoefficients();
> 
> checkForDirtyCoefficients() should not be called here because it can interfere with audio thread processing (and is not necessary with simplification below since "responseKernel" will now
> directly get the *actual* parameter value and not worry about smoothing at all)

Removed.

>> Source/WebCore/webaudio/BiquadProcessor.cpp:165
>> +        }
> 
> A simplification is to replace lines 155 - 165 with the following:
> 
> 
> // Compute the frequency response on a separate temporary kernel to avoid interfering with the processing running in the audio thread on the main kernels.
> OwnPtr<BiquadDSPKernel> responseKernel = adoptPtr(new BiquadDSPKernel(this));
> 
> responseKernel->getFrequencyResponse(nFrequencies, frequencyHz, magResponse, phaseResponse);
> 
> 
> 
> This simplification goes hand-in-hand with my other comments in the BiquadDSPKernel files

Done.

>> Source/WebCore/webaudio/BiquadProcessor.h:65
>> +    virtual void getFrequencyResponse(int nFrequencies, float* frequencyHz,
> 
> method should not be virtual

Done.

>> Source/WebCore/webaudio/BiquadProcessor.h:68
>> +    virtual void checkForDirtyCoefficients();
> 
> method should not be virtual

Done.

[Chris Rogers]

Comment on attachment 113591 [details]
Patch

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

Looking good - just some nit comments at this point.  I'm assuming you've rebuilt and verified that the latest changes still work?

If you've verified it still works, then looks good after nits are addressed -- passing over to Ken for final review...

> Source/WebCore/platform/audio/Biquad.cpp:387
> +    // Y(z) = (b0 + b1*z^(-1) + b2*z^(-2))/(1 + a1*z^(-1) + a2*z^(-2))

nit: I think H(z) is more correct here than Y(z) since we're talking about the system response

> Source/WebCore/platform/audio/Biquad.cpp:398
> +    // from underneath us.

This comment is a little outdated since the member variables will no longer change (since this is now a separate temporary Biquad object).

Instead, I would just say that we're making local copies as an optimization

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:109
> +{

small nit: Maybe best to be consistent and either put all params on same line or on all separate lines

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:130
> +    m_biquad.getFrequencyResponse(nFrequencies, &frequency[0], magResponse, phaseResponse);

nit: the 2nd argument is better as "frequency.data()"

> Source/WebCore/webaudio/BiquadDSPKernel.h:52
> +                                      float* magResponse, float* phaseResponse);

nit: Since this method is similar in several classes, we should be consistent in the formatting of the arguments (pick one single way and go with that)

> Source/WebCore/webaudio/BiquadDSPKernel.h:57
> +    void updateCoefficientsIfNecessary(bool useSmoothing, bool forceUpdate);

Probably good to add a comment here describing the parameters in a little more detail.  For example, saying that the actual
audio rendering smooths (de-zippers) the parameter changes to avoid sudden changes/glitches, while the coefficients need
to be updated immediately to exact values when asking for the frequency response curve.

> Source/WebCore/webaudio/BiquadProcessor.cpp:141
> +                                           float* phaseResponse)

nit: be consistent in formatting with other versions of this method

[Kenneth Russell]

Comment on attachment 113591 [details]
Patch

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

Code looks good to me overall. r- for const issues.

> Source/WebCore/platform/audio/Biquad.h:77
> +                              float* frequency,

Because frequency is only an input it should be const float*. This will improve understandability of the code. This change is needed in a few places.

> Source/WebCore/webaudio/BiquadDSPKernel.h:51
> +    virtual void getFrequencyResponse(int nFrequencies, float* frequencyHz,

frequencyHz should be const float* per comment in Biquad.h. Also, does this really need to be virtual?

> Source/WebCore/webaudio/BiquadProcessor.h:65
> +    void getFrequencyResponse(int nFrequencies, float* frequencyHz, float* magResponse, float* phaseResponse);

frequencyHz should be const float*.

[Raymond Toy]

Comment on attachment 113591 [details]
Patch

Changes made.  Update patch will be uploaded right away.

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

>> Source/WebCore/platform/audio/Biquad.cpp:387
>> +    // Y(z) = (b0 + b1*z^(-1) + b2*z^(-2))/(1 + a1*z^(-1) + a2*z^(-2))
> 
> nit: I think H(z) is more correct here than Y(z) since we're talking about the system response

Done.

>> Source/WebCore/platform/audio/Biquad.cpp:398
>> +    // from underneath us.
> 
> This comment is a little outdated since the member variables will no longer change (since this is now a separate temporary Biquad object).
> 
> Instead, I would just say that we're making local copies as an optimization

I meant to remove that but forgot.  Done.

>> Source/WebCore/platform/audio/Biquad.h:77
>> +                              float* frequency,
> 
> Because frequency is only an input it should be const float*. This will improve understandability of the code. This change is needed in a few places.

Agreed.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:109
>> +{
> 
> small nit: Maybe best to be consistent and either put all params on same line or on all separate lines

I separated them this way because 2 are inputs and 2 are outputs.  Putting them in separate lines, everywhere.

>> Source/WebCore/webaudio/BiquadDSPKernel.cpp:130
>> +    m_biquad.getFrequencyResponse(nFrequencies, &frequency[0], magResponse, phaseResponse);
> 
> nit: the 2nd argument is better as "frequency.data()"

Done.  (Original std::vector version had this, but Mac OS X didn't support that.)

>> Source/WebCore/webaudio/BiquadDSPKernel.h:51
>> +    virtual void getFrequencyResponse(int nFrequencies, float* frequencyHz,
> 
> frequencyHz should be const float* per comment in Biquad.h. Also, does this really need to be virtual?

Changed to const float*.

Probably doesn't need to be virtual.

>> Source/WebCore/webaudio/BiquadDSPKernel.h:52
>> +                                      float* magResponse, float* phaseResponse);
> 
> nit: Since this method is similar in several classes, we should be consistent in the formatting of the arguments (pick one single way and go with that)

Done.

>> Source/WebCore/webaudio/BiquadDSPKernel.h:57
>> +    void updateCoefficientsIfNecessary(bool useSmoothing, bool forceUpdate);
> 
> Probably good to add a comment here describing the parameters in a little more detail.  For example, saying that the actual
> audio rendering smooths (de-zippers) the parameter changes to avoid sudden changes/glitches, while the coefficients need
> to be updated immediately to exact values when asking for the frequency response curve.

Done.

>> Source/WebCore/webaudio/BiquadProcessor.cpp:141
>> +                                           float* phaseResponse)
> 
> nit: be consistent in formatting with other versions of this method

Done.

>> Source/WebCore/webaudio/BiquadProcessor.h:65
>> +    void getFrequencyResponse(int nFrequencies, float* frequencyHz, float* magResponse, float* phaseResponse);
> 
> frequencyHz should be const float*.

Done.

[Raymond Toy]

Created attachment 113665 [details]
Patch

[Chris Rogers]

Comment on attachment 113665 [details]
Patch

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

> Source/WebCore/webaudio/BiquadFilterNode.h:63
> +    virtual void getFrequencyResponse(const Float32Array* frequencyHz,

'virtual' not needed here

[Kenneth Russell]

Comment on attachment 113665 [details]
Patch

Looks good to me modulo Chris's point about an unnecessary virtual. r=me

[Raymond Toy]

Created attachment 113710 [details]
Patch

[Raymond Toy]

Comment on attachment 113665 [details]
Patch

Last comment fixed as requested.

Thanks for your time for the review!


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

>> Source/WebCore/webaudio/BiquadFilterNode.h:63
>> +    virtual void getFrequencyResponse(const Float32Array* frequencyHz,
> 
> 'virtual' not needed here

Removed.

[Kenneth Russell]

Comment on attachment 113710 [details]
Patch

Looks fine.

[WebKit Review Bot]

Comment on attachment 113710 [details]
Patch

Clearing flags on attachment: 113710

Committed r99337: <http://trac.webkit.org/changeset/99337>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.