Bug 143480

Summary: [GStreamer] extra-headers and keep-alive properties for HTTP source element
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, pnormand, slomo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch cgarcia: review+

Description Philippe Normand 2015-04-07 06:22:08 PDT
These properties are useful for adaptive streaming use-cases, the demuxer tries to set them on the src element used for fragments downloading.
Comment 1 Philippe Normand 2015-04-07 06:31:05 PDT
Created attachment 250264 [details]
patch
Comment 2 WebKit Commit Bot 2015-04-07 06:32:49 PDT
Attachment 250264 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:251:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:253:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:254:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:255:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:258:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:259:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:260:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:261:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:362:  Declaration has space between type name and * in const GstStructure *s  [whitespace/declaration] [3]
Total errors found: 9 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Philippe Normand 2015-04-07 06:35:52 PDT
Created attachment 250265 [details]
patch
Comment 4 Carlos Garcia Campos 2015-04-07 06:58:47 PDT
Comment on attachment 250265 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:112
> +    bool keep_alive;

this should be keepAlive

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:252
> +            FALSE, (GParamFlags) (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)));

Use C++ style cast for GParamFlags

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:256
> +            GST_TYPE_STRUCTURE, (GParamFlags) (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)));

Use C++ style cast for GParamFlags

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:270
> +    priv->keep_alive = FALSE;

keep_alive -> keepAlive, FALSE -> false But this is false already since the private struct is filled with 0 on allocation

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:334
> +    if (priv->extraHeaders) {
> +        gst_structure_free(priv->extraHeaders);
> +        priv->extraHeaders = nullptr;
> +    }

Don't we have a GUniquePtr impl for GstStructure?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:471
> +static gboolean webKitWebSrcSetExtraHeader(GQuark fieldId, const GValue* value, gpointer userData)

This could be bool

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:474
> +    ResourceRequest* request = static_cast<ResourceRequest*>(userData);
> +    const gchar* fieldName = g_quark_to_string(fieldId);

Move this to where it's first needed.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:477
> +    if (G_VALUE_TYPE(value) == G_TYPE_STRING)

G_VALUE_HOLDS_STRING?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:504
> +                return FALSE;

Do we want to return if any of the elements fail, or should we try with the rest?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:506
> +    } else if (G_VALUE_TYPE(value) == GST_TYPE_LIST) {

I would return TRUE in previous if instead of the else

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:511
> +                return FALSE;

Do we want to return if any of the elements fail, or should we try with the rest?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:513
> +        }
> +    } else

Ditto
Comment 5 Philippe Normand 2015-04-07 09:04:12 PDT
Comment on attachment 250265 [details]
patch

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

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:504
>> +                return FALSE;
> 
> Do we want to return if any of the elements fail, or should we try with the rest?

This code is inspired from gst's souphttpsrc element, I'd rather avoid a different behavior here.

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:511
>> +                return FALSE;
> 
> Do we want to return if any of the elements fail, or should we try with the rest?

This code is inspired from gst's souphttpsrc element, I'd rather avoid a different behavior here.
Comment 6 Philippe Normand 2015-04-07 09:05:04 PDT
Created attachment 250269 [details]
patch
Comment 7 Carlos Garcia Campos 2015-04-07 09:19:14 PDT
Comment on attachment 250269 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/GUniquePtrGStreamer.h:35
> +template<> struct GPtrDeleter<GstStructure> {
> +    void operator() (GstStructure* ptr) const
> +    {
> +        if (ptr)
> +            gst_structure_free(ptr);
> +    }
> +};

I just asked if we had this already, but now that we are adding it, it would be easier to use the WTF_DEFINE_GPTR_DELETER macro.

WTF_DEFINE_GPTR_DELETER(GstStructure, gst_structure_free)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:479
> +        return FALSE;

false

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:485
> +    return TRUE;

true

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:491
> +        unsigned n = gst_value_array_get_size(value);

Sorry that I missed this in my previous review, use a more descriptive name than 'n' for the variable.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:501
> +        unsigned n = gst_value_list_get_size(value);

Ditto.
Comment 8 Sebastian Dröge (slomo) 2015-04-07 18:53:54 PDT
Comment on attachment 250269 [details]
patch

Looks good to me too! You probably also want to implement another properties from souphttpsrc here: compress. That's very useful for HTTP adaptive streaming (it's used for the playlists/Manifests there, which are plaintext).
Comment 9 Philippe Normand 2015-04-08 00:11:23 PDT
Committed r182523: <http://trac.webkit.org/changeset/182523>
Comment 10 Philippe Normand 2015-04-08 00:12:15 PDT
(In reply to comment #8)
> Comment on attachment 250269 [details]
> patch
> 
> Looks good to me too! You probably also want to implement another properties
> from souphttpsrc here: compress. That's very useful for HTTP adaptive
> streaming (it's used for the playlists/Manifests there, which are plaintext).

Yeah I saw it's used by hlsdemux, thought about adding it as well, but is it really worth it?
Comment 11 Sebastian Dröge (slomo) 2015-04-08 09:11:44 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Comment on attachment 250269 [details]
> > patch
> > 
> > Looks good to me too! You probably also want to implement another properties
> > from souphttpsrc here: compress. That's very useful for HTTP adaptive
> > streaming (it's used for the playlists/Manifests there, which are plaintext).
> 
> Yeah I saw it's used by hlsdemux, thought about adding it as well, but is it
> really worth it?

They can easily become 1-2MB uncompressed, and as it's plaintext it compresses well and can get down to 100kb or something. Makes a big difference with the time needed from clicking play to getting the first media on the screen/speakers :) Especially if it's audio-only and you have a mobile data connection.