[webkit-reviews] review granted: [Bug 219715] [GPUProcess] Cut in half amount of IPC needed to do WebAudio : [Attachment 415927] 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 219715: [GPUProcess] Cut in half amount of IPC needed to do WebAudio
https://bugs.webkit.org/show_bug.cgi?id=219715

Attachment 415927: Patch

https://bugs.webkit.org/attachment.cgi?id=415927&action=review




--- Comment #19 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 415927
  --> https://bugs.webkit.org/attachment.cgi?id=415927
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415927&action=review

r=me

> Source/WebCore/platform/audio/cocoa/CARingBuffer.cpp:332
> +    // When the RingBuffer is backed by shared memory, make sure we pull
frame bounds from shared memory before fetching.
> +    updateFrameBounds();
> +
> +    uint64_t start, end;
> +    getCurrentFrameBoundsWithoutUpdate(start, end);

Should this just be getCurrentFrameBounds()? Seems like the behavior would be
equivalent.

> Source/WebKit/Shared/Cocoa/SharedRingBufferStorage.h:73
> +    static constexpr unsigned boundsBufferSize { 32 };
> +    struct FrameBounds {
> +	   std::pair<uint64_t, uint64_t> boundsBuffer[boundsBufferSize];
> +	   Atomic<unsigned> boundsBufferIndex { 0 };
> +    };

The logic of this data structure and its related code seem right to me, but I
don't fully understand why it's OK for this class and the related CARingBuffer
class to assume that the reader will not fall behind by more than 32 frames. I
get it that falling behind is very rare and will definitely cause audio
distortion. But this logic (and the pre-existing
kGeneralRingTimeBoundsQueueSize in CARingBuffer) seem to take the audio
distortion bug and embiggen it into a buffer overflow bug. Seems like something
we should follow up on with our audio experts. Maybe they can explain why 32 is
always enough. Or maybe we need to adjust this logic in both places. (We could
use a strategy similar to your previous tryLock() strategy; or we could have
the *reader* update the boundsBufferIndex and have the writer keep writing to
the same index until the reader is ready for the next one).