Bug 182829 - [GStreamer] Push smaller buffers from HTTP source
Summary: [GStreamer] Push smaller buffers from HTTP source
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-15 05:44 PST by Charlie Turner
Modified: 2018-02-18 02:02 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.02 KB, patch)
2018-02-15 05:46 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2018-02-15 09:01 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2018-02-15 10:06 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2018-02-16 11:23 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2018-02-16 12:22 PST, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch for landing (4.93 KB, patch)
2018-02-18 01:26 PST, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2018-02-15 05:44:47 PST
[GStreamer] Push smaller buffers from HTTP source
Comment 1 Charlie Turner 2018-02-15 05:46:57 PST
Created attachment 333894 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-02-15 06:05:02 PST
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?
Comment 3 Philippe Normand 2018-02-15 06:44:58 PST
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.
Comment 4 Charlie Turner 2018-02-15 08:43:08 PST
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.
Comment 5 Charlie Turner 2018-02-15 08:47:38 PST
(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.
Comment 6 Charlie Turner 2018-02-15 09:01:12 PST
Created attachment 333904 [details]
Patch
Comment 7 Charlie Turner 2018-02-15 10:06:03 PST
Created attachment 333912 [details]
Patch

Spelling mistakes
Comment 8 Philippe Normand 2018-02-15 10:07:10 PST
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.
Comment 9 Charlie Turner 2018-02-15 10:21:28 PST
(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.
Comment 10 Xabier Rodríguez Calvar 2018-02-15 23:42:22 PST
Comment on attachment 333912 [details]
Patch

The log level is too verbose, please consider DEBUG and even TRACE for some messages.
Comment 11 Philippe Normand 2018-02-16 01:12:51 PST
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 :)
Comment 12 Charlie Turner 2018-02-16 11:18:55 PST
(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..
Comment 13 Charlie Turner 2018-02-16 11:23:25 PST
Created attachment 334055 [details]
Patch
Comment 14 Charlie Turner 2018-02-16 12:22:17 PST
Created attachment 334058 [details]
Patch

Change log messages in loop to TRACE level rather than LOG
Comment 15 Charlie Turner 2018-02-18 01:26:34 PST
Created attachment 334122 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2018-02-18 02:02:45 PST
Comment on attachment 334122 [details]
Patch for landing

Clearing flags on attachment: 334122

Committed r228603: <https://trac.webkit.org/changeset/228603>
Comment 17 WebKit Commit Bot 2018-02-18 02:02:46 PST
All reviewed patches have been landed.  Closing bug.