Bug 182829 – [GStreamer] Push smaller buffers from HTTP source

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

[GStreamer] Push smaller buffers from HTTP source

[Charlie Turner]

Created attachment 333894 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 333894 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        Reviewed by NOBODY (OOPS!).

This line normally goes before the explanation, right after the bug url

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:35
> +#include <cstdint> // uint64_t and friends, PRIxy

No need to add a comment, I think.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:920
> +        GstBuffer* subBuffer = gst_buffer_copy_region(priv->buffer.get(), GST_BUFFER_COPY_ALL, currentOffset, currentOffsetSize);

Would it be possible to create the chunks by wrapping existing buffer data instead of copying?

[Philippe Normand]

I dont't think this matches souphttpsrc behavior, does it?
IIRC you said the issue is specific to our src element and isn't exposed when souphttpsrc is used, so I'm a bit confused.

[Charlie Turner]

Thanks for the review,

(In reply to Carlos Garcia Campos from comment #2)
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:920
> > +        GstBuffer* subBuffer = gst_buffer_copy_region(priv->buffer.get(), GST_BUFFER_COPY_ALL, currentOffset, currentOffsetSize);
> 
> Would it be possible to create the chunks by wrapping existing buffer data
> instead of copying?

The memory blocks should only be getting ref'd rather than actually copied, the function is advertised to avoid full memory copies.

[Charlie Turner]

(In reply to Philippe Normand from comment #3)
> I dont't think this matches souphttpsrc behavior, does it?
> IIRC you said the issue is specific to our src element and isn't exposed
> when souphttpsrc is used, so I'm a bit confused.

souphttpsrc's architecture is quite different to our http src, it inherits  from a basesrc and overrides the create() method, which tells souphttpsrc how many bytes to copy.

However, because our http src is a bin and we need to feed an appsrc with data from WebKit's networking stack, we don't have the same hook to override. Instead we just get big blocks of data and seem to need to split it like this to keep the appsrc's queue with plenty of buffers.

I'm not sure how we can better match souphttp's behavior when we don't inherit from the same classes.

[Charlie Turner]

Created attachment 333904 [details]
Patch

[Charlie Turner]

Created attachment 333912 [details]
Patch

Spelling mistakes

[Philippe Normand]

Comment on attachment 333912 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:931
> +        if (UNLIKELY(ret != GST_FLOW_OK && ret != GST_FLOW_EOS && ret != GST_FLOW_FLUSHING))

Why the last condition? It wasn't in the previous code.

[Charlie Turner]

(In reply to Philippe Normand from comment #8)
> Comment on attachment 333912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333912&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:931
> > +        if (UNLIKELY(ret != GST_FLOW_OK && ret != GST_FLOW_EOS && ret != GST_FLOW_FLUSHING))
> 
> Why the last condition? It wasn't in the previous code.

This came in from the downstream port I work on; it's to avoid generating errors after pipeline flushes (like seeking) and instead wait for more data to come in.

[Xabier Rodríguez Calvar]

Comment on attachment 333912 [details]
Patch

The log level is too verbose, please consider DEBUG and even TRACE for some messages.

[Philippe Normand]

Comment on attachment 333912 [details]
Patch

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

Ok this is almost ready. I also agree with Xabier's comment about log verbosity.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:922
> +            GST_ELEMENT_ERROR(src, CORE, FAILED, ("Failed to allocate sub-buffer"), (nullptr));

Add a break here please?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:932
> +            GST_ELEMENT_ERROR(src, CORE, FAILED, (nullptr), (nullptr));

Ditto :)

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #10)
> Comment on attachment 333912 [details]
> Patch
> 
> The log level is too verbose, please consider DEBUG and even TRACE for some
> messages.

But LOG is for more verbose messages that DEBUG according to https://gstreamer.freedesktop.org/documentation/tutorials/basic/debugging-tools.html

The description of LOG seems to fit what I'm logging more closely than either TRACE or DEBUG..

[Charlie Turner]

Created attachment 334055 [details]
Patch

[Charlie Turner]

Created attachment 334058 [details]
Patch

Change log messages in loop to TRACE level rather than LOG

[Charlie Turner]

Created attachment 334122 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 334122 [details]
Patch for landing

Clearing flags on attachment: 334122

Committed r228603: <https://trac.webkit.org/changeset/228603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.