Bug 230329 - Use SharedMemory for transferring appended buffers from SourceBuffer to the GPU process
Summary: Use SharedMemory for transferring appended buffers from SourceBuffer to the G...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-15 17:19 PDT by Jer Noble
Modified: 2021-09-21 23:25 PDT (History)
15 users (show)

See Also:


Attachments
WIP (20.50 KB, patch)
2021-09-15 17:20 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
WIP (25.48 KB, patch)
2021-09-16 01:58 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (60.16 KB, patch)
2021-09-20 06:02 PDT, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Mark SourceBufferPrivate::append with WTF_EXPORT (67.88 KB, text/plain)
2021-09-20 06:33 PDT, Jean-Yves Avenard [:jya]
no flags Details
Patch (67.88 KB, patch)
2021-09-20 06:35 PDT, Jean-Yves Avenard [:jya]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (67.89 KB, patch)
2021-09-20 07:05 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (68.28 KB, patch)
2021-09-20 22:35 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (69.11 KB, patch)
2021-09-21 21:01 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (69.09 KB, patch)
2021-09-21 21:04 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (69.10 KB, patch)
2021-09-21 21:09 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2021-09-15 17:19:09 PDT
Use SharedMemory for transferring appended buffers from SourceBuffer to the GPU process
Comment 1 Jer Noble 2021-09-15 17:20:47 PDT
Created attachment 438307 [details]
WIP
Comment 2 Jer Noble 2021-09-15 17:49:25 PDT
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.
Comment 3 Jean-Yves Avenard [:jya] 2021-09-16 01:58:11 PDT
Created attachment 438328 [details]
WIP
Comment 4 Radar WebKit Bug Importer 2021-09-19 20:00:35 PDT
<rdar://problem/83291495>
Comment 5 Jean-Yves Avenard [:jya] 2021-09-20 06:02:31 PDT
Created attachment 438664 [details]
Patch
Comment 6 Jean-Yves Avenard [:jya] 2021-09-20 06:33:56 PDT
Created attachment 438668 [details]
Mark SourceBufferPrivate::append with WTF_EXPORT
Comment 7 Jean-Yves Avenard [:jya] 2021-09-20 06:35:24 PDT
Created attachment 438669 [details]
Patch

Mark SourceBufferPrivate::append with WTF_EXPORT
Comment 8 Jean-Yves Avenard [:jya] 2021-09-20 07:05:37 PDT
Created attachment 438673 [details]
Patch

Mark SourceBufferPrivate::append with WTF_EXPORT #2
Comment 9 Peng Liu 2021-09-20 16:05:09 PDT
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()`?
Comment 10 Eric Carlson 2021-09-20 16:27:17 PDT
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
Comment 11 Jean-Yves Avenard [:jya] 2021-09-20 19:54:00 PDT
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.
Comment 12 Jean-Yves Avenard [:jya] 2021-09-20 22:35:12 PDT
Created attachment 438776 [details]
Patch

Apply comments, and tweaks to ShareBuffer::copyTo
Comment 13 Jer Noble 2021-09-21 09:51:28 PDT
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.
Comment 14 Jean-Yves Avenard [:jya] 2021-09-21 17:35:16 PDT
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.
Comment 15 Jean-Yves Avenard [:jya] 2021-09-21 21:01:17 PDT
Created attachment 438913 [details]
Patch

Apply comment, re-write SharedBuffer::copyTo, now using a O(LOG(N)) search for the first segment
Comment 16 Jean-Yves Avenard [:jya] 2021-09-21 21:04:48 PDT
Created attachment 438915 [details]
Patch

update reviewer in Changelog
Comment 17 Jean-Yves Avenard [:jya] 2021-09-21 21:09:42 PDT
Created attachment 438916 [details]
Patch

Renamed FreeBlock to FreeDataSegment
Comment 18 EWS 2021-09-21 23:25:40 PDT
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].