Summary: | WK2 AVKit fullscreen doesn't display video. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 128573 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Jeremy Jones
2014-02-10 16:23:43 PST
Created attachment 224244 [details]
Patch
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 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 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 Created attachment 224684 [details]
Patch
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. Created attachment 224885 [details]
Patch
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()? (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. (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]); Created attachment 224889 [details]
Patch
Adding #if PLATFORM(COCOA) to fix gtk and elf builds. Created attachment 225081 [details]
Patch
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 on attachment 225081 [details] Patch Clearing flags on attachment: 225081 Committed r164617: <http://trac.webkit.org/changeset/164617> All reviewed patches have been landed. Closing bug. |