Support casting for embedded YT videos

Support casting for embedded YT videos

This is part 1 of the code to start supporting embedded YT for cast. The main
issue is that YT embedded videos need to be identified via the frame that
contains it. This CL sets the frame URL of each video in the MediaPlayerAndroid
object.

Clank downstream piece to come later. 
Min / Aaron: will send you the doc I have that explains this in more detail.

BUG=358573
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277425

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277879

[aberent, 2014-05-28 17:32:30]

https://codereview.chromium.org/302453012/diff/20001/content/common/media/med...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/302453012/diff/20001/content/common/media/med...
content/common/media/media_player_messages_android.h:222: GURL /*
frame_source_url */);
I don't like adding this message just to get round the fact that we don't have
an IPC_MESSAGE_ROUTED6. Would we be better collecting together the data in the
initialize message into a number of structs or classes (e.g. one for player data
and one for the item to be played) and hence incorporating this data in the
initialize message? Alternatively we could create IPC_MESSAGE_ROUTED6.

[May, 2014-06-02 13:20:34]

Friendly ping -- Min, any comments on this? Adam, I know you were OOO most of
the time, PTAL when you can. Thanks!

[qinmin, 2014-06-02 17:19:21]

just some nits, the logic lgtm to me

https://codereview.chromium.org/302453012/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/302453012/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:546: if (player) {
remove {} here

https://codereview.chromium.org/302453012/diff/20001/media/base/android/media...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/302453012/diff/20001/media/base/android/media...
media/base/android/media_player_android.h:47: virtual void SetFrameUrl(const
GURL& frame_url);
no need to be virtual, and for simple setter and getter function, use

void set_frame_url(const GURL& frame_url) {
  frame_url_ = frame_url;
}

https://codereview.chromium.org/302453012/diff/20001/media/base/android/media...
media/base/android/media_player_android.h:78: virtual GURL GetFrameUrl();
GURL frame_url() const {
  return frame_url_;
}

[mark a. foltz, 2014-06-02 19:25:37]

Spoke w/Aaron briefly about this patch and he was wondering why we are plumbing
a frame URL down into the media engine - seems like an odd dependency to take,
assuming that there is a frame with a URL embedding the media object.

Can you please share the doc you mentioned in the description motivating the
change?

[chromium-reviews, 2014-06-02 20:24:00]

Mark, I shared the doc with you (on my N5, not sure if the app sends you
notifications). The dependency is not fully explained in the doc though.

For background: we've implemented YT casting by using the tab URL to
determine if we need to send the video to the generic player or to the YT
Chromecast receiver. This is already in clank starting M34 beta.

To support embedded yt, we needed to change this logic to look at the
immediate frame of the video instead of the tab URL to determine if it's a
YT video or not. This is the motivation for plumbing the frame URL all the
way through the media path.
On Jun 2, 2014 8:25 PM, <mfoltz@chromium.org> wrote:

> Spoke w/Aaron briefly about this patch and he was wondering why we are
> plumbing
> a frame URL down into the media engine - seems like an odd dependency to
> take,
> assuming that there is a frame with a URL embedding the media object.
>
> Can you please share the doc you mentioned in the description motivating
> the
> change?
>
>
> https://codereview.chromium.org/302453012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mark a. foltz, 2014-06-02 21:38:36]

I see.  I am a bit skeptical as a long term solution for this as it relies
on a collaboration between the YT embedding code, the YT MRP, and Chrome in
terms of the URL syntax.  And I am not sure how other sites would take
advantage of this functionality.

Would there be any way to automatically match a MRP with a video src URL or
a bit of markup in the <video> tag?

m.



On Mon, Jun 2, 2014 at 1:23 PM, May Lippert <maybelle@google.com> wrote:

> Mark, I shared the doc with you (on my N5, not sure if the app sends you
> notifications). The dependency is not fully explained in the doc though.
>
> For background: we've implemented YT casting by using the tab URL to
> determine if we need to send the video to the generic player or to the YT
> Chromecast receiver. This is already in clank starting M34 beta.
>
> To support embedded yt, we needed to change this logic to look at the
> immediate frame of the video instead of the tab URL to determine if it's a
> YT video or not. This is the motivation for plumbing the frame URL all the
> way through the media path.
>  On Jun 2, 2014 8:25 PM, <mfoltz@chromium.org> wrote:
>
>> Spoke w/Aaron briefly about this patch and he was wondering why we are
>> plumbing
>> a frame URL down into the media engine - seems like an odd dependency to
>> take,
>> assuming that there is a frame with a URL embedding the media object.
>>
>> Can you please share the doc you mentioned in the description motivating
>> the
>> change?
>>
>>
>> https://codereview.chromium.org/302453012/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[May, 2014-06-03 18:50:12]

It's not a long term solution (never was intended to be), for that we will be
looking at using the presentation API.


On 2014/06/02 21:38:36, mark a. foltz wrote:
> I see.  I am a bit skeptical as a long term solution for this as it relies
> on a collaboration between the YT embedding code, the YT MRP, and Chrome in
> terms of the URL syntax.  And I am not sure how other sites would take
> advantage of this functionality.
> 
> Would there be any way to automatically match a MRP with a video src URL or
> a bit of markup in the <video> tag?
> 
> m.
> 
> 
> 
> On Mon, Jun 2, 2014 at 1:23 PM, May Lippert <mailto:maybelle@google.com>
wrote:
> 
> > Mark, I shared the doc with you (on my N5, not sure if the app sends you
> > notifications). The dependency is not fully explained in the doc though.
> >
> > For background: we've implemented YT casting by using the tab URL to
> > determine if we need to send the video to the generic player or to the YT
> > Chromecast receiver. This is already in clank starting M34 beta.
> >
> > To support embedded yt, we needed to change this logic to look at the
> > immediate frame of the video instead of the tab URL to determine if it's a
> > YT video or not. This is the motivation for plumbing the frame URL all the
> > way through the media path.
> >  On Jun 2, 2014 8:25 PM, <mailto:mfoltz@chromium.org> wrote:
> >
> >> Spoke w/Aaron briefly about this patch and he was wondering why we are
> >> plumbing
> >> a frame URL down into the media engine - seems like an odd dependency to
> >> take,
> >> assuming that there is a frame with a URL embedding the media object.
> >>
> >> Can you please share the doc you mentioned in the description motivating
> >> the
> >> change?
> >>
> >>
> >> https://codereview.chromium.org/302453012/
> >>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[May, 2014-06-04 14:05:08]

Updated to merge all the player parameters into a struct to avoid 2 IPC calls on
initialization.

Miguel -- you're listed as an owner of the tests, I needed to change one of the
util files for the tests

Chris -- PTAL at the IPC changes. I added a parameter for the media OnInitialize
message and merged everything into a single struct.

https://codereview.chromium.org/302453012/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/302453012/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:546: if (player) {
On 2014/06/02 17:19:21, qinmin wrote:
> remove {} here

Done.

https://codereview.chromium.org/302453012/diff/20001/content/common/media/med...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/302453012/diff/20001/content/common/media/med...
content/common/media/media_player_messages_android.h:222: GURL /*
frame_source_url */);
On 2014/05/28 17:32:31, aberent wrote:
> I don't like adding this message just to get round the fact that we don't have
> an IPC_MESSAGE_ROUTED6. Would we be better collecting together the data in the
> initialize message into a number of structs or classes (e.g. one for player
data
> and one for the item to be played) and hence incorporating this data in the
> initialize message? Alternatively we could create IPC_MESSAGE_ROUTED6.

Done.

https://codereview.chromium.org/302453012/diff/20001/media/base/android/media...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/302453012/diff/20001/media/base/android/media...
media/base/android/media_player_android.h:47: virtual void SetFrameUrl(const
GURL& frame_url);
On 2014/06/02 17:19:21, qinmin wrote:
> no need to be virtual, and for simple setter and getter function, use
> 
> void set_frame_url(const GURL& frame_url) {
>   frame_url_ = frame_url;
> }

Done.

https://codereview.chromium.org/302453012/diff/20001/media/base/android/media...
media/base/android/media_player_android.h:78: virtual GURL GetFrameUrl();
On 2014/06/02 17:19:21, qinmin wrote:
> GURL frame_url() const {
>   return frame_url_;
> }

Done.

[Miguel Garcia, 2014-06-04 15:22:46]

lgtm for tests

[acolwell GONE FROM CHROMIUM, 2014-06-04 15:43:39]

not lgtm. Please work with YT to get the info you need placed in the actual URLs
passed to the media element. You should not be relying on the frame URL or tab
URL to accomplish what you are trying to do.

[acolwell GONE FROM CHROMIUM, 2014-06-09 20:02:09]

Based on offline conversations, I'm willing to let this proceed to satisfy M37
commitments.

Just have a few comments.

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:531: type,
media_player_params.player_id, media_player_params.url,
nit: Looks like you could just pass media_player_params in here. Any reason not
to?

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:540:
player->set_frame_url(media_player_params.frame_url);
nit: Any reason not to make this part of the player's constructor? It seems like
this should be handled in CreateMediaPlayer() like all the other params.

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:126:
MediaPlayerHostMsg_Initialize_Type type,
nit: Any reason why this can't go in the params struct as well?

https://codereview.chromium.org/302453012/diff/40001/content/common/media/med...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/302453012/diff/40001/content/common/media/med...
content/common/media/media_player_messages_android.h:226: IPC_MESSAGE_ROUTED2(
Is this still needed?

https://codereview.chromium.org/302453012/diff/40001/media/base/android/media...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/302453012/diff/40001/media/base/android/media...
media/base/android/media_player_android.h:65: frame_url_ = frame_url;
nit: ISTM this value should be passed in via the constructor.

[May, 2014-06-10 18:22:20]

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:531: type,
media_player_params.player_id, media_player_params.url,
On 2014/06/09 20:02:09, acolwell wrote:
> nit: Looks like you could just pass media_player_params in here. Any reason
not
> to?

Done.

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:540:
player->set_frame_url(media_player_params.frame_url);
On 2014/06/09 20:02:09, acolwell wrote:
> nit: Any reason not to make this part of the player's constructor? It seems
like
> this should be handled in CreateMediaPlayer() like all the other params.

Done.

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/302453012/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:126:
MediaPlayerHostMsg_Initialize_Type type,
On 2014/06/09 20:02:09, acolwell wrote:
> nit: Any reason why this can't go in the params struct as well?

No real reason, just preference. merged it in the struct.

https://codereview.chromium.org/302453012/diff/40001/content/common/media/med...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/302453012/diff/40001/content/common/media/med...
content/common/media/media_player_messages_android.h:226: IPC_MESSAGE_ROUTED2(
On 2014/06/09 20:02:09, acolwell wrote:
> Is this still needed?
Removed. Good catch.

https://codereview.chromium.org/302453012/diff/40001/media/base/android/media...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/302453012/diff/40001/media/base/android/media...
media/base/android/media_player_android.h:65: frame_url_ = frame_url;
On 2014/06/09 20:02:09, acolwell wrote:
> nit: ISTM this value should be passed in via the constructor.

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-10 19:41:47]

lgtm

[May, 2014-06-16 14:00:58]

The CQ bit was checked by maybelle@chromium.org

[commit-bot: I haz the power, 2014-06-16 14:02:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maybelle@chromium.org/302453012/80001

[commit-bot: I haz the power, 2014-06-16 14:23:12]

Change committed as 277425

[binjin, 2014-06-16 14:49:34]

A revert of this CL has been created in
https://codereview.chromium.org/339613002/ by binjin@chromium.org.

The reason for reverting is: It appears to break compile on android:

../../media/base/android/media_source_player_unittest.cc: In constructor
'media::MediaSourcePlayerTest::MediaSourcePlayerTest()':
../../media/base/android/media_source_player_unittest.cc:172:40:error: no
matching function for call to 'media::MediaSourcePlayer::MediaSourcePlayer(int,
media::MockMediaPlayerManager*, base::Callback<void(int)>,
base::Callback<void(int)>, scoped_ptr<media::DemuxerAndroid>)'
         surface_texture_a_is_next_(true) {}
                                        ^
../../media/base/android/media_source_player_unittest.cc:172:40: note:
candidates are:
In file included from ../../base/basictypes.h:18:0,
                 from
../../media/base/android/media_source_player_unittest.cc:7:
../../media/base/android/media_source_player.h:269:28: note:
media::MediaSourcePlayer::MediaSourcePlayer(const media::MediaSourcePlayer&)
   DISALLOW_COPY_AND_ASSIGN(MediaSourcePlayer);
                            ^
../../base/macros.h:29:3: note: in definition of macro
'DISALLOW_COPY_AND_ASSIGN'
   TypeName(const TypeName&);               \
   ^
../../media/base/android/media_source_player.h:269:28: note:   candidate expects
1 argument, 5 provided
   DISALLOW_COPY_AND_ASSIGN(MediaSourcePlayer);
                            ^
../../base/macros.h:29:3: note: in definition of macro
'DISALLOW_COPY_AND_ASSIGN'
   TypeName(const TypeName&);               \
   ^
In file included from
../../media/base/android/media_source_player_unittest.cc:15:0:
../../media/base/android/media_source_player.h:41:3: note:
media::MediaSourcePlayer::MediaSourcePlayer(int, media::MediaPlayerManager*,
const RequestMediaResourcesCB&, const ReleaseMediaResourcesCB&,
scoped_ptr<media::DemuxerAndroid>, const GURL&)
   MediaSourcePlayer(int player_id,
   ^
../../media/base/android/media_source_player.h:41:3: note:   candidate expects 6
arguments, 5 provided
ninja: build stopped: subcommand failed.
.

[jam, 2014-06-16 14:52:17]

reverting since this broke the build

please don't use NOTRY=true again. changes must have green try runs before
landing.

[May, 2014-06-17 14:27:36]

The CQ bit was checked by maybelle@chromium.org

[commit-bot: I haz the power, 2014-06-17 14:28:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maybelle@chromium.org/302453012/120001

[May, 2014-06-17 16:06:57]

+palmer

Added wrong cpalmer for review a while back.

Chris - main change here is that I combined all parameters for
MediaPlayerMsg_OnInitialize into a struct and added a new field (frame_url)

[commit-bot: I haz the power, 2014-06-17 17:10:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:11:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[palmer, 2014-06-17 17:35:00]

IPC security LGTM

[Miguel Garcia, 2014-06-17 22:01:13]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-06-17 22:03:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maybelle@chromium.org/302453012/120001

[commit-bot: I haz the power, 2014-06-17 22:12:08]

Change committed as 277879