Bug 108088 - [GStreamer] webkitwebsrc is exposed to application-side
: [GStreamer] webkitwebsrc is exposed to application-side
Status: NEW
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Xabier Rodríguez Calvar
:
Depends on: 109279
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-28 09:27 PST by Philippe Normand
Modified: 2014-12-07 01:15 PST (History)
11 users (show)

See Also:


Attachments
Patch (7.30 KB, patch)
2013-02-01 06:12 PST, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
Patch (7.32 KB, patch)
2013-02-01 09:37 PST, Philippe Normand
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2013-02-01 09:49 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (8.04 KB, patch)
2013-02-01 09:52 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch for landing (8.80 KB, patch)
2013-02-02 07:31 PST, Philippe Normand
webkit-ews: commit‑queue-
Details | Formatted Diff | Diff
Patch for landing #2 (8.86 KB, patch)
2013-02-02 07:45 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (7.92 KB, patch)
2013-04-24 08:58 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2013-04-25 02:24 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2013-01-28 09:27:27 PST
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.
Comment 1 Philippe Normand 2013-01-28 09:53:32 PST
Well... maybe it's time to move to a decodebin-based pipeline
Comment 2 Sebastian Dröge (slomo) 2013-01-31 02:20:05 PST
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.
Comment 3 Philippe Normand 2013-01-31 02:23:35 PST
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.
Comment 4 Philippe Normand 2013-01-31 02:32:38 PST
If we go for option b) nothing prevents people to use the element from their app but they'll be warned :)
Comment 5 Philippe Normand 2013-02-01 06:12:26 PST
Created attachment 186028 [details]
Patch
Comment 6 Martin Robinson 2013-02-01 09:01:28 PST
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.
Comment 7 Philippe Normand 2013-02-01 09:14:22 PST
(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.
Comment 8 Martin Robinson 2013-02-01 09:25:01 PST
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?
Comment 9 Philippe Normand 2013-02-01 09:37:09 PST
Created attachment 186065 [details]
Patch
Comment 10 Early Warning System Bot 2013-02-01 09:49:10 PST
Comment on attachment 186065 [details]
Patch

Attachment 186065 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16301849
Comment 11 Philippe Normand 2013-02-01 09:49:45 PST
Created attachment 186067 [details]
Patch
Comment 12 Philippe Normand 2013-02-01 09:52:25 PST
Created attachment 186069 [details]
Patch
Comment 13 Martin Robinson 2013-02-01 09:54:38 PST
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?
Comment 14 Early Warning System Bot 2013-02-01 10:00:02 PST
Comment on attachment 186069 [details]
Patch

Attachment 186069 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16302835
Comment 15 Early Warning System Bot 2013-02-01 10:00:38 PST
Comment on attachment 186069 [details]
Patch

Attachment 186069 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16307852
Comment 16 Philippe Normand 2013-02-02 07:31:29 PST
Created attachment 186230 [details]
Patch for landing
Comment 17 Martin Robinson 2013-02-02 07:38:10 PST
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.
Comment 18 Early Warning System Bot 2013-02-02 07:39:48 PST
Comment on attachment 186230 [details]
Patch for landing

Attachment 186230 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16338836
Comment 19 Philippe Normand 2013-02-02 07:45:41 PST
Created attachment 186231 [details]
Patch for landing #2
Comment 20 Early Warning System Bot 2013-02-02 07:55:52 PST
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
Comment 21 Philippe Normand 2013-02-02 08:03:02 PST
Committed r141695: <http://trac.webkit.org/changeset/141695>
Comment 22 WebKit Review Bot 2013-02-08 03:12:26 PST
Re-opened since this is blocked by bug 109279
Comment 23 Philippe Normand 2013-02-08 06:56:15 PST
Special attention should be directed to the http/tests/media/video-play-stall.html test when this patch is revised.
Comment 24 Philippe Normand 2013-02-09 09:41:19 PST
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 :)
Comment 25 Martin Robinson 2013-02-09 10:09:12 PST
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?
Comment 26 Philippe Normand 2013-02-09 10:21:50 PST
(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 :)
Comment 27 Martin Robinson 2013-02-09 10:24:36 PST
(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.
Comment 28 Philippe Normand 2013-02-09 10:35:47 PST
(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.
Comment 29 Eric Seidel 2013-03-01 10:39:51 PST
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.
Comment 30 Philippe Normand 2013-04-17 05:46:57 PDT
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.
Comment 31 Xabier Rodríguez Calvar 2013-04-17 05:52:34 PDT
I'll have a look at this as soon as I have a moment.
Comment 32 Xabier Rodríguez Calvar 2013-04-24 08:58:01 PDT
Created attachment 199490 [details]
Patch

Rebased and added the scheduling flag support to the gstreamer source.
Comment 33 Philippe Normand 2013-04-24 09:51:15 PDT
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!
Comment 34 Xabier Rodríguez Calvar 2013-04-25 02:24:08 PDT
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.
Comment 35 Philippe Normand 2013-04-26 02:00:14 PDT
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?
Comment 36 Xabier Rodríguez Calvar 2013-04-26 02:30:33 PDT
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.
Comment 37 Xabier Rodríguez Calvar 2013-04-26 10:33:14 PDT
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.
Comment 38 Philippe Normand 2013-04-29 01:33:08 PDT
(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.
Comment 39 Xabier Rodríguez Calvar 2013-06-18 03:39:19 PDT
(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.
Comment 40 Xabier Rodríguez Calvar 2013-07-15 04:24:40 PDT
(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.
Comment 41 Andre Moreira Magalhaes 2013-08-14 17:54:19 PDT
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).
Comment 42 Philippe Normand 2014-09-17 08:19:24 PDT
(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 :)
Comment 43 Sebastian Dröge (slomo) 2014-09-18 02:31:43 PDT
(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.
Comment 44 Philippe Normand 2014-11-26 07:17:51 PST
I'll move the SCHEDULING query support to a separate patch, we need that for HLS on gst 1.4.x.
Comment 45 Sebastian Dröge (slomo) 2014-12-07 01:15:33 PST
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? :)