Bug 143480 – [GStreamer] extra-headers and keep-alive properties for HTTP source element

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

These properties are useful for adaptive streaming use-cases, the demuxer tries to set them on the src element used for fragments downloading.

[Philippe Normand]

Created attachment 250264 [details]
patch

[WebKit Commit Bot]

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.

[Philippe Normand]

Created attachment 250265 [details]
patch

[Carlos Garcia Campos]

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

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 250269 [details]
patch

[Carlos Garcia Campos]

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.

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

Committed r182523: <http://trac.webkit.org/changeset/182523>

[Philippe Normand]

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

[Sebastian Dröge (slomo)]

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