Bug 216995 – [Media in GPU Process] Use VideoLayerManager to manage layers of MediaPlayerPrivateRemote

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

[Peng Liu]

[Media in GPU Process] Use VideoLayerManager to manage layers of MediaPlayerPrivateRemote

[Peng Liu]

Created attachment 409740 [details]
WIP patch

[Radar WebKit Bug Importer]

<rdar://problem/69710141>

[Peng Liu]

Created attachment 409911 [details]
Fix build failures on GTK and WPE ports

[Peng Liu]

Created attachment 409914 [details]
Fix build failures on the GTK port again

[Peng Liu]

Created attachment 409917 [details]
Fix build failures on the GTK and WPE ports

[Peng Liu]

Created attachment 409918 [details]
Fix build failures on the GTK and WPE ports

[Peng Liu]

Created attachment 409925 [details]
Patch

[Eric Carlson]

Comment on attachment 409925 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:137
> +#if PLATFORM(COCOA)

`setVideoLayer` is pure-virtual and is not Cocoa-only, so would get rid of the compile flag and let other ports just ignore the call.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:138
> +        IntSize defaultSize = snappedIntRect(m_player->playerContentBoxRect()).size();

Nit: this variable isn't necessary

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:159
> +    RefPtr<const Logger> m_logger;

Can you make this be a Ref <> since it looks like it can never be NULL?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:363
>      RefPtr<WebCore::PlatformMediaResourceLoader> m_mediaResourceLoader;

Ditto.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:365
> +    std::unique_ptr<WebCore::VideoLayerManager> m_videoLayerManager;

Can you make this be a UniqueRef<> since it looks like it can never be NULL?

[Peng Liu]

Created attachment 409945 [details]
Patch for landing

[Peng Liu]

Comment on attachment 409925 [details]
Patch

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

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:137
>> +#if PLATFORM(COCOA)
> 
> `setVideoLayer` is pure-virtual and is not Cocoa-only, so would get rid of the compile flag and let other ports just ignore the call.

Right! Fixed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:138
>> +        IntSize defaultSize = snappedIntRect(m_player->playerContentBoxRect()).size();
> 
> Nit: this variable isn't necessary

Correct. Fixed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:159
>> +    RefPtr<const Logger> m_logger;
> 
> Can you make this be a Ref <> since it looks like it can never be NULL?

Yes. Fixed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:363
>>      RefPtr<WebCore::PlatformMediaResourceLoader> m_mediaResourceLoader;
> 
> Ditto.

Fixed.
And filed bug 217074 to follow up.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:365
>> +    std::unique_ptr<WebCore::VideoLayerManager> m_videoLayerManager;
> 
> Can you make this be a UniqueRef<> since it looks like it can never be NULL?

Fixed.

[Peng Liu]

Created attachment 409947 [details]
Patch for landing

[Peng Liu]

Created attachment 409950 [details]
Patch for landing

[Simon Fraser (smfr)]

Comment on attachment 409950 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/VideoLayerManager.h:37
> +class VideoLayerManager {

Is this managing the layer for a single video, or all videos? I Don't think Manager is a good name here.

> Source/WebCore/platform/graphics/avfoundation/objc/VideoLayerManagerObjC.h:41
> +class VideoLayerManagerObjC final

We usually call these subclasses "Cocoa", not "ObjC". It's not about the language it's written in, it's about the platform.

[Peng Liu]

Comment on attachment 409925 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:137
>>> +#if PLATFORM(COCOA)
>> 
>> `setVideoLayer` is pure-virtual and is not Cocoa-only, so would get rid of the compile flag and let other ports just ignore the call.
> 
> Right! Fixed.

Oops, I am afraid we have to keep the compile flag for now because only Cocoa platforms have VideoLayerManager.

[Peng Liu]

Comment on attachment 409950 [details]
Patch for landing

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

>> Source/WebCore/platform/graphics/VideoLayerManager.h:37
>> +class VideoLayerManager {
> 
> Is this managing the layer for a single video, or all videos? I Don't think Manager is a good name here.

It manages layers for a video element (video content layer, inline video container layer, fullscreen video container layer, and text track representation layer).

>> Source/WebCore/platform/graphics/avfoundation/objc/VideoLayerManagerObjC.h:41
>> +class VideoLayerManagerObjC final
> 
> We usually call these subclasses "Cocoa", not "ObjC". It's not about the language it's written in, it's about the platform.

Got it! Let me fix the naming in a follow-up patch.

[EWS]

Committed r267725: <https://trac.webkit.org/changeset/267725>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409950 [details].

[Peng Liu]

Reopen this bug because of a build failure.

[Peng Liu]

Created attachment 410007 [details]
A follow-up patch to fix a build failure

[zalan]

Committed r267741: <https://trac.webkit.org/changeset/267741>

[Peng Liu]

Reopening to attach new patch.

[Peng Liu]

Created attachment 410027 [details]
A follow-up patch to remove WEBCORE_EXPORT of the VideoLayerManagerObjC constructor

[EWS]

Committed r267769: <https://trac.webkit.org/changeset/267769>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410027 [details].