A follow-up task of webkit.org/b/206043.
<rdar://problem/58505617>
Created attachment 390594 [details] Patch
Created attachment 390599 [details] Rebased patch
Comment on attachment 390594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390594&action=review > Source/WebCore/ChangeLog:8 > + Add an interface to MediaPlayer to set the frame of a video in the inline mode. "set the frame" is ambiguous when used in the context of video. > Source/WebCore/platform/graphics/MediaPlayer.h:332 > + void setVideoInlineFrame(const FloatRect&); Is this really a frame, or just a size? Does it ever have non-zero origin? Maybe call it setSize which would be much less confusing. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:470 > + // Initially the size of the platformLayer will be 0 because we do not provide mediaPlayerContentBoxRect() in this class. A size will be 0x0 > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:473 > + // Since the renderer in the Web process will take care of the video layers configurations, video layer's configuration > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:474 > + // the RemoteMediaPlayerProxy does not need to manage the layers by itself. This is a rather long-winded comment. Maybe prune it down to the minimum that will help a future you remember why the code is this way. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:515 > +void RemoteMediaPlayerProxy::setVideoInlineFrameFenced(const FloatRect bounds, IPC::Attachment fencePort) const FloatRect& or FloatRect but not const FloatRect > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:521 > + m_player->setVideoInlineFrame(bounds); "bounds" and "frame" have different meanings in AppKit/UIKit. "frame" is in the coordinate space of the superview. "bounds" establishes your local coordinate system Don't mix and match them. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:112 > + void setVideoInlineFrameFenced(const WebCore::FloatRect bounds, IPC::Attachment); Please use MachPort, not IPC::Attachment (existing code also needs to be migrated). > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:60 > + SetVideoInlineFrameFenced(WebCore::FloatRect bounds, IPC::Attachment fencePort) MachPort > Source/WebKit/WebKit.xcodeproj/project.pbxproj:2685 > + 1D32F89B23A84BA600B1EA6A /* RemoteMediaResourceProxy.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RemoteMediaResourceProxy.h; sourceTree = "<group>"; tabWidth = 4; }; > + 1D32F89D23A84C5B00B1EA6A /* RemoteMediaResourceProxy.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RemoteMediaResourceProxy.cpp; sourceTree = "<group>"; tabWidth = 4; }; Why tabWidth > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1009 > +void MediaPlayerPrivateRemote::setVideoInlineFrameFenced(const FloatRect& rect, mach_port_name_t fencePort) MachPort > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:123 > + void setVideoInlineFrameFenced(const WebCore::FloatRect&, mach_port_name_t); MachPort > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:233 > + void setSize(const WebCore::IntSize&) final { }; No semicolon > Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:38 > +using LayerHostingContextID = uint32_t; Pull in LayerHostingContext.h? > Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:40 > +RetainPtr<CALayer> createVideoLayerRemote(MediaPlayerPrivateRemote*, LayerHostingContextID); This is cocoa code inside ENABLE(GPU_PROCESS). I think we should assume that other platforms will want to build with ENABLE(GPU_PROCESS), so you need #if PLATFORM(COCOA) as well. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:34 > +@interface WKVideoLayerRemote : CALayer PLATFORM(COCOA) > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:36 > +@property (nonatomic, retain) CALayer *videoSublayer; Layers retain their sublayers already. Does this need to be retain? > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:37 > +@property CGRect videoLayerFrame; nonatomic? > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:100 > + [self performSelector:@selector(resolveBounds) withObject:nil afterDelay:animationDuration + 0.1]; Noooooo. Source of test flakiness for decades to come! Also please don't use performSelector:withObject:afterDelay:. Use an explicit timer, or dispatch with a block. Then you don't need all the cancelPreviousPerformRequestsWithTarget. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:121 > + _context = nil; Why is the context nulled out here? > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123 > + mach_port_name_t fencePort = _fenceSendRight.leakSendRight(); > + mediaPlayerPrivateRemote->setVideoInlineFrameFenced(self.videoLayerFrame, fencePort); MachPort cleans this up.
Created attachment 390621 [details] WIP patch
Created attachment 390662 [details] WIP patch - fixing build errors
Created attachment 390674 [details] WIP patch - fixing iOS build failures
Created attachment 390716 [details] WIP patch - updated according to review comments
Comment on attachment 390594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390594&action=review >> Source/WebCore/ChangeLog:8 >> + Add an interface to MediaPlayer to set the frame of a video in the inline mode. > > "set the frame" is ambiguous when used in the context of video. Right, actually what we need to set is the size of the video container layer. Will update the comment. >> Source/WebCore/platform/graphics/MediaPlayer.h:332 >> + void setVideoInlineFrame(const FloatRect&); > > Is this really a frame, or just a size? Does it ever have non-zero origin? Maybe call it setSize which would be much less confusing. Just a size. There is a setSize() function in MediaPlayer class. In the new patch, we use its implementation in MediaPlayerPrivate to set the video container layer size when the media in GPU process feature is on (video layers are hosted remotely). For the case that the media in GPU process feature is off, that function should do nothing. So we have to add a flag to the MediaPlayerPrivate class to decide which behavior should choose. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:470 >> + // Initially the size of the platformLayer will be 0 because we do not provide mediaPlayerContentBoxRect() in this class. > > A size will be 0x0 Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:473 >> + // Since the renderer in the Web process will take care of the video layers configurations, > > video layer's configuration Removed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:474 >> + // the RemoteMediaPlayerProxy does not need to manage the layers by itself. > > This is a rather long-winded comment. Maybe prune it down to the minimum that will help a future you remember why the code is this way. Removed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:515 >> +void RemoteMediaPlayerProxy::setVideoInlineFrameFenced(const FloatRect bounds, IPC::Attachment fencePort) > > const FloatRect& or FloatRect but not const FloatRect Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:521 >> + m_player->setVideoInlineFrame(bounds); > > "bounds" and "frame" have different meanings in AppKit/UIKit. "frame" is in the coordinate space of the superview. "bounds" establishes your local coordinate system Don't mix and match them. Right, fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:112 >> + void setVideoInlineFrameFenced(const WebCore::FloatRect bounds, IPC::Attachment); > > Please use MachPort, not IPC::Attachment (existing code also needs to be migrated). Fixed. Will migrate the exiting code on the video fullscreen API in webkit.org/b/207683. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:60 >> + SetVideoInlineFrameFenced(WebCore::FloatRect bounds, IPC::Attachment fencePort) > > MachPort Fixed. >> Source/WebKit/WebKit.xcodeproj/project.pbxproj:2685 >> + 1D32F89D23A84C5B00B1EA6A /* RemoteMediaResourceProxy.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RemoteMediaResourceProxy.cpp; sourceTree = "<group>"; tabWidth = 4; }; > > Why tabWidth Because of some misunderstanding :-) Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1009 >> +void MediaPlayerPrivateRemote::setVideoInlineFrameFenced(const FloatRect& rect, mach_port_name_t fencePort) > > MachPort Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:123 >> + void setVideoInlineFrameFenced(const WebCore::FloatRect&, mach_port_name_t); > > MachPort Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:233 >> + void setSize(const WebCore::IntSize&) final { }; > > No semicolon Fixed. >> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:38 >> +using LayerHostingContextID = uint32_t; > > Pull in LayerHostingContext.h? Right. Added the header file. >> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:40 >> +RetainPtr<CALayer> createVideoLayerRemote(MediaPlayerPrivateRemote*, LayerHostingContextID); > > This is cocoa code inside ENABLE(GPU_PROCESS). I think we should assume that other platforms will want to build with ENABLE(GPU_PROCESS), so you need #if PLATFORM(COCOA) as well. Right, fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:34 >> +@interface WKVideoLayerRemote : CALayer > > PLATFORM(COCOA) Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:36 >> +@property (nonatomic, retain) CALayer *videoSublayer; > > Layers retain their sublayers already. Does this need to be retain? Not need to be retain. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:37 >> +@property CGRect videoLayerFrame; > > nonatomic? Right. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:100 >> + [self performSelector:@selector(resolveBounds) withObject:nil afterDelay:animationDuration + 0.1]; > > Noooooo. Source of test flakiness for decades to come! > > Also please don't use performSelector:withObject:afterDelay:. Use an explicit timer, or dispatch with a block. Then you don't need all the cancelPreviousPerformRequestsWithTarget. Changed to dispatch_after(). >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:121 >> + _context = nil; > > Why is the context nulled out here? Removed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123 >> + mediaPlayerPrivateRemote->setVideoInlineFrameFenced(self.videoLayerFrame, fencePort); > > MachPort cleans this up. Done.
Created attachment 390727 [details] Patch
Comment on attachment 390727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390727&action=review > Source/WebCore/platform/graphics/MediaPlayer.h:331 > + void setHostVideoLayerRemotely(bool); setHostsVideoLayerRemotely here and everywhere. > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:326 > + bool hostVideoLayerRemotely() { return m_hostVideoLayerRemotely; } hostsVideoLayerRemotely > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378 > + bool m_hostVideoLayerRemotely { false }; m_hostsVideoLayerRemotely > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:520 > + deallocateSendRightSafely(machPort.port()); I think you should be using MachSendRight so you don't have to call this manually. Tim? Maybe we meant to say use MachSendRight rather than MachPort. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:109 > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock); What is this 100ms for?
Comment on attachment 390727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390727&action=review >> Source/WebCore/platform/graphics/MediaPlayer.h:331 >> + void setHostVideoLayerRemotely(bool); > > setHostsVideoLayerRemotely here and everywhere. OK, will fix it. >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:326 >> + bool hostVideoLayerRemotely() { return m_hostVideoLayerRemotely; } > > hostsVideoLayerRemotely ditto. >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378 >> + bool m_hostVideoLayerRemotely { false }; > > m_hostsVideoLayerRemotely ditto. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:520 >> + deallocateSendRightSafely(machPort.port()); > > I think you should be using MachSendRight so you don't have to call this manually. Tim? Maybe we meant to say use MachSendRight rather than MachPort. Good idea. Will update the patch to use MachSendRight. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:109 >> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, animationDuration + 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock); > > What is this 100ms for? To avoid excessive XPC messages when a user change the video element size continuously.
Created attachment 390854 [details] Revised patch based on review comments
(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 390727 [details] > Patch > > > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:520 > > + deallocateSendRightSafely(machPort.port()); > > I think you should be using MachSendRight so you don't have to call this > manually. Tim? Maybe we meant to say use MachSendRight rather than MachPort. Whoops, yes.
Comment on attachment 390854 [details] Revised patch based on review comments View in context: https://bugs.webkit.org/attachment.cgi?id=390854&action=review > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378 > + bool m_hostsVideoLayerRemotely { false }; WebCore shouldn't care whether things are remotely hosted. This should probably be handled via an abstraction.
Comment on attachment 390854 [details] Revised patch based on review comments View in context: https://bugs.webkit.org/attachment.cgi?id=390854&action=review >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:378 >> + bool m_hostsVideoLayerRemotely { false }; > > WebCore shouldn't care whether things are remotely hosted. This should probably be handled via an abstraction. Agree. Your comment reminds me that maybe we can avoid this flag by modifying the RenderVideo implementation. I will upload a WIP patch for review.
Created attachment 391015 [details] WIP patch: RenderVideo only calls mediaPlayer->setSize() when supportsAcceleratedRendering is false
Created attachment 391028 [details] WIP patch: RenderVideo only calls mediaPlayer->setSize() when supportsAcceleratedRendering is false
Comment on attachment 391028 [details] WIP patch: RenderVideo only calls mediaPlayer->setSize() when supportsAcceleratedRendering is false View in context: https://bugs.webkit.org/attachment.cgi?id=391028&action=review > Source/WebCore/rendering/RenderVideo.cpp:261 > + if (!mediaPlayer->supportsAcceleratedRendering()) { Simon, what do you think about this change? I think it's correct for iOS/Mac ports, and the layout tests for other ports pass. With this change, we do not need the hostsVideoLayerRemotely flag.
Created attachment 391090 [details] A simplified patch
Created attachment 391094 [details] Rebased patch (with a simplified implementation)
Comment on attachment 391094 [details] Rebased patch (with a simplified implementation) View in context: https://bugs.webkit.org/attachment.cgi?id=391094&action=review > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:106 > + _pendingResolveBoundsBlock = dispatch_block_create((dispatch_block_flags_t)0, ^{ > + _pendingResolveBoundsBlock = nil; > + [self resolveBounds]; This block is capturing |self|, so I think you have a retain cycle here?
Created attachment 391104 [details] Fix the retain cycle problem
Created attachment 391111 [details] Fix the retain cycle problem (with BlockPtr)
Comment on attachment 391111 [details] Fix the retain cycle problem (with BlockPtr) View in context: https://bugs.webkit.org/attachment.cgi?id=391111&action=review > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105 > + _pendingResolveBoundsBlock = makeBlockPtr(dispatch_block_create((dispatch_block_flags_t)0, [self] { > + _pendingResolveBoundsBlock = nil; > + [self resolveBounds]; > + })); Pretty sure this is still a retain cycle. self is retained by the block.
Comment on attachment 391111 [details] Fix the retain cycle problem (with BlockPtr) View in context: https://bugs.webkit.org/attachment.cgi?id=391111&action=review >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105 >> + })); > > Pretty sure this is still a retain cycle. self is retained by the block. OK it doesn't, but we prefer weak ptrs so you can remove -dealloc. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:106 > + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (animationDuration + 0.1) * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock.get()); I really want a comment that justifies this 100ms timer. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:145 > + [videoLayerRemote setName:@"WKVideoLayerRemote"]; Maybe just do this always.
(In reply to Simon Fraser (smfr) from comment #26) > Comment on attachment 391111 [details] > Fix the retain cycle problem (with BlockPtr) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391111&action=review > > >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105 > >> + })); > > > > Pretty sure this is still a retain cycle. self is retained by the block. > > OK it doesn't, but we prefer weak ptrs so you can remove -dealloc. I suggested WeakObjCPtr<> but you can also just use __weak :D
Comment on attachment 391111 [details] Fix the retain cycle problem (with BlockPtr) View in context: https://bugs.webkit.org/attachment.cgi?id=391111&action=review >>>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:105 >>>> + })); >>> >>> Pretty sure this is still a retain cycle. self is retained by the block. >> >> OK it doesn't, but we prefer weak ptrs so you can remove -dealloc. > > I suggested WeakObjCPtr<> but you can also just use __weak :D Got it. I will use WeakObjcPtr<> here and remove -dealloc. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:106 >> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (animationDuration + 0.1) * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingResolveBoundsBlock.get()); > > I really want a comment that justifies this 100ms timer. Will add some comment in the new patch. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:145 >> + [videoLayerRemote setName:@"WKVideoLayerRemote"]; > > Maybe just do this always. Sure.
Created attachment 391122 [details] Revise the patch based on review comments
Created attachment 391123 [details] Rebase the patch
Created attachment 391129 [details] Fix a unified build failure on iOS
Comment on attachment 391129 [details] Fix a unified build failure on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=391129&action=review > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:97 > + auto _self = weakSelf.get(); Nit - Local variables usually aren’t prefixed with _. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:101 > + _self.get()->_pendingResolveBoundsBlock = nil; Nit - no need for the .get() > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:143 > + WKVideoLayerRemote *videoLayerRemote = [[WKVideoLayerRemote alloc] init]; `auto videoLayerRemote = adoptNS([[WKVideoLayerRemote alloc] init]);` here, otherwise it’ll leak.
Comment on attachment 391129 [details] Fix a unified build failure on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=391129&action=review > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:43 > + CALayer* _videoSublayer; > + CAContext* _context; Nit - * on the other side. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:124 > + MachSendRight _fenceSendRight = MachSendRight::adopt([_context createFencePort]); Nit - `auto fenceSendRight = MachSendRight::adopt([_context createFencePort]);` (no underscore)
Created attachment 391177 [details] Update the patch based on Wenson's comments
Comment on attachment 391129 [details] Fix a unified build failure on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=391129&action=review >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:43 >> + CAContext* _context; > > Nit - * on the other side. Right, fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:97 >> + auto _self = weakSelf.get(); > > Nit - Local variables usually aren’t prefixed with _. Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:101 >> + _self.get()->_pendingResolveBoundsBlock = nil; > > Nit - no need for the .get() Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:124 >> + MachSendRight _fenceSendRight = MachSendRight::adopt([_context createFencePort]); > > Nit - `auto fenceSendRight = MachSendRight::adopt([_context createFencePort]);` (no underscore) Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:143 >> + WKVideoLayerRemote *videoLayerRemote = [[WKVideoLayerRemote alloc] init]; > > `auto videoLayerRemote = adoptNS([[WKVideoLayerRemote alloc] init]);` here, otherwise it’ll leak. Got it! Fixed. Thanks!
Comment on attachment 391177 [details] Update the patch based on Wenson's comments View in context: https://bugs.webkit.org/attachment.cgi?id=391177&action=review > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:56 > +- (WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote Nit - * on the wrong side. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:61 > +- (void)setMediaPlayerPrivateRemote:(WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote (Here too) > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:96 > + _pendingResolveBoundsBlock = makeBlockPtr(dispatch_block_create((dispatch_block_flags_t)0, [weakSelf = WeakObjCPtr<WKVideoLayerRemote>(self)] { I think this would still leak, since dispatch_block_create() returns a +1 object and makeBlockPtr then `Block_copy`s it. I think you want either just ^{} or a C++ lambda ([] {}) that will be automatically converted into an Objective-C block. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123 > + if (auto *mediaPlayerPrivateRemote = self.mediaPlayerPrivateRemote) { Nit - * on the wrong side. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:144 > + [videoLayerRemote setFrame:CGRectMake(0, 0, 0, 0)]; Nit - I think you can remove this line, since the frame (by default) is already NSZeroRect.
Otherwise, this looks reasonable to me (though I’m not familiar with media code, so one of the media folks should probably give the final r+)
Comment on attachment 391177 [details] Update the patch based on Wenson's comments View in context: https://bugs.webkit.org/attachment.cgi?id=391177&action=review >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:56 >> +- (WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote > > Nit - * on the wrong side. Should be like below? - (WebKit::MediaPlayerPrivateRemote*)mediaPlayerPrivateRemote >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:123 >> + if (auto *mediaPlayerPrivateRemote = self.mediaPlayerPrivateRemote) { > > Nit - * on the wrong side. Got it, will fix it. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:144 >> + [videoLayerRemote setFrame:CGRectMake(0, 0, 0, 0)]; > > Nit - I think you can remove this line, since the frame (by default) is already NSZeroRect. Right. Will remove it. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:154 > + [videoSublayer setFrame:videoLayerRemote.get().bounds]; Can be removed as well.
Comment on attachment 391177 [details] Update the patch based on Wenson's comments View in context: https://bugs.webkit.org/attachment.cgi?id=391177&action=review >>> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:56 >>> +- (WebKit::MediaPlayerPrivateRemote *)mediaPlayerPrivateRemote >> >> Nit - * on the wrong side. > > Should be like below? > - (WebKit::MediaPlayerPrivateRemote*)mediaPlayerPrivateRemote Yep! Since WebKit::MediaPlayerPrivateRemote is a C++ class, as opposed to an Objective-C class.
Created attachment 391256 [details] A timer based implementation
Created attachment 391257 [details] A timer based implementation (with the fix for the style checking error)
Comment on attachment 391257 [details] A timer based implementation (with the fix for the style checking error) View in context: https://bugs.webkit.org/attachment.cgi?id=391257&action=review > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:685 > - notImplemented(); > + // We always render to a layer, so this function is empty. No need to land this in this patch. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:690 > - notImplemented(); > + // We always render to a layer, so this function is empty. Ditto. > Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:28 > +#if ENABLE(GPU_PROCESS) && PLATFORM(COCOA) I don't think PLATFORM(COCOA) is needed here, if you just replace `CALayer` with `PlatformLayer`. The call site in MediaPlayerPrivateRemote isn't guarded by a PLATFORM(COCOA) call. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:30 > +#import <pal/spi/cocoa/QuartzCoreSPI.h> Does this need to be in the header, or can this be moved to the implementation file? > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:38 > +@property (nonatomic, assign) WebKit::MediaPlayerPrivateRemote *mediaPlayerPrivateRemote; > +@property (nonatomic, assign) CALayer *videoSublayer; Why are these marked as "assign"? > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:42 > + CALayer *_videoSublayer; RetainPtr<>? > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:43 > + CAContext *_context; Ditto. > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:65 > + if (self) { > + self.masksToBounds = YES; > + _resolveBoundsTimer = makeUnique<WebCore::Timer>([weakSelf = WeakObjCPtr<WKVideoLayerRemote>(self)] { > + auto localSelf = weakSelf.get(); > + if (!localSelf) > + return; > + > + [localSelf resolveBounds]; > + }); > + _shouldRestartWhenTimerFires = false; > + } > + > + return self; I'd do an early return here: ``` if (!self) return nil; ```` If you want to be super fancy, you could do: ``` if (!(self = [super init])) return nil; ``` > Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:107 > + _delay = Seconds(animationDuration + 0.1); This (0.1) should be declared at the top of the file as `static const Seconds PostAnimationDelay { 100_ms }`, rather than just a magic number here.
Created attachment 391356 [details] Revise the patch according to Jer's comments
Comment on attachment 391257 [details] A timer based implementation (with the fix for the style checking error) View in context: https://bugs.webkit.org/attachment.cgi?id=391257&action=review >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:685 >> + // We always render to a layer, so this function is empty. > > No need to land this in this patch. Removed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:690 >> + // We always render to a layer, so this function is empty. > > Ditto. Removed. >> Source/WebKit/WebProcess/GPU/media/VideoLayerRemote.h:28 >> +#if ENABLE(GPU_PROCESS) && PLATFORM(COCOA) > > I don't think PLATFORM(COCOA) is needed here, if you just replace `CALayer` with `PlatformLayer`. The call site in MediaPlayerPrivateRemote isn't guarded by a PLATFORM(COCOA) call. Right. Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:30 >> +#import <pal/spi/cocoa/QuartzCoreSPI.h> > > Does this need to be in the header, or can this be moved to the implementation file? Moved to the implementation file. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.h:38 >> +@property (nonatomic, assign) CALayer *videoSublayer; > > Why are these marked as "assign"? We do not need to keep a pointer to the videoSublayer because it will be the first sublayer of WKVideoLayerRemote. So it is removed in the revised patch. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:42 >> + CALayer *_videoSublayer; > > RetainPtr<>? Removed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:43 >> + CAContext *_context; > > Ditto. Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:65 >> + return self; > > I'd do an early return here: > > ``` > if (!self) > return nil; > ```` > > If you want to be super fancy, you could do: > > ``` > if (!(self = [super init])) > return nil; > ``` Fixed. >> Source/WebKit/WebProcess/GPU/media/cocoa/VideoLayerRemoteCocoa.mm:107 >> + _delay = Seconds(animationDuration + 0.1); > > This (0.1) should be declared at the top of the file as `static const Seconds PostAnimationDelay { 100_ms }`, rather than just a magic number here. Fixed.
The commit-queue encountered the following flaky tests while processing attachment 391356 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 391356 [details] Revise the patch according to Jer's comments Clearing flags on attachment: 391356 Committed r257127: <https://trac.webkit.org/changeset/257127>
All reviewed patches have been landed. Closing bug.
Reopen this bug to fix a unified build issue.
Created attachment 391408 [details] A follow-up patch to fix Catalyst/watchOS/tvOS build failures
Comment on attachment 391408 [details] A follow-up patch to fix Catalyst/watchOS/tvOS build failures Clearing flags on attachment: 391408 Committed r257153: <https://trac.webkit.org/changeset/257153>