Bug 208252

<rdar://problem/59813445>

Created attachment 391766 [details] WIP Patch

Created attachment 391789 [details] WIP patch - rebased

Comment on attachment 391789 [details] WIP patch - rebased View in context: https://bugs.webkit.org/attachment.cgi?id=391789&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1088 > + return adoptNS([[CALayer alloc] init]); Nit: rather than duplicating this code in each MPP, you could put it in VideoLayerManagerObjC since all three use it. You also might want to layer name to help when debugging the layer tree. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:67 > +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)) > + EnterFullscreen() -> () Async > + ExitFullscreen() -> () Async > +#endif Nit: Why make this macOS/iOS only even if it won't work for other ports yet? > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:593 > + return m_videoFullscreenLayer; Many <video> and *all* <audio> elements never enter fullscreen, so you should probably create m_videoFullscreenLayer here rather than in prepareForPlayback. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:604 > + if (!videoFullscreenLayer) { > + connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::ExitFullscreen(), WTFMove(completionHandler), m_id); > + return; > + } > + > + ASSERT(videoFullscreenLayer == m_videoFullscreenLayer.get()); > + connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::EnterFullscreen(), WTFMove(completionHandler), m_id); It seems a bit strange to have setVideoFullscreenLayer call enterFullscreen and exitFullscreen in the web process, and to have enterFullscreen and exitFullscreen call setVideoFullscreenLayer in the GPU process. > Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:276 > +#ifndef NDEBUG Why only do this in Debug builds?

Comment on attachment 391789 [details] WIP patch - rebased View in context: https://bugs.webkit.org/attachment.cgi?id=391789&action=review >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1088 >> + return adoptNS([[CALayer alloc] init]); > > Nit: rather than duplicating this code in each MPP, you could put it in VideoLayerManagerObjC since all three use it. You also might want to layer name to help when debugging the layer tree. This is a great suggestion. But it will lead to a lot of changes. We had better do that in a refactoring. I filed a bug for it. https://bugs.webkit.org/show_bug.cgi?id=208342 >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:67 >> +#endif > > Nit: Why make this macOS/iOS only even if it won't work for other ports yet? Let's enable it for all ports with the media in GPU Process feature support. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:593 >> + return m_videoFullscreenLayer; > > Many <video> and *all* <audio> elements never enter fullscreen, so you should probably create m_videoFullscreenLayer here rather than in prepareForPlayback. You are right! Let's create m_videoFullscreenLayer in createVideoFullscreenLayer() instead of prepareForPlayback(). An even better fix will delay the creation of m_fullscreenLayerHostingContext and the corresponding CALayer in RemoteMediaPlayerProxy (in the GPU Process) as well. But that will require MediaPlayerPrivateRemote::createVideoFullscreenLayer() to create the layer asynchronously. I plan do that in https://bugs.webkit.org/show_bug.cgi?id=208342. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:604 >> + connection().sendWithAsyncReply(Messages::RemoteMediaPlayerProxy::EnterFullscreen(), WTFMove(completionHandler), m_id); > > It seems a bit strange to have setVideoFullscreenLayer call enterFullscreen and exitFullscreen in the web process, and to have enterFullscreen and exitFullscreen call setVideoFullscreenLayer in the GPU process. This will be fixed in the refactoring. https://bugs.webkit.org/show_bug.cgi?id=208342 >> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:276 >> +#ifndef NDEBUG > > Why only do this in Debug builds? Will enable it always.

Created attachment 392028 [details] Patch

Comment on attachment 392028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392028&action=review > Source/WebCore/html/HTMLMediaElement.h:182 > + RetainPtr<PlatformLayer> createVideoFullscreenLayer(); Should this really create a new one every time? What is the "video fullscreen layer"? Is it the layer with the video in it, the layer that goes fullscreen, or something else? > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:508 > + m_inlineLayerHostingContext->setRootLayer(m_player->platformLayer()); LayerHostingContext takes a CALayer*. You should really be in a .mm file if you're calling this code. RemoteMediaPlayerProxy.cpp doesn't sound Cocoa-specific. > Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:51 > + [videoFullscreenLayer setName:@"Web video fullscreen manager layer (remote)"]; That's a lot of words. Why "videoFullscreenLayer" if it's really "videoFullscreenRemoteManagerLayer"? > Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:52 > + [videoFullscreenLayer setPosition:CGPointMake(0, 0)]; CGPointZero. Maybe set the anchor point too because this won't be top-left aligned if it has non-zero size. > Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:53 > + [videoFullscreenLayer setBackgroundColor:cachedCGColor(WebCore::Color::transparent)]; This shouldn't be necessary. > Source/WebKit/WebProcess/GPU/media/cocoa/MediaPlayerPrivateRemoteCocoa.mm:37 > +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)) We should have an ENABLE_ or something for this combination. Why doesn't iOS just define ENABLE_VIDEO_PRESENTATION_MODE ?

Comment on attachment 392028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392028&action=review >> Source/WebCore/html/HTMLMediaElement.h:182 >> + RetainPtr<PlatformLayer> createVideoFullscreenLayer(); > > Should this really create a new one every time? > > What is the "video fullscreen layer"? Is it the layer with the video in it, the layer that goes fullscreen, or something else? When we run MediaPlayerPrivate* in the Web process, yes, a new layer will be created every time. That’s also the behavior before this patch and it will be changed in https://bugs.webkit.org/show_bug.cgi?id=208342. When we run MediaPlayerPrivate* in the GPU process, no, we will create the layer in the first time when the function is called and reuse it in the future. The "video fullscreen layer" is a container layer to embed the real video content layer. It is used in both video fullscreen and PiP scenarios. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:508 >> + m_inlineLayerHostingContext->setRootLayer(m_player->platformLayer()); > > LayerHostingContext takes a CALayer*. You should really be in a .mm file if you're calling this code. RemoteMediaPlayerProxy.cpp doesn't sound Cocoa-specific. Right. Let's move it to RemoteMediaPlayerProxyCocoa.mm. The same to other functions related to LayerHostingContext. >> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:51 >> + [videoFullscreenLayer setName:@"Web video fullscreen manager layer (remote)"]; > > That's a lot of words. > > Why "videoFullscreenLayer" if it's really "videoFullscreenRemoteManagerLayer"? Agree. We use the name "videoFullscreenLayer" in all other places except here (setName). I will change the name here as well as the name in VideoFullscreenManager. >> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:52 >> + [videoFullscreenLayer setPosition:CGPointMake(0, 0)]; > > CGPointZero. > > Maybe set the anchor point too because this won't be top-left aligned if it has non-zero size. Fixed. Will try to set the anchor point later because we may need to change the anchor point of the corresponding layer in the web process as well. >> Source/WebKit/GPUProcess/media/cocoa/RemoteMediaPlayerProxyCocoa.mm:53 >> + [videoFullscreenLayer setBackgroundColor:cachedCGColor(WebCore::Color::transparent)]; > > This shouldn't be necessary. Removed. >> Source/WebKit/WebProcess/GPU/media/cocoa/MediaPlayerPrivateRemoteCocoa.mm:37 >> +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)) > > We should have an ENABLE_ or something for this combination. Why doesn't iOS just define ENABLE_VIDEO_PRESENTATION_MODE ? Good question! But I would like to keep it in the current way. Maybe fix it in another patch later.

Created attachment 392039 [details] Patch for landing

Created attachment 392045 [details] Patch for landing (fix unified build failures)

Comment on attachment 392045 [details] Patch for landing (fix unified build failures) Rejecting attachment 392045 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 392045, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/13330939

Created attachment 392047 [details] Patch for landing

Comment on attachment 392047 [details] Patch for landing Clearing flags on attachment: 392047 Committed r257680: <https://trac.webkit.org/changeset/257680>