Bug 178003 - [MSE] Move SourceBuffer's pending append data into the platform implementations
Summary: [MSE] Move SourceBuffer's pending append data into the platform implementations
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: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-06 02:02 PDT by Zan Dobersek
Modified: 2017-10-18 01:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.77 KB, patch)
2017-10-06 02:24 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (12.94 KB, patch)
2017-10-17 02:10 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-10-06 02:02:20 PDT
[MSE] Move SourceBuffer's pending append data into the platform implementations
Comment 1 Zan Dobersek 2017-10-06 02:24:55 PDT
Created attachment 323000 [details]
Patch
Comment 2 Enrique Ocaña 2017-10-06 02:37:56 PDT
Comment on attachment 323000 [details]
Patch

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

The patch looks good in general but I have a small question:

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:143
> +    GstBuffer* buffer = gst_buffer_new_wrapped_full(static_cast<GstMemoryFlags>(0), bufferData, bufferLength, 0, bufferLength, new Vector<unsigned char>(WTFMove(data)),

Why is it better to keep the original data wrapped in a Vector here? Do we have any way to "steal" the internal data forever (as a char*) and avoid having the new Vector alive during the whole life of the GstBuffer?
Comment 3 Zan Dobersek 2017-10-06 02:48:25 PDT
Comment on attachment 323000 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:143
>> +    GstBuffer* buffer = gst_buffer_new_wrapped_full(static_cast<GstMemoryFlags>(0), bufferData, bufferLength, 0, bufferLength, new Vector<unsigned char>(WTFMove(data)),
> 
> Why is it better to keep the original data wrapped in a Vector here? Do we have any way to "steal" the internal data forever (as a char*) and avoid having the new Vector alive during the whole life of the GstBuffer?

Vector either stores the data in an inline buffer, or it manages the pointer to the FastMalloc-ed data. Directly wrapping the Vector object avoids a lot of complexity, where in case of the inline buffer being used, we'd have to detect that and then actually copy the data into a separately-managed allocation.
Comment 4 Enrique Ocaña 2017-10-06 06:49:23 PDT
Comment on attachment 323000 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:143
>>> +    GstBuffer* buffer = gst_buffer_new_wrapped_full(static_cast<GstMemoryFlags>(0), bufferData, bufferLength, 0, bufferLength, new Vector<unsigned char>(WTFMove(data)),
>> 
>> Why is it better to keep the original data wrapped in a Vector here? Do we have any way to "steal" the internal data forever (as a char*) and avoid having the new Vector alive during the whole life of the GstBuffer?
> 
> Vector either stores the data in an inline buffer, or it manages the pointer to the FastMalloc-ed data. Directly wrapping the Vector object avoids a lot of complexity, where in case of the inline buffer being used, we'd have to detect that and then actually copy the data into a separately-managed allocation.

Ok, everything clear, copies are bad and IMHO the patch is fine as is.
Comment 5 Jer Noble 2017-10-06 08:48:36 PDT
Comment on attachment 323000 [details]
Patch

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

r=me, with nit.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:564
> +    // Ensure the m_pendingAppendData object is moved from, in case the platform implementation
> +    // rejects appending the buffer for whatever reason.
> +    auto data = WTFMove(m_pendingAppendData);
> +    m_private->append(WTFMove(data));

Nit: I would be more comfortable with an explicit call to "clear()" (with this comment) than a double move.  Alternatively, this could be an "ASSERT(!m_pendingAppend.size())", since the private not doing anything with the passed in data is an exceptional condition.

Also, a future improvement to this area could be to move from storing incoming data as a "Vector<unsigned char>" to a "SharedBuffer".  Appends would be cheaper, and the Private's might be able to use the SharedBuffer directly.
Comment 6 Zan Dobersek 2017-10-17 01:55:07 PDT
(In reply to Jer Noble from comment #5)
> Comment on attachment 323000 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323000&action=review
> 
> r=me, with nit.
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:564
> > +    // Ensure the m_pendingAppendData object is moved from, in case the platform implementation
> > +    // rejects appending the buffer for whatever reason.
> > +    auto data = WTFMove(m_pendingAppendData);
> > +    m_private->append(WTFMove(data));
> 
> Nit: I would be more comfortable with an explicit call to "clear()" (with
> this comment) than a double move.  Alternatively, this could be an
> "ASSERT(!m_pendingAppend.size())", since the private not doing anything with
> the passed in data is an exceptional condition.
> 

I'll use the clear() call for now since none of the implementations guarantees to move from the moved-in Vector.

> Also, a future improvement to this area could be to move from storing
> incoming data as a "Vector<unsigned char>" to a "SharedBuffer".  Appends
> would be cheaper, and the Private's might be able to use the SharedBuffer
> directly.

At least for the GStreamer backend, the segmented data kept in SharedBuffer would have to be flattened into a continuous allocation and then wrapped into a GstBuffer, which would essentially mean every appended data is copied twice like it is now -- first from the BufferSource into the SharedBuffer segment, and then from that segment into the flattened representation. With this patch, the only copy is from the BufferSource, at the expense of potential reallocations that occur when these copies go beyond the currently-allocated Vector<> buffer.
Comment 7 Zan Dobersek 2017-10-17 02:10:23 PDT
Created attachment 324003 [details]
Patch for landing
Comment 8 Zan Dobersek 2017-10-18 01:30:16 PDT
Comment on attachment 324003 [details]
Patch for landing

Clearing flags on attachment: 324003

Committed r223596: <https://trac.webkit.org/changeset/223596>
Comment 9 Zan Dobersek 2017-10-18 01:30:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-10-18 01:30:42 PDT
<rdar://problem/35048022>