Diff
Modified: trunk/LayoutTests/ChangeLog (270156 => 270157)
--- trunk/LayoutTests/ChangeLog 2020-11-22 03:54:00 UTC (rev 270156)
+++ trunk/LayoutTests/ChangeLog 2020-11-22 04:51:13 UTC (rev 270157)
@@ -1,5 +1,19 @@
2020-11-21 Chris Dumez <[email protected]>
+ Poor resampling quality when using AudioContext sampleRate parameter
+ https://bugs.webkit.org/show_bug.cgi?id=219201
+
+ Reviewed by Geoff Garen.
+
+ 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.
+
+2020-11-21 Chris Dumez <[email protected]>
+
Unreviewed, reverting r270141.
Caused assertions on bots
Added: trunk/LayoutTests/webaudio/audiocontext-large-samplerate-expected.txt (0 => 270157)
--- trunk/LayoutTests/webaudio/audiocontext-large-samplerate-expected.txt (rev 0)
+++ trunk/LayoutTests/webaudio/audiocontext-large-samplerate-expected.txt 2020-11-22 04:51:13 UTC (rev 270157)
@@ -0,0 +1,11 @@
+Tests that we do not crash when using a very large sample rate
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS context.sampleRate is 384000
+PASS context.state is "running"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/webaudio/audiocontext-large-samplerate.html (0 => 270157)
--- trunk/LayoutTests/webaudio/audiocontext-large-samplerate.html (rev 0)
+++ trunk/LayoutTests/webaudio/audiocontext-large-samplerate.html 2020-11-22 04:51:13 UTC (rev 270157)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<script src=""
+<body>
+<script>
+description("Tests that we do not crash when using a very large sample rate");
+jsTestIsAsync = true;
+
+context = new AudioContext({ sampleRate: 384000 });
+shouldBe("context.sampleRate", "384000");
+
+node = new ConstantSourceNode(context, { offset: 0.5 });
+node.connect(context.destination);
+node.start();
+
+setTimeout(() => {
+ shouldBeEqualToString("context.state", "running");
+ finishJSTest();
+}, 100);
+</script>
+</body>
+</html>
Added: trunk/LayoutTests/webaudio/audiocontext-low-samplerate-expected.txt (0 => 270157)
--- trunk/LayoutTests/webaudio/audiocontext-low-samplerate-expected.txt (rev 0)
+++ trunk/LayoutTests/webaudio/audiocontext-low-samplerate-expected.txt 2020-11-22 04:51:13 UTC (rev 270157)
@@ -0,0 +1,11 @@
+Tests that we do not crash when using a very low sample rate
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS context.sampleRate is 3000
+PASS context.state is "running"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/webaudio/audiocontext-low-samplerate.html (0 => 270157)
--- trunk/LayoutTests/webaudio/audiocontext-low-samplerate.html (rev 0)
+++ trunk/LayoutTests/webaudio/audiocontext-low-samplerate.html 2020-11-22 04:51:13 UTC (rev 270157)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<script src=""
+<body>
+<script>
+description("Tests that we do not crash when using a very low sample rate");
+jsTestIsAsync = true;
+
+context = new AudioContext({ sampleRate: 3000 });
+shouldBe("context.sampleRate", "3000");
+
+node = new ConstantSourceNode(context, { offset: 0.5 });
+node.connect(context.destination);
+node.start();
+
+setTimeout(() => {
+ shouldBeEqualToString("context.state", "running");
+ finishJSTest();
+}, 100);
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (270156 => 270157)
--- trunk/Source/WebCore/ChangeLog 2020-11-22 03:54:00 UTC (rev 270156)
+++ trunk/Source/WebCore/ChangeLog 2020-11-22 04:51:13 UTC (rev 270157)
@@ -1,3 +1,40 @@
+2020-11-21 Chris Dumez <[email protected]>
+
+ Poor resampling quality when using AudioContext sampleRate parameter
+ https://bugs.webkit.org/show_bug.cgi?id=219201
+
+ Reviewed by Geoff Garen.
+
+ 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:
+
2020-11-21 Simon Fraser <[email protected]>
Propagate the 'wheelEventGesturesBecomeNonBlocking' setting to the ScrollingTree
Modified: trunk/Source/WebCore/platform/audio/MultiChannelResampler.cpp (270156 => 270157)
--- trunk/Source/WebCore/platform/audio/MultiChannelResampler.cpp 2020-11-22 03:54:00 UTC (rev 270156)
+++ trunk/Source/WebCore/platform/audio/MultiChannelResampler.cpp 2020-11-22 04:51:13 UTC (rev 270157)
@@ -38,25 +38,28 @@
// ChannelProvider provides a single channel of audio data (one channel at a time) for each channel
// of data provided to us in a multi-channel provider.
-class MultiChannelResampler::ChannelProvider : public AudioSourceProvider {
+class MultiChannelResampler::ChannelProvider {
WTF_MAKE_FAST_ALLOCATED;
public:
- explicit ChannelProvider(unsigned numberOfChannels)
- : m_numberOfChannels(numberOfChannels)
+ explicit ChannelProvider(unsigned numberOfChannels, unsigned requestFrames)
+ : m_multiChannelBus(AudioBus::create(numberOfChannels, requestFrames))
{
}
void setProvider(AudioSourceProvider* multiChannelProvider)
{
- m_currentChannel = 0;
- m_framesToProcess = 0;
m_multiChannelProvider = multiChannelProvider;
}
// provideInput() will be called once for each channel, starting with the first channel.
// Each time it's called, it will provide the next channel of data.
- void provideInput(AudioBus* bus, size_t framesToProcess) override
+ void provideInputForChannel(AudioBus* bus, size_t framesToProcess, unsigned channelIndex)
{
+ ASSERT(channelIndex < m_multiChannelBus->numberOfChannels());
+ ASSERT(framesToProcess <= m_multiChannelBus->length());
+ if (framesToProcess > m_multiChannelBus->length())
+ return;
+
bool isBusGood = bus && bus->numberOfChannels() == 1;
ASSERT(isBusGood);
if (!isBusGood)
@@ -64,37 +67,30 @@
// Get the data from the multi-channel provider when the first channel asks for it.
// For subsequent channels, we can just dish out the channel data from that (stored in m_multiChannelBus).
- if (!m_currentChannel) {
+ if (!channelIndex) {
m_framesToProcess = framesToProcess;
- m_multiChannelBus = AudioBus::create(m_numberOfChannels, framesToProcess);
m_multiChannelProvider->provideInput(m_multiChannelBus.get(), framesToProcess);
}
// All channels must ask for the same amount. This should always be the case, but let's just make sure.
- bool isGood = m_multiChannelBus.get() && framesToProcess == m_framesToProcess;
+ bool isGood = framesToProcess == m_framesToProcess;
ASSERT(isGood);
if (!isGood)
return;
// Copy the channel data from what we received from m_multiChannelProvider.
- ASSERT(m_currentChannel <= m_numberOfChannels);
- if (m_currentChannel < m_numberOfChannels) {
- memcpy(bus->channel(0)->mutableData(), m_multiChannelBus->channel(m_currentChannel)->data(), sizeof(float) * framesToProcess);
- ++m_currentChannel;
- }
+ memcpy(bus->channel(0)->mutableData(), m_multiChannelBus->channel(channelIndex)->data(), sizeof(float) * framesToProcess);
}
private:
AudioSourceProvider* m_multiChannelProvider { nullptr };
RefPtr<AudioBus> m_multiChannelBus;
- unsigned m_numberOfChannels { 0 };
- unsigned m_currentChannel { 0 };
size_t m_framesToProcess { 0 }; // Used to verify that all channels ask for the same amount.
};
-MultiChannelResampler::MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, Optional<unsigned> requestFrames)
+MultiChannelResampler::MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, unsigned requestFrames)
: m_numberOfChannels(numberOfChannels)
- , m_channelProvider(makeUnique<ChannelProvider>(m_numberOfChannels))
+ , m_channelProvider(makeUnique<ChannelProvider>(m_numberOfChannels, requestFrames))
{
// Create each channel's resampler.
for (unsigned channelIndex = 0; channelIndex < numberOfChannels; ++channelIndex)
@@ -112,14 +108,25 @@
// channelProvider wraps the original multi-channel provider and dishes out one channel at a time.
m_channelProvider->setProvider(provider);
- for (unsigned channelIndex = 0; channelIndex < m_numberOfChannels; ++channelIndex) {
- // Depending on the sample-rate scale factor, and the internal buffering used in a SincResampler
- // kernel, this call to process() will only sometimes call provideInput() on the channelProvider.
- // However, if it calls provideInput() for the first channel, then it will call it for the remaining
- // channels, since they all buffer in the same way and are processing the same number of frames.
- m_kernels[channelIndex]->process(m_channelProvider.get(),
- destination->channel(channelIndex)->mutableData(),
- framesToProcess);
+ // We need to ensure that SincResampler only calls provideInput() once for each channel or it will confuse the logic
+ // inside ChannelProvider. To ensure this, we chunk the number of requested frames into SincResampler::chunkSize()
+ // sized chunks. SincResampler guarantees it will only call provideInput() once once we resample this way.
+ m_outputFramesReady = 0;
+ while (m_outputFramesReady < framesToProcess) {
+ size_t chunkSize = m_kernels[0]->chunkSize();
+ size_t framesThisTime = std::min(framesToProcess - m_outputFramesReady, chunkSize);
+
+ for (unsigned channelIndex = 0; channelIndex < m_numberOfChannels; ++channelIndex) {
+ ASSERT(chunkSize == m_kernels[channelIndex]->chunkSize());
+ bool wasProvideInputCalled = false;
+ m_kernels[channelIndex]->process(destination->channel(channelIndex)->mutableData() + m_outputFramesReady, framesThisTime, [this, channelIndex, &wasProvideInputCalled](AudioBus* bus, size_t framesToProcess) {
+ ASSERT_WITH_MESSAGE(!wasProvideInputCalled, "provideInputForChannel should only be called once");
+ wasProvideInputCalled = true;
+ m_channelProvider->provideInputForChannel(bus, framesToProcess, channelIndex);
+ });
+ }
+
+ m_outputFramesReady += framesThisTime;
}
m_channelProvider->setProvider(nullptr);
Modified: trunk/Source/WebCore/platform/audio/MultiChannelResampler.h (270156 => 270157)
--- trunk/Source/WebCore/platform/audio/MultiChannelResampler.h 2020-11-22 03:54:00 UTC (rev 270156)
+++ trunk/Source/WebCore/platform/audio/MultiChannelResampler.h 2020-11-22 04:51:13 UTC (rev 270157)
@@ -41,7 +41,7 @@
WTF_MAKE_FAST_ALLOCATED;
public:
// requestFrames constrols the size of the buffer in frames when AudioSourceProvider::provideInput() is called.
- explicit MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, Optional<unsigned> requestFrames = WTF::nullopt);
+ explicit MultiChannelResampler(double scaleFactor, unsigned numberOfChannels, unsigned requestFrames = SincResampler::defaultRequestFrames);
~MultiChannelResampler();
// Process given AudioSourceProvider for streaming applications.
@@ -59,6 +59,7 @@
class ChannelProvider;
std::unique_ptr<ChannelProvider> m_channelProvider;
+ size_t m_outputFramesReady { 0 };
};
} // namespace WebCore
Modified: trunk/Source/WebCore/platform/audio/SincResampler.cpp (270156 => 270157)
--- trunk/Source/WebCore/platform/audio/SincResampler.cpp 2020-11-22 03:54:00 UTC (rev 270156)
+++ trunk/Source/WebCore/platform/audio/SincResampler.cpp 2020-11-22 04:51:13 UTC (rev 270157)
@@ -111,14 +111,18 @@
namespace WebCore {
-constexpr unsigned defaultRequestFrames { 512 };
constexpr unsigned kernelSize { 32 };
constexpr unsigned numberOfKernelOffsets { 32 };
-SincResampler::SincResampler(double scaleFactor, Optional<unsigned> requestFrames)
+static size_t calculateChunkSize(unsigned blockSize, double scaleFactor)
+{
+ return blockSize / scaleFactor;
+}
+
+SincResampler::SincResampler(double scaleFactor, unsigned requestFrames)
: m_scaleFactor(scaleFactor)
, m_kernelStorage(kernelSize * (numberOfKernelOffsets + 1))
- , m_requestFrames(requestFrames.valueOr(defaultRequestFrames))
+ , m_requestFrames(requestFrames)
, m_inputBuffer(m_requestFrames + kernelSize) // See input buffer layout above.
, m_r1(m_inputBuffer.data())
, m_r2(m_inputBuffer.data() + kernelSize / 2)
@@ -137,6 +141,7 @@
m_r3 = m_r0 + m_requestFrames - kernelSize;
m_r4 = m_r0 + m_requestFrames - kernelSize / 2;
m_blockSize = m_r4 - m_r2;
+ m_chunkSize = calculateChunkSize(m_blockSize, m_scaleFactor);
// m_r1 at the beginning of the buffer.
ASSERT(m_r1 == m_inputBuffer.data());
@@ -187,10 +192,10 @@
}
}
-void SincResampler::consumeSource(float* buffer, unsigned numberOfSourceFrames)
+void SincResampler::consumeSource(float* buffer, unsigned numberOfSourceFrames, const Function<void(AudioBus* bus, size_t framesToProcess)>& provideInput)
{
- ASSERT(m_sourceProvider);
- if (!m_sourceProvider)
+ ASSERT(provideInput);
+ if (!provideInput)
return;
// Wrap the provided buffer by an AudioBus for use by the source provider.
@@ -200,14 +205,14 @@
// FIXME: Find a way to make the following const-correct:
m_internalBus->setChannelMemory(0, buffer, numberOfSourceFrames);
- m_sourceProvider->provideInput(m_internalBus.get(), numberOfSourceFrames);
+ provideInput(m_internalBus.get(), numberOfSourceFrames);
}
namespace {
-// BufferSourceProvider is an AudioSourceProvider wrapping an in-memory buffer.
+// BufferSourceProvider is an audio source provider wrapping an in-memory buffer.
-class BufferSourceProvider : public AudioSourceProvider {
+class BufferSourceProvider {
public:
explicit BufferSourceProvider(const float* source, size_t numberOfSourceFrames)
: m_source(source)
@@ -216,7 +221,7 @@
}
// Consumes samples from the in-memory buffer.
- void provideInput(AudioBus* bus, size_t framesToProcess) override
+ void provideInput(AudioBus* bus, size_t framesToProcess)
{
ASSERT(m_source && bus);
if (!m_source || !bus)
@@ -253,7 +258,9 @@
while (remaining) {
unsigned framesThisTime = std::min(remaining, m_requestFrames);
- process(&sourceProvider, destination, framesThisTime);
+ process(destination, framesThisTime, [&sourceProvider](AudioBus* bus, size_t framesToProcess) {
+ sourceProvider.provideInput(bus, framesToProcess);
+ });
destination += framesThisTime;
remaining -= framesThisTime;
@@ -260,13 +267,11 @@
}
}
-void SincResampler::process(AudioSourceProvider* sourceProvider, float* destination, size_t framesToProcess)
+void SincResampler::process(float* destination, size_t framesToProcess, const Function<void(AudioBus* bus, size_t framesToProcess)>& provideInput)
{
- ASSERT(sourceProvider);
- if (!sourceProvider)
+ ASSERT(provideInput);
+ if (!provideInput)
return;
-
- m_sourceProvider = sourceProvider;
unsigned numberOfDestinationFrames = framesToProcess;
@@ -273,7 +278,7 @@
// Step (1)
// Prime the input buffer at the start of the input stream.
if (!m_isBufferPrimed) {
- consumeSource(m_r0, m_requestFrames);
+ consumeSource(m_r0, m_requestFrames, provideInput);
m_isBufferPrimed = true;
}
@@ -521,7 +526,7 @@
// Step (5)
// Refresh the buffer with more input.
- consumeSource(m_r0, m_requestFrames);
+ consumeSource(m_r0, m_requestFrames, provideInput);
}
}
Modified: trunk/Source/WebCore/platform/audio/SincResampler.h (270156 => 270157)
--- trunk/Source/WebCore/platform/audio/SincResampler.h 2020-11-22 03:54:00 UTC (rev 270156)
+++ trunk/Source/WebCore/platform/audio/SincResampler.h 2020-11-22 04:51:13 UTC (rev 270157)
@@ -41,19 +41,23 @@
class SincResampler final {
WTF_MAKE_FAST_ALLOCATED;
public:
+ static constexpr unsigned defaultRequestFrames { 512 };
+
// scaleFactor == sourceSampleRate / destinationSampleRate
// requestFrames controls the size in frames of the buffer requested by each provideInput() call.
- SincResampler(double scaleFactor, Optional<unsigned> requestFrames = WTF::nullopt);
+ SincResampler(double scaleFactor, unsigned requestFrames = defaultRequestFrames);
+ size_t chunkSize() const { return m_chunkSize; }
+
// Processes numberOfSourceFrames from source to produce numberOfSourceFrames / scaleFactor frames in destination.
void process(const float* source, float* destination, unsigned numberOfSourceFrames);
- // Process with input source callback function for streaming applications.
- void process(AudioSourceProvider*, float* destination, size_t framesToProcess);
+ // Process with provideInput callback function for streaming applications.
+ void process(float* destination, size_t framesToProcess, const Function<void(AudioBus* bus, size_t framesToProcess)>& provideInput);
protected:
void initializeKernel();
- void consumeSource(float* buffer, unsigned numberOfSourceFrames);
+ void consumeSource(float* buffer, unsigned numberOfSourceFrames, const Function<void(AudioBus* bus, size_t framesToProcess)>& provideInput);
void updateRegions(bool isSecondLoad);
double m_scaleFactor;
@@ -72,6 +76,8 @@
// The number of source frames processed per pass.
unsigned m_blockSize { 0 };
+ size_t m_chunkSize { 0 };
+
// Source is copied into this buffer for each processing pass.
AudioFloatArray m_inputBuffer;
@@ -85,9 +91,6 @@
const float* m_source { nullptr };
unsigned m_sourceFramesAvailable { 0 };
-
- // m_sourceProvider is used to provide the audio input stream to the resampler.
- AudioSourceProvider* m_sourceProvider { nullptr };
// The buffer is primed once at the very beginning of processing.
bool m_isBufferPrimed { false };
