[webkit-reviews] review granted: [Bug 208090] [GPUP] Implement Modern EME API in the GPU Process : [Attachment 392066] Part 3: RemoteCDM

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 208090: [GPUP] Implement Modern EME API in the GPU Process
https://bugs.webkit.org/show_bug.cgi?id=208090

Attachment 392066: Part 3: RemoteCDM

https://bugs.webkit.org/attachment.cgi?id=392066&action=review




--- Comment #44 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 392066
  --> https://bugs.webkit.org/attachment.cgi?id=392066
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::S
upportsKeySystem(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().registerFacto
ry(factories);
> +	   return;
> +    }
> +#endif
> +    WebCore::CDMFactory::platformRegisterFactories(factories);
>  }

Didn't Sam recommend we not use a strategy for this?