Bug 205094

<rdar://problem/57815393>

Created attachment 385328 [details] WIP patch

Created attachment 385331 [details] WIP patch

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

Created attachment 385396 [details] WIP patch

Created attachment 385410 [details] WIP patch

Created attachment 385414 [details] WIP patch

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?

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

Created attachment 385447 [details] WIP patch

Created attachment 385462 [details] WIP patch

Created attachment 385491 [details] WIP patch

Created attachment 385498 [details] WIP patch

Created attachment 385502 [details] WIP patch

Created attachment 385509 [details] WIP patch

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.

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.

Created attachment 385578 [details] Patch

> > Can we use a WTF::Function or CompletionHandler? Actually a CompletionHandler is probably not great since it might be called multiple times.

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

Created attachment 385606 [details] Patch

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

Comment on attachment 385606 [details] Patch Clearing flags on attachment: 385606 Committed r253482: <https://trac.webkit.org/changeset/253482>

All reviewed patches have been landed. Closing bug.