Bug 181191 - Highpass Biquads use old formulas
Summary: Highpass Biquads use old formulas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari Technology Preview
Hardware: All macOS 10.13
: P2 Normal
Assignee: Lauro Moura
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-29 18:16 PST by Ricci Adams
Modified: 2020-08-14 17:49 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.89 KB, patch)
2020-08-12 13:26 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff
Updated patch removing old tests (56.07 KB, patch)
2020-08-12 20:34 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ricci Adams 2017-12-29 18:16:49 PST
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
Comment 1 Ricci Adams 2017-12-30 17:24:04 PST
Note: The relevant commit to Blink is https://codereview.chromium.org/1885723003
Comment 2 Lauro Moura 2020-08-12 13:26:46 PDT
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.
Comment 3 Lauro Moura 2020-08-12 13:29:41 PDT
(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.
Comment 4 Chris Dumez 2020-08-12 13:33:52 PDT
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.
Comment 5 Darin Adler 2020-08-12 13:34:40 PDT
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?
Comment 6 Lauro Moura 2020-08-12 14:18:32 PDT
(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?
Comment 7 Lauro Moura 2020-08-12 14:24:47 PDT
(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
Comment 8 Darin Adler 2020-08-12 14:28:21 PDT
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?
Comment 9 Chris Dumez 2020-08-12 14:30:42 PDT
(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.
Comment 10 Lauro Moura 2020-08-12 15:43:28 PDT
(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).
Comment 11 Darin Adler 2020-08-12 16:07:24 PDT
(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.
Comment 12 Lauro Moura 2020-08-12 20:34:03 PDT
Created attachment 406492 [details]
Updated patch removing old tests

Patch updated by removing old webaudio/biquad tests and rebaselining imported WPT ones.
Comment 13 Lauro Moura 2020-08-12 21:36:16 PDT
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.
Comment 14 EWS 2020-08-12 21:39:06 PDT
Committed r265599: <https://trac.webkit.org/changeset/265599>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406492 [details].
Comment 15 Radar WebKit Bug Importer 2020-08-12 21:40:17 PDT
<rdar://problem/66959686>
Comment 16 Darin Adler 2020-08-14 17:49:28 PDT
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?