[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]

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.