Bug 219201 - Poor resampling quality when using AudioContext sampleRate parameter
Summary: Poor resampling quality when using AudioContext sampleRate parameter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-20 05:45 PST by Ashley Gullen
Modified: 2020-11-21 20:51 PST (History)
10 users (show)

See Also:


Attachments
Patch (13.41 KB, patch)
2020-11-20 15:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (19.04 KB, patch)
2020-11-20 16:15 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.69 KB, patch)
2020-11-21 19:37 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ashley Gullen 2020-11-20 05:45:13 PST
Testing Safari TP116 with the new unprefixed Web Audio API. When using the AudioContext constructor's sampleRate parameter to set a different sampleRate to the default, the playback quality is poor with clear audible glitches. Presumably it's not correctly resampling the audio output.

Demo URL: https://downloads.scirra.com/labs/bugs/audio-samplerate/

Steps to reproduce:
1. Observe the default sample rate, and select a different sample rate in the dropdown box.
Testing on a 2015 MacBook Pro, the default sample rate is 44100, so in this case I select 48000 as the sample rate to create.

2. Click "Create" to create an AudioContext at the chosen sample rate

3. Click "Play" a few times

Expected result:
Good playback quality regardless of the AudioContext sample rate

Observed result:
Poor playback quality when the AudioContext sample rate is not the default
Comment 1 Ashley Gullen 2020-11-20 05:47:25 PST
Note this bug also prevents us working around Bug 218261 by changing the AudioContext sample rate.
Comment 2 Chris Dumez 2020-11-20 10:21:08 PST
I will investigate.

On my machine the default sample rate is 48000. If I select 44100 as sample rate when I create, I don't have an audio quality problem. I will try and use a higher sample rate than 48000.
Comment 3 Chris Dumez 2020-11-20 10:22:49 PST
(In reply to Chris Dumez from comment #2)
> I will investigate.
> 
> On my machine the default sample rate is 48000. If I select 44100 as sample
> rate when I create, I don't have an audio quality problem. I will try and
> use a higher sample rate than 48000.

Ok. If I tweak your test case to let me set 80000 as sample rate, I can reproduce the bad audio quality. I will work on it.
Comment 4 Chris Dumez 2020-11-20 15:00:01 PST
Created attachment 414734 [details]
Patch
Comment 5 Geoffrey Garen 2020-11-20 15:27:02 PST
Comment on attachment 414734 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:14
> +        called than once per channel and this resulted in very poor resampling quality.

called than => called more than

> Source/WebCore/platform/audio/MultiChannelResampler.cpp:65
>      void provideInput(AudioBus* bus, size_t framesToProcess) override

I feel like setCurrentChannel() and m_currentChannel should just go away; and instead we should pass currentChannel as an argument to this function.

Keeping m_currentChannel as state on the side is a bit of extra mental load, and it invites our caller to call the two functions out of sync.
Comment 6 Chris Dumez 2020-11-20 15:36:53 PST
(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 414734 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414734&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:14
> > +        called than once per channel and this resulted in very poor resampling quality.
> 
> called than => called more than
> 
> > Source/WebCore/platform/audio/MultiChannelResampler.cpp:65
> >      void provideInput(AudioBus* bus, size_t framesToProcess) override
> 
> I feel like setCurrentChannel() and m_currentChannel should just go away;
> and instead we should pass currentChannel as an argument to this function.
> 
> Keeping m_currentChannel as state on the side is a bit of extra mental load,
> and it invites our caller to call the two functions out of sync.

I agree. The thing is that provideInput() is part of a generic interface that is used for many things so I cannot easily change its parameters. I will see if I can add an overload to SincResampler that takes in a lambda instead of a client.
Comment 7 Chris Dumez 2020-11-20 16:15:31 PST
Created attachment 414743 [details]
Patch
Comment 8 EWS 2020-11-20 17:46:19 PST
Committed r270141: <https://trac.webkit.org/changeset/270141>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414743 [details].
Comment 9 Radar WebKit Bug Importer 2020-11-20 17:47:17 PST
<rdar://problem/71648006>
Comment 10 Chris Dumez 2020-11-21 19:04:13 PST
Reverted r270141 for reason:

Caused assertions on bots

Committed r270155: <https://trac.webkit.org/changeset/270155>
Comment 11 Chris Dumez 2020-11-21 19:37:58 PST
Created attachment 414780 [details]
Patch
Comment 12 Chris Dumez 2020-11-21 20:51:38 PST
Comment on attachment 414780 [details]
Patch

Clearing flags on attachment: 414780

Committed r270157: <https://trac.webkit.org/changeset/270157>
Comment 13 Chris Dumez 2020-11-21 20:51:40 PST
All reviewed patches have been landed.  Closing bug.