Propagate media session id into MediaPlayerAndroid

Propagate media session id into MediaPlayerAndroid

The media session id will be used to implicitly activate a user defined media session.

BUG=497735, 581728
Committed: https://crrev.com/541ee5c14eb321ab188cce3d077649cc216404b8
Cr-Commit-Position: refs/heads/master@{#380632}

[davve, 2016-02-25 09:53:42]

Description was changed from

==========
Propagate media session id into MediaPlayerAndroid
==========

to

==========
Propagate media session id into MediaPlayerAndroid

The media session id will be used to implicitly activate a user defined media
session.

BUG=497735, 581728
==========

[davve, 2016-02-25 09:57:31]

davve@opera.com changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org, philipj@opera.com

[davve, 2016-02-25 09:57:32]

One more of the simple ones. PTAL.

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:47: player_id,
Is this reformat ok? Made by git cl format.

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player_unittest.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
media/base/android/media_codec_player_unittest.cc:647:
scoped_ptr<MockDemuxerAndroid>(demuxer_), GURL(), 0);
I have the constant problem here as well (as in the previous CL). Not what the
best place for the constant is. Including blink headers from doesn't seem right,
or?

[mlamouri (slow - plz ping), 2016-02-25 14:13:55]

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
manager->GetLocalPlayer(player_id)->media_session_id()),
What does that mean for a remote player to have the same session id than the
local player?

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player_unittest.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
media/base/android/media_codec_player_unittest.cc:647:
scoped_ptr<MockDemuxerAndroid>(demuxer_), GURL(), 0);
On 2016/02/25 at 09:57:32, David Vest wrote:
> I have the constant problem here as well (as in the previous CL). Not what the
best place for the constant is. Including blink headers from doesn't seem right,
or?

Hmm, maybe you should have version of the constant in media/base/ and one in
content/? I'm afraid media/base/ will not be able to access content/ nor blink
but I think we need a constant instead of a plain 0 here.

(We could even add a build time assert to check that the media/ and content/
constants are the same.)

[philipj_slow, 2016-02-29 09:50:37]

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
manager->GetLocalPlayer(player_id)->media_session_id()),
On 2016/02/25 14:13:54, Mounir Lamouri wrote:
> What does that mean for a remote player to have the same session id than the
> local player?

Looks like a problem brewing here, in the interaction between media session and
remote playback. It makes good sense to allow both local and remote playback at
the same time, and it would be silly if two media elements have to belong to the
same session for this to work.

Right now we have the invariant that a WebMediaPlayer belongs to a single
session for its whole lifetime, but the possibility for playback to go remote at
any time makes that invariant kind of useless.

Has anyone thought more about the implications of this? One would presumably
also want some kind of continuity in the notification.

[whywhat, 2016-02-29 16:26:29]

lgtm

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:47: player_id,
On 2016/02/25 at 09:57:32, David Vest wrote:
> Is this reformat ok? Made by git cl format.

Yes. Might cause merge problems but is not a big deal :)

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
manager->GetLocalPlayer(player_id)->media_session_id()),
On 2016/02/25 at 14:13:54, Mounir Lamouri wrote:
> What does that mean for a remote player to have the same session id than the
local player?

AFAIK, the remote player won't use the media session id at all so it's there
just to satisfy MPA constructor. Perhaps, we could pass an invalid id there?

philipj@ I think the remotely played elements belong to a special remote session
that is defined by the platform / protocol used. E.g. on Android, only one
connection can be made to a remote device at a time so each remotely played
video behaves like it is in its own session (starting one interrupts the other).
On desktop multiple connections can be made to multiple devices, however, only
one element (e.g. last) will get the focus  (e.g. MediaRouter dialog will only
show one connection with metadata and media controls; user has to click on
another connection to switch focus and control it).

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player_unittest.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
media/base/android/media_codec_player_unittest.cc:647:
scoped_ptr<MockDemuxerAndroid>(demuxer_), GURL(), 0);
On 2016/02/25 at 14:13:55, Mounir Lamouri wrote:
> On 2016/02/25 at 09:57:32, David Vest wrote:
> > I have the constant problem here as well (as in the previous CL). Not what
the best place for the constant is. Including blink headers from doesn't seem
right, or?
> 
> Hmm, maybe you should have version of the constant in media/base/ and one in
content/? I'm afraid media/base/ will not be able to access content/ nor blink
but I think we need a constant instead of a plain 0 here.
> 
> (We could even add a build time assert to check that the media/ and content/
constants are the same.)

What is 0? The default session? Is it used anywhere but tests?

[davve, 2016-03-01 05:42:22]

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
manager->GetLocalPlayer(player_id)->media_session_id()),
On 2016/02/29 16:26:29, whywhat wrote:
> On 2016/02/25 at 14:13:54, Mounir Lamouri wrote:
> > What does that mean for a remote player to have the same session id than the
> local player?
> 
> AFAIK, the remote player won't use the media session id at all so it's there
> just to satisfy MPA constructor. Perhaps, we could pass an invalid id there?

Right now there is no defined invalid id, but we could define one to -1 or so.
Perhaps as a enum friend to the default id.

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
File media/base/android/media_codec_player_unittest.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/media/base/android/medi...
media/base/android/media_codec_player_unittest.cc:647:
scoped_ptr<MockDemuxerAndroid>(demuxer_), GURL(), 0);
On 2016/02/29 16:26:29, whywhat wrote:
> On 2016/02/25 at 14:13:55, Mounir Lamouri wrote:
> > On 2016/02/25 at 09:57:32, David Vest wrote:
> > > I have the constant problem here as well (as in the previous CL). Not what
> the best place for the constant is. Including blink headers from doesn't seem
> right, or?
> > 
> > Hmm, maybe you should have version of the constant in media/base/ and one in
> content/? I'm afraid media/base/ will not be able to access content/ nor blink
> but I think we need a constant instead of a plain 0 here.
> > 
> > (We could even add a build time assert to check that the media/ and content/
> constants are the same.)
> 
> What is 0? The default session? Is it used anywhere but tests?

Yes, the default session. Not used much yet, but I think we eventually will need
a notion of the default media session in media/ and content/browser/

[philipj_slow, 2016-03-03 11:10:52]

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
manager->GetLocalPlayer(player_id)->media_session_id()),
On 2016/02/29 16:26:29, whywhat wrote:
> On 2016/02/25 at 14:13:54, Mounir Lamouri wrote:
> > What does that mean for a remote player to have the same session id than the
> local player?
> 
> AFAIK, the remote player won't use the media session id at all so it's there
> just to satisfy MPA constructor. Perhaps, we could pass an invalid id there?
> 
> philipj@ I think the remotely played elements belong to a special remote
session
> that is defined by the platform / protocol used. E.g. on Android, only one
> connection can be made to a remote device at a time so each remotely played
> video behaves like it is in its own session (starting one interrupts the
other).
> On desktop multiple connections can be made to multiple devices, however, only
> one element (e.g. last) will get the focus  (e.g. MediaRouter dialog will only
> show one connection with metadata and media controls; user has to click on
> another connection to switch focus and control it).

OK, so if remote playback can be modeled as one or multiple other session,
that's good. It effectively means that a media element can be transferred
between sessions, something that isn't possible with the web-exposed API so far.
If the remote session is exposed to the web, and it has to be if one is to
customize the metadata for it, then it also means that there's a new category of
sessions which can be active at the same time as kind=content, or *some* other
change to explain the behavior.

What do you all think about the metadata problem? If a media element can jump
between sessions like this, it begins to make less sense to tie the metadata to
the session. Do we need to revisit this, tying metadata to the HTMLMediaElement
and AudioContext instances instead? Or is there another way to make sense of the
situation?

[davve, 2016-03-08 07:57:59]

On 2016/03/03 11:10:52, philipj_UTC7 wrote:
>
https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
> File chrome/browser/media/android/remote/remote_media_player_bridge.cc
(right):
> 
>
https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
> chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
> manager->GetLocalPlayer(player_id)->media_session_id()),
> On 2016/02/29 16:26:29, whywhat wrote:
> > On 2016/02/25 at 14:13:54, Mounir Lamouri wrote:
> > > What does that mean for a remote player to have the same session id than
the
> > local player?
> > 
> > AFAIK, the remote player won't use the media session id at all so it's there
> > just to satisfy MPA constructor. Perhaps, we could pass an invalid id there?
> > 
> > philipj@ I think the remotely played elements belong to a special remote
> session
> > that is defined by the platform / protocol used. E.g. on Android, only one
> > connection can be made to a remote device at a time so each remotely played
> > video behaves like it is in its own session (starting one interrupts the
> other).
> > On desktop multiple connections can be made to multiple devices, however,
only
> > one element (e.g. last) will get the focus  (e.g. MediaRouter dialog will
only
> > show one connection with metadata and media controls; user has to click on
> > another connection to switch focus and control it).
> 
> OK, so if remote playback can be modeled as one or multiple other session,
> that's good. It effectively means that a media element can be transferred
> between sessions, something that isn't possible with the web-exposed API so
far.
> If the remote session is exposed to the web, and it has to be if one is to
> customize the metadata for it, then it also means that there's a new category
of
> sessions which can be active at the same time as kind=content, or *some* other
> change to explain the behavior.
> 
> What do you all think about the metadata problem? If a media element can jump
> between sessions like this, it begins to make less sense to tie the metadata
to
> the session. Do we need to revisit this, tying metadata to the
HTMLMediaElement
> and AudioContext instances instead? Or is there another way to make sense of
the
> situation?

I feel this is getting a bit out-of-scope for this review. Is there an
spec-issue where we could continue this discussion? Perhaps also file a bug if
we want this issue to block shipping or so.

In the mean time, if "the remote player won't use the media session id at all"
does it make sense to pass 0 (for default) for the remote bridge and let the
protocol sort it out for now? (and possibly be broken in the sense that you
can't control the session.)

[philipj_slow, 2016-03-08 16:10:47]

On 2016/03/08 07:57:59, David Vest wrote:
> On 2016/03/03 11:10:52, philipj_UTC7 wrote:
> >
>
https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
> > File chrome/browser/media/android/remote/remote_media_player_bridge.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1640123004/diff/20001/chrome/browser/media/an...
> > chrome/browser/media/android/remote/remote_media_player_bridge.cc:51:
> > manager->GetLocalPlayer(player_id)->media_session_id()),
> > On 2016/02/29 16:26:29, whywhat wrote:
> > > On 2016/02/25 at 14:13:54, Mounir Lamouri wrote:
> > > > What does that mean for a remote player to have the same session id than
> the
> > > local player?
> > > 
> > > AFAIK, the remote player won't use the media session id at all so it's
there
> > > just to satisfy MPA constructor. Perhaps, we could pass an invalid id
there?
> > > 
> > > philipj@ I think the remotely played elements belong to a special remote
> > session
> > > that is defined by the platform / protocol used. E.g. on Android, only one
> > > connection can be made to a remote device at a time so each remotely
played
> > > video behaves like it is in its own session (starting one interrupts the
> > other).
> > > On desktop multiple connections can be made to multiple devices, however,
> only
> > > one element (e.g. last) will get the focus  (e.g. MediaRouter dialog will
> only
> > > show one connection with metadata and media controls; user has to click on
> > > another connection to switch focus and control it).
> > 
> > OK, so if remote playback can be modeled as one or multiple other session,
> > that's good. It effectively means that a media element can be transferred
> > between sessions, something that isn't possible with the web-exposed API so
> far.
> > If the remote session is exposed to the web, and it has to be if one is to
> > customize the metadata for it, then it also means that there's a new
category
> of
> > sessions which can be active at the same time as kind=content, or *some*
other
> > change to explain the behavior.
> > 
> > What do you all think about the metadata problem? If a media element can
jump
> > between sessions like this, it begins to make less sense to tie the metadata
> to
> > the session. Do we need to revisit this, tying metadata to the
> HTMLMediaElement
> > and AudioContext instances instead? Or is there another way to make sense of
> the
> > situation?
> 
> I feel this is getting a bit out-of-scope for this review. Is there an
> spec-issue where we could continue this discussion? Perhaps also file a bug if
> we want this issue to block shipping or so.
> 
> In the mean time, if "the remote player won't use the media session id at all"
> does it make sense to pass 0 (for default) for the remote bridge and let the
> protocol sort it out for now? (and possibly be broken in the sense that you
> can't control the session.)

I've filed https://github.com/whatwg/mediasession/issues/123

Any behavior that doesn't crash together with a TODO seems fine for now.

[philipj_slow, 2016-03-08 16:34:17]

third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h lgtm

https://codereview.chromium.org/1640123004/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h
(right):

https://codereview.chromium.org/1640123004/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:21: //
The media session for media that don't have an explicit
"media that doesn't" or "media elements that don't"

[davve, 2016-03-09 08:14:21]

davve@opera.com changed reviewers:
+ qinmin@chromium.org

[davve, 2016-03-09 08:14:21]

qinmin, could you please take a look at content/browser/media/android and
media/base/android/?

[qinmin, 2016-03-09 18:20:13]

lgtm

[whywhat, 2016-03-10 22:34:56]

https://codereview.chromium.org/1640123004/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:54:
media::kDefaultMediaSessionID),
What does using the default session mean in practice? Would it pause other tabs
and add it to the current tab's session so that all players would be controlled
altogether?
I think to play it safe, the remote element should be part of no session at all
(e.g. kInvalidMediaSessionId), until we figure it out.

https://codereview.chromium.org/1640123004/diff/100001/media/base/android/med...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/1640123004/diff/100001/media/base/android/med...
media/base/android/media_player_android.h:29: enum { kDefaultMediaSessionID = 0
};
nit: I believe Id is preferred to ID but I don't feel very strongly, the
codebase seems to prefer Id over ID but there's not that many instances:

https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20file:%5C.h...

https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20file:%5C.h...

[davve, 2016-03-11 09:53:27]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 09:53:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640123004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640123004/140001

[commit-bot: I haz the power, 2016-03-11 10:08:46]

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

[commit-bot: I haz the power, 2016-03-11 10:08:47]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[davve, 2016-03-11 12:34:08]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 12:34:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640123004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640123004/160001

[davve, 2016-03-11 13:20:30]

https://codereview.chromium.org/1640123004/diff/100001/chrome/browser/media/a...
File chrome/browser/media/android/remote/remote_media_player_bridge.cc (right):

https://codereview.chromium.org/1640123004/diff/100001/chrome/browser/media/a...
chrome/browser/media/android/remote/remote_media_player_bridge.cc:54:
media::kDefaultMediaSessionID),
On 2016/03/10 22:34:56, whywhat wrote:
> What does using the default session mean in practice? Would it pause other
tabs
> and add it to the current tab's session so that all players would be
controlled
> altogether?
> I think to play it safe, the remote element should be part of no session at
all
> (e.g. kInvalidMediaSessionId), until we figure it out.

Ok, kInvalidMediaSessionId it is.

https://codereview.chromium.org/1640123004/diff/100001/media/base/android/med...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/1640123004/diff/100001/media/base/android/med...
media/base/android/media_player_android.h:29: enum { kDefaultMediaSessionID = 0
};
On 2016/03/10 22:34:56, whywhat wrote:
> nit: I believe Id is preferred to ID but I don't feel very strongly, the
> codebase seems to prefer Id over ID but there's not that many instances:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20file:%5C.h...
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20file:%5C.h...

Done.

[commit-bot: I haz the power, 2016-03-11 14:46:54]

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

[commit-bot: I haz the power, 2016-03-11 14:46:55]

Dry run: This issue passed the CQ dry run.

[davve, 2016-03-11 14:48:48]

The CQ bit was checked by davve@opera.com

[davve, 2016-03-11 14:48:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, philipj@opera.com, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1640123004/#ps160001
(title: "Fix stray kDefaultMediaSessionID")

[commit-bot: I haz the power, 2016-03-11 14:49:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640123004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640123004/160001

[commit-bot: I haz the power, 2016-03-11 14:55:16]

Description was changed from

==========
Propagate media session id into MediaPlayerAndroid

The media session id will be used to implicitly activate a user defined media
session.

BUG=497735, 581728
==========

to

==========
Propagate media session id into MediaPlayerAndroid

The media session id will be used to implicitly activate a user defined media
session.

BUG=497735, 581728
==========

[commit-bot: I haz the power, 2016-03-11 14:55:17]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-11 14:56:50]

Description was changed from

==========
Propagate media session id into MediaPlayerAndroid

The media session id will be used to implicitly activate a user defined media
session.

BUG=497735, 581728
==========

to

==========
Propagate media session id into MediaPlayerAndroid

The media session id will be used to implicitly activate a user defined media
session.

BUG=497735, 581728

Committed: https://crrev.com/541ee5c14eb321ab188cce3d077649cc216404b8
Cr-Commit-Position: refs/heads/master@{#380632}
==========

[commit-bot: I haz the power, 2016-03-11 14:56:51]

Patchset 9 (id:??) landed as
https://crrev.com/541ee5c14eb321ab188cce3d077649cc216404b8
Cr-Commit-Position: refs/heads/master@{#380632}