Bug 34003 - [GTK] Pass cookies to GStreamer
Summary: [GTK] Pass cookies to GStreamer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-22 07:37 PST by Sebastian Dröge (slomo)
Modified: 2010-01-28 15:56 PST (History)
9 users (show)

See Also:


Attachments
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch (5.12 KB, patch)
2010-01-22 07:54 PST, Sebastian Dröge (slomo)
gns: review-
Details | Formatted Diff | Diff
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch (3.23 KB, patch)
2010-01-22 07:55 PST, Sebastian Dröge (slomo)
gns: review-
Details | Formatted Diff | Diff
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch (5.44 KB, patch)
2010-01-23 13:59 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch (3.06 KB, patch)
2010-01-23 14:00 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch (5.83 KB, patch)
2010-01-23 14:11 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch (3.06 KB, patch)
2010-01-23 14:11 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch (3.23 KB, patch)
2010-01-23 14:13 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch (5.39 KB, patch)
2010-01-25 02:33 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch (3.22 KB, patch)
2010-01-25 02:34 PST, Sebastian Dröge (slomo)
no flags Details | Formatted Diff | Diff
test for this problem (8.73 KB, patch)
2010-01-26 04:48 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
test for the problem (4.13 KB, patch)
2010-01-28 06:17 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
slomo's patch with my test, as a whole patch (7.57 KB, patch)
2010-01-28 09:22 PST, Gustavo Noronha (kov)
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Dröge (slomo) 2010-01-22 07:37:44 PST
Hi,
the attached patch adds support for passing cookies to the GStreamer HTTP elements. this fixes the new youtube HTML5 stuff for example.
Comment 1 Sebastian Dröge (slomo) 2010-01-22 07:54:20 PST
Created attachment 47203 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch
Comment 2 Sebastian Dröge (slomo) 2010-01-22 07:55:23 PST
Created attachment 47204 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch
Comment 3 WebKit Review Bot 2010-01-22 07:58:09 PST
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.
Comment 4 Gustavo Noronha (kov) 2010-01-22 09:19:46 PST
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).
Comment 5 Marc-Andre Lureau 2010-01-22 09:28:52 PST
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)...
Comment 6 Sebastian Dröge (slomo) 2010-01-22 09:58:13 PST
(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
Comment 7 Dan Winship 2010-01-22 10:03:14 PST
(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.
Comment 8 Gustavo Noronha (kov) 2010-01-22 13:51:15 PST
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.
Comment 9 Marc-Andre Lureau 2010-01-22 14:10:26 PST
(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).
Comment 10 Philippe Normand 2010-01-22 14:24:57 PST
(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 ;)
Comment 11 Sebastian Dröge (slomo) 2010-01-23 01:39:47 PST
(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)
Comment 12 Sebastian Dröge (slomo) 2010-01-23 02:51:50 PST
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 ;)
Comment 13 Sebastian Dröge (slomo) 2010-01-23 13:59:06 PST
Created attachment 47276 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch
Comment 14 Sebastian Dröge (slomo) 2010-01-23 14:00:31 PST
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.
Comment 15 WebKit Review Bot 2010-01-23 14:03:20 PST
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.
Comment 16 Sebastian Dröge (slomo) 2010-01-23 14:11:28 PST
Created attachment 47279 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch
Comment 17 Sebastian Dröge (slomo) 2010-01-23 14:11:57 PST
Created attachment 47280 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch
Comment 18 Sebastian Dröge (slomo) 2010-01-23 14:13:40 PST
Created attachment 47281 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch
Comment 19 Sebastian Dröge (slomo) 2010-01-25 02:33:28 PST
Created attachment 47331 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch
Comment 20 Sebastian Dröge (slomo) 2010-01-25 02:34:01 PST
Created attachment 47332 [details]
0002-Pass-cookies-to-the-GStreamer-HTTP-source.patch
Comment 21 Gustavo Noronha (kov) 2010-01-25 03:55:17 PST
(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 =).
Comment 22 Gustavo Noronha (kov) 2010-01-25 11:07:37 PST
Comment on attachment 47331 [details]
0001-Update-copy-of-the-source-whenever-playbin2-s-source.patch

Looks good to me.
Comment 23 WebKit Commit Bot 2010-01-25 12:03:53 PST
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>
Comment 24 Sebastian Dröge (slomo) 2010-01-25 23:08:41 PST
(In reply to comment #22)
> (From update of attachment 47331 [details])
> Looks good to me.

Thanks but what about the second, actually useful patch? :)
Comment 25 Philippe Normand 2010-01-26 03:54:56 PST
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 ^^ :)
Comment 26 Gustavo Noronha (kov) 2010-01-26 04:21:43 PST
(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.
Comment 27 Gustavo Noronha (kov) 2010-01-26 04:48:37 PST
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.
Comment 28 Gustavo Noronha (kov) 2010-01-28 06:17:06 PST
Created attachment 47612 [details]
test for the problem
Comment 29 Gustavo Noronha (kov) 2010-01-28 09:22:41 PST
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.
Comment 30 Sebastian Dröge (slomo) 2010-01-28 12:58:31 PST
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 ;)
Comment 31 Eric Carlson 2010-01-28 15:08:34 PST
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 @@
> +2010-01-25  Sebastian Dröge  <sebastian.droege@collabora.co.uk>

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


>  2010-01-22  Sebastian Dröge  <sebastian.droege@collabora.co.uk>

  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.
Comment 32 Gustavo Noronha (kov) 2010-01-28 15:50:06 PST
(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.
Comment 33 Gustavo Noronha (kov) 2010-01-28 15:56:58 PST
Landed patch with test as r54026.