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
Can you please add a ChangeLog and check the code style with Tools/Scripts/check-webkit-style ?
ah also see http://www.webkit.org/coding/contributing.html :)
Created attachment 82777 [details] Patch
LGTM. Any reviewer to flip the switch? :)
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"?
Created attachment 82964 [details] Patch
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
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); }
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 ;)
Committed r79200: <http://trac.webkit.org/changeset/79200>
I think the GRefPtr stuff should be part of another patch, I'll try to prepare that.