Bug 89514 – Add url to supportsType

• Component: Media
• Status: RESOLVED FIXED

[Danilo de Paula]

Add url to supportsType

[Danilo de Paula]

Created attachment 148423 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/MediaPlayer.h:206:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/HTMLMediaElement.h:131:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 21 files


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

[Danilo de Paula]

Created attachment 148427 [details]
Patch

[Eric Carlson]

Comment on attachment 148427 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        In the case of two media players where one is used for MediaStream, it needs to know the URL to check if it's
> +        a MediaStream generated blob, to return if it's supported or not.

I don't understand this. Please elaborate.

> Source/WebCore/ChangeLog:13
> +        No new tests since there's no change on code behavior.

If there is no change in behavior, why is the code change necessary?

[Philippe Normand]

This is related with bug 87514 [GStreamer] Adding Farstream backend to WebRTC implementation

[WebKit Review Bot]

Comment on attachment 148427 [details]
Patch

Attachment 148427 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13002018

[Danilo de Paula]

Created attachment 148543 [details]
Patch

[Danilo de Paula]

(In reply to comment #4)
> (From update of attachment 148427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148427&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        In the case of two media players where one is used for MediaStream, it needs to know the URL to check if it's
> > +        a MediaStream generated blob, to return if it's supported or not.
> 
> I don't understand this. Please elaborate.

When the user calls webkitGetUserMedia it will create a blob specific for that stream.

On GTK we've created an StreamMediaPlayer to be used for the propose of playing rtp streams created by the mediastream call, however we're having issues on how to identify if we should use this player or the gstreamer one, since the MediaPlayerPrivateInterface::supportsType(type,codec) doesn't have access to the media's url.

With this patch the new player will be able to identify if the media is a mediastream blob created by us (using MediaStreamRegistry::registry().lookupMediaStreamDescriptor(url)), and then return MediaPlayer::IsSupported;

While the players that don't implement MediaStream or those using the same player could just ignore that.

> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests since there's no change on code behavior.
> 
> If there is no change in behavior, why is the code change necessary?

Since basically the current ports will just ignore the parameter, I initially thought that no tests would be necessary. But since it modifies the MediaPlayer API I'll check which tests need to be changed.

[Philippe Normand]

Comment on attachment 148543 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        adding url to MediaPlayer::supportsType
> +
> +        In the case of two media players where one is used for MediaStream, it needs to know the URL to check if it's
> +        a MediaStream generated blob, to return if it's supported or not.
> +
> +        Add url to supportsType
> +        https://bugs.webkit.org/show_bug.cgi?id=89514
> +
> +        Reviewed by NOBODY (OOPS!).

The format is wrong. Usually it's

Bug title
link

Reviewed by ...

Explanation of the change.

[Danilo de Paula]

Created attachment 148780 [details]
Patch

[Danilo de Paula]

Eric: if I've changed only the privateinterfaces API and its usage, I believe the current media tests will cover the modification, since the current players won't use the new parameter.

The new parameter will be use when [1] is accepted

Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp
[1] http://pastebin.com/vHZA2K9g

[Eric Carlson]

Comment on attachment 148780 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        When a blob is created as the address for a Media Stream, the MediaEngine
> +        will ask it's players if they support that media. However, a player built
> +        for MediaStream needs to know to URL to decide if it's or it's not supported.

Nit: "if it's or it's not supported" is slightly awkward, I suggest something like "if it is supported or not"

> Source/WebCore/platform/graphics/MediaPlayer.h:426
> +typedef MediaPlayer::SupportsType (*MediaEngineSupportsType)(const String& type, const String& codecs, const String& keySystem, const String& url);

Why not pass the url as a KURL rather than as a string?

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731
> +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationCF::supportsType(const String& type, const String& codecs, const String& url)
>  {
> +    UNUSED_PARAM(url);
> +

The "UNUSED_PARAM()" will be unnecessary if you leave out the parameter name, e.g.. const String& codecs, const String&)

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:678
> +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationObjC::supportsType(const String& type, const String& codecs, const String& url)
>  {
> +    UNUSED_PARAM(url);
> +

Ditto here and throughout the rest of the patch.

[Danilo de Paula]

Created attachment 149005 [details]
Patch

[Danilo de Paula]

(In reply to comment #12)
> (From update of attachment 148780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148780&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        When a blob is created as the address for a Media Stream, the MediaEngine
> > +        will ask it's players if they support that media. However, a player built
> > +        for MediaStream needs to know to URL to decide if it's or it's not supported.
> 
> Nit: "if it's or it's not supported" is slightly awkward, I suggest something like "if it is supported or not"

Agreed 

> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:426
> > +typedef MediaPlayer::SupportsType (*MediaEngineSupportsType)(const String& type, const String& codecs, const String& keySystem, const String& url);
> 
> Why not pass the url as a KURL rather than as a string?

I was using String because it was enough and all the other parameters were Strings. But I agree what a KURL is semantic better and might be needed/wanted in the future. Fixed

> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731
> > +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationCF::supportsType(const String& type, const String& codecs, const String& url)
> >  {
> > +    UNUSED_PARAM(url);
> > +
> 
> The "UNUSED_PARAM()" will be unnecessary if you leave out the parameter name, e.g.. const String& codecs, const String&)

Right. Fixed for all code.

[Danilo de Paula]

Created attachment 149008 [details]
Patch

[Danilo de Paula]

In that case I had to change m_url to be a KURL.
I could construct a KURL for the supportsType calls, but changing m_url to KURL make more sense.

[WebKit Review Bot]

Comment on attachment 149008 [details]
Patch

Attachment 149008 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13072037

[Danilo de Paula]

Created attachment 149037 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149037 [details]
Patch

Attachment 149037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13038418

[Danilo de Paula]

Created attachment 149044 [details]
Patch

[Eric Carlson]

Comment on attachment 149044 [details]
Patch

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

This looks great modulo the two minor issues noted. Thanks!

> Source/WebCore/ChangeLog:15
> +        for MediaStream needs to know to URL to decide if it's supported or not.
> +
> +
> +        * dom/DOMImplementation.cpp:

Minor nit: extra blank line here.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731
> +    UNUSED_PARAM(url);
> +

Oops, it looks like you forgot to change this one.

[Danilo de Paula]

Created attachment 149051 [details]
Patch

[Danilo de Paula]

(In reply to comment #21)
> (From update of attachment 149044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149044&action=review
> 
> This looks great modulo the two minor issues noted. Thanks!
> 
> > Source/WebCore/ChangeLog:15
> > +        for MediaStream needs to know to URL to decide if it's supported or not.
> > +
> > +
> > +        * dom/DOMImplementation.cpp:
> 
> Minor nit: extra blank line here.
> 
> > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731
> > +    UNUSED_PARAM(url);
> > +
> 
> Oops, it looks like you forgot to change this one.

Thanks for the review! Can you add it to the queue?

[Eric Carlson]

Comment on attachment 149051 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:730
> +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationCF::supportsType(const String& type, const String& codecs, const KURL& url)
>  {
>      // Only return "IsSupported" if there is no codecs parameter for now as there is no way to ask if it supports an

This will fail to compile: "const KURL& url" -> "const KURL&"

[Danilo de Paula]

Created attachment 149056 [details]
Patch

[Eric Carlson]

Comment on attachment 149056 [details]
Patch

@Danilo, I am adding this to the commit queue because you asked in a comment, but in the future you can just set the cq? flag to signal to the reviewer that you would like the patch to be added to the commit queue when it gets an r+.

Thanks!

[WebKit Review Bot]

Comment on attachment 149056 [details]
Patch

Clearing flags on attachment: 149056

Committed r121053: <http://trac.webkit.org/changeset/121053>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.