Bug 113884 - [GStreamer] playbin uri getter is invalid
Summary: [GStreamer] playbin uri getter is invalid
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-03 09:49 PDT by Philippe Normand
Modified: 2013-04-09 23:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.64 KB, patch)
2013-04-03 11:17 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (3.05 KB, patch)
2013-04-03 11:48 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2013-04-09 04:05 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2013-04-09 06:17 PDT, Philippe Normand
mrobinson: review+
mrobinson: commit-queue-
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-04-03 09:49:13 PDT
The uri property getter in playbin > 0.10.36 is now done by accessing the current-uri property. While setting the uri is still done via the ... uri property :) It'd be nice to have convenience methods for this in the player.
Comment 1 Philippe Normand 2013-04-03 10:14:52 PDT
Well, we store url in a member variable. let's just use it instead of trying to get the gobject property. That way no ifdef needed.
Comment 2 Philippe Normand 2013-04-03 11:17:16 PDT
Created attachment 196380 [details]
Patch
Comment 3 Martin Robinson 2013-04-03 11:30:55 PDT
Comment on attachment 196380 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1299
> -        RefPtr<SecurityOrigin> securityOrigin = SecurityOrigin::create(currentUrl);
> +        RefPtr<SecurityOrigin> securityOrigin = SecurityOrigin::create(m_url);
>          if (securityOrigin->canRequest(newUrl)) {
>              LOG_MEDIA_MESSAGE("New media url: %s", newUrl.string().utf8().data());
>  

Does this eventually call ::load(...) again? If not, I can see this being an issue if there is a chain of redirects like: http://hosta/file --> http://hostb/file --> file2.
Comment 4 Philippe Normand 2013-04-03 11:36:06 PDT
Hum, no ::load() is called only once per MediaPlayerPrivate instance :)
Comment 5 Martin Robinson 2013-04-03 11:39:33 PDT
(In reply to comment #4)
> Hum, no ::load() is called only once per MediaPlayerPrivate instance :)

Perhaps you can simply update m_url for each redirect then?
Comment 6 Philippe Normand 2013-04-03 11:46:50 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Hum, no ::load() is called only once per MediaPlayerPrivate instance :)
> 
> Perhaps you can simply update m_url for each redirect then?

Ah right that's a thing I forgot to do in this patch. Good catch!
Comment 7 Philippe Normand 2013-04-03 11:48:35 PDT
Created attachment 196382 [details]
Patch
Comment 8 Martin Robinson 2013-04-03 11:59:31 PDT
Comment on attachment 196382 [details]
Patch

Would be nice to have a test, but perhaps it is difficult to simulate this kind of thing without a quicktime server.
Comment 9 Martin Robinson 2013-04-03 12:01:14 PDT
Comment on attachment 196382 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1294
> +            newUrl = KURL(KURL(), m_url.baseAsString() + newLocation);

Ah! Just noticed this:

You should consider trying newUrl = KURL(m_url, newLocation);

The first argument is a base. :)
Comment 10 Philippe Normand 2013-04-03 12:07:45 PDT
It should be possible to have an http/tests/media test for this I think. I only need to find how to craft a .mov file embedding a redirect url :)
Comment 11 Philippe Normand 2013-04-03 12:57:18 PDT
Comment on attachment 196382 [details]
Patch

I will provide a test
Comment 12 Philippe Normand 2013-04-09 04:05:50 PDT
Created attachment 197022 [details]
Patch
Comment 13 Philippe Normand 2013-04-09 06:17:53 PDT
Created attachment 197066 [details]
Patch
Comment 14 Martin Robinson 2013-04-09 07:05:23 PDT
Comment on attachment 197066 [details]
Patch

Okay. Looks good. My only nit is that reftest.mov does not have a very descriptive name. Do you think you can rename it to something like redirect-to-counting-captioned.mov?
Comment 15 Philippe Normand 2013-04-09 23:59:11 PDT
Committed r148079: <http://trac.webkit.org/changeset/148079>