Bug 89514 - Add url to supportsType
Summary: Add url to supportsType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 87514
  Show dependency treegraph
 
Reported: 2012-06-19 14:24 PDT by Danilo Cesar Lemes de Paula
Modified: 2012-06-22 13:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (28.75 KB, patch)
2012-06-19 14:25 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (28.74 KB, patch)
2012-06-19 14:32 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (31.20 KB, patch)
2012-06-20 05:53 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (31.21 KB, patch)
2012-06-21 06:25 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (30.52 KB, patch)
2012-06-22 05:43 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (32.01 KB, patch)
2012-06-22 06:02 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (32.05 KB, patch)
2012-06-22 09:12 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (32.03 KB, patch)
2012-06-22 09:46 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (31.96 KB, patch)
2012-06-22 10:24 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff
Patch (31.96 KB, patch)
2012-06-22 10:43 PDT, Danilo Cesar Lemes de Paula
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Danilo Cesar Lemes de Paula 2012-06-19 14:24:17 PDT
Add url to supportsType
Comment 1 Danilo Cesar Lemes de Paula 2012-06-19 14:25:48 PDT
Created attachment 148423 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-19 14:28:48 PDT
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.
Comment 3 Danilo Cesar Lemes de Paula 2012-06-19 14:32:29 PDT
Created attachment 148427 [details]
Patch
Comment 4 Eric Carlson 2012-06-19 14:41:05 PDT
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?
Comment 5 Philippe Normand 2012-06-19 15:59:41 PDT
This is related with bug 87514 [GStreamer] Adding Farstream backend to WebRTC implementation
Comment 6 WebKit Review Bot 2012-06-19 16:11:20 PDT
Comment on attachment 148427 [details]
Patch

Attachment 148427 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13002018
Comment 7 Danilo Cesar Lemes de Paula 2012-06-20 05:53:47 PDT
Created attachment 148543 [details]
Patch
Comment 8 Danilo Cesar Lemes de Paula 2012-06-20 06:13:08 PDT
(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.
Comment 9 Philippe Normand 2012-06-20 12:52:24 PDT
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.
Comment 10 Danilo Cesar Lemes de Paula 2012-06-21 06:25:42 PDT
Created attachment 148780 [details]
Patch
Comment 11 Danilo Cesar Lemes de Paula 2012-06-21 08:34:56 PDT
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
Comment 12 Eric Carlson 2012-06-21 10:07:56 PDT
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.
Comment 13 Danilo Cesar Lemes de Paula 2012-06-22 05:43:54 PDT
Created attachment 149005 [details]
Patch
Comment 14 Danilo Cesar Lemes de Paula 2012-06-22 05:47:43 PDT
(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.
Comment 15 Danilo Cesar Lemes de Paula 2012-06-22 06:02:25 PDT
Created attachment 149008 [details]
Patch
Comment 16 Danilo Cesar Lemes de Paula 2012-06-22 06:04:36 PDT
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.
Comment 17 WebKit Review Bot 2012-06-22 06:50:05 PDT
Comment on attachment 149008 [details]
Patch

Attachment 149008 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13072037
Comment 18 Danilo Cesar Lemes de Paula 2012-06-22 09:12:08 PDT
Created attachment 149037 [details]
Patch
Comment 19 WebKit Review Bot 2012-06-22 09:34:10 PDT
Comment on attachment 149037 [details]
Patch

Attachment 149037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13038418
Comment 20 Danilo Cesar Lemes de Paula 2012-06-22 09:46:07 PDT
Created attachment 149044 [details]
Patch
Comment 21 Eric Carlson 2012-06-22 10:10:55 PDT
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.
Comment 22 Danilo Cesar Lemes de Paula 2012-06-22 10:24:04 PDT
Created attachment 149051 [details]
Patch
Comment 23 Danilo Cesar Lemes de Paula 2012-06-22 10:25:50 PDT
(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?
Comment 24 Eric Carlson 2012-06-22 10:32:17 PDT
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&"
Comment 25 Danilo Cesar Lemes de Paula 2012-06-22 10:43:17 PDT
Created attachment 149056 [details]
Patch
Comment 26 Eric Carlson 2012-06-22 13:03:28 PDT
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!
Comment 27 WebKit Review Bot 2012-06-22 13:28:43 PDT
Comment on attachment 149056 [details]
Patch

Clearing flags on attachment: 149056

Committed r121053: <http://trac.webkit.org/changeset/121053>
Comment 28 WebKit Review Bot 2012-06-22 13:28:51 PDT
All reviewed patches have been landed.  Closing bug.