-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Eliminate double layer hosting of video content when UI-Side compositing is enabled #9910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
EWS run on previous version of this PR (hash 554a0ba) Details |
554a0ba to
3416692
Compare
|
EWS run on previous version of this PR (hash 3416692) Details |
eric-carlson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto hostingContextID = videoElement.layerHostingContextID != m_layerHostingContextID) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can initialize and check in the same if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is done a lot in WebKit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you did it yourself in https://bugs.webkit.org/show_bug.cgi?id=162971
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Space between // and Denotes. This is really long and should be wrapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved inside of the if (auto playerLayer = model->playerLayer()) block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto videoLayerIter = m_videoLayers.find(layerID) != m_videoLayers())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (auto videoLayerIter = m_videoLayers.find(layerID) != m_videoLayers())
3416692 to
f30834a
Compare
|
EWS run on previous version of this PR (hash f30834a) Details
|
f30834a to
a9ad486
Compare
|
EWS run on previous version of this PR (hash a9ad486) Details
|
a9ad486 to
2272cbd
Compare
|
EWS run on previous version of this PR (hash 2272cbd) Details
|
d073f0c to
f8624ec
Compare
|
EWS run on previous version of this PR (hash f8624ec) Details |
|
EWS run on previous version of this PR (hash d073f0c) Details |
f8624ec to
da3eaa7
Compare
|
EWS run on previous version of this PR (hash da3eaa7) Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks somewhat unrelated, is it strictly needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's missing a new line between these methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to avoid referencing HTML element classes inside the graphics layer class, you could possibly create a new, more generic class to use here instead of HTMLVideoElement. This class would then declare the virtual methods needed, and HTMLVideElement could subclass and override the methods. However, I don't think this is needed to move forward with the current patch, and could be done in a follow-up patch, if you think it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphicsLayer cannot know about HMTL elements.
pvollan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R=me.
da3eaa7 to
89dd664
Compare
|
EWS run on current version of this PR (hash 89dd664) Details
|
…ing is enabled https://bugs.webkit.org/show_bug.cgi?id=252021 <rdar://105246033> Reviewed by Eric Carlson and Per Arne Vollan. Currently, video content from the GPU process is remotely-hosted into the WebContent process, and is subsequently remotely hosted from the WebContent process to the UI process when UI-side compositing is enabled. This is unnecessary, and necessitates having an active CARenderer in the Web Content process. Rather than double hosting video content from GPU -> WebContent -> UI process, video content should be hosted directly from the GPU process into the UI process. To facilitate this, re-use the infrastructure built into VideoFullscreenManager to handle the required XPC communication to send resize information back down from the UI process to the GPU process. HTMLMediaElement will need to expose API to resize its video layer, and to provide the remote hosting context ID from the GPU process. Rather than have a separate identifier of its own, VideoFullscreenManager and PlaybackSessionManager can just use a HTMLMediaElement identifier for the purposes of passing messages across the XPC boundary. WebAVPlayerLayer must support use cases more like VideoLayerRemoteCocoa, including handling multiple VideoGravity settings, so it is improved to take these settings into account. WebAVPlayerLayerView has been pulled out of VideoFullScreenManagerAVKit into its own source and header files. GraphicsLayerCA, RemoteLayerTreeHost, and RemoteLayerTree all must be modified to accept a HTMLVideoElement (or its identifier) as a parameter. * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/dom/ImageOverlay.cpp: (WebCore::ImageOverlay::updateSubtree): * Source/WebCore/html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::setVideoInlineSizeFenced): (WebCore::HTMLMediaElement::layerHostingContextID): (WebCore::HTMLMediaElement::naturalSize): * Source/WebCore/html/HTMLMediaElement.h: (WebCore::HTMLMediaElement::identifier const): * Source/WebCore/platform/cocoa/VideoFullscreenModel.h: * Source/WebCore/platform/cocoa/VideoFullscreenModelVideoElement.h: * Source/WebCore/platform/cocoa/VideoFullscreenModelVideoElement.mm: (WebCore::VideoFullscreenModelVideoElement::setVideoInlineSizeFenced): * Source/WebCore/platform/cocoa/WebAVPlayerLayer.h: * Source/WebCore/platform/cocoa/WebAVPlayerLayer.mm: (-[WebAVPlayerLayer layoutSublayers]): (-[WebAVPlayerLayer resolveBounds]): * Source/WebCore/platform/graphics/GraphicsLayer.h: (WebCore::GraphicsLayer::layerMode const): (WebCore::GraphicsLayer::setContentsToVideoElement): * Source/WebCore/platform/graphics/MediaPlayer.cpp: (WebCore::MediaPlayer::setVideoInlineSizeFenced): (WebCore::MediaPlayer::hostingContextID const): * Source/WebCore/platform/graphics/MediaPlayer.h: * Source/WebCore/platform/graphics/MediaPlayerPrivate.h: (WebCore::MediaPlayerPrivateInterface::setVideoInlineSizeFenced): (WebCore::MediaPlayerPrivateInterface::hostingContextID const): * Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp: (WebCore::GraphicsLayerCA::createPlatformCALayer): (WebCore::GraphicsLayerCA::setContentsToPlatformLayer): (WebCore::GraphicsLayerCA::setContentsToVideoElement): * Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h: * Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp: (WebCore::operator<<): * Source/WebCore/platform/graphics/ca/PlatformCALayer.h: * Source/WebCore/platform/graphics/ca/cocoa/PlatformCALayerCocoa.mm: (WebCore::PlatformCALayerCocoa::PlatformCALayerCocoa): * Source/WebCore/platform/graphics/ca/cocoa/WebVideoContainerLayer.mm: (-[WebVideoContainerLayer setPosition:]): Deleted. * Source/WebCore/platform/graphics/cocoa/MediaPlayerPrivateWebM.h: * Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.h: * Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.h: * Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm: (-[WebVideoFullscreenInterfaceMacObjC setUpPIPForVideoView:withFrame:inWindow:]): (WebCore::VideoFullscreenInterfaceMac::createLayer): * Source/WebCore/rendering/RenderLayerBacking.cpp: (WebCore::RenderLayerBacking::updateConfiguration): * Source/WebKit/Platform/cocoa/LayerHostingContext.h: * Source/WebKit/Platform/cocoa/LayerHostingContext.mm: (WebKit::LayerHostingContext::createForPort): (WebKit::LayerHostingContext::createForExternalHostingProcess): (WebKit::LayerHostingContext::createTransportLayerForRemoteHosting): (WebKit::LayerHostingContext::updateCachedContextID): (WebKit::LayerHostingContext::cachedContextID): * Source/WebKit/Shared/PlaybackSessionContextIdentifier.h: (): Deleted. * Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm: (WebKit::RemoteLayerBackingStore::drawInContext): * Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.h: * Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.mm: (WebKit::RemoteLayerTreeTransaction::LayerCreationProperties::LayerCreationProperties): (WebKit::RemoteLayerTreeTransaction::LayerCreationProperties::encode const): (WebKit::RemoteLayerTreeTransaction::LayerCreationProperties::decode): (WebKit::RemoteLayerTreeTransaction::description const): * Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h: * Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm: (WebKit::VideoFullscreenManagerProxy::createLayerWithID): (WebKit::VideoFullscreenManagerProxy::createLayerHostViewWithID): (WebKit::VideoFullscreenManagerProxy::setupFullscreenWithID): (WebKit::VideoFullscreenManagerProxy::didCleanupFullscreen): * Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm: (WebKit::RemoteLayerTreeHost::makeNode): * Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeHostIOS.mm: (WebKit::RemoteLayerTreeHost::makeNode): * Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp: (WebKit::MediaPlayerPrivateRemote::hostingContextID const): (WebKit::MediaPlayerPrivateRemote::setLayerHostingContextID): (WebKit::MediaPlayerPrivateRemote::setCachedPresentationSize): (WebKit::MediaPlayerPrivateRemote::cachedPresentationSize): * Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h: * Source/WebKit/WebProcess/GPU/media/cocoa/MediaPlayerPrivateRemoteCocoa.mm: (WebKit::MediaPlayerPrivateRemote::layerHostingContextIdChanged): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/GraphicsLayerCARemote.cpp: (WebKit::GraphicsLayerCARemote::createPlatformCALayer): (WebKit::GraphicsLayerCARemote::layerMode const): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/GraphicsLayerCARemote.h: * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp: (WebKit::PlatformCALayerRemote::create): (WebKit::PlatformCALayerRemote::recursiveBuildTransaction): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.h: * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteCustom.h: * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemoteCustom.mm: (WebKit::PlatformCALayerRemoteCustom::create): (WebKit::PlatformCALayerRemoteCustom::PlatformCALayerRemoteCustom): (WebKit::PlatformCALayerRemoteCustom::hostingContextID): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeContext.h: (WebKit::RemoteLayerTreeContext::webPage): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeContext.mm: (WebKit::RemoteLayerTreeContext::layerDidEnterContext): * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::m_appHighlightsVisible): * Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm: (WebKit::TiledCoreAnimationDrawingArea::sendDidFirstLayerFlushIfNeeded): (WebKit::TiledCoreAnimationDrawingArea::sendEnterAcceleratedCompositingModeIfNeeded): (WebKit::TiledCoreAnimationDrawingArea::setLayerHostingMode): * Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.h: * Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm: (WebKit::PlaybackSessionManager::removeContext): (WebKit::PlaybackSessionManager::setUpPlaybackControlsManager): (WebKit::PlaybackSessionManager::contextIdForMediaElement): * Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h: * Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm: (WebKit::VideoFullscreenManager::setupRemoteLayerHosting): (WebKit::VideoFullscreenManager::enterVideoFullscreenForVideoElement): (WebKit::VideoFullscreenManager::setVideoLayerFrameFenced): Co-authored-by: Arunsundar Kannan <arunsundar_kannan@apple.com> Canonical link: https://commits.webkit.org/260575@main
89dd664 to
897c865
Compare
|
Committed 260575@main (897c865): https://commits.webkit.org/260575@main Reviewed commits have been landed. Closing PR #9910 and removing active labels. |
| #include "FloatRoundedRect.h" | ||
| #include "FloatSize.h" | ||
| #include "GraphicsLayerClient.h" | ||
| #include "HTMLMediaElementIdentifier.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a layering violation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphicsLayer cannot know about HMTL elements.
| noteLayerPropertyChanged(ContentsPlatformLayerChanged); | ||
| } | ||
|
|
||
| void GraphicsLayerCA::setContentsToVideoElement(HTMLVideoElement& videoElement, ContentsLayerPurpose purpose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way to fix the layering violation would be to call something on the HTMLVideoElement that sets up this GraphicsLayer.
897c865
89dd664