Bug 208203

Created attachment 391661 [details]
Patch

Created attachment 392004 [details]
Patch

Created attachment 392119 [details]
Patch

Created attachment 392145 [details]
Patch

Looks like you forgot to add MediaSessionManagerProxy.cpp to git.

I don't understand why you're introducing "Now Playing" into the cross-platform version of PlatformMediaSessionManager. We should just be creating a MediaSessionManagerMac or MediaSessionManageriOS in the GPUProcess rather than stuffing everything into the platform.

(In reply to Jer Noble from comment #6)
> I don't understand why you're introducing "Now Playing" into the
> cross-platform version of PlatformMediaSessionManager. We should just be
> creating a MediaSessionManagerMac or MediaSessionManageriOS in the
> GPUProcess rather than stuffing everything into the platform.

GPUProcess is already creating a MediaSessionManagerMac or MediaSessionManageriOS.

We need WebProcess session manager to talk with its GPUProcess counterpart, for instance from Page::setActivityState, hence why this patch is triggering some IPC from WebProcess to GPUProcess.

As of NowPlaying, it is already a concept introduced in platform-agnostic PlatformMediaSessionManager. See updateNowPlayingInfoIfNecessary/scheduleUpdateNowPlayingInfo/DelayCallingUpdateNowPlaying.

The proposed refactoring makes WebKit2 implementation smaller (as well as MediaSessionManagerMac/MediaSessionManageriOS) and simpler to understand.

The downside for other platforms is some unnecessary task enqueuing, but this could be optimised for these platforms. Or scheduleUpdateNowPlayingInfo could take a DelayCallingUpdateNowPlaying parameter, but this might require to duplicate the task enqueuing in both WebKit2 implementation and WebCore MediaSessionManagerCocoa.

An alternative is to keep PlatformMediaSessionManager as is, which would require  WebKit2 implementation to override scheduleUpdateNowPlayingInfo and all other methods also overridden by MediaSessionManagerCocoa/iOS. This does not seem great to me.

Created attachment 392261 [details]
Patch

(In reply to youenn fablet from comment #7)
> 
> An alternative is to keep PlatformMediaSessionManager as is, which would
> require  WebKit2 implementation to override scheduleUpdateNowPlayingInfo and
> all other methods also overridden by MediaSessionManagerCocoa/iOS. This does
> not seem great to me.

Another alternative would be to not use media sessions at all in the GPUProcess, and only an AudioSession / RemoteAudioSession / RemoteAudioSessionProxy. The MediaSessionManager doesn't have the information it needs for every PlatformMediaSession, as it can't get data from MediaElementSession it needs to implement now playing, or even decide which session is the "now playing" eligible one.

(In reply to Jer Noble from comment #9)
> (In reply to youenn fablet from comment #7)
> > 
> > An alternative is to keep PlatformMediaSessionManager as is, which would
> > require  WebKit2 implementation to override scheduleUpdateNowPlayingInfo and
> > all other methods also overridden by MediaSessionManagerCocoa/iOS. This does
> > not seem great to me.
> 
> Another alternative would be to not use media sessions at all in the
> GPUProcess, and only an AudioSession / RemoteAudioSession /
> RemoteAudioSessionProxy. The MediaSessionManager doesn't have the
> information it needs for every PlatformMediaSession, as it can't get data
> from MediaElementSession it needs to implement now playing, or even decide
> which session is the "now playing" eligible one.

Yeah, I thought about that too when discovering we were using HTMLMediaElement objects within platform code.
I did not go there since that would change non-GPU process enabled code paths (maybe other ports as well).
A bigger refactoring but this would probably clean the code base.

> it can't get data
> from MediaElementSession it needs to implement now playing, or even decide
> which session is the "now playing" eligible one.

I solved this in a local patch by making the gathering of the session async, which allows getting to WebProcess for that.

(In reply to youenn fablet from comment #11)
> > it can't get data
> > from MediaElementSession it needs to implement now playing, or even decide
> > which session is the "now playing" eligible one.
> 
> I solved this in a local patch by making the gathering of the session async,
> which allows getting to WebProcess for that.

I don't think this is the right approach. It now requires every session to be synchronized across the process boundary, and that looks like more work and a bigger refactoring in the end.

(In reply to Jer Noble from comment #12)
> (In reply to youenn fablet from comment #11)
> > > it can't get data
> > > from MediaElementSession it needs to implement now playing, or even decide
> > > which session is the "now playing" eligible one.
> > 
> > I solved this in a local patch by making the gathering of the session async,
> > which allows getting to WebProcess for that.
> 
> I don't think this is the right approach. It now requires every session to
> be synchronized across the process boundary, and that looks like more work
> and a bigger refactoring in the end.

(And then there's the bigger problem that both the WebProcess and GPUProcess are simultaneously trying to set an active AudioSession, and interrupt each other at a system level, which is not addressed by this patch, and is a more immediate concern.)

> I don't think this is the right approach. It now requires every session to
> be synchronized across the process boundary, and that looks like more work
> and a bigger refactoring in the end.

Not really, it just requires the GPU Process to query WebProcess for the NowPlaying session identifier and corresponding NowPlayingInfo whenever needed, assuming this retrieval can work asynchronously (seems to be the case but I am not sure).

> (And then there's the bigger problem that both the WebProcess and GPUProcess
> are simultaneously trying to set an active AudioSession, and interrupt each
> other at a system level, which is not addressed by this patch, and is a more
> immediate concern.)

This patch addresses this concern by setting a WebKit2 platform session manager that, instead of updating the session state, thus the AudioSession, sends an IPC to GPUProcess to do it.

I can try both approaches. I quite like splitting this code.
PlatformMediaSession::Client is somehow too big.
I simplified the code by no longer making MediaStreamTrack a PlatformMediaSession::Client and doing the same for AudioContext would be nice too.

<rdar://problem/60020229>