Bug 205379

<rdar://problem/58035178>

Created attachment 385946 [details] WIP

Created attachment 386173 [details] WIP patch

Created attachment 386174 [details] Rebased WIP patch

Created attachment 386178 [details] Fix iOS build failure

Comment on attachment 386178 [details] Fix iOS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=386178&action=review Looks good. Just a couple of minor bits: > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:52 > + RefPtr<RemoteMediaResource> remoteMediaResource = adoptRef(*new RemoteMediaResource(m_remoteMediaResourceManager, remoteMediaResourceIdentifier)); This can be `auto remoteMediaResource = ...`. > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:30 > +#include "RemoteMediaPlayerProxy.h" This and some of the following includes may be able to be moved into the .cpp file and replaced with forward declarations.

Comment on attachment 386178 [details] Fix iOS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=386178&action=review > Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:100 > + RemoteMediaResourceManager& m_remoteMediaResourceManager; Maybe we could get it from m_gpuConnectionToWebProcess. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:46 > + , m_mediaResourceManager(mediaResourceManager) We could try to get it from either m_gpuConnectionToWebProcess or m_manager. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:319 > + return adoptRef(*new RemoteMediaResourceLoader(m_id, *this, m_mediaResourceManager)); Do we need to pass m_id given we pass this? Also, maybe we do not need to pass m_mediaResourceManager to RemoteMediaResourceLoader and let it go through its RemoteMediaPlayerProxy. Given RemoteMediaResourceLoader is ref counted, there is a possibility that RemoteMediaResourceLoader will outlive its RemoteMediaPlayerProxy. Ideally we would make it a unique_ptr but this does not seem very easy to do. The safest is probably to make RemoteMediaResourceLoader store a WeakPtr<RemoteMediaPlayerProxy> which should be fine as long as we are doing main thread only business. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:42 > class RemoteMediaPlayerProxy final? > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:51 > + IPC::Connection& WebProcessConnection() { return m_webProcessConnection; } s/WebProcessConnection/webProcessConnection. Do we need to expose it? Maybe we could add a requestResource method to RemoteMediaPlayerProxy? > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:52 > + // TODO: do we need to send message to the web process here? I guess we should stop the load if not already done so. > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:53 > + m_remoteMediaResourceManager.removeMediaResource(m_id); I see that PlatformMediaResourceLoader is ThreadSafeRefCounted, although I do not think we are doing background thread processing. Let's add an ASSERT(isMainThread()) here just in case. > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:58 > + // TODO: notify the web process that the job is done? Yes, we should probably have a way to cancel the load happening in Network Process if our client is no longer interested. > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:69 > + m_client->responseReceived(*this, response, [weakThis = makeWeakPtr(*this)](ShouldContinue shouldContinue) mutable { Since this is threadSafeRefCounted, you could do protectedThis = makeRef(*this) and not make it CanMakeWeakPtr. Adding an ASSERT(isMainThread()) maybe. > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:79 > + m_client->redirectReceived(*this, WTFMove(request), response, [](ResourceRequest&&) mutable { }); could use auto here and above (ShouldContinue). No need for mutable > Source/WebKit/GPUProcess/media/RemoteMediaResource.h:41 > + explicit RemoteMediaResource(RemoteMediaResourceManager&, RemoteMediaResourceIdentifier); No need for explicit, maybe we can remove CanMakeWeakPtr. Please make this constructor private as well since we should always have adoptRef close to the new. > Source/WebKit/GPUProcess/media/RemoteMediaResource.h:44 > + bool ready() { return m_ready; } const > Source/WebKit/GPUProcess/media/RemoteMediaResource.h:49 > + bool didPassAccessControlCheck() const override; final? > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:41 > + , m_remoteMediaResourceManager(remoteMediaResourceManager) See above comment about RemoteMediaResourceLoader being ref counted. We are not sure it will not outlive m_remoteMediaPlayerProxy and/or m_remoteMediaResourceManager. Maybe we can just have one m_remoteMediaPlayerProxy being a WeakPtr<RemoteMediaPlayerProxy> > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:55 > + m_remoteMediaPlayerProxy.WebProcessConnection().sendWithAsyncReply(Messages::RemoteMediaPlayerManager::RequestResource(m_mediaPlayerPrivateRemoteIdentifier, remoteMediaResourceIdentifier, request, options), [remoteMediaResource]() { If using auto, you might need to copyRef here. > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:56 > + if (remoteMediaResource) You can remove the if check here. > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:39 > +class RemoteMediaResourceLoader could be final. > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:42 > + explicit RemoteMediaResourceLoader(MediaPlayerPrivateRemoteIdentifier, RemoteMediaPlayerProxy&, RemoteMediaResourceManager&); No need for explicit. > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:49 > +void RemoteMediaResourceManager::addMediaResource(RemoteMediaResourceIdentifier remoteMediaResourceIdentifier, RefPtr<RemoteMediaResource> remoteMediaResource) Let's pass a Ref<RemoteMediaResource>&& > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:51 > + m_remoteMediaResources.add(remoteMediaResourceIdentifier, remoteMediaResource); Let's add some ASSERT(!m_remoteMediaResources.contains(remoteMediaResourceIdentifier)) and do m_remoteMediaResources.add(remoteMediaResourceIdentifier, WTFMove(remoteMediaResource)); > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:54 > +void RemoteMediaResourceManager::removeMediaResource(RemoteMediaResourceIdentifier remoteMediaResourceIdentifier) This method is only call in RemoteMediaResource destructor. Given m_remoteMediaResources holds a ref, we might never free it until RemoteMediaResourceManager goes away. Do we need m_remoteMediaResources to hold a Ref. Could it just take a RemoteMediaResource*? > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:56 > + m_remoteMediaResources.remove(remoteMediaResourceIdentifier); Let's add some ASSERT(m_remoteMediaResources.contains(remoteMediaResourceIdentifier)) > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:61 > + ASSERT(m_remoteMediaResources.contains(id) && m_remoteMediaResources.get(id)->ready()); At some point, this will probably be called straight from IPC, right? I think we should exit early in addition to ASSERTING. Also, we should probably RELEASE_ASSERT or MESSAGE_CHECK that id is not 0, otherwise we might corrupt the map. Ditto for other methods called from IPC. > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:49 > + explicit RemoteMediaResourceManager(); No need for explicit > Source/WebKit/Sources.txt:31 > +//GPUProcess/media/RemoteMediaResourceManager.cpp Why are they commented out? If this is a unify build issue, can we use @no-unify? > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:773 > +{ ASSERT(!m_mediaResources.contains(remoteMediaResourceIdentifier)). We could also check that remoteMediaResourceIdentifier is not null. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:776 > + m_mediaResources.add(remoteMediaResourceIdentifier, WTFMove(resource)); We are not removing any m_mediaResources entry here. This is fine to do as a follow-up, maybe add a FIXME somewhere, the patch being already quite big. One possibility would be for the RemoteMediaResourceProxy to remove the resource from the map when loadFailed/loadFinished. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:48 > + static std::unique_ptr<MediaPlayerPrivateRemote> create(WebCore::MediaPlayer* player, WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier, MediaPlayerPrivateRemoteIdentifier identifier, RemoteMediaPlayerManager& manager) Would be nicer as a MediaPlayer&. We probably cannot have ref all the way up due to existing interfaces, but we should try to do so. This is fine to do as a separate patch. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:53 > + explicit MediaPlayerPrivateRemote(WebCore::MediaPlayer*, WebCore::MediaPlayerEnums::MediaEngineIdentifier, MediaPlayerPrivateRemoteIdentifier, RemoteMediaPlayerManager&); Can we make it private? > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:57 > + WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier() const { return m_remoteEngineIdentifier; } Given the patch is big, maybe all these WebCore:: changes could be split in its own patch. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:296 > + WebCore::MediaPlayer* m_player { nullptr }; Wonder whether it would be safer to have a WeakPtr<MediaPlayer>, not for this patch though. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:302 > + HashMap<RemoteMediaResourceIdentifier, RefPtr<WebCore::PlatformMediaResource>> m_mediaResources; HashMap of Ref<> would be better. > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:249 > +void RemoteMediaPlayerManager::requestResource(MediaPlayerPrivateRemoteIdentifier id, RemoteMediaResourceIdentifier remoteMediaResourceIdentifier, WebCore::ResourceRequest&& request, WebCore::PlatformMediaResourceLoader::LoadOptions options, CompletionHandler<void()>&& completionHandler) No need for WebCore:: here > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:251 > + if (auto player = m_players.get(id)) I think s/auto/auto&/ is better. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:39 > + : m_gpuProcessConnection(WTFMove(connection)) s/m_gpuProcessConnection/m_connection otherwise we can think it is a GPUProcessConnection. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:51 > + completionHandler(WebCore::ShouldContinue::Yes); Given we are waiting for GPU process response, we should we have RemoteMediaResourceManager::ResponseReceived give back a WebCore::ShouldContinue. Then it would be up to the RemoteMediaResourceManager to return Yes automatically or actually ask its underling resource to say no. In case the RemoteMediaResource is gone, we should also return No. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.h:37 > +class RemoteMediaResourceProxy : public WebCore::PlatformMediaResourceClient { final. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.h:51 > + void loadFinished(WebCore::PlatformMediaResource&); All of these private and final?

Comment on attachment 386178 [details] Fix iOS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=386178&action=review > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:84 > + return true; Should we add a FIXME there to implement it?

Created attachment 386382 [details] Update the WIP patch based on comments from Youenn.

Comment on attachment 386382 [details] Update the WIP patch based on comments from Youenn. Also rebase to the latest repo. Will publish the responses to the comments later.

Created attachment 386467 [details] Patch (fixed the audio playback issue)

Created attachment 386483 [details] Patch for review

Comment on attachment 386178 [details] Fix iOS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=386178&action=review >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:100 >> + RemoteMediaResourceManager& m_remoteMediaResourceManager; > > Maybe we could get it from m_gpuConnectionToWebProcess. Right. Removed the reference. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:46 >> + , m_mediaResourceManager(mediaResourceManager) > > We could try to get it from either m_gpuConnectionToWebProcess or m_manager. Right, fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:319 >> + return adoptRef(*new RemoteMediaResourceLoader(m_id, *this, m_mediaResourceManager)); > > Do we need to pass m_id given we pass this? > Also, maybe we do not need to pass m_mediaResourceManager to RemoteMediaResourceLoader and let it go through its RemoteMediaPlayerProxy. > > Given RemoteMediaResourceLoader is ref counted, there is a possibility that RemoteMediaResourceLoader will outlive its RemoteMediaPlayerProxy. > Ideally we would make it a unique_ptr but this does not seem very easy to do. > The safest is probably to make RemoteMediaResourceLoader store a WeakPtr<RemoteMediaPlayerProxy> which should be fine as long as we are doing main thread only business. Added a function in RemoteMediaPlayerProxy to send messages to the web process on behalf of the RemoteMediaResourceLoader. So RemoteMediaResourceLoader does not need to have the m_id as well as the reference to the RemoteMediaResourceManager. Right, added WeakPtr<RemoteMediaPlayerProxy> in RemoteMediaResourceLoader. RemoteMediaResourceLoader can outlive its RemoteMediaPlayerProxy. It replaces the role of MediaResourceLoader, which is owned by WebCoreNSURLSesssion. We will keep the same behavior. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:42 >> class RemoteMediaPlayerProxy > > final? Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:51 >> + IPC::Connection& WebProcessConnection() { return m_webProcessConnection; } > > s/WebProcessConnection/webProcessConnection. > Do we need to expose it? Maybe we could add a requestResource method to RemoteMediaPlayerProxy? Removed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:52 >> + // TODO: do we need to send message to the web process here? > > I guess we should stop the load if not already done so. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:53 >> + m_remoteMediaResourceManager.removeMediaResource(m_id); > > I see that PlatformMediaResourceLoader is ThreadSafeRefCounted, although I do not think we are doing background thread processing. > Let's add an ASSERT(isMainThread()) here just in case. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:58 >> + // TODO: notify the web process that the job is done? > > Yes, we should probably have a way to cancel the load happening in Network Process if our client is no longer interested. Added a XPC message to implement that. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:69 >> + m_client->responseReceived(*this, response, [weakThis = makeWeakPtr(*this)](ShouldContinue shouldContinue) mutable { > > Since this is threadSafeRefCounted, you could do protectedThis = makeRef(*this) and not make it CanMakeWeakPtr. > Adding an ASSERT(isMainThread()) maybe. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:79 >> + m_client->redirectReceived(*this, WTFMove(request), response, [](ResourceRequest&&) mutable { }); > > could use auto here and above (ShouldContinue). > No need for mutable Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:84 >> + return true; > > Should we add a FIXME there to implement it? Implemented. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:41 >> + explicit RemoteMediaResource(RemoteMediaResourceManager&, RemoteMediaResourceIdentifier); > > No need for explicit, maybe we can remove CanMakeWeakPtr. > Please make this constructor private as well since we should always have adoptRef close to the new. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:44 >> + bool ready() { return m_ready; } > > const Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:49 >> + bool didPassAccessControlCheck() const override; > > final? Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:41 >> + , m_remoteMediaResourceManager(remoteMediaResourceManager) > > See above comment about RemoteMediaResourceLoader being ref counted. > We are not sure it will not outlive m_remoteMediaPlayerProxy and/or m_remoteMediaResourceManager. > Maybe we can just have one m_remoteMediaPlayerProxy being a WeakPtr<RemoteMediaPlayerProxy> Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:52 >> + RefPtr<RemoteMediaResource> remoteMediaResource = adoptRef(*new RemoteMediaResource(m_remoteMediaResourceManager, remoteMediaResourceIdentifier)); > > This can be `auto remoteMediaResource = ...`. Moved this function to RemoteMediaPlayerProxy. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:55 >> + m_remoteMediaPlayerProxy.WebProcessConnection().sendWithAsyncReply(Messages::RemoteMediaPlayerManager::RequestResource(m_mediaPlayerPrivateRemoteIdentifier, remoteMediaResourceIdentifier, request, options), [remoteMediaResource]() { > > If using auto, you might need to copyRef here. Ditto. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:56 >> + if (remoteMediaResource) > > You can remove the if check here. Ditto. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:30 >> +#include "RemoteMediaPlayerProxy.h" > > This and some of the following includes may be able to be moved into the .cpp file and replaced with forward declarations. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:39 >> +class RemoteMediaResourceLoader > > could be final. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:42 >> + explicit RemoteMediaResourceLoader(MediaPlayerPrivateRemoteIdentifier, RemoteMediaPlayerProxy&, RemoteMediaResourceManager&); > > No need for explicit. It needs to explicit after refactoring. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:49 >> +void RemoteMediaResourceManager::addMediaResource(RemoteMediaResourceIdentifier remoteMediaResourceIdentifier, RefPtr<RemoteMediaResource> remoteMediaResource) > > Let's pass a Ref<RemoteMediaResource>&& Use raw pointer here after refactoring because the RemoteMediaResourceManager does not own RemoteMediaResource. In the destructor of RemoteMediaResource, it will remove itself from the RemoteMediaResourceManager. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:51 >> + m_remoteMediaResources.add(remoteMediaResourceIdentifier, remoteMediaResource); > > Let's add some ASSERT(!m_remoteMediaResources.contains(remoteMediaResourceIdentifier)) > and do m_remoteMediaResources.add(remoteMediaResourceIdentifier, WTFMove(remoteMediaResource)); Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:54 >> +void RemoteMediaResourceManager::removeMediaResource(RemoteMediaResourceIdentifier remoteMediaResourceIdentifier) > > This method is only call in RemoteMediaResource destructor. > Given m_remoteMediaResources holds a ref, we might never free it until RemoteMediaResourceManager goes away. > Do we need m_remoteMediaResources to hold a Ref. > Could it just take a RemoteMediaResource*? Right. Changed the implementation to use raw pointers of RemoteMediaResource. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:56 >> + m_remoteMediaResources.remove(remoteMediaResourceIdentifier); > > Let's add some ASSERT(m_remoteMediaResources.contains(remoteMediaResourceIdentifier)) Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:61 >> + ASSERT(m_remoteMediaResources.contains(id) && m_remoteMediaResources.get(id)->ready()); > > At some point, this will probably be called straight from IPC, right? > I think we should exit early in addition to ASSERTING. > Also, we should probably RELEASE_ASSERT or MESSAGE_CHECK that id is not 0, otherwise we might corrupt the map. > > Ditto for other methods called from IPC. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:49 >> + explicit RemoteMediaResourceManager(); > > No need for explicit Fixed. >> Source/WebKit/Sources.txt:31 >> +//GPUProcess/media/RemoteMediaResourceManager.cpp > > Why are they commented out? > If this is a unify build issue, can we use @no-unify? Yes, we can. In this patch, these new files are included in the Xcode project. Will fix the unified build failure in webkit.org/b/205616. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:773 >> +{ > > ASSERT(!m_mediaResources.contains(remoteMediaResourceIdentifier)). > We could also check that remoteMediaResourceIdentifier is not null. Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:776 >> + m_mediaResources.add(remoteMediaResourceIdentifier, WTFMove(resource)); > > We are not removing any m_mediaResources entry here. > This is fine to do as a follow-up, maybe add a FIXME somewhere, the patch being already quite big. > One possibility would be for the RemoteMediaResourceProxy to remove the resource from the map when loadFailed/loadFinished. Implemented the mechanism to remove an entry when it is done. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:48 >> + static std::unique_ptr<MediaPlayerPrivateRemote> create(WebCore::MediaPlayer* player, WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier, MediaPlayerPrivateRemoteIdentifier identifier, RemoteMediaPlayerManager& manager) > > Would be nicer as a MediaPlayer&. > We probably cannot have ref all the way up due to existing interfaces, but we should try to do so. > This is fine to do as a separate patch. Will take care of that in a separate patch. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:53 >> + explicit MediaPlayerPrivateRemote(WebCore::MediaPlayer*, WebCore::MediaPlayerEnums::MediaEngineIdentifier, MediaPlayerPrivateRemoteIdentifier, RemoteMediaPlayerManager&); > > Can we make it private? We cannot make it private due to the implementation of makeUnique. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:57 >> + WebCore::MediaPlayerEnums::MediaEngineIdentifier remoteEngineIdentifier() const { return m_remoteEngineIdentifier; } > > Given the patch is big, maybe all these WebCore:: changes could be split in its own patch. Fixed in this patch. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:296 >> + WebCore::MediaPlayer* m_player { nullptr }; > > Wonder whether it would be safer to have a WeakPtr<MediaPlayer>, not for this patch though. Will take care of that in a separate patch. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:302 >> + HashMap<RemoteMediaResourceIdentifier, RefPtr<WebCore::PlatformMediaResource>> m_mediaResources; > > HashMap of Ref<> would be better. We can make that change, but it will introduce extra complexity. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:249 >> +void RemoteMediaPlayerManager::requestResource(MediaPlayerPrivateRemoteIdentifier id, RemoteMediaResourceIdentifier remoteMediaResourceIdentifier, WebCore::ResourceRequest&& request, WebCore::PlatformMediaResourceLoader::LoadOptions options, CompletionHandler<void()>&& completionHandler) > > No need for WebCore:: here Fixed. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:251 >> + if (auto player = m_players.get(id)) > > I think s/auto/auto&/ is better. s/auto/const auto&/ >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:39 >> + : m_gpuProcessConnection(WTFMove(connection)) > > s/m_gpuProcessConnection/m_connection otherwise we can think it is a GPUProcessConnection. Fixed. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:51 >> + completionHandler(WebCore::ShouldContinue::Yes); > > Given we are waiting for GPU process response, we should we have RemoteMediaResourceManager::ResponseReceived give back a WebCore::ShouldContinue. > Then it would be up to the RemoteMediaResourceManager to return Yes automatically or actually ask its underling resource to say no. > In case the RemoteMediaResource is gone, we should also return No. Added WebCore::PolicyChecker::ShouldContinue as the return value of the asynchronous ResponseReceived XPC message. Now the media resource client in the GPU Process can tell the media resource in the Web process whether the downloading should "continue". >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.h:37 >> +class RemoteMediaResourceProxy : public WebCore::PlatformMediaResourceClient { > > final. Fixed. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.h:51 >> + void loadFinished(WebCore::PlatformMediaResource&); > > All of these private and final? Fixed.

Created attachment 386484 [details] Patch for review (fixed gtk and wpe build failures)

Comment on attachment 386484 [details] Patch for review (fixed gtk and wpe build failures) Please fix RemoteMediaResourceManager.cpp. We also might not need to implement some WebCore interfaces, which would simplify the implementation, in this patch or as a follow-up. I am thinking of RemoteMediaResourceProxy for instance. Would it be possible add a test as well? One possibility is to enable media loading from GPU Process for just this test. Then make a page that is controlled by a service worker, the page then triggers a media load that is intercepted by the service worker. The test would succeed as soon as the load is intercepted by the service worker. You can look at LayoutTests/http/wpt/service-workers or LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker for service worker tests. View in context: https://bugs.webkit.org/attachment.cgi?id=386484&action=review > Source/WebCore/loader/MediaResourceLoader.cpp:175 > + m_client->responseReceived(*this, response, [this, protectedThis = makeRef(*this), completionHandler = completionHandlerCaller.release()] (PolicyChecker::ShouldContinue shouldContinue) mutable { Could use auto > Source/WebCore/loader/PolicyChecker.h:75 > + using NewWindowPolicyDecisionFunction = CompletionHandler<void(const ResourceRequest&, WeakPtr<FormState>&&, const String& frameName, const NavigationAction&, PolicyChecker::ShouldContinue)>; s/PolicyChecker::ShouldContinue/ShouldContinue/ > Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:110 > +} // namespace WebKit This patch is already big so we could also have separated these refactoring in preliminary patches. This makes reviews/rollback/diagnosis easier. > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:73 > + if (m_client) { Usually we do if(!m_client) return if the follow-up code is multi line. > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:95 > + return true; One-liner maybe? > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:48 > + return m_remoteMediaPlayerProxy->requestResource(WTFMove(request), options); How do we know that m_remoteMediaPlayerProxy is not null? > Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:44 > + RefPtr<WebCore::PlatformMediaResource> requestResource(WebCore::ResourceRequest&&, LoadOptions) final; private? > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:74 > + if (!m_remoteMediaResources.get(id)->ready()) { We probably cannot guarantee that m_remoteMediaResources.get(id) returns non null given it is coming from IPC. The easiest would be to do something like: auto* resource = m_remoteMediaResources.get(id); if (!resource || !resource->ready()) return completionHandler(); ... Ditto for other methods called from IPC. > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:79 > + RELEASE_ASSERT(m_remoteMediaResources.contains(id) && m_remoteMediaResources.get(id)->ready()); This RELEASE_ASSERT is covered by the above if check. > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:39 > +#include <wtf/WeakPtr.h> We do not need WeakPtr. We could probably forward declare Connection, Decoder, DataReference, ResourceRequest and remove the corresponding includes. We probably need in theory HashMap.h > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:52 > + void addMediaResource(RemoteMediaResourceIdentifier, RemoteMediaResource*); Would probably be better as a RemoteMediaResource& > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:66 > + , m_mediaResourceLoader(player->createResourceLoader()) Since we are in MediaPlayerPrivateRemote, we could try to create our own resource loader that would not need to implement PlatformMediaResourceLoader. Maybe not for this patch though. It seems that we could even remove m_mediaResourceLoader and just do IPC to NetworkProcess from MediaPlayerPrivateRemote::requestResource I also wonder whether we need RemoteMediaResourceProxy to implement WebCore::PlatformMediaResourceClient. It seems this object does not surface at WebCore level. That could further simplify the code. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:38 > +RemoteMediaResourceProxy::RemoteMediaResourceProxy(Ref<IPC::Connection>&& connection, WebCore::PlatformMediaResource* platformMediaResource, RemoteMediaResourceIdentifier id) We can probably take a PlatformMediaResource&

Can you update the GPU process TestExpectations to enable audio-only tests?

Sure, I will do that.

Comment on attachment 386484 [details] Patch for review (fixed gtk and wpe build failures) View in context: https://bugs.webkit.org/attachment.cgi?id=386484&action=review Reported a follow-up bug to track the refactoring to simplify the media resource loader, especially in the web process side. About testing, as Eric commented, we can update the TestExpectations to enable some layout tests to verify the media resource loader. I also added two layout tests to verify the audio playback. >> Source/WebCore/loader/MediaResourceLoader.cpp:175 >> + m_client->responseReceived(*this, response, [this, protectedThis = makeRef(*this), completionHandler = completionHandlerCaller.release()] (PolicyChecker::ShouldContinue shouldContinue) mutable { > > Could use auto Fixed. >> Source/WebCore/loader/PolicyChecker.h:75 >> + using NewWindowPolicyDecisionFunction = CompletionHandler<void(const ResourceRequest&, WeakPtr<FormState>&&, const String& frameName, const NavigationAction&, PolicyChecker::ShouldContinue)>; > > s/PolicyChecker::ShouldContinue/ShouldContinue/ Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.h:110 >> +} // namespace WebKit > > This patch is already big so we could also have separated these refactoring in preliminary patches. > This makes reviews/rollback/diagnosis easier. Right. It was done in webkit.org/b/205631. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:73 >> + if (m_client) { > > Usually we do if(!m_client) return if the follow-up code is multi line. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:95 >> + return true; > > One-liner maybe? return m_client ? m_client->shouldCacheResponse(*this, response) : true; >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.cpp:48 >> + return m_remoteMediaPlayerProxy->requestResource(WTFMove(request), options); > > How do we know that m_remoteMediaPlayerProxy is not null? Added null pointer checking. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceLoader.h:44 >> + RefPtr<WebCore::PlatformMediaResource> requestResource(WebCore::ResourceRequest&&, LoadOptions) final; > > private? Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:74 >> + if (!m_remoteMediaResources.get(id)->ready()) { > > We probably cannot guarantee that m_remoteMediaResources.get(id) returns non null given it is coming from IPC. > The easiest would be to do something like: > auto* resource = m_remoteMediaResources.get(id); > if (!resource || !resource->ready()) > return completionHandler(); > ... > Ditto for other methods called from IPC. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:79 >> + RELEASE_ASSERT(m_remoteMediaResources.contains(id) && m_remoteMediaResources.get(id)->ready()); > > This RELEASE_ASSERT is covered by the above if check. Removed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:39 >> +#include <wtf/WeakPtr.h> > > We do not need WeakPtr. > We could probably forward declare Connection, Decoder, DataReference, ResourceRequest and remove the corresponding includes. > We probably need in theory HashMap.h Fixed, and removed unnecessary header files. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.h:52 >> + void addMediaResource(RemoteMediaResourceIdentifier, RemoteMediaResource*); > > Would probably be better as a RemoteMediaResource& Do you mean the function argument only? If yes, then fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:66 >> + , m_mediaResourceLoader(player->createResourceLoader()) > > Since we are in MediaPlayerPrivateRemote, we could try to create our own resource loader that would not need to implement PlatformMediaResourceLoader. > Maybe not for this patch though. > It seems that we could even remove m_mediaResourceLoader and just do IPC to NetworkProcess from MediaPlayerPrivateRemote::requestResource > > I also wonder whether we need RemoteMediaResourceProxy to implement WebCore::PlatformMediaResourceClient. > It seems this object does not surface at WebCore level. > That could further simplify the code. Sound like a follow-up patch will be a better idea. Created webkit.org/b/205637 to track it. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:38 >> +RemoteMediaResourceProxy::RemoteMediaResourceProxy(Ref<IPC::Connection>&& connection, WebCore::PlatformMediaResource* platformMediaResource, RemoteMediaResourceIdentifier id) > > We can probably take a PlatformMediaResource& Fixed.

Created attachment 386514 [details] Update the patch based on comments from Youenn and Eric

Created attachment 386516 [details] Update the patch based on comments from Youenn and Eric

Comment on attachment 386516 [details] Update the patch based on comments from Youenn and Eric View in context: https://bugs.webkit.org/attachment.cgi?id=386516&action=review > Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:88 > + m_client->redirectReceived(*this, WTFMove(request), response, [](auto&&) { }); See comment below, we should probably pass a real completion handler here. > Source/WebKit/GPUProcess/media/RemoteMediaResource.h:53 > + bool shouldCacheResponse(const WebCore::ResourceResponse&); This does not seem to be used right now, should we remove the code from now? > Source/WebKit/GPUProcess/media/RemoteMediaResource.h:67 > + bool m_ready; /sm_ready;/m_ready { false }; And remove init in constructor. > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:66 > + if (!resource || !resource->ready()) { I am not exactly sure what the ready check is supposed to cover. Instead of doing the ready check, could we simply add resource to m_remoteMediaResources in the asyncReply of m_webProcessConnection->sendWithAsyncReply(Messages::RemoteMediaPlayerManager::RequestResource? Then the if (!resource) would cover both cases. In practice, I do not see how ready() could be false in any of the methods called from IPC. > Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:82 > + m_remoteMediaResources.get(id)->redirectReceived(WTFMove(request), response); In theory, the call to redirectReceived could mutate the request. We should then have completionHandler be a CompletionHandler<void(ResourceRequest&&)> And we would write something like m_remoteMediaResources.get(id)->redirectReceived(WTFMove(request), response, WTFMove(completionHandler)); In the if (!resource) case, we would call completionHandler({ }) which would probably cancel the load in WebProcess side. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:59 > + completionHandler(WTFMove(request)); Then here we would not need to capture request since it would be given by the async reply. > Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:65 > + return true; We should probably return false, or return what the GPU Process would tell us through sync IPC.

Comment on attachment 386516 [details] Update the patch based on comments from Youenn and Eric View in context: https://bugs.webkit.org/attachment.cgi?id=386516&action=review >> Source/WebKit/GPUProcess/media/RemoteMediaResource.cpp:88 >> + m_client->redirectReceived(*this, WTFMove(request), response, [](auto&&) { }); > > See comment below, we should probably pass a real completion handler here. Done. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:53 >> + bool shouldCacheResponse(const WebCore::ResourceResponse&); > > This does not seem to be used right now, should we remove the code from now? Removed. >> Source/WebKit/GPUProcess/media/RemoteMediaResource.h:67 >> + bool m_ready; > > /sm_ready;/m_ready { false }; > And remove init in constructor. Fixed. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:66 >> + if (!resource || !resource->ready()) { > > I am not exactly sure what the ready check is supposed to cover. > Instead of doing the ready check, could we simply add resource to m_remoteMediaResources in the asyncReply of m_webProcessConnection->sendWithAsyncReply(Messages::RemoteMediaPlayerManager::RequestResource? > Then the if (!resource) would cover both cases. > In practice, I do not see how ready() could be false in any of the methods called from IPC. RemoteMediaPlayerProxy::requestResource() needs to return a Ref<PlatformMediaResource> to the caller (WebCoreNSURLSessionDataTask._restart()) immediately, but since we use an async XPC message to implement the function, it needs to return a PlatformMediaResource which is not ready and set it to be ready later after it receives response of the XPC message. To implement the logic you proposed, I think we have to modify the caller as well. In RemoteMediaPlayerManager::requestResource(), if the resource request gets responses before calling completionHandler(), then some method of RemoteMediaResourceManager may be called before the resource is ready. >> Source/WebKit/GPUProcess/media/RemoteMediaResourceManager.cpp:82 >> + m_remoteMediaResources.get(id)->redirectReceived(WTFMove(request), response); > > In theory, the call to redirectReceived could mutate the request. > We should then have completionHandler be a CompletionHandler<void(ResourceRequest&&)> > And we would write something like m_remoteMediaResources.get(id)->redirectReceived(WTFMove(request), response, WTFMove(completionHandler)); > > In the if (!resource) case, we would call completionHandler({ }) which would probably cancel the load in WebProcess side. I thought we can "complete" the completion handler in the Web process without the involvement of the GPU process, but sounds like completing the callback in the GPU process is better. I also added the RequestRequest as the return value of the RedirectReceived XPC message. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:59 >> + completionHandler(WTFMove(request)); > > Then here we would not need to capture request since it would be given by the async reply. Fixed. >> Source/WebKit/WebProcess/GPU/media/RemoteMediaResourceProxy.cpp:65 >> + return true; > > We should probably return false, or return what the GPU Process would tell us through sync IPC. Return false for now. Added a TODO in the comment. Will take care of it on webkit.org/b/205637.

Created attachment 386549 [details] Patch for landing

Created attachment 386563 [details] Patch for landing (rebased)

Comment on attachment 386563 [details] Patch for landing (rebased) Clearing flags on attachment: 386563 Committed r253964: <https://trac.webkit.org/changeset/253964>