Skip to content

Commit a675a51

Browse files
cdumezcdumez
committed
[GPUProcess] Regression(r268632) Garbage is rendered on speakers when using WebAudio
https://bugs.webkit.org/show_bug.cgi?id=218729 Reviewed by Eric Carlson. Source/WebCore: * platform/audio/cocoa/AudioDestinationCocoa.h: * platform/mock/MockAudioDestinationCocoa.cpp: (WebCore::MockAudioDestinationCocoa::start): (WebCore::MockAudioDestinationCocoa::stop): (WebCore::MockAudioDestinationCocoa::tick): * platform/mock/MockAudioDestinationCocoa.h: Make AudioDestinationCocoa::m_dispatchToRenderThread private so that subclasses cannot set it. Update MockAudioDestinationCocoa to use m_dispatchToRenderThread to dispatch to the render thread when available, instead of unconditionally dispatching to its own WorkQueue and then expecting AudioDestinationCocoa::render() to render to the actual rendering thread. Source/WebKit: RemoteAudioDestinationProxy::requestBuffer() was calling AudioDestinationCocoa::render() and expecting RemoteAudioDestinationProxy::renderOnRenderingThead() to get called as a result. It would take care of writing to the CARingBuffer and sending the IPC back to the GPU process in renderOnRenderingThead(). The issue was that AudioDestinationCocoa uses a PushPullFIFO internally for buffering. It first fetches available frames from the FIFO and then only calls renderOnRenderingThead() with the number of frames that remain to processed (usually 0). As a result, RemoteAudioDestinationProxy::renderOnRenderingThead() would often store 0 frames instead of 128 (or sometimes a number of frames less than 128), even though the full 128 frames were actually rendered. To address the issue, stop overriding renderOnRenderingThead() in RemoteAudioDestinationProxy. Instead, do the writing to the CARingBuffer and the IPC response in RemoteAudioDestinationProxy::requestBuffer(), directly after calling AudioDestinationCocoa::render(). After calling AudioDestinationCocoa::render() we know that |framesToRender| frames have been rendered / added to the buffer. * WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp: (WebKit::RemoteAudioDestinationProxy::requestBuffer): * WebProcess/GPU/media/RemoteAudioDestinationProxy.h: Canonical link: https://commits.webkit.org/231420@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@269630 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 1ed60da commit a675a51

7 files changed

Lines changed: 65 additions & 22 deletions

File tree

‎Source/WebCore/ChangeLog‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,21 @@
1+
2020-11-10 Chris Dumez <cdumez@apple.com>
2+
3+
[GPUProcess] Regression(r268632) Garbage is rendered on speakers when using WebAudio
4+
https://bugs.webkit.org/show_bug.cgi?id=218729
5+
6+
Reviewed by Eric Carlson.
7+
8+
* platform/audio/cocoa/AudioDestinationCocoa.h:
9+
* platform/mock/MockAudioDestinationCocoa.cpp:
10+
(WebCore::MockAudioDestinationCocoa::start):
11+
(WebCore::MockAudioDestinationCocoa::stop):
12+
(WebCore::MockAudioDestinationCocoa::tick):
13+
* platform/mock/MockAudioDestinationCocoa.h:
14+
Make AudioDestinationCocoa::m_dispatchToRenderThread private so that subclasses cannot set it.
15+
Update MockAudioDestinationCocoa to use m_dispatchToRenderThread to dispatch to the
16+
render thread when available, instead of unconditionally dispatching to its own WorkQueue and
17+
then expecting AudioDestinationCocoa::render() to render to the actual rendering thread.
18+
119
2020-11-10 Antti Koivisto <antti@apple.com>
220

321
[LFC][Integration] Allow object-fit

‎Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ class AudioDestinationCocoa : public AudioDestination, public AudioUnitRenderer,
5454
protected:
5555
WEBCORE_EXPORT bool hasEnoughFrames(UInt32 numberOfFrames) const;
5656
WEBCORE_EXPORT OSStatus render(double sampleTime, uint64_t hostTime, UInt32 numberOfFrames, AudioBufferList* ioData) final;
57-
WEBCORE_EXPORT virtual void renderOnRenderingThead(size_t framesToRender);
5857

5958
WEBCORE_EXPORT void setIsPlaying(bool);
6059
bool isPlaying() final { return m_isPlaying; }
@@ -65,14 +64,14 @@ class AudioDestinationCocoa : public AudioDestination, public AudioUnitRenderer,
6564

6665
WEBCORE_EXPORT void getAudioStreamBasicDescription(AudioStreamBasicDescription&);
6766

68-
Function<void(Function<void()>&&)> m_dispatchToRenderThread;
69-
7067
private:
7168
friend Ref<AudioDestination> AudioDestination::create(AudioIOCallback&, const String&, unsigned, unsigned, float);
7269

7370
void start(Function<void(Function<void()>&&)>&&, CompletionHandler<void(bool)>&&) override;
7471
void stop(CompletionHandler<void(bool)>&&) override;
7572

73+
void renderOnRenderingThead(size_t framesToRender);
74+
7675
// AudioSourceProvider.
7776
WEBCORE_EXPORT void provideInput(AudioBus*, size_t framesToProcess) final;
7877

@@ -93,6 +92,8 @@ class AudioDestinationCocoa : public AudioDestination, public AudioUnitRenderer,
9392
std::unique_ptr<MultiChannelResampler> m_resampler;
9493
AudioIOPosition m_outputTimestamp;
9594

95+
Function<void(Function<void()>&&)> m_dispatchToRenderThread;
96+
9697
float m_contextSampleRate;
9798

9899
Lock m_isPlayingLock;

‎Source/WebCore/platform/mock/MockAudioDestinationCocoa.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,12 @@ MockAudioDestinationCocoa::~MockAudioDestinationCocoa() = default;
4848
void MockAudioDestinationCocoa::start(Function<void(Function<void()>&&)>&& dispatchToRenderThread, CompletionHandler<void(bool)>&& completionHandler)
4949
{
5050
m_dispatchToRenderThread = WTFMove(dispatchToRenderThread);
51+
if (!m_dispatchToRenderThread) {
52+
m_dispatchToRenderThread = [this](Function<void()>&& function) {
53+
m_workQueue->dispatch(WTFMove(function));
54+
};
55+
}
56+
5157
m_timer.startRepeating(Seconds { m_numberOfFramesToProcess / sampleRate() });
5258
setIsPlaying(true);
5359

@@ -61,7 +67,7 @@ void MockAudioDestinationCocoa::stop(CompletionHandler<void(bool)>&& completionH
6167
m_timer.stop();
6268
setIsPlaying(false);
6369

64-
m_workQueue->dispatch([completionHandler = WTFMove(completionHandler)]() mutable {
70+
m_dispatchToRenderThread([completionHandler = WTFMove(completionHandler)]() mutable {
6571
callOnMainThread([completionHandler = WTFMove(completionHandler)]() mutable {
6672
completionHandler(true);
6773
});
@@ -70,7 +76,7 @@ void MockAudioDestinationCocoa::stop(CompletionHandler<void(bool)>&& completionH
7076

7177
void MockAudioDestinationCocoa::tick()
7278
{
73-
m_workQueue->dispatch([this, sampleRate = sampleRate(), numberOfFramesToProcess = m_numberOfFramesToProcess] {
79+
m_dispatchToRenderThread([this, sampleRate = sampleRate(), numberOfFramesToProcess = m_numberOfFramesToProcess] {
7480
AudioStreamBasicDescription streamFormat;
7581
getAudioStreamBasicDescription(streamFormat);
7682

‎Source/WebCore/platform/mock/MockAudioDestinationCocoa.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ class MockAudioDestinationCocoa final : public AudioDestinationCocoa {
5454

5555
Ref<WorkQueue> m_workQueue;
5656
RunLoop::Timer<MockAudioDestinationCocoa> m_timer;
57+
Function<void(Function<void()>&&)> m_dispatchToRenderThread;
5758
uint32_t m_numberOfFramesToProcess { 384 };
5859
};
5960

‎Source/WebKit/ChangeLog‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,30 @@
1+
2020-11-10 Chris Dumez <cdumez@apple.com>
2+
3+
[GPUProcess] Regression(r268632) Garbage is rendered on speakers when using WebAudio
4+
https://bugs.webkit.org/show_bug.cgi?id=218729
5+
6+
Reviewed by Eric Carlson.
7+
8+
RemoteAudioDestinationProxy::requestBuffer() was calling AudioDestinationCocoa::render()
9+
and expecting RemoteAudioDestinationProxy::renderOnRenderingThead() to get called as
10+
a result. It would take care of writing to the CARingBuffer and sending the IPC back
11+
to the GPU process in renderOnRenderingThead(). The issue was that AudioDestinationCocoa
12+
uses a PushPullFIFO internally for buffering. It first fetches available frames from
13+
the FIFO and then only calls renderOnRenderingThead() with the number of frames that
14+
remain to processed (usually 0). As a result, RemoteAudioDestinationProxy::renderOnRenderingThead()
15+
would often store 0 frames instead of 128 (or sometimes a number of frames less than
16+
128), even though the full 128 frames were actually rendered.
17+
18+
To address the issue, stop overriding renderOnRenderingThead() in
19+
RemoteAudioDestinationProxy. Instead, do the writing to the CARingBuffer and the IPC
20+
response in RemoteAudioDestinationProxy::requestBuffer(), directly after calling
21+
AudioDestinationCocoa::render(). After calling AudioDestinationCocoa::render()
22+
we know that |framesToRender| frames have been rendered / added to the buffer.
23+
24+
* WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp:
25+
(WebKit::RemoteAudioDestinationProxy::requestBuffer):
26+
* WebProcess/GPU/media/RemoteAudioDestinationProxy.h:
27+
128
2020-11-10 Carlos Garcia Campos <cgarcia@igalia.com>
229

330
[GTK4] WebView is flipped

‎Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.cpp‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -132,34 +132,26 @@ void RemoteAudioDestinationProxy::stop(CompletionHandler<void(bool)>&& completio
132132
}
133133

134134
#if PLATFORM(COCOA)
135-
void RemoteAudioDestinationProxy::requestBuffer(double sampleTime, uint64_t hostTime, uint64_t numberOfFrames, CompletionHandler<void(uint64_t, uint64_t)>&& completionHandler)
135+
void RemoteAudioDestinationProxy::requestBuffer(double sampleTime, uint64_t hostTime, uint64_t framesToRender, CompletionHandler<void(uint64_t, uint64_t)>&& completionHandler)
136136
{
137137
ASSERT(!isMainThread());
138138

139-
if (!hasEnoughFrames(numberOfFrames))
140-
completionHandler(0, 0);
141-
142-
m_renderCompletionHandler = WTFMove(completionHandler);
143-
m_audioBufferList->setSampleCount(numberOfFrames);
144-
145-
AudioDestinationCocoa::render(sampleTime, hostTime, numberOfFrames, m_audioBufferList->list());
146-
}
147-
148-
void RemoteAudioDestinationProxy::renderOnRenderingThead(size_t framesToRender)
149-
{
150-
ASSERT(m_renderCompletionHandler);
139+
if (!hasEnoughFrames(framesToRender))
140+
return completionHandler(0, 0);
151141

152142
auto startFrame = m_currentFrame;
143+
m_audioBufferList->setSampleCount(framesToRender);
144+
145+
AudioDestinationCocoa::render(sampleTime, hostTime, framesToRender, m_audioBufferList->list());
153146

154-
AudioDestinationCocoa::renderOnRenderingThead(framesToRender);
155147
m_currentFrame += framesToRender;
156148

157149
m_ringBuffer->store(m_audioBufferList->list(), framesToRender, startFrame);
158150

159151
uint64_t boundsStartFrame;
160152
uint64_t boundsEndFrame;
161153
m_ringBuffer->getCurrentFrameBounds(boundsStartFrame, boundsEndFrame);
162-
m_renderCompletionHandler(boundsStartFrame, boundsEndFrame);
154+
completionHandler(boundsStartFrame, boundsEndFrame);
163155
}
164156
#endif
165157

‎Source/WebKit/WebProcess/GPU/media/RemoteAudioDestinationProxy.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,6 @@ class RemoteAudioDestinationProxy
8787

8888
#if PLATFORM(COCOA)
8989
void storageChanged(SharedMemory*) final;
90-
void renderOnRenderingThead(size_t framesToRender) final;
9190
#endif
9291

9392
// IPC::MessageReceiver
@@ -105,7 +104,6 @@ class RemoteAudioDestinationProxy
105104
std::unique_ptr<WebCore::CARingBuffer> m_ringBuffer;
106105
std::unique_ptr<WebCore::WebAudioBufferList> m_audioBufferList;
107106
uint64_t m_currentFrame { 0 };
108-
WTF::Function<void(uint64_t, uint64_t)> m_renderCompletionHandler;
109107
#endif
110108

111109
Function<void(Function<void()>&&)> m_dispatchToRenderThread;

0 commit comments

Comments
 (0)