Biquad getFrequencyResponse needs a layout test.

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

Need to add layout test for Biquad getFrequencyResponse.  It's probably good enough to test just one kind of biquad filter.

[Raymond Toy]

Created attachment 128804 [details]
Patch

[Chris Rogers]

Comment on attachment 128804 [details]
Patch

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

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:36
> +var filterGain = 5;

This may seem silly to you, but it would be nice to use values that would be more normally found in audio/music processing.

How about testing a *peaking* filter with:
cutoff = 1000
Q = 1
gain = 5

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:151
> +

Could you please have an initial test to check for NaN and Inf in both mag and phase results?

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:175
> +        var error = Math.abs(phaseResponse[k] - expectedPhase[k]);

Don't you have to handle phase wrapping here?  For example, the difference between a phase near -Pi and one very near +Pi is actually very small...

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:211
> +    filter.type = filter.LOWPASS;

See comment above about filter type.

[Raymond Toy]

Created attachment 129281 [details]
Patch

[Raymond Toy]

Comment on attachment 128804 [details]
Patch

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

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:36
>> +var filterGain = 5;
> 
> This may seem silly to you, but it would be nice to use values that would be more normally found in audio/music processing.
> 
> How about testing a *peaking* filter with:
> cutoff = 1000
> Q = 1
> gain = 5

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:151
>> +
> 
> Could you please have an initial test to check for NaN and Inf in both mag and phase results?

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:175
>> +        var error = Math.abs(phaseResponse[k] - expectedPhase[k]);
> 
> Don't you have to handle phase wrapping here?  For example, the difference between a phase near -Pi and one very near +Pi is actually very small...

Don't think that's possible for the peaking filter, but I added support for wrapping, just in case we add tests for other filter types.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:211
>> +    filter.type = filter.LOWPASS;
> 
> See comment above about filter type.

Changed to PEAKING.

[WebKit Review Bot]

Comment on attachment 129281 [details]
Patch

Attachment 129281 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11714084

New failing tests:
webaudio/biquad-getFrequencyResponse.html

[Raymond Toy]

Created attachment 129326 [details]
Patch

[Raymond Toy]

(In reply to comment #6)
> Created an attachment (id=129326) [details]
> Patch

Oops.  Previous patch had typo in threshold and didn't update the expected results.

[Chris Rogers]

Comment on attachment 129326 [details]
Patch

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

Nice test!  comments below:

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:17
> +// lowpass biquad filter and compare it with the expected frequency response.

lowpass -> peaking

I'd add one more sentence here describing that it doesn't matter that much which filter we pick since we're testing the response code and not
a specific filter.  The filters themselves are tested extensively in the other biquad tests.

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:33
> +// The filter properties

properties -> parameters

nit: add period at end of sentence

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:34
> +var filterCutoff = 1000;  // Hz.

nit: extra space before //

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:36
> +var filterGain = 5;

could add // Decibels.

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:44
> +// Convert frequency in Hz to a normalized frequency.

"normalized frequency" -> "normalized frequency between 0 -> 1 corresponding to 0 -> Nyquist frequency"

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:65
> +    // b2 * cos(2*w) + b1 * cos(w) + b0 - i * (b1 * sin(w) + b2 * sin(2*w))

The use of "w" is inconsistent with line 76 where you use "omega" and the terms are in reverse order.  Let's just use omega here and use the ordering from 76

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:69
> +    // 1 + a1 * cos(w) + a2 * cos(2*w) - i * (a1 * sin(w) + a2 * sin(2*w))

ditto

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:71
> +    // where w = pi * f

I'd use omega instead of w, and define "omega" up-front (above line 65)

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:95
> +// given by |frequencySamples|.

frequencySamples -> frequencies

in the comment and the function

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:100
> +    var filterCoef = createFilter(filter.type, normalizedFreq, filter.Q.value, filter.gain.value);

filterCoef -> filterCoefficients (or simply coefficients)

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:103
> +    var phase = [];

these are lists, so plural is better:

magnitudes
phases

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:111
> +    return {magnitude : magnitude, phase : phase};

plural

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:114
> +// Compute the set of frequency samples.

// Compute a set of linearly spaced frequencies.

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:115
> +function createFrequencySampleArray(nSamples, sampleRate)

nSamples -> nFrequences

(the word sample is quite confusing here since it usually means a PCM sample)

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:117
> +    var freqSamples = new Float32Array(nSamples);

freqSamples -> frequencies

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:119
> +    var freqDelta = sampleRate / 2 / nSamples;

nit: double /  is hard to read

To make this clearer, can't we instead have:
var nyquist = 0.5 * sampleRate;
var freqDelta = nyquist / nSamples;

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:128
> +function linearToDecibels(x)

Can't we put this in audio-testing.js?

If we do, make sure to check if this conflicts with other layout tests which similarly define this function.  If it becomes too much of a hassle then this could be done in another patch...

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:138
> +// If no such number, returns false.

What if the first bad index is 0 and we return 0 instead of false - is that a problem?

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:191
> +

For the following checks on the expected results I would add a comment clarifying that this test doesn't test the implementation, but is
just a sanity check on the test itself.  This is an important distinction, since a failure here isn't really a failure of the impl.

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:272
> +    // Arbitrarily test a lowpass filter, but any kind of filter can be tested.  (Should we test all

lowpass -> peaking

I think you can remove the comment about testing all the filter types and just say that any filter type should do.

[Raymond Toy]

Created attachment 129345 [details]
Patch

[Raymond Toy]

Comment on attachment 129326 [details]
Patch

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

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:17
>> +// lowpass biquad filter and compare it with the expected frequency response.
> 
> lowpass -> peaking
> 
> I'd add one more sentence here describing that it doesn't matter that much which filter we pick since we're testing the response code and not
> a specific filter.  The filters themselves are tested extensively in the other biquad tests.

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:33
>> +// The filter properties
> 
> properties -> parameters
> 
> nit: add period at end of sentence

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:34
>> +var filterCutoff = 1000;  // Hz.
> 
> nit: extra space before //

Removed.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:36
>> +var filterGain = 5;
> 
> could add // Decibels.

Added.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:44
>> +// Convert frequency in Hz to a normalized frequency.
> 
> "normalized frequency" -> "normalized frequency between 0 -> 1 corresponding to 0 -> Nyquist frequency"

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:65
>> +    // b2 * cos(2*w) + b1 * cos(w) + b0 - i * (b1 * sin(w) + b2 * sin(2*w))
> 
> The use of "w" is inconsistent with line 76 where you use "omega" and the terms are in reverse order.  Let's just use omega here and use the ordering from 76

Use omega everywhere.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:69
>> +    // 1 + a1 * cos(w) + a2 * cos(2*w) - i * (a1 * sin(w) + a2 * sin(2*w))
> 
> ditto

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:71
>> +    // where w = pi * f
> 
> I'd use omega instead of w, and define "omega" up-front (above line 65)

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:95
>> +// given by |frequencySamples|.
> 
> frequencySamples -> frequencies
> 
> in the comment and the function

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:100
>> +    var filterCoef = createFilter(filter.type, normalizedFreq, filter.Q.value, filter.gain.value);
> 
> filterCoef -> filterCoefficients (or simply coefficients)

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:103
>> +    var phase = [];
> 
> these are lists, so plural is better:
> 
> magnitudes
> phases

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:114
>> +// Compute the set of frequency samples.
> 
> // Compute a set of linearly spaced frequencies.

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:115
>> +function createFrequencySampleArray(nSamples, sampleRate)
> 
> nSamples -> nFrequences
> 
> (the word sample is quite confusing here since it usually means a PCM sample)

Done. (but without the typo :-) )

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:117
>> +    var freqSamples = new Float32Array(nSamples);
> 
> freqSamples -> frequencies

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:119
>> +    var freqDelta = sampleRate / 2 / nSamples;
> 
> nit: double /  is hard to read
> 
> To make this clearer, can't we instead have:
> var nyquist = 0.5 * sampleRate;
> var freqDelta = nyquist / nSamples;

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:128
>> +function linearToDecibels(x)
> 
> Can't we put this in audio-testing.js?
> 
> If we do, make sure to check if this conflicts with other layout tests which similarly define this function.  If it becomes too much of a hassle then this could be done in another patch...

I'm going to defer this to another patch.  The convolution test has linearToDecibel, which is slightly different.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:138
>> +// If no such number, returns false.
> 
> What if the first bad index is 0 and we return 0 instead of false - is that a problem?

Yes, that's a problem.  Rearranged to make it more explicit, without this problem.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:191
>> +
> 
> For the following checks on the expected results I would add a comment clarifying that this test doesn't test the implementation, but is
> just a sanity check on the test itself.  This is an important distinction, since a failure here isn't really a failure of the impl.

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:272
>> +    // Arbitrarily test a lowpass filter, but any kind of filter can be tested.  (Should we test all
> 
> lowpass -> peaking
> 
> I think you can remove the comment about testing all the filter types and just say that any filter type should do.

Done.

[Chris Rogers]

Comment on attachment 129345 [details]
Patch

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

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:33
> +var nFrequencySamples = 1000;

nFrequencySamples -> numberOfFrequencies

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:69
> +    // b2 * cos(2 * omega) + b1 * cos(omega) + b0 - i * (b1 * sin(omega) + b2 * sin(2 * omega))

Please use coefficient ordering of line 78:79 (in other words: b0, b1, b2)

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:83
> +    var magnitude = Math.sqrt((Math.pow(numeratorReal, 2) + Math.pow(numeratorImag, 2))

Math.pow(..., 2) is more cumbersome than simple multiplies:
numeratorReal * numeratorReal    and    numeratorImag * numeratorImag

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:84
> +                              / (Math.pow(denominatorReal, 2) + Math.pow(denominatorImag, 2)));

ditto

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:91
> +    }

Just curious - does atan2() ever return values out-of-bounds of -Pi -> +Pi  ?

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:117
> +function createFrequencySampleArray(nFrequencies, sampleRate)

Please don't use the word "sample"

createFrequencySampleArray -> createFrequencies

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:144
> +         if (!isValidNumber(signal[k])) {

nit: funny indentation

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:145
> +            return {found : true, index : k};

Please simply return index (and -1 if no bad index found)

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:148
> +    return {found : false}

Can't we just return -1 as the index if no bad number is found -- this solution is more complex and harder to read than necessary

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:164
> +function compareResponses(filter, freq, magResponse, phaseResponse)

freq -> frequencies

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:180
> +    hasBadNumber = findBadNumber(magResponse);

findBadNumber() should just directly return index (-1 if no bad index found)

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:230
> +        message += " expected: " + linearToDecibels(expectedMagnitude[maxMagErrorIndex]);

typo: expectedMagnitude -> expectedMagnitudes

Have you actually made these tests fail by purposefully (temporarily) introducing bugs into the implementation??  It seems you would have caught this if so.

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:252
> +        message += " expected: " + expectedPhase[maxPhaseErrorIndex];

typo: expectedPhase -> expectedPhases

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:282
> +    freqSamples = createFrequencySampleArray(nFrequencySamples, context.sampleRate);

freqSamples -> frequencies

[Raymond Toy]

Created attachment 129923 [details]
Patch

[Raymond Toy]

Comment on attachment 129345 [details]
Patch

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

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:33
>> +var nFrequencySamples = 1000;
> 
> nFrequencySamples -> numberOfFrequencies

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:69
>> +    // b2 * cos(2 * omega) + b1 * cos(omega) + b0 - i * (b1 * sin(omega) + b2 * sin(2 * omega))
> 
> Please use coefficient ordering of line 78:79 (in other words: b0, b1, b2)

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:83
>> +    var magnitude = Math.sqrt((Math.pow(numeratorReal, 2) + Math.pow(numeratorImag, 2))
> 
> Math.pow(..., 2) is more cumbersome than simple multiplies:
> numeratorReal * numeratorReal    and    numeratorImag * numeratorImag

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:91
>> +    }
> 
> Just curious - does atan2() ever return values out-of-bounds of -Pi -> +Pi  ?

The spec says no, except it can return NaN, but that only happens if either arg is NaN.  But we check for NaN in the phase response later so we catch this issue.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:117
>> +function createFrequencySampleArray(nFrequencies, sampleRate)
> 
> Please don't use the word "sample"
> 
> createFrequencySampleArray -> createFrequencies

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:144
>> +         if (!isValidNumber(signal[k])) {
> 
> nit: funny indentation

Fixed.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:145
>> +            return {found : true, index : k};
> 
> Please simply return index (and -1 if no bad index found)

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:148
>> +    return {found : false}
> 
> Can't we just return -1 as the index if no bad number is found -- this solution is more complex and harder to read than necessary

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:164
>> +function compareResponses(filter, freq, magResponse, phaseResponse)
> 
> freq -> frequencies

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:180
>> +    hasBadNumber = findBadNumber(magResponse);
> 
> findBadNumber() should just directly return index (-1 if no bad index found)

Done.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:230
>> +        message += " expected: " + linearToDecibels(expectedMagnitude[maxMagErrorIndex]);
> 
> typo: expectedMagnitude -> expectedMagnitudes
> 
> Have you actually made these tests fail by purposefully (temporarily) introducing bugs into the implementation??  It seems you would have caught this if so.

I did the tests earlier, but not after submitting the first patch.

Fixed.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:252
>> +        message += " expected: " + expectedPhase[maxPhaseErrorIndex];
> 
> typo: expectedPhase -> expectedPhases

Fixed.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:282
>> +    freqSamples = createFrequencySampleArray(nFrequencySamples, context.sampleRate);
> 
> freqSamples -> frequencies

Done.

[Chris Rogers]

Comment on attachment 129923 [details]
Patch

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

Almost finished - looks great!

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:73
> +    // Compute H(exp(i * pi * f)).  No native complex numbers in javascript, so break H(exp(i*pi*f))

spacing nit: H(exp(i*pi*f))

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:96
> +        phase -= Math.PI;

Isn't this wrapping wrong: should be -= 2 * Math.PI;

> LayoutTests/webaudio/biquad-getFrequencyResponse.html:98
> +        phase += Math.PI;

ditto

[Raymond Toy]

Created attachment 129944 [details]
Patch

[Raymond Toy]

Comment on attachment 129923 [details]
Patch

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

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:73
>> +    // Compute H(exp(i * pi * f)).  No native complex numbers in javascript, so break H(exp(i*pi*f))
> 
> spacing nit: H(exp(i*pi*f))

Fixed.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:96
>> +        phase -= Math.PI;
> 
> Isn't this wrapping wrong: should be -= 2 * Math.PI;

Fixed.

>> LayoutTests/webaudio/biquad-getFrequencyResponse.html:98
>> +        phase += Math.PI;
> 
> ditto

Fixed.

[WebKit Review Bot]

Comment on attachment 129944 [details]
Patch

Attachment 129944 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11802150

New failing tests:
editing/selection/select-line-break-with-opposite-directionality.html

[Raymond Toy]

Created attachment 129972 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129972 [details]
Patch

Rejecting attachment 129972 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
_by_email
    return self._reviewer_only(self.account_by_email(email))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 632, in account_by_email
    return self._email_to_account_map().get(email.lower()) if email else None
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 525, in _email_to_account_map
    assert(email not in self._accounts_by_email)  # We should never have duplicate emails.
AssertionError

Full output: http://queues.webkit.org/results/11795260

[WebKit Review Bot]

Comment on attachment 129972 [details]
Patch

Clearing flags on attachment: 129972

Committed r109662: <http://trac.webkit.org/changeset/109662>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.