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

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

Summary: [Media in GPU Process] Use VideoLayerManager to manage layers of MediaPlayerP...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Media (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Peng Liu

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2020-09-25 14:21 PDT by Peng Liu
Modified:	2020-09-29 16:25 PDT (History)
CC List:	13 users (show)

See Also:

Attachments
WIP patch (42.93 KB, patch) 2020-09-25 14:24 PDT, Peng Liu	no flags	Details \| Formatted Diff \| Diff
Fix build failures on GTK and WPE ports (47.86 KB, patch) 2020-09-28 11:59 PDT, Peng Liu	no flags	Details \| Formatted Diff \| Diff
Fix build failures on the GTK port again (49.56 KB, patch) 2020-09-28 13:37 PDT, Peng Liu	no flags	Details \| Formatted Diff \| Diff
Fix build failures on the GTK and WPE ports (49.72 KB, patch) 2020-09-28 13:50 PDT, Peng Liu	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Fix build failures on the GTK and WPE ports (49.71 KB, patch) 2020-09-28 13:58 PDT, Peng Liu	no flags	Details \| Formatted Diff \| Diff
Patch (51.30 KB, patch) 2020-09-28 15:54 PDT, Peng Liu	eric.carlson: review+	Details \| Formatted Diff \| Diff
Patch for landing (51.49 KB, patch) 2020-09-28 18:54 PDT, Peng Liu	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch for landing (51.49 KB, patch) 2020-09-28 19:08 PDT, Peng Liu	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Patch for landing (51.51 KB, patch) 2020-09-28 19:17 PDT, Peng Liu	no flags	Details \| Formatted Diff \| Diff
A follow-up patch to fix a build failure (2.24 KB, patch) 2020-09-29 09:03 PDT, Peng Liu	zalan: review+	Details \| Formatted Diff \| Diff
A follow-up patch to remove WEBCORE_EXPORT of the VideoLayerManagerObjC constructor (1.57 KB, patch) 2020-09-29 11:35 PDT, Peng Liu	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Peng Liu 2020-09-25 14:21:26 PDT

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

Comment 1 Peng Liu 2020-09-25 14:24:29 PDT

Created attachment 409740 [details]
WIP patch

Comment 2 Radar WebKit Bug Importer 2020-09-28 11:36:55 PDT

<rdar://problem/69710141>

Comment 3 Peng Liu 2020-09-28 11:59:46 PDT

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

Comment 4 Peng Liu 2020-09-28 13:37:43 PDT

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

Comment 5 Peng Liu 2020-09-28 13:50:42 PDT

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

Comment 6 Peng Liu 2020-09-28 13:58:53 PDT

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

Comment 7 Peng Liu 2020-09-28 15:54:07 PDT

Created attachment 409925 [details]
Patch

Comment 8 Eric Carlson 2020-09-28 16:47:28 PDT

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?

Comment 9 Peng Liu 2020-09-28 18:54:44 PDT

Created attachment 409945 [details]
Patch for landing

Comment 10 Peng Liu 2020-09-28 18:56:00 PDT

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.

Comment 11 Peng Liu 2020-09-28 19:08:00 PDT

Created attachment 409947 [details]
Patch for landing

Comment 12 Peng Liu 2020-09-28 19:17:53 PDT

Created attachment 409950 [details]
Patch for landing

Comment 13 Simon Fraser (smfr) 2020-09-28 20:57:29 PDT

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.

Comment 14 Peng Liu 2020-09-28 21:00:02 PDT

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.

Comment 15 Peng Liu 2020-09-28 21:07:19 PDT

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.

Comment 16 EWS 2020-09-28 21:52:59 PDT

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

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

Comment 17 Peng Liu 2020-09-29 08:59:38 PDT

Reopen this bug because of a build failure.

Comment 18 Peng Liu 2020-09-29 09:03:19 PDT

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

Comment 19 zalan 2020-09-29 09:34:58 PDT

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

Comment 20 Peng Liu 2020-09-29 11:35:10 PDT

Reopening to attach new patch.

Comment 21 Peng Liu 2020-09-29 11:35:11 PDT

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

Comment 22 EWS 2020-09-29 16:25:03 PDT

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

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