Summary: | WebVideoFullscreen, should make the hand off of the video layer explicit. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||||
Component: | WebKit2 | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, glenn, jer.noble, jonlee, philipj, sergio, simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jeremy Jones
2014-02-14 14:16:51 PST
Created attachment 225724 [details]
Patch
This change introduces a more explicit hand-off of the video layer. This describes the interactions between WebVideoFullscreenInterface and WebVideoFullscreenModel WebVideoFullscreenModel <-> WebVideoFullscreenInterface enterFullScreen(*) -> <- borrowVideoLayer willLendVideoLayer -> didLendVideoLayer -> <- didEnterFullscreen ... <- requestExitFullscreen exitFullscreen -> <- returnVideoLayer <- didExitFullscreen (*) enterFullScreen actually comes from WebVideoFullscreenControllerAVKit. It also adopts new changes from AVKit. Comment on attachment 225724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225724&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:103 > + //How do I keep myself around until the callback is fired? Is this comment still valid, or did you fix it with the retain and release? If it is valid, please file a bug and include the number here with a "FIXME:" > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:326 > + //Maybe get initial rect. Nit: this should have a "FIXME:", and there should be a space before "Maybe". > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:84 > + //give initial values to all the properties Nit: I don't think this comment is necessary, it states the obvious. > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.cpp:157 > + if (!m_targetFullscreen) { Nit: you could use an early return instead of indentation here. > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.cpp:173 > + if (m_targetFullscreen) { Ditto. Comment on attachment 225724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225724&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:72 > + _changeObserver.setTarget(nil); This shouldn't be necessary, since _changeObserver is our member variable. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:121 > + [self release]; // Don't go away until we are out of fullscreen! This comment should say something like "Balance the -retain we did in enterFullscreen:" Does this code run in WK1? Do we need the WebThreadRun? I hate seeing it code that can run in WK2 :\ > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:285 > + dispatch_async(dispatch_get_main_queue(), ^{ Why do we need the dispatch_async() with lots of scarey protecters in it? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:297 > + m_playerViewController.get().playerController = (AVPlayerController *)playerController(); We usually prefer [m_playerViewController setPlayerController:] syntax to avoid the .get() > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:307 > + > + Extra line. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:341 > + dispatch_async(dispatch_get_main_queue(), ^{ Again, why async? > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:121 > + m_videoFullscreenInterface->willLendVideoLayer(m_mediaElement->platformLayer()); > + m_borrowedVideoLayer = m_mediaElement->borrowPlatformLayer(); > + if (m_borrowedVideoLayer.get()) The naming of these things makes it hard to me to reason about what's going on here. This is called "Element" but isn't an element. m_videoFullscreenInterface: interface to what? > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:132 > + __block RefPtr<WebVideoFullscreenModelMediaElement> protect(this); > + WebThreadRun(^{ So much block. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:77 > + m_videoView = remoteDrawingAreaProxy->remoteLayerTreeHost().getLayer(videoLayerID); > + willLendVideoLayer(m_videoView.get().layer); Dont' you need the fix I landed today to unparent the UIView? > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:124 > + m_page->send(Messages::WebVideoFullscreenManager::ReturnVideoLayer(), m_page->pageID()); Do we actually return the layer? I thought we just got a new layer transaction from the web process with a new remote layer in it. > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:92 > + bool m_targetFullscreen; This is ambiguous: target fullscreen rather than something else? Target is fullscreen? (In reply to comment #3) > (From update of attachment 225724 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225724&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:103 > > + //How do I keep myself around until the callback is fired? > > Is this comment still valid, or did you fix it with the retain and release? If it is valid, please file a bug and include the number here with a "FIXME:" > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:326 > > + //Maybe get initial rect. > > Nit: this should have a "FIXME:", and there should be a space before "Maybe". > Deleted. This is speculation and there is already a bug for this. > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:84 > > + //give initial values to all the properties > > Nit: I don't think this comment is necessary, it states the obvious. Deleted. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.cpp:157 > > + if (!m_targetFullscreen) { > > Nit: you could use an early return instead of indentation here. Done. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.cpp:173 > > + if (m_targetFullscreen) { > > Ditto. Done. (In reply to comment #4) > (From update of attachment 225724 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225724&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:72 > > + _changeObserver.setTarget(nil); > > This shouldn't be necessary, since _changeObserver is our member variable. Deleted. > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:121 > > + [self release]; // Don't go away until we are out of fullscreen! > > This comment should say something like "Balance the -retain we did in enterFullscreen:" Updated both comments. > > Does this code run in WK1? Do we need the WebThreadRun? I hate seeing it code that can run in WK2 :\ > Yes this code runs in both WK1 and WK2. In WK1 we need to make sure we are on the WebThread before calling into the _model. This requires a non-trivial refactor, so I've filed the following to cover this change: https://bugs.webkit.org/show_bug.cgi?id=129708 > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:285 > > + dispatch_async(dispatch_get_main_queue(), ^{ > > Why do we need the dispatch_async() with lots of scarey protecters in it? In WebKit1 this will be called from the WebThread. We have to dispatch to the main queue before accessing UIKit. The second block is required because the UIKit animation is asynchronous and this is UIKit's way of running code on completion of the animation. Obj-c blocks will automatically retain obj-c objects, but don't know about our RefCounted<> type. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:297 > > + m_playerViewController.get().playerController = (AVPlayerController *)playerController(); > > We usually prefer [m_playerViewController setPlayerController:] syntax to avoid the .get() Fixed many instances of this. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:307 > > + > > + > > Extra line. > Deleted. > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:341 > > + dispatch_async(dispatch_get_main_queue(), ^{ > > Again, why async? See above comment. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:121 > > + m_videoFullscreenInterface->willLendVideoLayer(m_mediaElement->platformLayer()); > > + m_borrowedVideoLayer = m_mediaElement->borrowPlatformLayer(); > > + if (m_borrowedVideoLayer.get()) > > The naming of these things makes it hard to me to reason about what's going on here. This is called "Element" but isn't an element. m_videoFullscreenInterface: interface to what? This is still in my queue: https://bugs.webkit.org/show_bug.cgi?id=128843 > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:132 > > + __block RefPtr<WebVideoFullscreenModelMediaElement> protect(this); > > + WebThreadRun(^{ > > So much block. See above comment. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:77 > > + m_videoView = remoteDrawingAreaProxy->remoteLayerTreeHost().getLayer(videoLayerID); > > + willLendVideoLayer(m_videoView.get().layer); > > Dont' you need the fix I landed today to unparent the UIView? I've incorporated that change. See WebVideoFullscreenManagerProxy::didCommitLayerTree. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:124 > > + m_page->send(Messages::WebVideoFullscreenManager::ReturnVideoLayer(), m_page->pageID()); > > Do we actually return the layer? I thought we just got a new layer transaction from the web process with a new remote layer in it. We don't return it to the remote layer tree host. At this level we just release the UIView. Down the chain this does further clean-up. In HTMLMediaElement, this signals that AVPlayerLayer is once again visible to the renderer. See HTMLMediaElement::returnPlatformLayer > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:92 > > + bool m_targetFullscreen; > > This is ambiguous: target fullscreen rather than something else? Target is fullscreen? Changed to m_targetIsFullscreen Created attachment 225824 [details]
Patch
Comment on attachment 225824 [details] Patch Clearing flags on attachment: 225824 Committed r165087: <http://trac.webkit.org/changeset/165087> All reviewed patches have been landed. Closing bug. This broke the build: Ld WebCore Undefined symbols for architecture arm64: "__ZN7WebCore32WebVideoFullscreenChangeObserverD2Ev", referenced from: -exported_symbol[s_list] command line option ld: symbol(s) not found for architecture arm64 Removing that line fixed it for me. See bug 129730 for fix to linker error |