Bug 108088 – [GStreamer] webkitwebsrc is exposed to application-side

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

[Philippe Normand]

See https://bugs.kde.org/show_bug.cgi?id=310763 and https://projects.kde.org/projects/kdesupport/phonon/phonon-gstreamer/repository/revisions/master/entry/gstreamer/pipeline.cpp#L859

We call gst_element_register() to register our http source element and it seems that applications using webkit (for UI I guess) and gst playbin to play some http media get webkitwebsrc auto-plugged.

Webkitwebsrc should remain private to our media player and not "leak out" like that.

[Philippe Normand]

Well... maybe it's time to move to a decodebin-based pipeline

[Sebastian Dröge (slomo)]

I only see the following options here
a) Make it possible for applications to use webkitwebsrc too
b) Use a different URI protocol (e.g. http+webkit://)
c) Make webkitwebsrc private and stop using playbin2.


c) is not really a great option, you'll lose all the multi-stream handling (e.g. multiple audio streams), the handling of multiple "groups" (e.g. fragments in HLS or chained oggs), clever sink selection and a convenient API. IMHO only a) and b) should be considered.

[Philippe Normand]

The b) option would be interesting to explore I think.
a) doesn't make much sense to me, webkitwebsrc uses some rather internal WebCore APIs, I'm not sure we can't guarantee they'd work out of WebKit's context.

[Philippe Normand]

If we go for option b) nothing prevents people to use the element from their app but they'll be warned :)

[Philippe Normand]

Created attachment 186028 [details]
Patch

[Martin Robinson]

Comment on attachment 186028 [details]
Patch

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

Looks good, but I think it could use a teensy bit of cleanup.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:307
> +void MediaPlayerPrivateGStreamer::setPlaybinURL(KURL url)

This should likely accept a const KURL& url to avoid a copy.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:335
> +    // Clean out everything after file:// url path.
> +    if (kurl.isLocalFile())
> +        cleanUrl = cleanUrl.substring(0, kurl.pathEnd());

kurl.removeFragmentIdentifier() ? Probably better to put this into setPlaybinURL.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:579
> +    ASSERT(url.protocol().substring(0, 7) == "webkit+");
> +    url.setProtocol(url.protocol().substring(7));

You should probably make a getPlaybinURL method.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:598
> -    static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 };
> +    static gchar* protocols[] = {(gchar*) "webkit+http", (gchar*) "webkit+https", 0 };

So do we even support file URLs? I'm pretty sure you don't need these casts here.

[Philippe Normand]

(In reply to comment #6)
> (From update of attachment 186028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186028&action=review
> 
> Looks good, but I think it could use a teensy bit of cleanup.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:307
> > +void MediaPlayerPrivateGStreamer::setPlaybinURL(KURL url)
> 
> This should likely accept a const KURL& url to avoid a copy.
> 

I tried that when writing the initial patch but it didn't work out, will give it another chance.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:335
> > +    // Clean out everything after file:// url path.
> > +    if (kurl.isLocalFile())
> > +        cleanUrl = cleanUrl.substring(0, kurl.pathEnd());
> 
> kurl.removeFragmentIdentifier() ? Probably better to put this into setPlaybinURL.

Well this code is shuffled around, I didn't mean to modify it but I guess I can give this a try :)

> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:579
> > +    ASSERT(url.protocol().substring(0, 7) == "webkit+");
> > +    url.setProtocol(url.protocol().substring(7));
> 
> You should probably make a getPlaybinURL method.
> 

Hum but the element has no access to the player.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:598
> > -    static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 };
> > +    static gchar* protocols[] = {(gchar*) "webkit+http", (gchar*) "webkit+https", 0 };
> 
> So do we even support file URLs? I'm pretty sure you don't need these casts here.

file URLs are supported via the gst filesrc element.
IIRC the casts were needed when I did the initial port but I'll check it again.

[Martin Robinson]

Comment on attachment 186028 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:579
>>> +    url.setProtocol(url.protocol().substring(7));
>> 
>> You should probably make a getPlaybinURL method.
> 
> Hum but the element has no access to the player.

Hrm. Maybe a static method just for converting then?

[Philippe Normand]

Created attachment 186065 [details]
Patch

[Early Warning System Bot]

Comment on attachment 186065 [details]
Patch

Attachment 186065 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16301849

[Philippe Normand]

Created attachment 186067 [details]
Patch

[Philippe Normand]

Created attachment 186069 [details]
Patch

[Martin Robinson]

Comment on attachment 186069 [details]
Patch

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

Thanks!

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:343
> +    // Clean out everything after file:// url path.
> +    if (kurl.isLocalFile())
> +        kurl.removeFragmentIdentifier();

Mind moving this into MediaPlayerPrivateGStreamer::setPlaybinURL?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:142
> +            static KURL getPlaybinURL(const gchar* uri);

Probably should be called convertPlaybinURL since it's not a getter.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:596
> -    static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 };
> +    static gchar* protocols[] = {"webkit+http", "webkit+https", 0 };

I guess you need  const here?

[Early Warning System Bot]

Comment on attachment 186069 [details]
Patch

Attachment 186069 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16302835

[Early Warning System Bot]

Comment on attachment 186069 [details]
Patch

Attachment 186069 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16307852

[Philippe Normand]

Created attachment 186230 [details]
Patch for landing

[Martin Robinson]

Comment on attachment 186230 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:343
> +    // Clean out everything after file:// url path.
> +    if (kurl.isLocalFile())
> +        kurl.removeFragmentIdentifier();

I think it makes a lot of sense to move this into MediaPlayerPrivateGStreamer::setPlaybinURL.

[Early Warning System Bot]

Comment on attachment 186230 [details]
Patch for landing

Attachment 186230 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16338836

[Philippe Normand]

Created attachment 186231 [details]
Patch for landing #2

[Early Warning System Bot]

Comment on attachment 186231 [details]
Patch for landing #2

Attachment 186231 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16342829

[Philippe Normand]

Committed r141695: <http://trac.webkit.org/changeset/141695>

[WebKit Review Bot]

Re-opened since this is blocked by bug 109279

[Philippe Normand]

Special attention should be directed to the http/tests/media/video-play-stall.html test when this patch is revised.

[Philippe Normand]

Well this patch can land again once those 2 GStreamer bugs are fixed:
https://bugzilla.gnome.org/show_bug.cgi?id=638749
https://bugzilla.gnome.org/show_bug.cgi?id=693484

I checked the media and http/tests/media tests pass and that on-disk buffering still works :)

[Martin Robinson]

Is WebKit broken without the new versions of GStreamer? If so we need to update the versions in configure.ac. Either way the patch needs to bump the version in the jhbuild, right?

[Philippe Normand]

(In reply to comment #25)
> Is WebKit broken without the new versions of GStreamer?

No it's not, we rolled out the patch for now, so no functionality is broken. We still expose webkitwebsrc to apps but this has been so for 3 years already.

> If so we need to update the versions in configure.ac. Either way the patch needs to bump the version in the jhbuild, right?

We'll need to bump the gst versions in jhbuild yes. 1.0.6 is supposed to be released soon but I'm not sure my patches will land on time for that release :)

[Martin Robinson]

(In reply to comment #26)
> (In reply to comment #25)
> > Is WebKit broken without the new versions of GStreamer?
> 
> No it's not, we rolled out the patch for now, so no functionality is broken. We still expose webkitwebsrc to apps but this has been so for 3 years already.

Oh, I mean when we land the patch again.

[Philippe Normand]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Is WebKit broken without the new versions of GStreamer?
> > 
> > No it's not, we rolled out the patch for now, so no functionality is broken. We still expose webkitwebsrc to apps but this has been so for 3 years already.
> 
> Oh, I mean when we land the patch again.

Ok so yes, for sure we can't land this again without bumping our gst requirements.

[Eric Seidel (no email)]

Comment on attachment 186069 [details]
Patch

Cleared Martin Robinson's review+ from obsolete attachment 186069 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Philippe Normand]

We need a new patch doing something simirlar to http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=d975a70f12d13ea057de1ef760a79108ead43990

with ifdefs for GStreamer 1.1.x I suppose.

[Xabier Rodríguez Calvar]

I'll have a look at this as soon as I have a moment.

[Xabier Rodríguez Calvar]

Created attachment 199490 [details]
Patch

Rebased and added the scheduling flag support to the gstreamer source.

[Philippe Normand]

Comment on attachment 199490 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:645
> -    static gchar* protocols[] = {(gchar*) "http", (gchar*) "https", 0 };
> +    static gchar* const protocols[] = {(gchar*) "webkit+http", (gchar*) "webkit+https", 0 };

This is the 0.10 code path, right? If so, please revert this.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:670
> -    KURL url(KURL(), uri);
> +    KURL url = WebCore::MediaPlayerPrivateGStreamer::convertPlaybinURL(uri);

Ditto!

[Xabier Rodríguez Calvar]

Created attachment 199641 [details]
Patch

Fixed issues with the scheduling flag and the 0.10 codepath. We need to wait for next GStreamer release before finishing this.

[Philippe Normand]

Comment on attachment 199641 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:557
> +        gst_query_parse_scheduling(query, &flags, &minSize, &maxSize, &align);

I'm not sure any element answers the query before us so I suspect the query is empty so no parsing would be needed. For min/max/align you could use the default values I suppose.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558
> +        flags = static_cast<GstSchedulingFlags>(static_cast<unsigned>(flags) | GST_SCHEDULING_FLAG_BANDWIDTH_LIMITED);

Why all the casts? flags |= GST_SCHEDULING_FLAG_BANDWIDTH_LIMITED; not sufficient?

[Xabier Rodríguez Calvar]

Ok, I'll fix this when we have GStreamer release and we can push this.

(In reply to comment #35)
> (From update of attachment 199641 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199641&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:557
> > +        gst_query_parse_scheduling(query, &flags, &minSize, &maxSize, &align);
> 
> I'm not sure any element answers the query before us so I suspect the query is empty so no parsing would be needed. For min/max/align you could use the default values I suppose.

Ok.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558
> > +        flags = static_cast<GstSchedulingFlags>(static_cast<unsigned>(flags) | GST_SCHEDULING_FLAG_BANDWIDTH_LIMITED);
> 
> Why all the casts? flags |= GST_SCHEDULING_FLAG_BANDWIDTH_LIMITED; not sufficient?

No, typing problems.

[Xabier Rodríguez Calvar]

It seems that https://bugzilla.gnome.org/show_bug.cgi?id=693484 didn't make it for the release. We'll have to wait for what comes next after 1.0.7.

[Philippe Normand]

(In reply to comment #37)
> It seems that https://bugzilla.gnome.org/show_bug.cgi?id=693484 didn't make it for the release. We'll have to wait for what comes next after 1.0.7.

It will be shipped in 1.1.x/1.2.

[Xabier Rodríguez Calvar]

(In reply to comment #38)
> It will be shipped in 1.1.x/1.2.

Adding dependency to the bug about bumping the gst version.

[Xabier Rodríguez Calvar]

(In reply to comment #39)
> (In reply to comment #38)
> > It will be shipped in 1.1.x/1.2.
> 
> Adding dependency to the bug about bumping the gst version.

Removing dependency as fixes needed in GStreamer for this didn't make it to 1.0.8.

[Andre Moreira Magalhaes]

As discussed on IRC, this patch would break the use case where a gstreamer plugin uses the webkit gst source internally to download data (through gst_element_make_from_uri) which is the case for DASH/MSS and others.

In this case, if using the proposed patch, the DASH plugin for example would use the webkit src to download the main manifest file (playlist) and souphttpsrc or any other available source to download the fragments internally which is undesirable.

The bug is still valid though as we may want to allow apps to opt not to use the webkit src explicitly (see https://bugzilla.gnome.org/show_bug.cgi?id=705976).

[Philippe Normand]

(In reply to comment #41)
> As discussed on IRC, this patch would break the use case where a gstreamer plugin uses the webkit gst source internally to download data (through gst_element_make_from_uri) which is the case for DASH/MSS and others.
> 
> In this case, if using the proposed patch, the DASH plugin for example would use the webkit src to download the main manifest file (playlist) and souphttpsrc or any other available source to download the fragments internally which is undesirable.
> 

I've been wondering if it'd be possible for the demuxer to reuse the same src element instead of creating a new one for each fragment. Is that technically possible?

If we could do that, the fragments would be downloaded using the src element (or new instances of it) that was used to fetch the main playlist.

> The bug is still valid though as we may want to allow apps to opt not to use the webkit src explicitly (see https://bugzilla.gnome.org/show_bug.cgi?id=705976).

Yes :)

[Sebastian Dröge (slomo)]

(In reply to comment #42)
> (In reply to comment #41)
> > As discussed on IRC, this patch would break the use case where a gstreamer plugin uses the webkit gst source internally to download data (through gst_element_make_from_uri) which is the case for DASH/MSS and others.
> > 
> > In this case, if using the proposed patch, the DASH plugin for example would use the webkit src to download the main manifest file (playlist) and souphttpsrc or any other available source to download the fragments internally which is undesirable.
> > 
> 
> I've been wondering if it'd be possible for the demuxer to reuse the same src element instead of creating a new one for each fragment. Is that technically possible?
> 
> If we could do that, the fragments would be downloaded using the src element (or new instances of it) that was used to fetch the main playlist.

It should already use the same source element for all the fragments, but a different one than for the main playlist.

Using the same one also for the main playlist will be very tricky and so far I only know a workaround that can make this possible. Problem here is that you would need to re-use the source element of the pipeline inside some element in the middle of the pipeline.

A workaround here would be to implement the source as a GstBin that includes the actual source element... and then on EOS (of the playlist) make the actual source element available in some kind of free-list, where it then would be taken by the adaptive streaming demuxer. This however will only work for adaptive streaming formats and must not be done for anything else, it will break otherwise.

[Philippe Normand]

I'll move the SCHEDULING query support to a separate patch, we need that for HLS on gst 1.4.x.

[Sebastian Dröge (slomo)]

Is there a plan here now?

I think the main problem that is left to be solved is that GStreamer elements instantiated by the playbin inside WebKit would want to use the webkitwebsrc for http/https URIs (hlsdemux!). So we need to have it available with highest rank inside webkit pipelines, and only hide it in external pipelines.

This is currently not possible with GStreamer, elements are registered globally and have a global rank. To make this possible will need to make these things configurable somehow. Maybe with a per-thread setting (think g_main_context_push_thread_default() / pop_thread_default()) and have elements inherit it from the threads where they're created.

Better ideas? :)

[Philippe Normand]

I think we can actually land the original patch again.

Why is it an issue anyway to download the playlist/manifest with webkitwebsrc and the fragments with souphttpsrc? Both elements now behave equally from the point of view of adaptivedemux, the important properties adaptivedemux/uridownloader uses are correctly exposed on both src elements.

[Sebastian Dröge (slomo)]

Cookies and authentication

[Philippe Normand]

(In reply to comment #47)
> Cookies and authentication

Ok that can be fixed if the HTTP context sharing goes through :)
https://bugzilla.gnome.org/show_bug.cgi?id=726314

[Carlos Garcia Campos]

Can we close this now after r219252?

[Xabier Rodríguez Calvar]

(In reply to Carlos Garcia Campos from comment #49)
> Can we close this now after r219252?

I think so, but I leave Charlie to have the final word.

[Charlie Turner]

It's not leaking out anymore, but the best approach would be to solve https://bugzilla.gnome.org/show_bug.cgi?id=705976 instead of using this workaround, but as of today there's no better alternative.

I'll close.