[GPUP] Implement Modern EME API in the GPU Process
Created attachment 391455 [details] WIP
Created attachment 391460 [details] WIP
Created attachment 391463 [details] WIP
Created attachment 391464 [details] WIP
Created attachment 391467 [details] WIP
Created attachment 391473 [details] WIP
Created attachment 391509 [details] WIP
Created attachment 391510 [details] WIP
Created attachment 391511 [details] Part 1: WebCore
Created attachment 391512 [details] Part 2: WebKit + SharedBuffer
Created attachment 391513 [details] Part 3: RemoteCDM (Depends on Parts 1 & 2)
Created attachment 391514 [details] Part 1: WebCore
Created attachment 391515 [details] Part 2: WebKit + SharedBuffer
Comment on attachment 391513 [details] Part 3: RemoteCDM (Depends on Parts 1 & 2) This adds a lot of additional sync IPC. Can you help me understand why these all need to be synchronous?
(In reply to Sam Weinig from comment #14) > Comment on attachment 391513 [details] > Part 3: RemoteCDM (Depends on Parts 1 & 2) > > This adds a lot of additional sync IPC. Can you help me understand why these > all need to be synchronous? Yeah, I know. :( A ton of changes would have to be made to the algorithms inside CDM.cpp in order to make all the messaging asynchronous; see getSupportedConfiguration() in that file, e.g. I've already done some of that, by pushing up the list of "supportedInitTypes" rather issuing a sync XPC to ask whether a given type is supported. There are a few other changes I'm planning to make to push more information up across from the GPUProcess so that queries are local and don't need XPCs at all, but it won't be possible for all the methods. Another possibility I considered is to move the entire implementation of CDM into a background thread, the way we do for MSE parsing. We can't send sync messages from background threads, so that'd be hard, and would require getting replies on the main thread and doing semaphore magic to move the results back to the work queue.
...(continued) I'm not saying that work is impossible, and I do plan on converting sync messages to asyncs in future patches.
Created attachment 391534 [details] Part 1: WebCore
...or maybe I can move all of the CDM::getSupportedConfiguration into platform and then into the GPUProcess.
(In reply to Jer Noble from comment #18) > ...or maybe I can move all of the CDM::getSupportedConfiguration into > platform and then into the GPUProcess. Looks like that technique will work; the only DOM part of that implementation is asking the Document for it's securityOrigin, and that can stay in CDM and be passed into the algorithm from outside. I'll post a new patch that removes most of the sync messages from RemoteCDMProxy.messages.in.
Created attachment 391622 [details] Part 1: WebCore
Created attachment 391623 [details] Part 2: WebKit + SharedBuffer
Created attachment 391625 [details] Part 3: RemoteCDM (depends on Parts 1 & 2)
Okay, almost all the synchronous messages have been removed entirely or converted to Async ones. The remaining Sync messages are creating objects, and they need to be synchronous because the creation message returns with it the configuration properties which themselves allow WebContent-side queries to not require any XPC at all. If the creation methods weren't sync, those property queries would have to block until the configuration message was received.
Comment on attachment 391623 [details] Part 2: WebKit + SharedBuffer View in context: https://bugs.webkit.org/attachment.cgi?id=391623&action=review > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:36 > SharedBufferDataReference() = default; > + SharedBufferDataReference(RefPtr<WebCore::SharedBuffer>&& buffer) Can we remove these? The default constructor was only needed in a time when decoding needed to instantiate a default constructed object, which is not the case any more. Can we make m_buffer a Ref? > Source/WebKit/Scripts/webkit/messages.py:230 > + 'WebKit::RemoteCDMIdentifier', This change seems unrelated to the rest of this patch. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3211 > +Optional<RefPtr<SharedBuffer>> ArgumentCoder<RefPtr<WebCore::SharedBuffer>>::decode(Decoder& decoder) Optional<Ref<SharedBuffer>> ArgumentCoder<Ref<... > Source/WebKit/Shared/WebCoreArgumentCoders.h:835 > + static Optional<RefPtr<WebCore::SharedBuffer>> decode(Decoder&); Optional<Ref<... > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:132 > + m_connection->send(Messages::ServiceWorkerFetchTask::DidReceiveData { { SharedBuffer::create(data, size) }, static_cast<int64_t>(size) }, m_fetchIdentifier); This is an unnecessary copy of the data. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3503 > + dataReference = SharedBuffer::create(data.get()); This is a great improvement. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4922 > + send(Messages::WebPageProxy::DataCallback({ SharedBuffer::create(pdfPageData.get()) }, callbackID)); Great!
(In reply to Alex Christensen from comment #24) > Comment on attachment 391623 [details] > Part 2: WebKit + SharedBuffer > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391623&action=review > > > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:36 > > SharedBufferDataReference() = default; > > + SharedBufferDataReference(RefPtr<WebCore::SharedBuffer>&& buffer) > > Can we remove these? The default constructor was only needed in a time when > decoding needed to instantiate a default constructed object, which is not > the case any more. > Can we make m_buffer a Ref? Probably not. For sync replies, you still need to create a default object and pass that object in by reference to the reply handler. Doing so would mean not being able to use a SharedBufferDataReference in a sync reply. > > Source/WebKit/Scripts/webkit/messages.py:230 > > + 'WebKit::RemoteCDMIdentifier', > > This change seems unrelated to the rest of this patch. Yep! That should have gone into Part 3. > > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:132 > > + m_connection->send(Messages::ServiceWorkerFetchTask::DidReceiveData { { SharedBuffer::create(data, size) }, static_cast<int64_t>(size) }, m_fetchIdentifier); > > This is an unnecessary copy of the data. There are two call sites which generate a "DidReceiveData" message. Should there be two separate messages, one that takes a DataReference and one that takes a SharedDataReference? Or can the FetchTaskClient be modified to require the caller gives a SharedBuffer?
(In reply to Jer Noble from comment #25) > (In reply to Alex Christensen from comment #24) > > Comment on attachment 391623 [details] > > > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:132 > > > + m_connection->send(Messages::ServiceWorkerFetchTask::DidReceiveData { { SharedBuffer::create(data, size) }, static_cast<int64_t>(size) }, m_fetchIdentifier); > > > > This is an unnecessary copy of the data. > > There are two call sites which generate a "DidReceiveData" message. Should > there be two separate messages, one that takes a DataReference and one that > takes a SharedDataReference? Or can the FetchTaskClient be modified to > require the caller gives a SharedBuffer? It looks like there's a precambrian explosion of void*/size_t didReceiveData() across all of WebCore, and a random sampling shows that the majority starts off as a RefPtr<SharedBuffer>. So there's a huge opportunity for reducing copies, but also a ton of refactoring work. So I'll just add a DidReceiveRawData() message that takes an IPC::DataReference.
Comment on attachment 391622 [details] Part 1: WebCore View in context: https://bugs.webkit.org/attachment.cgi?id=391622&action=review > Source/WebCore/Modules/encryptedmedia/MediaKeySystemAccess.cpp:89 > + instance->initializeWithConfiguration(*m_configuration, useDistinctiveIdentifier ? AllowDistinctiveIdentifiers::Yes : AllowDistinctiveIdentifiers::No, persistentStateAllowed ? AllowPersistentState::Yes : AllowPersistentState::No, [this, protectedThis = makeRef(*this), useDistinctiveIdentifier, persistentStateAllowed, instance = WTFMove(instance), promise = WTFMove(promise)] (auto successValue) mutable { Wouldn't a weakPtr be better than a refPtr when using the GPU process? > Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:110 > + m_instance->setServerCertificate(WTFMove(certificate), [promise = WTFMove(promise)] (auto success) { Doesn't this need one or the other? > Source/WebCore/platform/encryptedmedia/CDMPrivate.cpp:214 > + persistentStateRequirement = CDMRequirement::NotAllowed; Nit: two spaces. > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:275 > + auto initialize = [&] () { > + if (configuration.distinctiveIdentifier == CDMRequirement::Required) Doesn't this need a protectedThis or weakThis?
(In reply to Eric Carlson from comment #27) > Comment on attachment 391622 [details] > Part 1: WebCore > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391622&action=review > > > Source/WebCore/Modules/encryptedmedia/MediaKeySystemAccess.cpp:89 > > + instance->initializeWithConfiguration(*m_configuration, useDistinctiveIdentifier ? AllowDistinctiveIdentifiers::Yes : AllowDistinctiveIdentifiers::No, persistentStateAllowed ? AllowPersistentState::Yes : AllowPersistentState::No, [this, protectedThis = makeRef(*this), useDistinctiveIdentifier, persistentStateAllowed, instance = WTFMove(instance), promise = WTFMove(promise)] (auto successValue) mutable { > > Wouldn't a weakPtr be better than a refPtr when using the GPU process? It doesn't /really/ need either, if I change this block to take all the arguments it needs an not use `this`. > > Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:110 > > + m_instance->setServerCertificate(WTFMove(certificate), [promise = WTFMove(promise)] (auto success) { > > Doesn't this need one or the other? Nope, it only references the promise inside the block, which was moved in. > > Source/WebCore/platform/encryptedmedia/CDMPrivate.cpp:214 > > + persistentStateRequirement = CDMRequirement::NotAllowed; > > Nit: two spaces. Fixed. > > Source/WebCore/platform/graphics/avfoundation/objc/CDMInstanceFairPlayStreamingAVFObjC.mm:275 > > + auto initialize = [&] () { > > + if (configuration.distinctiveIdentifier == CDMRequirement::Required) > > Doesn't this need a protectedThis or weakThis? Nope. It's run synchronously inside this function.
Created attachment 391979 [details] Patch for landing: Part 1: WebCore
Created attachment 391980 [details] Patch for landing: Part 1: WebCore
Created attachment 391982 [details] Patch for landing: Part 1: WebCore
Created attachment 391985 [details] Patch for landing: Part 1: WebCore
Created attachment 391989 [details] Part 2: WebKit + SharedBuffer
Created attachment 391994 [details] Patch for landing: Part 1: WebCore
Comment on attachment 391989 [details] Part 2: WebKit + SharedBuffer View in context: https://bugs.webkit.org/attachment.cgi?id=391989&action=review > Source/WebKit/ChangeLog:13 > + by the SharedBuffer object at creation time, leading to zero-copy decoding, but is not done in this patch. Wouldn't this still be one-copy because we need to copy it into shared memory when encoding? decodeSharedBuffer is in desperate need of at least a FIXME comment saying that the SharedBuffer should just adopt SharedMemory::Handle as a new type of DataSegment, which would require moving SharedMemory to WebCore. I think we should to that before this to prevent performance regressions, but if you promise to do that in the next week or two I'd be ok with a FIXME comment. > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39 > + : m_buffer(WTFMove(buffer)) > + { > + } We're starting to put these on the line before. : m_buffer(WTFMove(buffer)) { } > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:192 > + auto fileWrapper = adoptNS([pageClient().allocFileWrapperInstance() initRegularFileWithContents:dataReference.buffer()->createNSData().autorelease()]); This used to be a Ref<SharedBuffer> but now it is a RefPtr<SharedBuffer> which would need a null check. > Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:198 > + if (UNLIKELY(m_interceptController.isIntercepting(m_coreLoader->identifier()))) { I'm not a huge fan of all this duplicate code. > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:132 > + m_connection->send(Messages::ServiceWorkerFetchTask::DidReceiveSharedBuffer { { SharedBuffer::create(data, size) }, static_cast<int64_t>(size) }, m_fetchIdentifier); This looks like an unnecessary copy from {data, size} into a SharedBuffer which will then copy it into shared memory to serialize. This at least needs a FIXME comment saying to improve it.
Comment on attachment 391989 [details] Part 2: WebKit + SharedBuffer View in context: https://bugs.webkit.org/attachment.cgi?id=391989&action=review >> Source/WebKit/ChangeLog:13 >> + by the SharedBuffer object at creation time, leading to zero-copy decoding, but is not done in this patch. > > Wouldn't this still be one-copy because we need to copy it into shared memory when encoding? > decodeSharedBuffer is in desperate need of at least a FIXME comment saying that the SharedBuffer should just adopt SharedMemory::Handle as a new type of DataSegment, which would require moving SharedMemory to WebCore. I think we should to that before this to prevent performance regressions, but if you promise to do that in the next week or two I'd be ok with a FIXME comment. I don't this current patch (with the fix you mention below) will result in any extra copies, and with the change you mention here, will definitely reduce copies, with the potential (if both ends are SharedMemory-backed) of zero-copies. So I'm not sure that this needs a FIXME, though it does need a follow up. >> Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39 >> + } > > We're starting to put these on the line before. > : m_buffer(WTFMove(buffer)) { } Ok. >> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:192 >> + auto fileWrapper = adoptNS([pageClient().allocFileWrapperInstance() initRegularFileWithContents:dataReference.buffer()->createNSData().autorelease()]); > > This used to be a Ref<SharedBuffer> but now it is a RefPtr<SharedBuffer> which would need a null check. dataReference.isEmpty() above performs the null-check. >> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:198 >> + if (UNLIKELY(m_interceptController.isIntercepting(m_coreLoader->identifier()))) { > > I'm not a huge fan of all this duplicate code. Okay, I'll abstract it. >> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:132 >> + m_connection->send(Messages::ServiceWorkerFetchTask::DidReceiveSharedBuffer { { SharedBuffer::create(data, size) }, static_cast<int64_t>(size) }, m_fetchIdentifier); > > This looks like an unnecessary copy from {data, size} into a SharedBuffer which will then copy it into shared memory to serialize. This at least needs a FIXME comment saying to improve it. Oh right, this can revert to the DidReceiveData message.
Created attachment 392008 [details] Patch for landing: Part 2: WebKit + SharedBuffer
Comment on attachment 391994 [details] Patch for landing: Part 1: WebCore Clearing flags on attachment: 391994 Committed r257660: <https://trac.webkit.org/changeset/257660>
Comment on attachment 392008 [details] Patch for landing: Part 2: WebKit + SharedBuffer Clearing flags on attachment: 392008 Committed r257667: <https://trac.webkit.org/changeset/257667>
Created attachment 392035 [details] Part 3: RemoteCDM
Created attachment 392048 [details] Part 3: RemoteCDM
Created attachment 392050 [details] Patch
Created attachment 392066 [details] Part 3: RemoteCDM
Comment on attachment 392066 [details] Part 3: RemoteCDM View in context: https://bugs.webkit.org/attachment.cgi?id=392066&action=review > Source/WebKit/ChangeLog:10 > + Allow the existing CDMFactory machinery to work normally by explicitly clearing out all the existing > + when the GPU process is enabled, and resetting those factories to default when the GPU process is disabled. "clearing out all the existing when the GPU process" - all the existing what? > Source/WebKit/GPUProcess/media/RemoteCDMFactoryProxy.cpp:70 > + auto factory = factoryForKeySystem(keySystem); > + if (!factory) { > + completion({ }, { }); > + return; > + } > + > + auto privateCDM = factory->createCDM(keySystem); > + if (!privateCDM) { > + completion({ }, { }); > + return; > + } It is probably worth logging these failures. > Source/WebKit/GPUProcess/media/RemoteCDMFactoryProxy.cpp:144 > +RemoteCDMInstanceProxy* RemoteCDMFactoryProxy::getInstance(const RemoteCDMInstanceIdentifier& id) RemoteCDMInstanceIdentifier id > Source/WebKit/WebProcess/GPU/media/RemoteCDMFactory.cpp:68 > + bool supported = false; > + gpuProcessConnection().connection().sendSync(Messages::RemoteCDMFactoryProxy::SupportsKeySystem(keySystem), Messages::RemoteCDMFactoryProxy::SupportsKeySystem::Reply(supported), { }); > + return supported; Can the key systems a CDM supports change dynamically? If not, we should cache the results to avoid making a sync IPC more than once for a given string. > Source/WebKit/WebProcess/GPU/media/RemoteCDMInstanceSession.h:58 > + Nit: extra blank line > Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:63 > void WebMediaStrategy::registerCDMFactories(Vector<WebCore::CDMFactory*>& factories) > { > - return WebCore::CDMFactory::platformRegisterFactories(factories); > +#if ENABLE(GPU_PROCESS) > + if (m_useGPUProcess) { > + WebProcess::singleton().ensureGPUProcessConnection().cdmFactory().registerFactory(factories); > + return; > + } > +#endif > + WebCore::CDMFactory::platformRegisterFactories(factories); > } Didn't Sam recommend we not use a strategy for this?
<rdar://problem/59955230>
(In reply to Eric Carlson from comment #44) > Comment on attachment 392066 [details] > Part 3: RemoteCDM > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392066&action=review > > > Source/WebKit/ChangeLog:10 > > + Allow the existing CDMFactory machinery to work normally by explicitly clearing out all the existing > > + when the GPU process is enabled, and resetting those factories to default when the GPU process is disabled. > > "clearing out all the existing when the GPU process" - all the existing what? Ah, that's referring to a previous implementation where we would reset the factories when the "use GPU Process for media" setting changed. Now that we're using the PlatformStrategies, we don't need to do this. > > Source/WebKit/GPUProcess/media/RemoteCDMFactoryProxy.cpp:70 > > + auto factory = factoryForKeySystem(keySystem); > > + if (!factory) { > > + completion({ }, { }); > > + return; > > + } > > + > > + auto privateCDM = factory->createCDM(keySystem); > > + if (!privateCDM) { > > + completion({ }, { }); > > + return; > > + } > > It is probably worth logging these failures. Oky. > > Source/WebKit/GPUProcess/media/RemoteCDMFactoryProxy.cpp:144 > > +RemoteCDMInstanceProxy* RemoteCDMFactoryProxy::getInstance(const RemoteCDMInstanceIdentifier& id) > > RemoteCDMInstanceIdentifier id Why? > > Source/WebKit/WebProcess/GPU/media/RemoteCDMFactory.cpp:68 > > + bool supported = false; > > + gpuProcessConnection().connection().sendSync(Messages::RemoteCDMFactoryProxy::SupportsKeySystem(keySystem), Messages::RemoteCDMFactoryProxy::SupportsKeySystem::Reply(supported), { }); > > + return supported; > > Can the key systems a CDM supports change dynamically? The list of keySystems supported (e.g., by FairPlay) is effectively a regular expression. > If not, we should > cache the results to avoid making a sync IPC more than once for a given > string. I think I'd rather modify the DOM code so that we could make SupportsKeySystem asynchronous. > > Source/WebKit/WebProcess/GPU/media/RemoteCDMInstanceSession.h:58 > > + > > Nit: extra blank line Ok. > > Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:63 > > void WebMediaStrategy::registerCDMFactories(Vector<WebCore::CDMFactory*>& factories) > > { > > - return WebCore::CDMFactory::platformRegisterFactories(factories); > > +#if ENABLE(GPU_PROCESS) > > + if (m_useGPUProcess) { > > + WebProcess::singleton().ensureGPUProcessConnection().cdmFactory().registerFactory(factories); > > + return; > > + } > > +#endif > > + WebCore::CDMFactory::platformRegisterFactories(factories); > > } > > Didn't Sam recommend we not use a strategy for this? Ryosuke disagrees, and has a compelling justification.
Comment on attachment 392066 [details] Part 3: RemoteCDM View in context: https://bugs.webkit.org/attachment.cgi?id=392066&action=review >>> Source/WebKit/GPUProcess/media/RemoteCDMFactoryProxy.cpp:144 >>> +RemoteCDMInstanceProxy* RemoteCDMFactoryProxy::getInstance(const RemoteCDMInstanceIdentifier& id) >> >> RemoteCDMInstanceIdentifier id > > Why? To match all of the other RemoteCDMInstanceProxy methods? >>> Source/WebKit/WebProcess/GPU/media/WebMediaStrategy.cpp:63 >>> } >> >> Didn't Sam recommend we not use a strategy for this? > > Ryosuke disagrees, and has a compelling justification. We need a cage match!
Created attachment 392414 [details] Patch for landing: Part 3: WebKit
Created attachment 392422 [details] Patch for landing: Part 3: RemoteCDM
Created attachment 392423 [details] Patch for landing: Part 3: RemoteCDM
Created attachment 392424 [details] Patch for landing: Part 3: RemoteCDM
Created attachment 392427 [details] Patch for landing: Part 3: RemoteCDM
Created attachment 392428 [details] Patch for landing: Part 3: RemoteCDM
Created attachment 392429 [details] Patch for landing: Part 3: RemoteCDM
Comment on attachment 392429 [details] Patch for landing: Part 3: RemoteCDM Clearing flags on attachment: 392429 Committed r257867: <https://trac.webkit.org/changeset/257867>