Use SharedMemory for transferring appended buffers from SourceBuffer to the GPU process
Created attachment 438307 [details] WIP
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.
Created attachment 438328 [details] WIP
<rdar://problem/83291495>
Created attachment 438664 [details] Patch
Created attachment 438668 [details] Mark SourceBufferPrivate::append with WTF_EXPORT
Created attachment 438669 [details] Patch Mark SourceBufferPrivate::append with WTF_EXPORT
Created attachment 438673 [details] Patch Mark SourceBufferPrivate::append with WTF_EXPORT #2
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 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 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.
Created attachment 438776 [details] Patch Apply comments, and tweaks to ShareBuffer::copyTo
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 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.
Created attachment 438913 [details] Patch Apply comment, re-write SharedBuffer::copyTo, now using a O(LOG(N)) search for the first segment
Created attachment 438915 [details] Patch update reviewer in Changelog
Created attachment 438916 [details] Patch Renamed FreeBlock to FreeDataSegment
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].