Bug 128564 – WK2 AVKit fullscreen doesn't display video.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Need to find a way to get the video PlatformLayer from WebPageProxy.

[Jeremy Jones]

Created attachment 224244 [details]
Patch

[Jeremy Jones]

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?

[Simon Fraser (smfr)]

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

[Jeremy Jones]

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

[Radar WebKit Bug Importer]

<rdar://problem/16080367>

[Jeremy Jones]

Created attachment 224684 [details]
Patch

[Simon Fraser (smfr)]

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.

[Jeremy Jones]

Created attachment 224885 [details]
Patch

[Simon Fraser (smfr)]

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()?

[Jeremy Jones]

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

[Jeremy Jones]

(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]);

[Jeremy Jones]

Created attachment 224889 [details]
Patch

[Jeremy Jones]

Adding #if PLATFORM(COCOA) to fix gtk and elf builds.

[Jeremy Jones]

Created attachment 225081 [details]
Patch

[Simon Fraser (smfr)]

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.

[WebKit Commit Bot]

Comment on attachment 225081 [details]
Patch

Clearing flags on attachment: 225081

Committed r164617: <http://trac.webkit.org/changeset/164617>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.