[webkit-reviews] review granted: [Bug 219818] [GPUProcess] Avoid doing an IPC per rendering quantum when using WebAudio : [Attachment 416343] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 16 11:43:10 PST 2020
Geoffrey Garen <ggaren at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 219818: [GPUProcess] Avoid doing an IPC per rendering quantum when using
WebAudio
https://bugs.webkit.org/show_bug.cgi?id=219818
Attachment 416343: Patch
https://bugs.webkit.org/attachment.cgi?id=416343&action=review
--- Comment #32 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 416343
--> https://bugs.webkit.org/attachment.cgi?id=416343
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=416343&action=review
r=me
Seems OK to land as-is, but I think there might be room for improvement.
> Source/WebCore/platform/audio/cocoa/CARingBuffer.cpp:182
> + for (unsigned i = 0; i < kGeneralRingTimeBoundsQueueSize; ++i)
> + m_lastReadFrameBuffer[i] = 0;
If you use std::array, you can just call fill() here instead.
> Source/WebCore/platform/audio/cocoa/CARingBuffer.h:64
> + static constexpr uint32_t kGeneralRingTimeBoundsQueueSize = 32;
Should this be 2 now (1 current reader frame + 1 write-ahead frame = 5.8ms
maximum total audio in the queue)?
> Source/WebCore/platform/audio/cocoa/CARingBuffer.h:94
> + uint64_t m_lastReadFrameBuffer[kGeneralRingTimeBoundsQueueSize] = { 0 };
I have a slight preference for std::array. Because array-to-pointer type
masquerading breaks my brain.
> Source/WebCore/platform/audio/cocoa/CARingBuffer.h:95
> + uint64_t m_lastReadFrameBuffer[kGeneralRingTimeBoundsQueueSize] = { 0 };
> + std::atomic<unsigned> m_lastReadFrameBufferIndex { 0 };
It seems like you're writing the "last read frame" as a stream of values in a
ring buffer (m_lastReadFrameBuffer).
Is that necessary?
I think that only the GPU Process updates this value, and I think that the
WebContent Process is only interested in the most up-to-date value. If so, we
can use a single std::atomic<uint64_t> instead.
More information about the webkit-reviews
mailing list