Bug 181191 – Highpass Biquads use old formulas

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

[Ricci Adams]

The WebAudio spec for biquad filters was updated in April 2016 to use the Audio Cookbook formulas.

Safari and WebKit are still using the old formulas.

See these URLs for more info:
https://github.com/WebAudio/web-audio-api/issues/771
https://www.chromestatus.com/feature/5687523284090880
https://github.com/GoogleChromeLabs/web-audio-samples/wiki/Detection-of-lowpass-BiquadFilter-implementation
https://rtoy.github.io/webaudio-hacks/more/biquad/biquad-lowpass-q.html?usedB=true

[Ricci Adams]

Note: The relevant commit to Blink is https://codereview.chromium.org/1885723003

[Lauro Moura]

Created attachment 406470 [details]
Patch

Patch based on lowpass changes from r265517. Also updated the biquad tests to use the same tolerance as the WPT ones.

[Lauro Moura]

(In reply to Lauro Moura from comment #2)
> Created attachment 406470 [details]
> Patch
> 
> Patch based on lowpass changes from r265517. Also updated the biquad tests
> to use the same tolerance as the WPT ones.

This patch updates the glib baseline to be all passes, but probably we'll be able to update the main one instead.

[Chris Dumez]

Comment on attachment 406470 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Lowpass and Highpass Biquads use old formulas

Please fix title now that this patch is only about the HighPass filter.

> Source/WebCore/platform/audio/Biquad.cpp:244
> +        double alpha = sin(theta) / (2 * resonance);

Please use float for all these variables like in r265517.

[Darin Adler]

Comment on attachment 406470 [details]
Patch

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

> LayoutTests/webaudio/biquad-allpass.html:33
> +        threshold: 3.9337e-8,

Why are these threshold values so precise; this doesn’t seem to enhance the tests? How about changing maxAllowedError to 1e-7 in biquad-testing.js for all the tests instead of adding all of these special values?

[Lauro Moura]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 406470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406470&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Lowpass and Highpass Biquads use old formulas
> 
> Please fix title now that this patch is only about the HighPass filter.

Right.

> 
> > Source/WebCore/platform/audio/Biquad.cpp:244
> > +        double alpha = sin(theta) / (2 * resonance);
> 
> Please use float for all these variables like in r265517.

But isn't the lowpass one also using doubles? Should I change it too?

[Lauro Moura]

(In reply to Darin Adler from comment #5)
> Comment on attachment 406470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406470&action=review
> 
> > LayoutTests/webaudio/biquad-allpass.html:33
> > +        threshold: 3.9337e-8,
> 
> Why are these threshold values so precise; this doesn’t seem to enhance the
> tests? How about changing maxAllowedError to 1e-7 in biquad-testing.js for
> all the tests instead of adding all of these special values?

They were introduced in this chromium change as initial thresholds after updating the formula to both filters to address some corner cases, although not much detail why so precise for the filters other than highpass and lowpass. Keeping them at least has the benefit of reducing the differences to the WPT tests.

https://codereview.chromium.org/1885723003

[Darin Adler]

Seems like a poor quality decision in the Chromium project. I would not replicate it here just because they did it.

You mention WPT tests: What is the relationship between WPT tests and these non-WPT tests?

[Chris Dumez]

(In reply to Lauro Moura from comment #6)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 406470 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406470&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        Lowpass and Highpass Biquads use old formulas
> > 
> > Please fix title now that this patch is only about the HighPass filter.
> 
> Right.
> 
> > 
> > > Source/WebCore/platform/audio/Biquad.cpp:244
> > > +        double alpha = sin(theta) / (2 * resonance);
> > 
> > Please use float for all these variables like in r265517.
> 
> But isn't the lowpass one also using doubles? Should I change it too?

Oh, never mind this comment. I made 2 changes on the same day and am confusing the two. We are indeed using double in the lowpass filter here so let's be consistent.

[Lauro Moura]

(In reply to Darin Adler from comment #8)
> Seems like a poor quality decision in the Chromium project. I would not
> replicate it here just because they did it.
> 
> You mention WPT tests: What is the relationship between WPT tests and these
> non-WPT tests?

These biquads tests are basically the same in our webaudio and WPT, changing only the harness around the tests (including this custom threshold).

Originally ours used 5.9e-8, "experimentally determined" (r106621/bug71413). And indeed in this original bug, Chris Rogers already raised concerns about maintenance issues with different thresholds.

I'm not really up to webaudio details, but while 1.e-7 would work for most (realistic?) cases, it could hide differences between these two highpass versions (they differed by ~2e-8).

[Darin Adler]

(In reply to Lauro Moura from comment #10)
> These biquads tests are basically the same in our webaudio and WPT, changing
> only the harness around the tests (including this custom threshold).

If so, then why do we need the webaudio tests? Let's just rely on the WPT ones.

[Lauro Moura]

Created attachment 406492 [details]
Updated patch removing old tests

Patch updated by removing old webaudio/biquad tests and rebaselining imported WPT ones.

[Lauro Moura]

Comment on attachment 406492 [details]
Updated patch removing old tests

The pending bubbles failures are not related to biquad, so cq+'ing.

Thanks for the review, Darin and Chris.

[EWS]

Committed r265599: <https://trac.webkit.org/changeset/265599>

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

[Radar WebKit Bug Importer]

<rdar://problem/66959686>

[Darin Adler]

Comment on attachment 406492 [details]
Updated patch removing old tests

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

> Source/WebCore/platform/audio/Biquad.cpp:249
> +        double b1 = -(2 * beta);

Why is this written like this ...

> Source/WebCore/platform/audio/Biquad.cpp:253
> +        double a1 = -2 * cosw;

... but this is written like this?