Bug 113884 – [GStreamer] playbin uri getter is invalid

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

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.

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 196380 [details]
Patch

[Martin Robinson]

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.

[Philippe Normand]

Hum, no ::load() is called only once per MediaPlayerPrivate instance :)

[Martin Robinson]

(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?

[Philippe Normand]

(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!

[Philippe Normand]

Created attachment 196382 [details]
Patch

[Martin Robinson]

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.

[Martin Robinson]

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. :)

[Philippe Normand]

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 :)

[Philippe Normand]

Comment on attachment 196382 [details]
Patch

I will provide a test

[Philippe Normand]

Created attachment 197022 [details]
Patch

[Philippe Normand]

Created attachment 197066 [details]
Patch

[Martin Robinson]

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?

[Philippe Normand]

Committed r148079: <http://trac.webkit.org/changeset/148079>