Bug 230329 – Use SharedMemory for transferring appended buffers from SourceBuffer to the GPU process

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Use SharedMemory for transferring appended buffers from SourceBuffer to the GPU process

[Jer Noble]

Created attachment 438307 [details]
WIP

[Jer Noble]

Comment on attachment 438307 [details]
WIP

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

> Source/WebCore/platform/graphics/cocoa/SourceBufferParser.cpp:139
> +        [&](const Ref<SharedBuffer>& buffer)
> +        {
> +            buffer->copyTo(destination, sizeToRead);
> +            return buffer->size();

This is definitely wrong; it doesn't take the `position` parameter into account.

[Jean-Yves Avenard [:jya]]

Created attachment 438328 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/83291495>

[Jean-Yves Avenard [:jya]]

Created attachment 438664 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 438668 [details]
Mark SourceBufferPrivate::append with WTF_EXPORT

[Jean-Yves Avenard [:jya]]

Created attachment 438669 [details]
Patch

Mark SourceBufferPrivate::append with WTF_EXPORT

[Jean-Yves Avenard [:jya]]

Created attachment 438673 [details]
Patch

Mark SourceBufferPrivate::append with WTF_EXPORT #2

[Peng Liu]

Comment on attachment 438673 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        will throw; as such, there's no need to append the data to a vector.

You mean throw an exception?

> Source/WebCore/platform/SharedBuffer.cpp:307
> +    for (i += 1; i < m_segments.size(); ++i) {

Probably s/i += 1;/++i/g?

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:1321
> +    WTFCrash();

Maybe `ASSERT_NOT_REACHED();` is better?

> Source/WebCore/platform/graphics/SourceBufferPrivate.h:71
> +    WEBCORE_EXPORT virtual void append(Vector<unsigned char>&&);

Seems a good fit for protected method?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:363
> +    Status ReadInto(std::size_t numToRead, CMBlockBufferRef outputBuffer, uint64_t* numActuallyRead)

Can this function share some code with `Read()`?

[Eric Carlson]

Comment on attachment 438673 [details]
Patch

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

>> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:1321
>> +    WTFCrash();
> 
> Maybe `ASSERT_NOT_REACHED();` is better?

RELEASE_ASSERT_NOT_REACHED

[Jean-Yves Avenard [:jya]]

Comment on attachment 438673 [details]
Patch

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

>> Source/WebCore/platform/graphics/SourceBufferPrivate.h:71
>> +    WEBCORE_EXPORT virtual void append(Vector<unsigned char>&&);
> 
> Seems a good fit for protected method?

no. This should be a pure virtual method. It is the main public entry points. However that's not what existing SourceBufferPrivate are currently implementing.

Once all have been switched to the new type, we can remove this one entirely

>> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:363
>> +    Status ReadInto(std::size_t numToRead, CMBlockBufferRef outputBuffer, uint64_t* numActuallyRead)
> 
> Can this function share some code with `Read()`?

There's too many differences between to only make the code more complicated. So there's very little to gain from it.
ReadInto doesn't do reads and copy. 

It build CMBlockBuffer that keeps references to the original content.

[Jean-Yves Avenard [:jya]]

Created attachment 438776 [details]
Patch

Apply comments, and tweaks to ShareBuffer::copyTo

[Jer Noble]

Comment on attachment 438776 [details]
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.

[Jean-Yves Avenard [:jya]]

Comment on attachment 438776 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:406
>> +                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.

I did what the CMBlockBufferCustomBlockSource documentation advised to do. `To avoid link-time issues, it is recommended that clients fill CMBlockBufferCustomBlockSource's function pointer fields `
I played it safe.

[Jean-Yves Avenard [:jya]]

Created attachment 438913 [details]
Patch

Apply comment, re-write SharedBuffer::copyTo, now using a O(LOG(N)) search for the first segment

[Jean-Yves Avenard [:jya]]

Created attachment 438915 [details]
Patch

update reviewer in Changelog

[Jean-Yves Avenard [:jya]]

Created attachment 438916 [details]
Patch

Renamed FreeBlock to FreeDataSegment

[EWS]

Committed r282865 (241996@main): <https://commits.webkit.org/241996@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438916 [details].