Bug 205094 – Add infrastructure needed for playing media player in the GPU process

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Add infrastructure needed for playing media player in the GPU process

[Radar WebKit Bug Importer]

<rdar://problem/57815393>

[Eric Carlson]

Created attachment 385328 [details]
WIP patch

[Eric Carlson]

Created attachment 385331 [details]
WIP patch

[Tim Horton]

Comment on attachment 385331 [details]
WIP patch

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

> Source/WebCore/platform/ContentType.h:52
> +#if ENABLE(GPU_PROCESS)

Not really any reason to restrict these encoders to only #if ENABLE(GPU_PROCESS). None of the rest have been.

> Source/WebCore/platform/graphics/MediaPlayerEngineIdentifiers.h:32
> +class MediaPlayerEngineIdentifiers {

Is there a reason for strings instead of an enum?

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:122
> +    bool sendSucceeded = gpuProcessConnection().sendSync(Messages::RemoteMediaPlayerManagerProxy::CreateMediaPlayer(id, remoteEngineIdentifier), Messages::RemoteMediaPlayerManagerProxy::CreateMediaPlayer::Reply(haveRemoteProxy), 0);

Ideally we would not be adding sync messages; see bug 204955 for the stress we're putting Said through :(

Is it possible to avoid this?

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:128
> +        remotePlayer->invalidate();

Did you mean to bail here, or do you really want to add it to m_players?

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:139
> +    if (!gpuProcessConnection().sendSync(Messages::RemoteMediaPlayerManagerProxy::GetSupportedTypes(remoteEngineIdentifier), Messages::RemoteMediaPlayerManagerProxy::GetSupportedTypes::Reply(types), 0))

Ditto

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:151
> +    // FIXME: convert WebCore::MediaPlayer::SupportsType to enum class so it can be encoded and sent via IPC

Turns out you don't actually have to `enum class`, you can just implement the EnumTraits even for normal enums. But obviously ideally do both.

IMO we should *start* there (I intentionally didn't land any DisplayListItem encoding with casts, doing all of the EnumTraitsing first).

[Eric Carlson]

Created attachment 385396 [details]
WIP patch

[Eric Carlson]

Created attachment 385410 [details]
WIP patch

[Eric Carlson]

Created attachment 385414 [details]
WIP patch

[Peng Liu]

Comment on attachment 385414 [details]
WIP patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:520
> +    if (!m_activeEngineIdentifier.isEmpty()) {

duplicated code?

> Source/WebCore/platform/graphics/MediaPlayerEngineIdentifiers.h:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

2019?

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2019?

[Peng Liu]

Comment on attachment 385414 [details]
WIP patch

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

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:65
> +void MediaPlayerPrivateRemote::MediaPlayerPrivateRemote::load(const String&)

MediaPlayerPrivateRemote::load(const String&)

[Eric Carlson]

Created attachment 385447 [details]
WIP patch

[Eric Carlson]

Created attachment 385462 [details]
WIP patch

[Eric Carlson]

Created attachment 385491 [details]
WIP patch

[Eric Carlson]

Created attachment 385498 [details]
WIP patch

[Eric Carlson]

Created attachment 385502 [details]
WIP patch

[Eric Carlson]

Created attachment 385509 [details]
WIP patch

[youenn fablet]

Comment on attachment 385509 [details]
WIP patch

A couple of nits below.
It would be nice to be able to remove the GPU players when their WebProcess counterparts get destroyed.

Given we have UseGPUProcessForMedia, maybe we could have a simple test that would instantiate a media player in GPU process and verify nothing is crashing/hanging?

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:207
> +static void addMediaEngine(std::unique_ptr<MediaPlayerFactory>);

std::unique_ptr<MediaPlayerFactory>&&

> Source/WebCore/platform/graphics/MediaPlayer.cpp:244
> +        if (registerRemoteEngine)

if (auto& registerRemoteEngine = registerRemotePlayerCallback())

> Source/WebCore/platform/graphics/MediaPlayer.cpp:398
> +

Two lines.

> Source/WebCore/platform/graphics/MediaPlayer.h:88
> +#if ENABLE(GPU_PROCESS)

We could remove this #if

> Source/WebCore/platform/graphics/MediaPlayer.h:683
> +    virtual ~MediaPlayerFactory() { }

Could use = default as well for consistency with constructor.

> Source/WebCore/platform/graphics/MediaPlayer.h:692
> +    virtual void clearMediaCacheForOrigins(const String&, const HashSet<RefPtr<SecurityOrigin>>&) const { }

Seems strange that clear would be const.

> Source/WebCore/platform/graphics/MediaPlayer.h:697
>  typedef void (*MediaEngineRegister)(MediaEngineRegistrar);

We could modernise with "using".

> Source/WebCore/platform/graphics/MediaPlayer.h:706
> +    using RegisterRemotePlayerCallback = std::function<void(MediaEngineRegistrar, MediaPlayerEnums::MediaEngineIdentifier)>;

Can we use a WTF::Function or CompletionHandler?

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:345
> +class MediaPlayerFactoryAVFoundationCF : public MediaPlayerFactory {

Could be made final.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:346
> +public:

Can we make them private?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:277
> +public:

Ditto final/private and with below classes.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:57
> +    , m_remoteMediaPlayerManagerProxy(makeUnique<RemoteMediaPlayerManagerProxy>(*this))

makeUniqueRef?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:84
> +        if (m_remoteMediaPlayerManagerProxy)

Might not be needed since we are creating it at constructor time.
Or we could lazily initialise it?

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:84
> +protected:

private?

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:67
> +    friend class PlayerProxy;

Do we need PlayerProxy to be a friend given it is inside RemoteMediaPlayerManagerProxy.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in:27
> +    CreateMediaPlayer(WebKit::MediaPlayerPrivateRemoteIdentifier id, enum:uint8_t WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier) -> (bool success) Synchronous

Do we need this one to be synchronous?
Is there a case where creation would fail? If so, could we just report the error asynchronously as any other asynchronous error might happen?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:63
> +    m_player = nullptr;

Probably not needed right now to null it.
Do we need to have a way at destruction time to call GPUProcess to state that it is gone?
That way, the GPU process player map could shrink.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:40
> +    , public MediaPlayerPrivateInterface

Do we need MediaPlayerPrivateRemote to inherit from MediaPlayerPrivateInterface since it is directly called from RemoteMediaPlayerManager?
Probably not a big deal given all virtual methods are final.

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:154
> +

Two lines

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:160
> +

Two lines

> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:184
> +        originData.append(origin->data());

Could use initialize/uncheckedAppend or WTF::map maybe.

[Eric Carlson]

Comment on attachment 385509 [details]
WIP patch

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

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:207
>> +static void addMediaEngine(std::unique_ptr<MediaPlayerFactory>);
> 
> std::unique_ptr<MediaPlayerFactory>&&

Fixed.

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:244
>> +        if (registerRemoteEngine)
> 
> if (auto& registerRemoteEngine = registerRemotePlayerCallback())

Fixed.

>> Source/WebCore/platform/graphics/MediaPlayer.cpp:398
>> +
> 
> Two lines.

Removed.

>> Source/WebCore/platform/graphics/MediaPlayer.h:88
>> +#if ENABLE(GPU_PROCESS)
> 
> We could remove this #if

Removed.

>> Source/WebCore/platform/graphics/MediaPlayer.h:683
>> +    virtual ~MediaPlayerFactory() { }
> 
> Could use = default as well for consistency with constructor.

Changed.

>> Source/WebCore/platform/graphics/MediaPlayer.h:692
>> +    virtual void clearMediaCacheForOrigins(const String&, const HashSet<RefPtr<SecurityOrigin>>&) const { }
> 
> Seems strange that clear would be const.

The method is on a const MediaPlayerFactory

>> Source/WebCore/platform/graphics/MediaPlayer.h:697
>>  typedef void (*MediaEngineRegister)(MediaEngineRegistrar);
> 
> We could modernise with "using".

Fixed.

>> Source/WebCore/platform/graphics/MediaPlayer.h:706
>> +    using RegisterRemotePlayerCallback = std::function<void(MediaEngineRegistrar, MediaPlayerEnums::MediaEngineIdentifier)>;
> 
> Can we use a WTF::Function or CompletionHandler?

Changed to CompletionHandler.

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:345
>> +class MediaPlayerFactoryAVFoundationCF : public MediaPlayerFactory {
> 
> Could be made final.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:346
>> +public:
> 
> Can we make them private?

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:277
>> +public:
> 
> Ditto final/private and with below classes.

Fixed.

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:84
>> +        if (m_remoteMediaPlayerManagerProxy)
> 
> Might not be needed since we are creating it at constructor time.
> Or we could lazily initialise it?

Changed to create when needed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:84
>> +protected:
> 
> private?

Fixed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:67
>> +    friend class PlayerProxy;
> 
> Do we need PlayerProxy to be a friend given it is inside RemoteMediaPlayerManagerProxy.

Changed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in:27
>> +    CreateMediaPlayer(WebKit::MediaPlayerPrivateRemoteIdentifier id, enum:uint8_t WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier) -> (bool success) Synchronous
> 
> Do we need this one to be synchronous?
> Is there a case where creation would fail? If so, could we just report the error asynchronously as any other asynchronous error might happen?

It doesn't need to be synchronous, but we'll have to do a bit of MediaPlayer refactoring so I'd prefer to do it in a followup since this patch. Filed https://bugs.webkit.org/show_bug.cgi?id=205197 for this work.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:63
>> +    m_player = nullptr;
> 
> Probably not needed right now to null it.
> Do we need to have a way at destruction time to call GPUProcess to state that it is gone?
> That way, the GPU process player map could shrink.

Yes, fixed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:40
>> +    , public MediaPlayerPrivateInterface
> 
> Do we need MediaPlayerPrivateRemote to inherit from MediaPlayerPrivateInterface since it is directly called from RemoteMediaPlayerManager?
> Probably not a big deal given all virtual methods are final.

It doesn't have to inherit, I'll look into changing that in an upcoming patch.

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:154
>> +
> 
> Two lines

Removed.

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:160
>> +
> 
> Two lines

Removed.

>> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:184
>> +        originData.append(origin->data());
> 
> Could use initialize/uncheckedAppend or WTF::map maybe.

Fixed.

[Eric Carlson]

Created attachment 385578 [details]
Patch

[youenn fablet]

> > Can we use a WTF::Function or CompletionHandler?

Actually a CompletionHandler is probably not great since it might be called multiple times.

[Eric Carlson]

Comment on attachment 385509 [details]
WIP patch

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

>>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:40
>>> +    , public MediaPlayerPrivateInterface
>> 
>> Do we need MediaPlayerPrivateRemote to inherit from MediaPlayerPrivateInterface since it is directly called from RemoteMediaPlayerManager?
>> Probably not a big deal given all virtual methods are final.
> 
> It doesn't have to inherit, I'll look into changing that in an upcoming patch.

Actually it does need to inherit because it is returned by the remote factory and called by MediaPlayer

[Eric Carlson]

Created attachment 385606 [details]
Patch

[youenn fablet]

(In reply to Eric Carlson from comment #20)
> Comment on attachment 385509 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385509&action=review
> 
> >>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:40
> >>> +    , public MediaPlayerPrivateInterface
> >> 
> >> Do we need MediaPlayerPrivateRemote to inherit from MediaPlayerPrivateInterface since it is directly called from RemoteMediaPlayerManager?
> >> Probably not a big deal given all virtual methods are final.
> > 
> > It doesn't have to inherit, I'll look into changing that in an upcoming patch.
> 
> Actually it does need to inherit because it is returned by the remote
> factory and called by MediaPlayer

Right, I mixed up with the proxy class in GPU process that might not need any specific inheritance, RemoteMediaPlayerProxy.

[WebKit Commit Bot]

Comment on attachment 385606 [details]
Patch

Clearing flags on attachment: 385606

Committed r253482: <https://trac.webkit.org/changeset/253482>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.