Bug 208090 – [GPUP] Implement Modern EME API in the GPU Process

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[GPUP] Implement Modern EME API in the GPU Process

[Jer Noble]

Created attachment 391455 [details]
WIP

[Jer Noble]

Created attachment 391460 [details]
WIP

[Jer Noble]

Created attachment 391463 [details]
WIP

[Jer Noble]

Created attachment 391464 [details]
WIP

[Jer Noble]

Created attachment 391467 [details]
WIP

[Jer Noble]

Created attachment 391473 [details]
WIP

[Jer Noble]

Created attachment 391509 [details]
WIP

[Jer Noble]

Created attachment 391510 [details]
WIP

[Jer Noble]

Created attachment 391511 [details]
Part 1: WebCore

[Jer Noble]

Created attachment 391512 [details]
Part 2: WebKit + SharedBuffer

[Jer Noble]

Created attachment 391513 [details]
Part 3: RemoteCDM (Depends on Parts 1 & 2)

[Jer Noble]

Created attachment 391514 [details]
Part 1: WebCore

[Jer Noble]

Created attachment 391515 [details]
Part 2: WebKit + SharedBuffer

[Sam Weinig]

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?

[Jer Noble]

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

[Jer Noble]

...(continued)

I'm not saying that work is impossible, and I do plan on converting sync messages to asyncs in future patches.

[Jer Noble]

Created attachment 391534 [details]
Part 1: WebCore

[Jer Noble]

...or maybe I can move all of the CDM::getSupportedConfiguration into platform and then into the GPUProcess.

[Jer Noble]

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

[Jer Noble]

Created attachment 391622 [details]
Part 1: WebCore

[Jer Noble]

Created attachment 391623 [details]
Part 2: WebKit + SharedBuffer

[Jer Noble]

Created attachment 391625 [details]
Part 3: RemoteCDM (depends on Parts 1 & 2)

[Jer Noble]

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.

[Alex Christensen]

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!

[Jer Noble]

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

[Jer Noble]

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

[Eric Carlson]

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?

[Jer Noble]

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

[Jer Noble]

Created attachment 391979 [details]
Patch for landing: Part 1: WebCore

[Jer Noble]

Created attachment 391980 [details]
Patch for landing: Part 1: WebCore

[Jer Noble]

Created attachment 391982 [details]
Patch for landing: Part 1: WebCore

[Jer Noble]

Created attachment 391985 [details]
Patch for landing: Part 1: WebCore

[Jer Noble]

Created attachment 391989 [details]
Part 2: WebKit + SharedBuffer

[Jer Noble]

Created attachment 391994 [details]
Patch for landing: Part 1: WebCore

[Alex Christensen]

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.

[Jer Noble]

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.

[Jer Noble]

Created attachment 392008 [details]
Patch for landing: Part 2: WebKit + SharedBuffer

[Jer Noble]

Comment on attachment 391994 [details]
Patch for landing: Part 1: WebCore

Clearing flags on attachment: 391994

Committed r257660: <https://trac.webkit.org/changeset/257660>

[Jer Noble]

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>

[Jer Noble]

Created attachment 392035 [details]
Part 3: RemoteCDM

[Jer Noble]

Created attachment 392048 [details]
Part 3: RemoteCDM

[Jer Noble]

Created attachment 392050 [details]
Patch

[Jer Noble]

Created attachment 392066 [details]
Part 3: RemoteCDM

[Eric Carlson]

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?

[Radar WebKit Bug Importer]

<rdar://problem/59955230>

[Jer Noble]

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

[Eric Carlson]

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!

[Jer Noble]

Created attachment 392414 [details]
Patch for landing: Part 3: WebKit

[Jer Noble]

Created attachment 392422 [details]
Patch for landing: Part 3: RemoteCDM

[Jer Noble]

Created attachment 392423 [details]
Patch for landing: Part 3: RemoteCDM

[Jer Noble]

Created attachment 392424 [details]
Patch for landing: Part 3: RemoteCDM

[Jer Noble]

Created attachment 392427 [details]
Patch for landing: Part 3: RemoteCDM

[Jer Noble]

Created attachment 392428 [details]
Patch for landing: Part 3: RemoteCDM

[Jer Noble]

Created attachment 392429 [details]
Patch for landing: Part 3: RemoteCDM

[WebKit Commit Bot]

Comment on attachment 392429 [details]
Patch for landing: Part 3: RemoteCDM

Clearing flags on attachment: 392429

Committed r257867: <https://trac.webkit.org/changeset/257867>