Bug 206132 – [Media in GPU process] Synchronize the properties of video layers in the GPU process with the hosting layer in the web process

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

A follow-up task of webkit.org/b/206043.

[Radar WebKit Bug Importer]

<rdar://problem/58505617>

[Peng Liu]

Created attachment 390594 [details]
Patch

[Peng Liu]

Created attachment 390599 [details]
Rebased patch

[Simon Fraser (smfr)]

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.

[Peng Liu]

Created attachment 390621 [details]
WIP patch

[Peng Liu]

Created attachment 390662 [details]
WIP patch - fixing build errors

[Peng Liu]

Created attachment 390674 [details]
WIP patch - fixing iOS build failures

[Peng Liu]

Created attachment 390716 [details]
WIP patch - updated according to review comments

[Peng Liu]

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.

[Peng Liu]

Created attachment 390727 [details]
Patch

[Simon Fraser (smfr)]

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?

[Peng Liu]

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.

[Peng Liu]

Created attachment 390854 [details]
Revised patch based on review comments

[Tim Horton]

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

[Sam Weinig]

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.

[Peng Liu]

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.

[Peng Liu]

Created attachment 391015 [details]
WIP patch: RenderVideo only calls mediaPlayer->setSize() when supportsAcceleratedRendering is false

[Peng Liu]

Created attachment 391028 [details]
WIP patch: RenderVideo only calls mediaPlayer->setSize() when supportsAcceleratedRendering is false

[Peng Liu]

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.

[Peng Liu]

Created attachment 391090 [details]
A simplified patch

[Peng Liu]

Created attachment 391094 [details]
Rebased patch (with a simplified implementation)

[Simon Fraser (smfr)]

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?

[Peng Liu]

Created attachment 391104 [details]
Fix the retain cycle problem

[Peng Liu]

Created attachment 391111 [details]
Fix the retain cycle problem (with BlockPtr)

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

[Tim Horton]

(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

[Peng Liu]

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.

[Peng Liu]

Created attachment 391122 [details]
Revise the patch based on review comments

[Peng Liu]

Created attachment 391123 [details]
Rebase the patch

[Peng Liu]

Created attachment 391129 [details]
Fix a unified build failure on iOS

[Wenson Hsieh]

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.

[Wenson Hsieh]

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)

[Peng Liu]

Created attachment 391177 [details]
Update the patch based on Wenson's comments

[Peng Liu]

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!

[Wenson Hsieh]

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.

[Wenson Hsieh]

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+)

[Peng Liu]

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.

[Wenson Hsieh]

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.

[Peng Liu]

Created attachment 391256 [details]
A timer based implementation

[Peng Liu]

Created attachment 391257 [details]
A timer based implementation (with the fix for the style checking error)

[Jer Noble]

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.

[Peng Liu]

Created attachment 391356 [details]
Revise the patch according to Jer's comments

[Peng Liu]

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.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 391356 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
fetch/fetch-worker-crash.html bug 187257 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Peng Liu]

Reopen this bug to fix a unified build issue.

[Peng Liu]

Created attachment 391408 [details]
A follow-up patch to fix Catalyst/watchOS/tvOS build failures

[Ryan Haddad]

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>

[Ryan Haddad]

All reviewed patches have been landed.  Closing bug.