Bug 108088 - [GStreamer] webkitwebsrc is exposed to application-side
: [GStreamer] webkitwebsrc is exposed to application-side
Status: NEW
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 109279
:
  Show dependency treegraph
 
Reported: 2013-01-28 09:27 PST by
Modified: 2013-10-02 12:28 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2013-01-28 09:53:32 PST -------
Well... maybe it's time to move to a decodebin-based pipeline
------- Comment #2 From 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 From 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 From 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 From 2013-02-01 06:12:26 PST -------
Created an attachment (id=186028) [details]
Patch
------- Comment #6 From 2013-02-01 09:01:28 PST -------
(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.

> 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 From 2013-02-01 09:14:22 PST -------
(In reply to comment #6)
> (From update of attachment 186028 [details] [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 From 2013-02-01 09:25:01 PST -------
(From update of attachment 186028 [details])
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 From 2013-02-01 09:37:09 PST -------
Created an attachment (id=186065) [details]
Patch
------- Comment #10 From 2013-02-01 09:49:10 PST -------
(From update of attachment 186065 [details])
Attachment 186065 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16301849
------- Comment #11 From 2013-02-01 09:49:45 PST -------
Created an attachment (id=186067) [details]
Patch
------- Comment #12 From 2013-02-01 09:52:25 PST -------
Created an attachment (id=186069) [details]
Patch
------- Comment #13 From 2013-02-01 09:54:38 PST -------
(From update of attachment 186069 [details])
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 From 2013-02-01 10:00:02 PST -------
(From update of attachment 186069 [details])
Attachment 186069 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16302835
------- Comment #15 From 2013-02-01 10:00:38 PST -------
(From update of attachment 186069 [details])
Attachment 186069 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16307852
------- Comment #16 From 2013-02-02 07:31:29 PST -------
Created an attachment (id=186230) [details]
Patch for landing
------- Comment #17 From 2013-02-02 07:38:10 PST -------
(From update of attachment 186230 [details])
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 From 2013-02-02 07:39:48 PST -------
(From update of attachment 186230 [details])
Attachment 186230 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16338836
------- Comment #19 From 2013-02-02 07:45:41 PST -------
Created an attachment (id=186231) [details]
Patch for landing #2
------- Comment #20 From 2013-02-02 07:55:52 PST -------
(From update of attachment 186231 [details])
Attachment 186231 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16342829
------- Comment #21 From 2013-02-02 08:03:02 PST -------
Committed r141695: <http://trac.webkit.org/changeset/141695>
------- Comment #22 From 2013-02-08 03:12:26 PST -------
Re-opened since this is blocked by bug 109279
------- Comment #23 From 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 From 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 From 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 From 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 From 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 From 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 From 2013-03-01 10:39:51 PST -------
(From update of attachment 186069 [details])
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 From 2013-04-17 05:46:57 PST -------
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 From 2013-04-17 05:52:34 PST -------
I'll have a look at this as soon as I have a moment.
------- Comment #32 From 2013-04-24 08:58:01 PST -------
Created an attachment (id=199490) [details]
Patch

Rebased and added the scheduling flag support to the gstreamer source.
------- Comment #33 From 2013-04-24 09:51:15 PST -------
(From update of attachment 199490 [details])
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 From 2013-04-25 02:24:08 PST -------
Created an attachment (id=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 From 2013-04-26 02:00:14 PST -------
(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.

> 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 From 2013-04-26 02:30:33 PST -------
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] [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 From 2013-04-26 10:33:14 PST -------
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 From 2013-04-29 01:33:08 PST -------
(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 From 2013-06-18 03:39:19 PST -------
(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 From 2013-07-15 04:24:40 PST -------
(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 From 2013-08-14 17:54:19 PST -------
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).