Bug 128564

Summary: WK2 AVKit fullscreen doesn't display video.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: WebKit2Assignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, glenn, jer.noble, jonlee, philipj, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128573    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jeremy Jones 2014-02-10 16:23:43 PST
Need to find a way to get the video PlatformLayer from WebPageProxy.
Comment 1 Jeremy Jones 2014-02-14 12:53:36 PST
Created attachment 224244 [details]
Patch
Comment 2 Jeremy Jones 2014-02-14 13:48:24 PST
Simon, there are a couple interesting changes here.
1) There is already mechanism for getting a PlatformCALayer from a PlatformLayer. I hooked this up to work with PlatformCALayerRemoteCustom.
2) RemoteLayerTreeTransaction has a new flag. It doesn't seem elegant. Do you have a suggestion for how to make it more generally useful?
Comment 3 Simon Fraser (smfr) 2014-02-14 13:56:46 PST
Comment on attachment 224244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224244&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebCore.exp.in:

A short summary of the change should go here.

> Source/WebKit2/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Shared/mac/RemoteLayerTreeTransaction.h:

Ditto

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:287
> +    if ([videoLayer isKindOfClass:[classAVPlayerLayer class]])
> +        avPlayerLayer = (CALayer<AVPlayerLayer>*)videoLayer;
> +    else

Why do we need to handle both cases here? Do they occur in different scenarios? A comment would be nice.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:290
> +        avPlayerLayer = [[[WebAVPlayerLayer alloc] init] autorelease];

[WebAVPlayerLayer layer] ?

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:97
> +    m_videoFullscreenInterface->setVideoLayer(m_mediaElement->platformLayer());

I hate the name "interface". Is it human interface, or a an interface class?

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:176
> +    bool videoLayerUnparented() const { return m_videoLayerUnparented; }

videoLayerIsUnparented

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> +    bool m_videoLayerUnparented;

This doesn't seem to expand to > 1 video layer (yeah, I know you can only take one fullscreen at a time, but even so...)

I think this data should be the layerID for a going-fullscreen video layer.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.cpp:73
> +    m_expectVideoLayerUnparentedTransaction = true;

This is confusing. Didn't you just get it? Or are you waiting for the next unparenting?

Might be clearer with a state machine with enum states.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:49
> +    void enterFullscreen() override;

virtual

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:55
> +    virtual void setVideoLayerID(WebCore::GraphicsLayer::PlatformLayerID);

override?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:208
> +    void buildTransaction(RemoteLayerTreeTransaction&);

willCommitLayerTree

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemoteCustom.mm:68
> +    [m_platformLayer setValue:[NSValue valueWithPointer:nullptr] forKey:platformCALayerPointer];

setValue:nil

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:60
> +    void buildTransaction(RemoteLayerTreeTransaction&);

willCommitLayerTree
Comment 4 Jeremy Jones 2014-02-14 14:22:39 PST
A few quick responses:

(In reply to comment #3)
> (From update of attachment 224244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224244&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * WebCore.exp.in:
> 
> A short summary of the change should go here.
> 
> > Source/WebKit2/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * Shared/mac/RemoteLayerTreeTransaction.h:
> 
> Ditto
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:287
> > +    if ([videoLayer isKindOfClass:[classAVPlayerLayer class]])
> > +        avPlayerLayer = (CALayer<AVPlayerLayer>*)videoLayer;
> > +    else
> 
> Why do we need to handle both cases here? Do they occur in different scenarios? A comment would be nice.

Yes, this is used both by WebKit and WebKit2.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:290
> > +        avPlayerLayer = [[[WebAVPlayerLayer alloc] init] autorelease];
> 
> [WebAVPlayerLayer layer] ?
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:97
> > +    m_videoFullscreenInterface->setVideoLayer(m_mediaElement->platformLayer());
> 
> I hate the name "interface". Is it human interface, or a an interface class?

I filed https://bugs.webkit.org/show_bug.cgi?id=128843

> 
> > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:176
> > +    bool videoLayerUnparented() const { return m_videoLayerUnparented; }
> 
> videoLayerIsUnparented
> 
> > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> > +    bool m_videoLayerUnparented;
> 
> This doesn't seem to expand to > 1 video layer (yeah, I know you can only take one fullscreen at a time, but even so...)
> 
> I think this data should be the layerID for a going-fullscreen video layer.
> 

So, I'll replace this with an array of PlatformLayerIDs

> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.cpp:73
> > +    m_expectVideoLayerUnparentedTransaction = true;
> 
> This is confusing. Didn't you just get it? Or are you waiting for the next unparenting?
> 
> Might be clearer with a state machine with enum states.

We get the ID before it is unparented to make sure it doesn't go away and so we will be ready to look for the transaction that unparents it. Why I have now isn't that great. I have a bug to make it more explicit:

https://bugs.webkit.org/show_bug.cgi?id=128844

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:49
> > +    void enterFullscreen() override;
> 
> virtual
> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:55
> > +    virtual void setVideoLayerID(WebCore::GraphicsLayer::PlatformLayerID);
> 
> override?

This is not an override. This function comes from messages.in, it isn't in the other interfaces.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:208
> > +    void buildTransaction(RemoteLayerTreeTransaction&);
> 
> willCommitLayerTree
> 
> > Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemoteCustom.mm:68
> > +    [m_platformLayer setValue:[NSValue valueWithPointer:nullptr] forKey:platformCALayerPointer];
> 
> setValue:nil
> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:60
> > +    void buildTransaction(RemoteLayerTreeTransaction&);
> 
> willCommitLayerTree
Comment 5 Radar WebKit Bug Importer 2014-02-16 15:32:47 PST
<rdar://problem/16080367>
Comment 6 Jeremy Jones 2014-02-19 15:04:11 PST
Created attachment 224684 [details]
Patch
Comment 7 Simon Fraser (smfr) 2014-02-19 15:17:38 PST
Comment on attachment 224684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224684&action=review

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:305
> +    else if (videoLayer)
> +    {

Brace on previous line.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> +    Vector<WebCore::GraphicsLayer::PlatformLayerID> m_unparentedVideoLayerIDs;

This could be a HashSet.

It's not clear why the "unparented" is important here. Maybe isVideoLayersPendingFullscreen or something?

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:405
> +    for (auto layerID : result.m_unparentedVideoLayerIDs) {
> +        if (!layerID)
> +            return false;
> +    }

Not sure why you need this check.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.cpp:71
> +    RemoteLayerTreeDrawingAreaProxy* remoteDrawingAreaProxy = toRemoteLayerTreeDrawingAreaProxy(drawingAreaProxy);

toRemoteLayerTreeDrawingAreaProxy(m_page->drawingArea()) and remove the previous line.

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:67
> +    bool m_enterFullscreenAfterVideoLayerUnparentedTransaction;
> +    WebCore::GraphicsLayer::PlatformLayerID m_videoLayerID;

Both these should be initialized in the constructor.

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:81
> +    RefPtr<WebCore::PlatformCALayer> m_platformCALayer;
> +    bool m_sendUnparentVideoLayerTransaction;

Also need initializing in the constructor.
Comment 8 Jeremy Jones 2014-02-21 11:38:55 PST
Created attachment 224885 [details]
Patch
Comment 9 Simon Fraser (smfr) 2014-02-21 11:45:44 PST
Comment on attachment 224885 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224885&action=review

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:212
> +@property (nonatomic, readwrite, getter = isReadyForDisplay) BOOL readyForDisplay;
> +@property (nonatomic, readwrite) CGRect videoRect;

No need to specify readwrite

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> +    Vector<WebCore::GraphicsLayer::PlatformLayerID> m_videoLayerIDsPendingFullscreen;

HashSet?

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemoteCustom.mm:68
> +    [m_platformLayer setValue:[NSValue valueWithPointer:nil] forKey:platformCALayerPointer];

setValue:nil?

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.cpp:114
> +    m_platformCALayer = PlatformCALayer::platformCALayer(videoLayer);

Should this use ::create()?
Comment 10 Jeremy Jones 2014-02-21 11:49:04 PST
(In reply to comment #7)
> (From update of attachment 224684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224684&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:305
> > +    else if (videoLayer)
> > +    {
> 
> Brace on previous line.

Done

> 
> > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> > +    Vector<WebCore::GraphicsLayer::PlatformLayerID> m_unparentedVideoLayerIDs;
> 
> This could be a HashSet.

Agreed. But, there is no encoder/decoder for a HashSet, and since we expect this to have 0 or 1 entries in the common case, I don't expect HashSet to provide a speed advantage, and it might have a space disadvantage.

I'll make the change or write a bug if you think it is worth it.

> 
> It's not clear why the "unparented" is important here. Maybe isVideoLayersPendingFullscreen or something?

How about: 
isVideoLayerIDPendingFullscreen()
addVideoLayerIDPendingFullscreen()
m_videoLayerIDsPendingFullscreen

> 
> > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:405
> > +    for (auto layerID : result.m_unparentedVideoLayerIDs) {
> > +        if (!layerID)
> > +            return false;
> > +    }
> 
> Not sure why you need this check.

Removed. I was following the pattern used by the other arrays of IDs.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.cpp:71
> > +    RemoteLayerTreeDrawingAreaProxy* remoteDrawingAreaProxy = toRemoteLayerTreeDrawingAreaProxy(drawingAreaProxy);
> 
> toRemoteLayerTreeDrawingAreaProxy(m_page->drawingArea()) and remove the previous line.

Done.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:67
> > +    bool m_enterFullscreenAfterVideoLayerUnparentedTransaction;
> > +    WebCore::GraphicsLayer::PlatformLayerID m_videoLayerID;
> 
> Both these should be initialized in the constructor.

Done.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:81
> > +    RefPtr<WebCore::PlatformCALayer> m_platformCALayer;
> > +    bool m_sendUnparentVideoLayerTransaction;
> 
> Also need initializing in the constructor.

m_sendUnparentVideoLayerTransaction : done.
m_platformCALayer : I expect this to be default constructed to nullptr.
Comment 11 Jeremy Jones 2014-02-21 12:13:40 PST
(In reply to comment #9)
> (From update of attachment 224885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224885&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:212
> > +@property (nonatomic, readwrite, getter = isReadyForDisplay) BOOL readyForDisplay;
> > +@property (nonatomic, readwrite) CGRect videoRect;
> 
> No need to specify readwrite

Done.

> 
> > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:190
> > +    Vector<WebCore::GraphicsLayer::PlatformLayerID> m_videoLayerIDsPendingFullscreen;
> 
> HashSet?

See previous comment.

> 
> > Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemoteCustom.mm:68
> > +    [m_platformLayer setValue:[NSValue valueWithPointer:nil] forKey:platformCALayerPointer];
> 
> setValue:nil?

Done. This follows the pattern in PlatformCALayerMac. Should I update that code too?

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.cpp:114
> > +    m_platformCALayer = PlatformCALayer::platformCALayer(videoLayer);
> 
> Should this use ::create()?

This is not a create. This is getting the associated object. 

i.e.
    platformCALayer = static_cast<PlatformCALayer*>([[static_cast<CALayer*>(platformLayer) valueForKey:platformCALayerPointer] pointerValue]);
Comment 12 Jeremy Jones 2014-02-21 12:17:29 PST
Created attachment 224889 [details]
Patch
Comment 13 Jeremy Jones 2014-02-24 12:21:34 PST
Adding #if PLATFORM(COCOA) to fix gtk and elf builds.
Comment 14 Jeremy Jones 2014-02-24 12:23:24 PST
Created attachment 225081 [details]
Patch
Comment 15 Simon Fraser (smfr) 2014-02-24 16:03:41 PST
Comment on attachment 225081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225081&action=review

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:81
> +    bool m_sendUnparentVideoLayerTransaction;

Hard to parse. And you don't really send a transaction, you set a flag as part of one.
Comment 16 WebKit Commit Bot 2014-02-24 16:34:41 PST
Comment on attachment 225081 [details]
Patch

Clearing flags on attachment: 225081

Committed r164617: <http://trac.webkit.org/changeset/164617>
Comment 17 WebKit Commit Bot 2014-02-24 16:34:45 PST
All reviewed patches have been landed.  Closing bug.