Skip to content

Commit 1c7c49a

Browse files
cdumezcdumez
committed
Poor resampling quality when using AudioContext sampleRate parameter
https://bugs.webkit.org/show_bug.cgi?id=219201 Reviewed by Geoff Garen. Source/WebCore: MultiChannelResampler uses a SincResampler per audio channel. In MultiChannelResampler::process(), it was calling SincResampler::process() for each channel, which would potentially end up calling MultiChannelResampler::ChannelProvider::provideInput() to provide channel data used for resampling. The issue was that MultiChannelResampler::ChannelProvider::provideInput() is implemented in such a way that things will break if provideInput() gets called more than once per channel. When using an AudioContext's sample rate larger than the hardware sample rate, provideInput() was getting called more than once per channel and this resulted in very poor resampling quality. To address the issue, MultiChannelResampler::process() now processes the data in chunks that are small enough to guarantee that MultiChannelResampler::ChannelProvider::provideInput() will never get called more than once per audio channel. The fix is based on the corresponding MultiChannelResampler / SincResampler implementation in Chrome: - https://github.com/chromium/chromium/blob/master/media/base/multi_channel_resampler.cc - https://github.com/chromium/chromium/blob/master/media/base/sinc_resampler.cc Tests: webaudio/audiocontext-large-samplerate.html webaudio/audiocontext-low-samplerate.html * platform/audio/MultiChannelResampler.cpp: (WebCore::MultiChannelResampler::ChannelProvider::setProvider): (WebCore::MultiChannelResampler::ChannelProvider::setCurrentChannel): (WebCore::MultiChannelResampler::process): * platform/audio/MultiChannelResampler.h: * platform/audio/SincResampler.cpp: (WebCore::calculateChunkSize): (WebCore::SincResampler::updateRegions): * platform/audio/SincResampler.h: LayoutTests: Add layout test coverage that would hit assertions in debug. * webaudio/audiocontext-large-samplerate-expected.txt: Added. * webaudio/audiocontext-large-samplerate.html: Added. * webaudio/audiocontext-low-samplerate-expected.txt: Added. * webaudio/audiocontext-low-samplerate.html: Added. Canonical link: https://commits.webkit.org/231866@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@270157 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 59b2c3b commit 1c7c49a

10 files changed

Lines changed: 185 additions & 52 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2020-11-21 Chris Dumez <cdumez@apple.com>
2+
3+
Poor resampling quality when using AudioContext sampleRate parameter
4+
https://bugs.webkit.org/show_bug.cgi?id=219201
5+
6+
Reviewed by Geoff Garen.
7+
8+
Add layout test coverage that would hit assertions in debug.
9+
10+
* webaudio/audiocontext-large-samplerate-expected.txt: Added.
11+
* webaudio/audiocontext-large-samplerate.html: Added.
12+
* webaudio/audiocontext-low-samplerate-expected.txt: Added.
13+
* webaudio/audiocontext-low-samplerate.html: Added.
14+
115
2020-11-21 Chris Dumez <cdumez@apple.com>
216

317
Unreviewed, reverting r270141.

‎LayoutTests/webaudio/audiocontext-large-samplerate-expected.txt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Tests that we do not crash when using a very large sample rate
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS context.sampleRate is 384000
7+
PASS context.state is "running"
8+
PASS successfullyParsed is true
9+
10+
TEST COMPLETE
11+

‎LayoutTests/webaudio/audiocontext-large-samplerate.html‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<script src="../resources/js-test.js"></script>
4+
<body>
5+
<script>
6+
description("Tests that we do not crash when using a very large sample rate");
7+
jsTestIsAsync = true;
8+
9+
context = new AudioContext({ sampleRate: 384000 });
10+
shouldBe("context.sampleRate", "384000");
11+
12+
node = new ConstantSourceNode(context, { offset: 0.5 });
13+
node.connect(context.destination);
14+
node.start();
15+
16+
setTimeout(() => {
17+
shouldBeEqualToString("context.state", "running");
18+
finishJSTest();
19+
}, 100);
20+
</script>
21+
</body>
22+
</html>

‎LayoutTests/webaudio/audiocontext-low-samplerate-expected.txt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Tests that we do not crash when using a very low sample rate
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS context.sampleRate is 3000
7+
PASS context.state is "running"
8+
PASS successfullyParsed is true
9+
10+
TEST COMPLETE
11+

‎LayoutTests/webaudio/audiocontext-low-samplerate.html‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<script src="../resources/js-test.js"></script>
4+
<body>
5+
<script>
6+
description("Tests that we do not crash when using a very low sample rate");
7+
jsTestIsAsync = true;
8+
9+
context = new AudioContext({ sampleRate: 3000 });
10+
shouldBe("context.sampleRate", "3000");
11+
12+
node = new ConstantSourceNode(context, { offset: 0.5 });
13+
node.connect(context.destination);
14+
node.start();
15+
16+
setTimeout(() => {
17+
shouldBeEqualToString("context.state", "running");
18+
finishJSTest();
19+
}, 100);
20+
</script>
21+
</body>
22+
</html>

‎Source/WebCore/ChangeLog‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,40 @@
1+
2020-11-21 Chris Dumez <cdumez@apple.com>
2+
3+
Poor resampling quality when using AudioContext sampleRate parameter
4+
https://bugs.webkit.org/show_bug.cgi?id=219201
5+
6+
Reviewed by Geoff Garen.
7+
8+
MultiChannelResampler uses a SincResampler per audio channel. In MultiChannelResampler::process(),
9+
it was calling SincResampler::process() for each channel, which would potentially end up calling
10+
MultiChannelResampler::ChannelProvider::provideInput() to provide channel data used for resampling.
11+
The issue was that MultiChannelResampler::ChannelProvider::provideInput() is implemented in such
12+
a way that things will break if provideInput() gets called more than once per channel. When using
13+
an AudioContext's sample rate larger than the hardware sample rate, provideInput() was getting
14+
called more than once per channel and this resulted in very poor resampling quality.
15+
16+
To address the issue, MultiChannelResampler::process() now processes the data in chunks that
17+
are small enough to guarantee that MultiChannelResampler::ChannelProvider::provideInput() will
18+
never get called more than once per audio channel.
19+
20+
The fix is based on the corresponding MultiChannelResampler / SincResampler implementation in
21+
Chrome:
22+
- https://github.com/chromium/chromium/blob/master/media/base/multi_channel_resampler.cc
23+
- https://github.com/chromium/chromium/blob/master/media/base/sinc_resampler.cc
24+
25+
Tests: webaudio/audiocontext-large-samplerate.html
26+
webaudio/audiocontext-low-samplerate.html
27+
28+
* platform/audio/MultiChannelResampler.cpp:
29+
(WebCore::MultiChannelResampler::ChannelProvider::setProvider):
30+
(WebCore::MultiChannelResampler::ChannelProvider::setCurrentChannel):
31+
(WebCore::MultiChannelResampler::process):
32+
* platform/audio/MultiChannelResampler.h:
33+
* platform/audio/SincResampler.cpp:
34+
(WebCore::calculateChunkSize):
35+
(WebCore::SincResampler::updateRegions):
36+
* platform/audio/SincResampler.h:
37+
138
2020-11-21 Simon Fraser <simon.fraser@apple.com>
239

340
Propagate the 'wheelEventGesturesBecomeNonBlocking' setting to the ScrollingTree

‎Source/WebCore/platform/audio/MultiChannelResampler.cpp‎

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -38,63 +38,59 @@ namespace WebCore {
3838

3939
// ChannelProvider provides a single channel of audio data (one channel at a time) for each channel
4040
// of data provided to us in a multi-channel provider.
41-
class MultiChannelResampler::ChannelProvider : public AudioSourceProvider {
41+
class MultiChannelResampler::ChannelProvider {
4242
WTF_MAKE_FAST_ALLOCATED;
4343
public:
44-
explicit ChannelProvider(unsigned numberOfChannels)
45-
: m_numberOfChannels(numberOfChannels)
44+
explicit ChannelProvider(unsigned numberOfChannels, unsigned requestFrames)
45+
: m_multiChannelBus(AudioBus::create(numberOfChannels, requestFrames))
4646
{
4747
}
4848

4949
void setProvider(AudioSourceProvider* multiChannelProvider)
5050
{
51-
m_currentChannel = 0;
52-
m_framesToProcess = 0;
5351
m_multiChannelProvider = multiChannelProvider;
5452
}
5553

5654
// provideInput() will be called once for each channel, starting with the first channel.
5755
// Each time it's called, it will provide the next channel of data.
58-
void provideInput(AudioBus* bus, size_t framesToProcess) override
56+
void provideInputForChannel(AudioBus* bus, size_t framesToProcess, unsigned channelIndex)
5957
{
58+
ASSERT(channelIndex < m_multiChannelBus->numberOfChannels());
59+
ASSERT(framesToProcess <= m_multiChannelBus->length());
60+
if (framesToProcess > m_multiChannelBus->length())
61+
return;
62+
6063
bool isBusGood = bus && bus->numberOfChannels() == 1;
6164
ASSERT(isBusGood);
6265
if (!isBusGood)
6366
return;
6467

6568
// Get the data from the multi-channel provider when the first channel asks for it.
6669
// For subsequent channels, we can just dish out the channel data from that (stored in m_multiChannelBus).
67-
if (!m_currentChannel) {
70+
if (!channelIndex) {
6871
m_framesToProcess = framesToProcess;
69-
m_multiChannelBus = AudioBus::create(m_numberOfChannels, framesToProcess);
7072
m_multiChannelProvider->provideInput(m_multiChannelBus.get(), framesToProcess);
7173
}
7274

7375
// All channels must ask for the same amount. This should always be the case, but let's just make sure.
74-
bool isGood = m_multiChannelBus.get() && framesToProcess == m_framesToProcess;
76+
bool isGood = framesToProcess == m_framesToProcess;
7577
ASSERT(isGood);
7678
if (!isGood)
7779
return;
7880

7981
// Copy the channel data from what we received from m_multiChannelProvider.
80-
ASSERT(m_currentChannel <= m_numberOfChannels);
81-
if (m_currentChannel < m_numberOfChannels) {
82-
memcpy(bus->channel(0)->mutableData(), m_multiChannelBus->channel(m_currentChannel)->data(), sizeof(float) * framesToProcess);
83-
++m_currentChannel;
84-
}
82+
memcpy(bus->channel(0)->mutableData(), m_multiChannelBus->channel(channelIndex)->data(), sizeof(float) * framesToProcess);
8583
}
8684

8785
private:
8886
AudioSourceProvider* m_multiChannelProvider { nullptr };
8987
RefPtr<AudioBus> m_multiChannelBus;
90-
unsigned m_numberOfChannels { 0 };
91-
unsigned m_currentChannel { 0 };
9288
size_t m_framesToProcess { 0 }; // Used to verify that all channels ask for the same amount.
9389
};
9490

95-
MultiChannelResampler::MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, Optional<unsigned> requestFrames)
91+
MultiChannelResampler::MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, unsigned requestFrames)
9692
: m_numberOfChannels(numberOfChannels)
97-
, m_channelProvider(makeUnique<ChannelProvider>(m_numberOfChannels))
93+
, m_channelProvider(makeUnique<ChannelProvider>(m_numberOfChannels, requestFrames))
9894
{
9995
// Create each channel's resampler.
10096
for (unsigned channelIndex = 0; channelIndex < numberOfChannels; ++channelIndex)
@@ -112,14 +108,25 @@ void MultiChannelResampler::process(AudioSourceProvider* provider, AudioBus* des
112108
// channelProvider wraps the original multi-channel provider and dishes out one channel at a time.
113109
m_channelProvider->setProvider(provider);
114110

115-
for (unsigned channelIndex = 0; channelIndex < m_numberOfChannels; ++channelIndex) {
116-
// Depending on the sample-rate scale factor, and the internal buffering used in a SincResampler
117-
// kernel, this call to process() will only sometimes call provideInput() on the channelProvider.
118-
// However, if it calls provideInput() for the first channel, then it will call it for the remaining
119-
// channels, since they all buffer in the same way and are processing the same number of frames.
120-
m_kernels[channelIndex]->process(m_channelProvider.get(),
121-
destination->channel(channelIndex)->mutableData(),
122-
framesToProcess);
111+
// We need to ensure that SincResampler only calls provideInput() once for each channel or it will confuse the logic
112+
// inside ChannelProvider. To ensure this, we chunk the number of requested frames into SincResampler::chunkSize()
113+
// sized chunks. SincResampler guarantees it will only call provideInput() once once we resample this way.
114+
m_outputFramesReady = 0;
115+
while (m_outputFramesReady < framesToProcess) {
116+
size_t chunkSize = m_kernels[0]->chunkSize();
117+
size_t framesThisTime = std::min(framesToProcess - m_outputFramesReady, chunkSize);
118+
119+
for (unsigned channelIndex = 0; channelIndex < m_numberOfChannels; ++channelIndex) {
120+
ASSERT(chunkSize == m_kernels[channelIndex]->chunkSize());
121+
bool wasProvideInputCalled = false;
122+
m_kernels[channelIndex]->process(destination->channel(channelIndex)->mutableData() + m_outputFramesReady, framesThisTime, [this, channelIndex, &wasProvideInputCalled](AudioBus* bus, size_t framesToProcess) {
123+
ASSERT_WITH_MESSAGE(!wasProvideInputCalled, "provideInputForChannel should only be called once");
124+
wasProvideInputCalled = true;
125+
m_channelProvider->provideInputForChannel(bus, framesToProcess, channelIndex);
126+
});
127+
}
128+
129+
m_outputFramesReady += framesThisTime;
123130
}
124131

125132
m_channelProvider->setProvider(nullptr);

‎Source/WebCore/platform/audio/MultiChannelResampler.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class MultiChannelResampler final {
4141
WTF_MAKE_FAST_ALLOCATED;
4242
public:
4343
// requestFrames constrols the size of the buffer in frames when AudioSourceProvider::provideInput() is called.
44-
explicit MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, Optional<unsigned> requestFrames = WTF::nullopt);
44+
explicit MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, unsigned requestFrames = SincResampler::defaultRequestFrames);
4545
~MultiChannelResampler();
4646

4747
// Process given AudioSourceProvider for streaming applications.
@@ -59,6 +59,7 @@ class MultiChannelResampler final {
5959

6060
class ChannelProvider;
6161
std::unique_ptr<ChannelProvider> m_channelProvider;
62+
size_t m_outputFramesReady { 0 };
6263
};
6364

6465
} // namespace WebCore

‎Source/WebCore/platform/audio/SincResampler.cpp‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -111,14 +111,18 @@
111111

112112
namespace WebCore {
113113

114-
constexpr unsigned defaultRequestFrames { 512 };
115114
constexpr unsigned kernelSize { 32 };
116115
constexpr unsigned numberOfKernelOffsets { 32 };
117116

118-
SincResampler::SincResampler(double scaleFactor, Optional<unsigned> requestFrames)
117+
static size_t calculateChunkSize(unsigned blockSize, double scaleFactor)
118+
{
119+
return blockSize / scaleFactor;
120+
}
121+
122+
SincResampler::SincResampler(double scaleFactor, unsigned requestFrames)
119123
: m_scaleFactor(scaleFactor)
120124
, m_kernelStorage(kernelSize * (numberOfKernelOffsets + 1))
121-
, m_requestFrames(requestFrames.valueOr(defaultRequestFrames))
125+
, m_requestFrames(requestFrames)
122126
, m_inputBuffer(m_requestFrames + kernelSize) // See input buffer layout above.
123127
, m_r1(m_inputBuffer.data())
124128
, m_r2(m_inputBuffer.data() + kernelSize / 2)
@@ -137,6 +141,7 @@ void SincResampler::updateRegions(bool isSecondLoad)
137141
m_r3 = m_r0 + m_requestFrames - kernelSize;
138142
m_r4 = m_r0 + m_requestFrames - kernelSize / 2;
139143
m_blockSize = m_r4 - m_r2;
144+
m_chunkSize = calculateChunkSize(m_blockSize, m_scaleFactor);
140145

141146
// m_r1 at the beginning of the buffer.
142147
ASSERT(m_r1 == m_inputBuffer.data());
@@ -187,10 +192,10 @@ void SincResampler::initializeKernel()
187192
}
188193
}
189194

190-
void SincResampler::consumeSource(float* buffer, unsigned numberOfSourceFrames)
195+
void SincResampler::consumeSource(float* buffer, unsigned numberOfSourceFrames, const Function<void(AudioBus* bus, size_t framesToProcess)>& provideInput)
191196
{
192-
ASSERT(m_sourceProvider);
193-
if (!m_sourceProvider)
197+
ASSERT(provideInput);
198+
if (!provideInput)
194199
return;
195200

196201
// Wrap the provided buffer by an AudioBus for use by the source provider.
@@ -200,14 +205,14 @@ void SincResampler::consumeSource(float* buffer, unsigned numberOfSourceFrames)
200205
// FIXME: Find a way to make the following const-correct:
201206
m_internalBus->setChannelMemory(0, buffer, numberOfSourceFrames);
202207

203-
m_sourceProvider->provideInput(m_internalBus.get(), numberOfSourceFrames);
208+
provideInput(m_internalBus.get(), numberOfSourceFrames);
204209
}
205210

206211
namespace {
207212

208-
// BufferSourceProvider is an AudioSourceProvider wrapping an in-memory buffer.
213+
// BufferSourceProvider is an audio source provider wrapping an in-memory buffer.
209214

210-
class BufferSourceProvider : public AudioSourceProvider {
215+
class BufferSourceProvider {
211216
public:
212217
explicit BufferSourceProvider(const float* source, size_t numberOfSourceFrames)
213218
: m_source(source)
@@ -216,7 +221,7 @@ class BufferSourceProvider : public AudioSourceProvider {
216221
}
217222

218223
// Consumes samples from the in-memory buffer.
219-
void provideInput(AudioBus* bus, size_t framesToProcess) override
224+
void provideInput(AudioBus* bus, size_t framesToProcess)
220225
{
221226
ASSERT(m_source && bus);
222227
if (!m_source || !bus)
@@ -253,27 +258,27 @@ void SincResampler::process(const float* source, float* destination, unsigned nu
253258

254259
while (remaining) {
255260
unsigned framesThisTime = std::min(remaining, m_requestFrames);
256-
process(&sourceProvider, destination, framesThisTime);
261+
process(destination, framesThisTime, [&sourceProvider](AudioBus* bus, size_t framesToProcess) {
262+
sourceProvider.provideInput(bus, framesToProcess);
263+
});
257264

258265
destination += framesThisTime;
259266
remaining -= framesThisTime;
260267
}
261268
}
262269

263-
void SincResampler::process(AudioSourceProvider* sourceProvider, float* destination, size_t framesToProcess)
270+
void SincResampler::process(float* destination, size_t framesToProcess, const Function<void(AudioBus* bus, size_t framesToProcess)>& provideInput)
264271
{
265-
ASSERT(sourceProvider);
266-
if (!sourceProvider)
272+
ASSERT(provideInput);
273+
if (!provideInput)
267274
return;
268-
269-
m_sourceProvider = sourceProvider;
270275

271276
unsigned numberOfDestinationFrames = framesToProcess;
272277

273278
// Step (1)
274279
// Prime the input buffer at the start of the input stream.
275280
if (!m_isBufferPrimed) {
276-
consumeSource(m_r0, m_requestFrames);
281+
consumeSource(m_r0, m_requestFrames, provideInput);
277282
m_isBufferPrimed = true;
278283
}
279284

@@ -521,7 +526,7 @@ void SincResampler::process(AudioSourceProvider* sourceProvider, float* destinat
521526

522527
// Step (5)
523528
// Refresh the buffer with more input.
524-
consumeSource(m_r0, m_requestFrames);
529+
consumeSource(m_r0, m_requestFrames, provideInput);
525530
}
526531
}
527532

0 commit comments

Comments
 (0)