Need to add layout test for Biquad getFrequencyResponse. It's probably good enough to test just one kind of biquad filter.
Created attachment 128804 [details] Patch
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.
Created attachment 129281 [details] Patch
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.
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
Created attachment 129326 [details] Patch
(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.
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.
Created attachment 129345 [details] Patch
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.
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
Created attachment 129923 [details] Patch
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.
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
Created attachment 129944 [details] Patch
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.
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
Created attachment 129972 [details] Patch
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
Comment on attachment 129972 [details] Patch Clearing flags on attachment: 129972 Committed r109662: <http://trac.webkit.org/changeset/109662>
All reviewed patches have been landed. Closing bug.