NOT FOR LANDING Implement WebMediaSession

NOT FOR LANDING Implement WebMediaSession

Blink CL: https://codereview.chromium.org/1259643002/

BUG=497735

[philipj_slow, 2015-07-29 13:02:48]

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

[philipj_slow, 2015-07-29 13:02:48]

Hi all,

I'm trying to get something very basic hooked up all the way so that Blink can
create a MediaSession that is backed by the very same code that's now involved
with the media playback notification.

I'm a total noob on this side of the Chromium-Blink divide, so some feedback
would be appreciated.

What we have here is a case quite similar to WebMediaPlayer(Android), where
Blink may at any time need an object on the browser side to communicate with
over IPC.

Is it possible that I could get away with Platform::createMediaSession(), which
is nice and simple, or will I have to hang the API off of something that has
more context? WebMediaPlayer has a manager in both browser and renderer, but
it's not obvious to me if I should mirror that, or if there's some extra
complexity with media players that don't apply here.

I will continue to dig (I've barely started) but would appreciate any feedback
that stops me from wasting time :)

[whywhat, 2015-07-30 14:31:22]

Thanks for the preview Philip. I've left one nit and one philosophical comment
:)

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediasession_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediasession_impl.h:19: virtual void
activate();
nit: we use "override" instead of "virtual" in Chromium (and also in Blink I
believe) for virtual method overrides and also dtors.

https://codereview.chromium.org/1259633002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.h:79: virtual
blink::WebMediaSession* createMediaSession();
As I understand, hanging media session from BlinkPlatform means that each
MediaSession (or its creation) won't be tied to any particular frame on the page
so implementing any per-frame logic if needed would be hard. If that's not a
problem, having createMediaSession here seems alright.

Otherwise MediaSession sounds more like PresentationRequest:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
It can just be constructed with an ExecutionContext where it gets the frame
from.

[philipj_slow, 2015-07-30 14:47:04]

Thanks for the feedback :) Still need to do a lot of experimenting here...

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediasession_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediasession_impl.h:19: virtual void
activate();
On 2015/07/30 14:31:22, whywhat wrote:
> nit: we use "override" instead of "virtual" in Chromium (and also in Blink I
> believe) for virtual method overrides and also dtors.

Oops, copypasta.

https://codereview.chromium.org/1259633002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.h:79: virtual
blink::WebMediaSession* createMediaSession();
On 2015/07/30 14:31:22, whywhat wrote:
> As I understand, hanging media session from BlinkPlatform means that each
> MediaSession (or its creation) won't be tied to any particular frame on the
page
> so implementing any per-frame logic if needed would be hard. If that's not a
> problem, having createMediaSession here seems alright.

Even though Blink-created MediaSessions won't have the per-tab sharing that the
default sessions have, it will be necessary to associate it with a tab so that
one can click the notification to get back to the responsible tab. I guess that
rules out the BlinkPlatform path. Thanks :)

> Otherwise MediaSession sounds more like PresentationRequest:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> It can just be constructed with an ExecutionContext where it gets the frame
> from.

Looks familiar, I looked at it a bit before. In for example
PresentationRequest::start, is ongoing request in any way tied to the
PresentationRequest instance that started it? It looks like the
WebPresentationClient involved is tied to the frame via the
PresentationController. It might be that MediaSession will need a callback to
the same instance to update some state (idle, active, interrupted) right before
the promise is resolved, so that scripts can't observe the change first.

I see that Presentation uses Mojo for the IPC, is that something you would
recommend in this case too?

[mcasas, 2015-07-31 14:14:27]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2015-07-31 14:14:28]

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediasession_impl.cc (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediasession_impl.cc:14:
RendererWebMediaSessionImpl::~RendererWebMediaSessionImpl() {
Empty methods can be inlined, although:
- I suspect you might want to fill them in :)
- Some (old) Windows compiler have issues
with empty dtors that are supposed to be 
overriden in other libraries (i.e. outside content/).

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediasession_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediasession_impl.h:13: class
RendererWebMediaSessionImpl : public blink::WebMediaSession {
I don't think the prefix "renderer" is needed in either the
file name nor the class, and the same applies for the
"impl" suffix - see other files around the same folder:
media_stream_track for WebMediaStreamTrack in Blink 
etc.

[mlamouri (slow - plz ping), 2015-08-05 15:50:16]

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webmediasession_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediasession_impl.h:19: virtual void
activate();
On 2015/07/30 at 14:47:04, philipj wrote:
> On 2015/07/30 14:31:22, whywhat wrote:
> > nit: we use "override" instead of "virtual" in Chromium (and also in Blink I
> > believe) for virtual method overrides and also dtors.
> 
> Oops, copypasta.

We don't actually use override when overriding Blink interfaces because it would
break Chromium if they were to change.

https://codereview.chromium.org/1259633002/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webmediasession_impl.h:23: 
nit: remove empty line

https://codereview.chromium.org/1259633002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.h (right):

https://codereview.chromium.org/1259633002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.h:79: virtual
blink::WebMediaSession* createMediaSession();
On 2015/07/30 at 14:47:04, philipj wrote:
> On 2015/07/30 14:31:22, whywhat wrote:
> > As I understand, hanging media session from BlinkPlatform means that each
> > MediaSession (or its creation) won't be tied to any particular frame on the
page
> > so implementing any per-frame logic if needed would be hard. If that's not a
> > problem, having createMediaSession here seems alright.
> 
> Even though Blink-created MediaSessions won't have the per-tab sharing that
the default sessions have, it will be necessary to associate it with a tab so
that one can click the notification to get back to the responsible tab. I guess
that rules out the BlinkPlatform path. Thanks :)
> 
> > Otherwise MediaSession sounds more like PresentationRequest:
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > It can just be constructed with an ExecutionContext where it gets the frame
> > from.
> 
> Looks familiar, I looked at it a bit before. In for example
PresentationRequest::start, is ongoing request in any way tied to the
PresentationRequest instance that started it? It looks like the
WebPresentationClient involved is tied to the frame via the
PresentationController. It might be that MediaSession will need a callback to
the same instance to update some state (idle, active, interrupted) right before
the promise is resolved, so that scripts can't observe the change first.

As Anton pointed, RenderBlinkPlatformImpl isn't the best way to do that. You
should have the MediaSession available from the WebFrameClient (implemented by
RenderFrameImpl).

> I see that Presentation uses Mojo for the IPC, is that something you would
recommend in this case too?

I guess we could have a MediaSessionService. Otherwise, you can just add new
Media IPC.

[philipj_slow, 2015-08-13 13:26:00]

OK, so now I've updated both CLs to something that just barely is able to get
the notification UI showing by creating a MediaSession and calling activate().

I've basically just copied the media player paths, the added
content::MediaSession::Activate/Deactivate implementations are terrible, and
BrowserMediaSessionManager::OnActivate/OnDeactivate are just placeholders.

While everything is wrong, let's talk about the design before getting into the
details.

Currently there can only be one instance of content::MediaSession, tied to the
WebContents. We'll need to refactor this so that there's a shared class that is
used for both Blink-created sessions and the default sessions.

I think that a lot of what is currently in content::MediaSession would not
belong in that shared code path, but should rather be moved to a
DefaultMediaSessionManager or some such, that is per-WebContents and can create
a MediaSession of the same kind that BrowserMediaSessionManager would create on
Blink's behalf.

Before I just go ahead and break everything, any thoughts on this refactoring,
anything non-obvious about the current setup to keep in mind?

[mlamouri (slow - plz ping), 2015-08-24 12:55:24]

Generally speaking, content::MediaSession does all the work that we would need
for the API (ie. requesting audio focus to the system, etc.). I'm not sure how
the default media session should behave regarding to that. Could we imagine
something where there are content::MediaSession clients that can activate
themselves wrt the content::MediaSession and be notified when they get
de-activated. We already have a similar system but it is at a player level.
Maybe that could be done at a higher level so clients could be at various
scales?

https://codereview.chromium.org/1259633002/diff/20001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1259633002/diff/20001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:299: // the tab title and
other information for the default media session.
tab title should be available from WebContents. I don't think we need to send
that here.

https://codereview.chromium.org/1259633002/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1259633002/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:74:
AbandonSystemAudioFocusIfNeeded();
Looking at that code, it's a bit odd that RequestSystemAudioFocus() doesn't
update audio_focus_state and doesn't call UpdateWebContents() but
AbandonSystemAudioFocusIfNeeded() does.

[whywhat, 2015-08-24 14:39:18]

https://codereview.chromium.org/1259633002/diff/20001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1259633002/diff/20001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:299: // the tab title and
other information for the default media session.
On 2015/08/24 at 12:55:24, Mounir Lamouri wrote:
> tab title should be available from WebContents. I don't think we need to send
that here.

It might make sense to send media session metadata which would be filled with
the tab title by default I guess.

[davve, 2015-08-24 16:03:03]

On 2015/08/24 12:55:24, Mounir Lamouri wrote:
> Generally speaking, content::MediaSession does all the work that we would need
> for the API (ie. requesting audio focus to the system, etc.). I'm not sure how
> the default media session should behave regarding to that. Could we imagine
> something where there are content::MediaSession clients that can activate
> themselves wrt the content::MediaSession and be notified when they get
> de-activated. We already have a similar system but it is at a player level.
> Maybe that could be done at a higher level so clients could be at various
> scales?

Wouldn't this mean that one tab (WebContents) can only request audio
focus once? And as consequence has to select one type of audio focus
exposed to the system?

I guess it's possible to work out sane interactions when multiple
sessions are in action at the same time within a WebContents. But it
does sound like unnecessary complexity when the system can handle it
for us. What would be the upside compared to having stand-alone media
sessions on the browser side each requesting audio focus?

[mlamouri (slow - plz ping), 2015-08-25 10:33:23]

On 2015/08/24 at 16:03:03, davve wrote:
> On 2015/08/24 12:55:24, Mounir Lamouri wrote:
> > Generally speaking, content::MediaSession does all the work that we would
need
> > for the API (ie. requesting audio focus to the system, etc.). I'm not sure
how
> > the default media session should behave regarding to that. Could we imagine
> > something where there are content::MediaSession clients that can activate
> > themselves wrt the content::MediaSession and be notified when they get
> > de-activated. We already have a similar system but it is at a player level.
> > Maybe that could be done at a higher level so clients could be at various
> > scales?
> 
> Wouldn't this mean that one tab (WebContents) can only request audio
> focus once? And as consequence has to select one type of audio focus
> exposed to the system?
> 
> I guess it's possible to work out sane interactions when multiple
> sessions are in action at the same time within a WebContents. But it
> does sound like unnecessary complexity when the system can handle it
> for us. What would be the upside compared to having stand-alone media
> sessions on the browser side each requesting audio focus?

Hmm, that's indeed correct on Android. If we want to generalize the system, it
might not work the same way. As long as we keep this Android specific we could
indeed try to re-use the fact that the system will deactive a MediaSession when
another one got audio focus.

[philipj_slow, 2015-08-26 07:39:48]

https://codereview.chromium.org/1259633002/diff/20001/content/browser/android...
File content/browser/android/web_contents_observer_proxy.cc (right):

https://codereview.chromium.org/1259633002/diff/20001/content/browser/android...
content/browser/android/web_contents_observer_proxy.cc:299: // the tab title and
other information for the default media session.
On 2015/08/24 14:39:18, whywhat wrote:
> On 2015/08/24 at 12:55:24, Mounir Lamouri wrote:
> > tab title should be available from WebContents. I don't think we need to
send
> that here.
> 
> It might make sense to send media session metadata which would be filled with
> the tab title by default I guess.

Right, when I wrote this comment, I was thinking that we would have a
content::MediaSession that basically just maps to Android's MediaSession, so
that it becomes the job of some other class to monitor all of the media players
in a tab and to come up with metadata since none can be provided by scripts in
this case.

However, we might consider what should happen when a script creates a
MediaSession put doesn't provide any metadata. If the fallback in this case can
be the same as for the default media session, then indeed we do not need to ever
send in a tab title at this level, it would just be the in the code that creates
the notification.

https://codereview.chromium.org/1259633002/diff/20001/content/browser/media/a...
File content/browser/media/android/media_session.cc (right):

https://codereview.chromium.org/1259633002/diff/20001/content/browser/media/a...
content/browser/media/android/media_session.cc:74:
AbandonSystemAudioFocusIfNeeded();
On 2015/08/24 12:55:24, Mounir Lamouri wrote:
> Looking at that code, it's a bit odd that RequestSystemAudioFocus() doesn't
> update audio_focus_state and doesn't call UpdateWebContents() but
> AbandonSystemAudioFocusIfNeeded() does.

Yes, this doesn't make any sense, I just hacked until it was possible to get a
notification showing using the API. If we split the responsibility for
monitoring media players out of this class, then I think we would end up with a
much cleaner Activate() and Deactivate(), that would be the only places to
request or abandon audio focus.

[philipj_slow, 2015-10-09 09:39:34]

Superseded by https://codereview.chromium.org/1370453002/