Bug 29842 – [GTK] data: uri support in media player

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

The test media/audio-data-url.html is currently skipped because we pass the raw data to playbin directly and there is no source element to handle that. We could maybe have a source element using giostreamsrc to read the data and decode it (if it is base64 encoded for instance).

[Philippe Normand]

Created attachment 40349 [details]
proposed patch

[Philippe Normand]

Created attachment 40357 [details]
proposed patch

[Sebastian Dröge (slomo)]

Some notes on the GStreamer part:

- use gst_ghost_pad_new_no_target_from_template() instead of gst_ghost_pad_new_no_target() (gives pad<->template association)
- use gst_element_set_details_simple() instead of the one with _simple() (results in smaller compiled code)
- do something useful if there's no giostreamsrc available (like posting a missing plugin message on the bus in GstElement::change_state when going from READY->PAUSED and returning FAILURE there)
- in set_uri: a) pass g_free as GDestroyNotify for the memory input stream and b) unref it after setting it in the giostreamsrc
- Small leak in failure case in set_uri, you don't free the string array

And then you might want to add the child in NULL->READY and remove it again in READY->NULL but that not exactly necessary :) Also, changing the URI in states >= PAUSED shouldn't be allowed.

[Sebastian Dröge (slomo)]

Oh and one note to the parsing of the data: URIs: RFC2397 says, that it's

"data:" [ mediatype ] [ ";base64" ] "," data

mediatype is a normal mimetype, which means that it can contain parameters.

Example:
data:text/plain;charset=ISO-8859-1;base64,ABCDEFGH

Currently you split the URI after the first ";" with "," as delimiter. In the above case this would give you

"charset=ISO-8859-1;base64" and "ABCDEFGH" and then you fail because the first thing is not only base64.

simple check if the first split string has "base64" as suffix or if you want to do it cleaner: split everything after the colon with "," as delimiter and then split the first part with ";" as delimiter and check if that's a mediatype that is supported (e.g. last string is "base64").

[Sebastian Dröge (slomo)]

Also, what's with images that are embedded into websites via a data: URI? You probably really want to check the mimetype before doing anything else and only use GStreamer if it's not something that is directly supported by Webkit (images, plaintext, HTML, ...)

[Philippe Normand]

(In reply to comment #5)
> Also, what's with images that are embedded into websites via a data: URI? You
> probably really want to check the mimetype before doing anything else and only
> use GStreamer if it's not something that is directly supported by Webkit
> (images, plaintext, HTML, ...)

the set_uri() will be called only for <audio> and <video> elements. But you
have an interesting point anyway. We can already check if one mime-type is
supported by our player or not, but this code is in the media player itself.
Could be worth outsourcing it and reusing it in set_uri(). Gustavo, what do you
think?

[Philippe Normand]

Created attachment 40371 [details]
updated patch

[Philippe Normand]

Created attachment 40372 [details]
updated patch with minor fixes and styles fixes

[Philippe Normand]

Created attachment 40373 [details]
final version

[Xan Lopez]

Comment on attachment 40373 [details]
final version

> diff --git a/WebCore/platform/graphics/gtk/DataSourceGStreamer.cpp b/WebCore/platform/graphics/gtk/DataSourceGStreamer.cpp
> new file mode 100644
> index 0000000..acd1524
> --- /dev/null
> +++ b/WebCore/platform/graphics/gtk/DataSourceGStreamer.cpp
> @@ -0,0 +1,266 @@
> +/*
> + *  Copyright (C) 2009  Igalia S.L

I don't think we have double space between the year and the copyright holder.


> +static gboolean
> +webkit_data_src_reset(WebkitDataSrc* src)

WebKit style does not put the return type in a different line (happens elsewhere, sorry I didn't notice before).

> +
> +    {
> +        ret = GST_ELEMENT_CLASS(parent_class)->change_state(element, transition);
> +        if (G_UNLIKELY(ret == GST_STATE_CHANGE_FAILURE))
> +            return ret;
> +    }

Why do you have a new block here?

> +
> +    switch (transition) {
> +    case GST_STATE_CHANGE_PLAYING_TO_PAUSED:
> +    case GST_STATE_CHANGE_PAUSED_TO_READY:
> +    case GST_STATE_CHANGE_READY_TO_NULL:
> +        break;
> +    default:
> +        break;
> +    }

This switch seems pointless?

> +
> +    return ret;
> +}
> +
> +/*** GSTURIHANDLER INTERFACE *************************************************/
> +
> +static GstURIType
> +webkit_data_src_uri_get_type(void)
> +{
> +    return GST_URI_SRC;
> +}
> +
> +static gchar **

No space here.

> +webkit_data_src_uri_get_protocols(void)
> +{
> +    static gchar* protocols[] = {(gchar*) "data", 0 };
> +
> +    return protocols;
> +}
> +
> +static const gchar *

Or here.

> +webkit_data_src_uri_get_uri(GstURIHandler* handler)
> +{
> +    WebkitDataSrc* src = WEBKIT_DATA_SRC(handler);
> +
> +    return src->uri;
> +}
> +

If you can upload a new patch fixing this details I'll r+ it right away :)

[Sebastian Dröge (slomo)]

(In reply to comment #10)
> > +
> > +    switch (transition) {
> > +    case GST_STATE_CHANGE_PLAYING_TO_PAUSED:
> > +    case GST_STATE_CHANGE_PAUSED_TO_READY:
> > +    case GST_STATE_CHANGE_READY_TO_NULL:
> > +        break;
> > +    default:
> > +        break;
> > +    }
> 
> This switch seems pointless?

It makes sure that people add code at the correct position in the future. Downwards state change code should be after chaining up to the parent class while upwards state change code should be before.

(But yes, other than that it's useless)

[Philippe Normand]

Created attachment 40427 [details]
updated patch

[Xan Lopez]

Comment on attachment 40427 [details]
updated patch

r=me

[WebKit Commit Bot]

Comment on attachment 40427 [details]
updated patch

Clearing flags on attachment: 40427

Committed r48963: <http://trac.webkit.org/changeset/48963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.