[webkit-reviews] review granted: [Bug 230329] Use SharedMemory for transferring appended buffers from SourceBuffer to the GPU process : [Attachment 438776] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Jean-Yves Avenard [:jya]
<jean-yves.avenard at apple.com>'s request for review:
Bug 230329: Use SharedMemory for transferring appended buffers from
SourceBuffer to the GPU process
https://bugs.webkit.org/show_bug.cgi?id=230329

Attachment 438776: Patch

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




--- Comment #13 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 438776
  --> https://bugs.webkit.org/attachment.cgi?id=438776
Patch

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

r=me with one change and a couple of nits.

> Source/WebCore/platform/SharedBuffer.cpp:288
> +    ASSERT(length + offset <= size());
>      auto destinationPtr = static_cast<uint8_t*>(destination);
> -    auto remaining = std::min(length, size());
> -    for (auto& segment : m_segments) {
> -	   size_t amountToCopyThisTime = std::min(remaining,
segment.segment->size());
> -	   memcpy(destinationPtr, segment.segment->data(),
amountToCopyThisTime);
> +    auto remaining = std::min(length, size() - offset);

I _think_ this could result in an out-of-bounds read if the ASSERT() case at
the top is violated and `offset > size()`.  Might want to add an early return
in that case.

> Source/WebCore/platform/SharedBuffer.cpp:291
> +    for (;; ++segment) {

This...

> Source/WebCore/platform/SharedBuffer.cpp:305
> +	   break;
> +    }
> +    for (++segment; segment != end(); ++segment) {

...and this. Is there a reason to break up the for loop this way? Seems like
the `break` could be replaced by a `++segment` and the loops joined.

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:357
> +    static void FreeBlock(void* refcon, void*, size_t)

Nit: could this be named "FreeDataSegment" instead?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:406
> +		   CMBlockBufferCustomBlockSource allocator;
> +		   allocator.version = 0;
> +		   allocator.AllocateBlock = nullptr;
> +		   allocator.FreeBlock = FreeBlock;
> +		   allocator.refCon = firstSegment.ptr();

Nit: this could be "...alloctator = { 0, nullptr, FreeDataSegment,
firstSegment.ptr() };" and be much more compact without sacrificing too much
readability. You could also do something like {.version= 0, .AllocateBlock =
nullptr} if the names are important.