Bug 54627 – [GStreamer] URI queries support in webkitwebsrc

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Andoni]

Created attachment 82770 [details]
Add support for uri queries

Attached patch that adds support for URI queries.

You can find it in the following branch too.
http://gitorious.org/~ylatuya/webkitgtk/ylatuyas-webkit/commits/hls

[Philippe Normand]

Can you please add a ChangeLog and check the code style with Tools/Scripts/check-webkit-style ?

[Philippe Normand]

ah also see http://www.webkit.org/coding/contributing.html :)

[Andoni]

Created attachment 82777 [details]
Patch

[Philippe Normand]

LGTM. Any reviewer to flip the switch? :)

[Martin Robinson]

Comment on attachment 82777 [details]
Patch

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

Thanks for your contribution! Please consider the following review comments. This patch is only a few changes from an r+.

> WebCore/ChangeLog:10
> +        [GStreamer] URI queries support in webkitwebsrc
> +        https://bugs.webkit.org/show_bug.cgi?id=54627
> +
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> +        (webkit_web_src_init):
> +        (webKitWebSrcQuery):

Do you mind explaining a bit what this adds? Those familiar with Gstreamer will understand what sry query support means, but not the majority of the people skimming the ChangeLog.

> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:120
> +static gboolean webKitWebSrcQuery(GstPad * pad, GstQuery * query);

No spaces before the asterisks here.

> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:493
> +
> +static gboolean webKitWebSrcQuery(GstPad * pad, GstQuery * query)
> +{
> +    WebKitWebSrc * src = WEBKIT_WEB_SRC(gst_pad_get_parent_element(pad));

Ditto.

> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:494
> +    gboolean ret;

Normally we don't abbreviate variable names in WebKit, so perhaps something like "success"?

[Andoni]

Created attachment 82964 [details]
Patch

[Andoni]

Updated patch with your suggestions.

I'm currently trying to add support for http live streaming streams[1] to gstreamer and as part of it I wanted to be able to watch them using webkit with the video tag.
The hls demuxer is using gst_element_from_uri() to fetch fragments and update the playlist so that the webkit source element is used.
It needs to have the 'location' property, this way the uri can be set in the element and it also needs to reply to URI queries so that the demuxer can know the uri of the playlist and use it to update it.

You can see a screenshot[2] of epiphany playing a multibitrate stream in gstreamer's bug reports[3] :) 


[1]http://tools.ietf.org/html/draft-pantos-http-live-streaming-05
[2]http://bugzilla-attachments.gnome.org/attachment.cgi?id=181032
[3]https://bugzilla.gnome.org/show_bug.cgi?id=594035

[Martin Robinson]

Comment on attachment 82964 [details]
Patch

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

Great stuff. Philippe do you mind landing this with the corrected ChangeLog?

> WebCore/ChangeLog:11
> +        No new tests. (OOPS!)

Just need to replace this with an explanation of why there are no tests.

> WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:511
> +    WebKitWebSrc* src = WEBKIT_WEB_SRC(gst_pad_get_parent_element(pad));
> +    gboolean success;
> +
> +    switch (GST_QUERY_TYPE(query)) {
> +    case GST_QUERY_URI:
> +        gst_query_set_uri(query, src->priv->uri);
> +        success = TRUE;
> +        break;
> +    default:
> +        success = FALSE;
> +        break;
> +    }
> +
> +    if (!success)
> +        success = gst_element_query(GST_ELEMENT(src->priv->appsrc), query);
> +
> +    gst_object_unref(src);
> +    return success;

This looks fine, but adding a GRefPtr specialization for GstObjects would make it even cleaner:

{
    GRefPtr<GstObject*> src = adoptGRef(WEBKIT_WEB_SRC(gst_pad_get_parent_element(pad)));
    if (GST_QUERY_TYPE(query) == GST_QUERY_URI) {
        gst_query_set_uri(query, src->priv->uri)
        return TRUE;
    }
    return gst_element_query(GST_ELEMENT(src->priv->appsrc), query);
}

[Philippe Normand]

I just noticed this patch was made against an old version of the module. I added DURATION query support since then, so the patch needs fixup before landing, I'll do that, complete the ChangeLog and land the patch ;)

[Philippe Normand]

Committed r79200: <http://trac.webkit.org/changeset/79200>

[Philippe Normand]

I think the GRefPtr stuff should be part of another patch, I'll try to prepare that.