Bug 34003 – [GTK] Pass cookies to GStreamer

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sebastian Dröge (slomo)]

Hi,
the attached patch adds support for passing cookies to the GStreamer HTTP elements. this fixes the new youtube HTML5 stuff for example.

[Sebastian Dröge (slomo)]

Created attachment 47203 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

[Sebastian Dröge (slomo)]

Created attachment 47204 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch

[WebKit Review Bot]

Attachment 47203 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:130:  Declaration has space between type name and * in GstElement *element  [whitespace/declaration] [3]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:132:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:134:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:136:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:274:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 47203 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

In addition to the style checker output (which seems valid for the case), I have the following comments:

 132     g_object_get (mp->m_playBin, "source", &element, NULL);
 133     old_element = mp->m_source;
 134     mp->m_source = NULL;

The old_element variable (besides having a bad name given the style) seems to be unnecessary. You could check, and unref mp->m_source directly instead of assigning it to another variable.

 135     if (old_element)
 136         gst_object_unref (old_element);
 137     mp->m_source = element;

This looks like a crash - we are not refing the element ourselves (perhaps we should), so unreffing it seems wrong (the same goes for the destructor).

[Marc-Andre Lureau]

Dan Winship told me yesterday that another solution would be to copy the webkit soup session and hand it to gstreamer.

I believe this solution would be more future-proof, because at some point we will need more context: referrer, auth/ssl, proxy (not sure if it's relevant for webkit/gnome, but that's the idea)...

[Sebastian Dröge (slomo)]

(In reply to comment #4)
> (From update of attachment 47203 [details])
> In addition to the style checker output (which seems valid for the case), I
> have the following comments:
> 
>  132     g_object_get (mp->m_playBin, "source", &element, NULL);
>  133     old_element = mp->m_source;
>  134     mp->m_source = NULL;
> 
> The old_element variable (besides having a bad name given the style) seems to
> be unnecessary. You could check, and unref mp->m_source directly instead of
> assigning it to another variable.

Ok :)

>  135     if (old_element)
>  136         gst_object_unref (old_element);
>  137     mp->m_source = element;
> 
> This looks like a crash - we are not refing the element ourselves (perhaps we
> should), so unreffing it seems wrong (the same goes for the destructor).

g_object_get will ref the element for us


(In reply to comment #5)
> Dan Winship told me yesterday that another solution would be to copy the webkit
> soup session and hand it to gstreamer.
> 
> I believe this solution would be more future-proof, because at some point we
> will need more context: referrer, auth/ssl, proxy (not sure if it's relevant
> for webkit/gnome, but that's the idea)...

Problem with this is, that SoupSession is not threadsafe unfortunately

[Dan Winship]

(In reply to comment #5)
> Dan Winship told me yesterday that another solution would be to copy the webkit
> soup session and hand it to gstreamer.
> 
> I believe this solution would be more future-proof, because at some point we
> will need more context: referrer, auth/ssl, proxy (not sure if it's relevant
> for webkit/gnome, but that's the idea)...

and also, then if the server response with a redirection, you can look up the right cookies for the new host, instead of continuing to send the cookies for the old host...

(I don't know if that's relevant in practice... I can imagine something like a video.google.com URL redirecting to youtube.com though...)

(In reply to comment #6)
> Problem with this is, that SoupSession is not threadsafe unfortunately

(SoupSession*Async* that is.) Ah, yes, there is that. Or more importantly, it can only have a single GMainContext, and IIRC souphttpsrc wants to set its own GMainContext on the session so that it control when I/O is and isn't happening. (The libsoup i/o rewrite / gio-ification might solve this problem.)

It's possible you could share SoupSessionFeatures (cookie jar, proxy URI resolver, etc), without sharing the whole session. Some of them might not currently be thread-safe, but they could be fixed.

[Gustavo Noronha (kov)]

Comment on attachment 47204 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch

>
> @@ -49,6 +49,8 @@
>  #include <math.h>
>  #include <wtf/gtk/GOwnPtr.h>
>  
> +#include <webkit/webkitwebview.h>

This should go along with the other includes above. Between math and wtf/gtk sounds right.

> +    if (element) {
> +        GParamSpec *pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (element), "cookies");

You have a number of style problems in this patch - mostly spaces between function names, and parenses, and misplaced *. Other than that, the patch looks good. I would still prefer to be able to set the same cookie jar, though, because I'm pretty sure we'll hit some corner case this is not going to work for. We might as well consider, for the future, writing a custom source that uses WebCore's machinery to download the video data, by the way.

[Marc-Andre Lureau]

(In reply to comment #8)

> for. We might as well consider, for the future, writing a custom source that
> uses WebCore's machinery to download the video data, by the way.

Firefox is using appsrc. However, having a source element make it difficult writing an Apple HTTP Live Streaming source element. (and other similar video streaming per playlist technologies as well - they need to perform request by themself)

I would prefer either a Soup session or an URI provider (by interface/factory).

[Philippe Normand]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > for. We might as well consider, for the future, writing a custom source that
> > uses WebCore's machinery to download the video data, by the way.
> 
> Firefox is using appsrc. However, having a source element make it difficult
> writing an Apple HTTP Live Streaming source element. (and other similar video
> streaming per playlist technologies as well - they need to perform request by
> themself)
> 
> I would prefer either a Soup session or an URI provider (by interface/factory).

We already developed a source element for data: uris ;) And I am sure we will consider any patch from you adding a nice Apple Live Streaming source element! But I feel like we deviate a bit ;)

[Sebastian Dröge (slomo)]

(In reply to comment #7)
> (In reply to comment #5)
> > Dan Winship told me yesterday that another solution would be to copy the webkit
> > soup session and hand it to gstreamer.
> > 
> > I believe this solution would be more future-proof, because at some point we
> > will need more context: referrer, auth/ssl, proxy (not sure if it's relevant
> > for webkit/gnome, but that's the idea)...
> 
> and also, then if the server response with a redirection, you can look up the
> right cookies for the new host, instead of continuing to send the cookies for
> the old host...
> 
> (I don't know if that's relevant in practice... I can imagine something like a
> video.google.com URL redirecting to youtube.com though...)

Yeah, I thought about that one too. It might be a good idea to let souphttpsrc notify about redirects so apps can do whatever is necessary for that URI redirect (cookie updates, storing the real URI somewhere, whatever)

> (In reply to comment #6)
> > Problem with this is, that SoupSession is not threadsafe unfortunately
> 
> (SoupSession*Async* that is.) Ah, yes, there is that. Or more importantly, it
> can only have a single GMainContext, and IIRC souphttpsrc wants to set its own
> GMainContext on the session so that it control when I/O is and isn't happening.
> (The libsoup i/o rewrite / gio-ification might solve this problem.)

Yes, souphttpsrc is using it's own GMainContext and runs it's own GMainLoop on it in another thread (one of the reasons being, that GStreamer doesn't require a main loop running in the default main context).

> It's possible you could share SoupSessionFeatures (cookie jar, proxy URI
> resolver, etc), without sharing the whole session. Some of them might not
> currently be thread-safe, but they could be fixed.

That's the thing I thought about tonight... I guess it would be possible and probably a good idea to add a SoupSessionFeatures property, then webkit can set it's CookieJar, proxy stuff, etc.

I was just worried about them not being threadsafe either.

(In reply to comment #8)
> 
> You have a number of style problems in this patch - mostly spaces between
> function names, and parenses, and misplaced *.

Yes, I noticed. But I wanted to get this patch in bugzilla before I left yesterday to get some first reviews until I'm back :)

> Other than that, the patch looks
> good. I would still prefer to be able to set the same cookie jar, though,
> because I'm pretty sure we'll hit some corner case this is not going to work
> for. We might as well consider, for the future, writing a custom source that
> uses WebCore's machinery to download the video data, by the way.

Another possibility, yes. totem's mozilla plugin does something like this for example.



Ok, so would you guys all be happy if souphttpsrc gets a property, that takes a GValueArray of SoupSessionFeature? (And of course style fixes everywhere)

[Sebastian Dröge (slomo)]

Actually it might be better to get something like the current patch in and then spend efforts on creating some kind of webkit source, which uses the webkit infrastructure. This would automatically use the cookies, authentication information, etc. and will probably integrate better in any way. I could work on that afterwards ;)

[Sebastian Dröge (slomo)]

Created attachment 47276 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

[Sebastian Dröge (slomo)]

Created attachment 47277 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch

This fixes the style problems and other things mentioned before. I'd say we should get this in this way to be able to play YouTube's HTML5 videos and make people happy. And afterwards I'll work on the webkit source element that uses the webkit infrastructure.

[WebKit Review Bot]

Attachment 47276 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:133:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:134:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:135:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:272:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sebastian Dröge (slomo)]

Created attachment 47279 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

[Sebastian Dröge (slomo)]

Created attachment 47280 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch

[Sebastian Dröge (slomo)]

Created attachment 47281 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch

[Sebastian Dröge (slomo)]

Created attachment 47331 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

[Sebastian Dröge (slomo)]

Created attachment 47332 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch

[Gustavo Noronha (kov)]

(In reply to comment #12)
> Actually it might be better to get something like the current patch in and then
> spend efforts on creating some kind of webkit source, which uses the webkit
> infrastructure. This would automatically use the cookies, authentication
> information, etc. and will probably integrate better in any way. I could work
> on that afterwards ;)

I'm happy with going this way, yeah, getting this in, and then working on a source =).

[Gustavo Noronha (kov)]

Comment on attachment 47331 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 47331 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

Clearing flags on attachment: 47331

Committed r53811: <http://trac.webkit.org/changeset/53811>

[Sebastian Dröge (slomo)]

(In reply to comment #22)
> (From update of attachment 47331 [details])
> Looks good to me.

Thanks but what about the second, actually useful patch? :)

[Philippe Normand]

For websites (like... Vimeo) requiring cookies AND referer:

+        Frame* frame = mp->m_player->frameView() ? mp->m_player->frameView()->frame() : 0;
+        Document* document = frame ? frame->document() : 0;
+        if (document) {
+            gchar* referer = g_strdup(document->documentURI().utf8().data());
+            GstStructure* extraHeaders = gst_structure_new("extra-headers", "Referer", G_TYPE_STRING,
+                                                           referer, NULL);
+            g_object_set(element, "extra-headers", extraHeaders, NULL);
+        }

slomo ^^ :)

[Gustavo Noronha (kov)]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 47331 [details] [details])
> > Looks good to me.
> 
> Thanks but what about the second, actually useful patch? :)

Well, one thing at a time, right? =)

I had little time, but I am now trying to come up with a test case that we could check in along with the patch. The patch itself looks good to me, but I would prefer we check it in with a test case.

[Gustavo Noronha (kov)]

Created attachment 47402 [details]
test for this problem

This is the test case I came up with up to now. There are a number of issues - the other http/media test seems to use video-test.js from where it normally is located, but I couldn't get it to work without copying it there. I dislike the name of the test =D. The test works when I run it manually, either in Epiphany or DumpRenderTree, but when running it through run-webkit-tests it seems to always fail, I don't really know why. The good news is it does test the problem.

[Gustavo Noronha (kov)]

Created attachment 47612 [details]
test for the problem

[Gustavo Noronha (kov)]

Created attachment 47624 [details]
slomo's patch with my test, as a whole patch

I consider the patch r+'ed by me, like I said in the comment, but I'd like to land it with a test, so I'm working on one with Eric Carlson, this is the result so far.

[Sebastian Dröge (slomo)]

FWIW, the webkit HTTP source element I'm writing is more or less finished now, including seeking support. It's only a bit fragile still, I'm working on that ;)

[Eric Carlson]

Comment on attachment 47624 [details]
slomo's patch with my test, as a whole patch

> diff --git a/LayoutTests/http/tests/media/resources/video-cookie-check-cookie.html b/LayoutTests/http/tests/media/resources/video-cookie-check-cookie.html

This file isn't used any more.


> +++ b/LayoutTests/http/tests/media/resources/video-cookie-check-cookie.php
> @@ -0,0 +1,23 @@
> +<?php
> +   if($_COOKIE["TEST"])
> +   {

Three space tabs?


> diff --git a/LayoutTests/http/tests/media/video-cookie.html b/LayoutTests/http/tests/media/video-cookie.html
> new file mode 100644
> index 0000000..e02b72e
> --- /dev/null
> +++ b/LayoutTests/http/tests/media/video-cookie.html

> +<html>
> +<body onload="loadCookie()">
> +<video id="video"></video>
> +<script src=../../../media/media-file.js></script>

  I would prefer to see the <script> elements in the <head> because they are triggered by the 'load' event.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 7168d3e..065f7b8 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,18 @@
> [phone]  Sebastian Dröge  <[e-mail]>

  "Dröge" - need to save as UTF8?


>  2010-01-22  Sebastian Dröge  <[e-mail]>

  Here too.


  This new test didn't work on the Mac the last time I looked, and I won't be able to look at it today, so why don't you add it to the skipped list and write a bug, assigned to me, to figure out why and fix it.
  
r=me with these changes.

[Gustavo Noronha (kov)]

(In reply to comment #31)
>   I would prefer to see the <script> elements in the <head> because they are
> triggered by the 'load' event.

I have made all the changes but this one, because when I move the script elements to head the test stops working. Since fixing this will apparently require a small rework of how the test is written, I'll leave this for a second pass. Hope you don't mind.

[Gustavo Noronha (kov)]

Landed patch with test as r54026.