Summary: | Poor resampling quality when using AudioContext sampleRate parameter | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ashley Gullen <ashley> | ||||||||
Component: | Web Audio | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ashley Gullen
2020-11-20 05:45:13 PST
Note this bug also prevents us working around Bug 218261 by changing the AudioContext sample rate. 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. (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. Created attachment 414734 [details]
Patch
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. (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. Created attachment 414743 [details]
Patch
Committed r270141: <https://trac.webkit.org/changeset/270141> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414743 [details]. Reverted r270141 for reason: Caused assertions on bots Committed r270155: <https://trac.webkit.org/changeset/270155> Created attachment 414780 [details]
Patch
Comment on attachment 414780 [details] Patch Clearing flags on attachment: 414780 Committed r270157: <https://trac.webkit.org/changeset/270157> All reviewed patches have been landed. Closing bug. 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. |