Bug 128844 – WebVideoFullscreen, should make the hand off of the video layer explicit.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jeremy Jones]

The transfer of ownership of the platformLayer for video is asynchronous. Make that aspect explicit in the interface.
e.g.  
model => interface  setVideoLayer()
model   => interface  enterFullScreen()
interface => model borrowVideoLayer()
model => interface videoLayerBorrowed()

[Jeremy Jones]

Created attachment 225724 [details]
Patch

[Jeremy Jones]

        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.

[Eric Carlson]

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.

[Simon Fraser (smfr)]

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?

[Jeremy Jones]

(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.

[Jeremy Jones]

(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

[Jeremy Jones]

Created attachment 225824 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 225824 [details]
Patch

Clearing flags on attachment: 225824

Committed r165087: <http://trac.webkit.org/changeset/165087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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

[Jon Lee]

Removing that line fixed it for me.

[Jon Lee]

See bug 129730 for fix to linker error