Bug 219201 – Poor resampling quality when using AudioContext sampleRate parameter

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

[Ashley Gullen]

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

[Ashley Gullen]

Note this bug also prevents us working around Bug 218261 by changing the AudioContext sample rate.

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 414734 [details]
Patch

[Geoffrey Garen]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 414743 [details]
Patch

[EWS]

Committed r270141: <https://trac.webkit.org/changeset/270141>

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

[Radar WebKit Bug Importer]

<rdar://problem/71648006>

[Chris Dumez]

Reverted r270141 for reason:

Caused assertions on bots

Committed r270155: <https://trac.webkit.org/changeset/270155>

[Chris Dumez]

Created attachment 414780 [details]
Patch

[Chris Dumez]

Comment on attachment 414780 [details]
Patch

Clearing flags on attachment: 414780

Committed r270157: <https://trac.webkit.org/changeset/270157>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Ashley Gullen]

Can confirm it works well now in TP117. Thanks for the fix! This also gives us a workaround to make use of ConvolverNode when using a WebAssembly Opus decoder outputting 48KHz buffers.