Bug 178003 – [MSE] Move SourceBuffer's pending append data into the platform implementations

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[MSE] Move SourceBuffer's pending append data into the platform implementations

[Zan Dobersek]

Created attachment 323000 [details]
Patch

[Enrique Ocaña]

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?

[Zan Dobersek]

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.

[Enrique Ocaña]

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.

[Jer Noble]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Created attachment 324003 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 324003 [details]
Patch for landing

Clearing flags on attachment: 324003

Committed r223596: <https://trac.webkit.org/changeset/223596>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35048022>