Bug 128844 - WebVideoFullscreen, should make the hand off of the video layer explicit.
Summary: WebVideoFullscreen, should make the hand off of the video layer explicit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-14 14:16 PST by Jeremy Jones
Modified: 2014-03-04 23:44 PST (History)
8 users (show)

See Also:


Attachments
Patch (49.03 KB, patch)
2014-03-03 17:39 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (48.89 KB, patch)
2014-03-04 15:31 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2014-02-14 14:16:51 PST
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()
Comment 1 Jeremy Jones 2014-03-03 17:39:52 PST
Created attachment 225724 [details]
Patch
Comment 2 Jeremy Jones 2014-03-03 17:41:40 PST
        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 3 Eric Carlson 2014-03-03 18:35:23 PST
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 4 Simon Fraser (smfr) 2014-03-03 18:55:42 PST
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?
Comment 5 Jeremy Jones 2014-03-04 14:15:33 PST
(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.
Comment 6 Jeremy Jones 2014-03-04 14:47:11 PST
(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
Comment 7 Jeremy Jones 2014-03-04 15:31:35 PST
Created attachment 225824 [details]
Patch
Comment 8 WebKit Commit Bot 2014-03-04 17:46:13 PST
Comment on attachment 225824 [details]
Patch

Clearing flags on attachment: 225824

Committed r165087: <http://trac.webkit.org/changeset/165087>
Comment 9 WebKit Commit Bot 2014-03-04 17:46:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Fraser (smfr) 2014-03-04 22:13:02 PST
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
Comment 11 Jon Lee 2014-03-04 23:14:23 PST
Removing that line fixed it for me.
Comment 12 Jon Lee 2014-03-04 23:44:37 PST
See bug 129730 for fix to linker error