[webkit-reviews] review granted: [Bug 191316] [EME][GStreamer] Make CDMInstance's available in decryptors, and factor out some EME utility classes. : [Attachment 353979] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 191316: [EME][GStreamer] Make CDMInstance's available in decryptors, and
factor out some EME utility classes.
https://bugs.webkit.org/show_bug.cgi?id=191316

Attachment 353979: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 353979
  --> https://bugs.webkit.org/attachment.cgi?id=353979
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353979&action=review

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-131
2
> -	   else if (gst_structure_has_name(structure, "drm-key-needed")) {
> -	       GST_DEBUG("drm-key-needed message from %s",
GST_MESSAGE_SRC_NAME(message));
> -	       GRefPtr<GstEvent> event;
> -	       gst_structure_get(structure, "event", GST_TYPE_EVENT,
&event.outPtr(), nullptr);
> -	       handleProtectionEvent(event.get());
> -	   } else if (gst_structure_has_name(structure, "drm-waiting-for-key"))
{

We are not handling the events in the decryptor anymore and this is wrong. I
sanctioned removing that code at r231633, bad bad calvaris.

Our problem now is that we are handling one first negotiations, which are the
ones that qtdemux is going to send us. The moment qtdemux has a preferred
system we won't smell any other protection event in regular playback. We need
to add a FIXME in the decryptor to not forget this in the future. I already
opened bug 191355. You might be taking care of this for next reworks but just
in case.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
350
>  #if ENABLE(ENCRYPTED_MEDIA)

It looks like this (and the corresponding #endif) needs to go as well.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1232
> +void
MediaPlayerPrivateGStreamerBase::initializationDataEncountered(InitData&&
initData)
>  {

Please, ASSERT(!isMainThread()); at the beginning of this function. As it is
now, it should only run in the streaming thread of the demuxers, both regular
and MSE playback. I wouldn't to see this function called in the future from the
main thread and have an unnecessary deferral to the main thread.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1275
> -    // FIXME.
> +    bool eventHandled = gst_element_send_event(pipeline(),
gst_event_new_custom(GST_EVENT_CUSTOM_DOWNSTREAM_OOB,
> +	   gst_structure_new("attempt-to-decrypt", "cdm-instance",
G_TYPE_POINTER, m_cdmInstance.get(), nullptr)));
> +    GST_DEBUG("attempting to decrypt, event handled %s",
boolForPrinting(eventHandled));

Good catch.

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:53
> +	   if (!m_systemId.isEmpty() && initData.m_systemId ==
GST_PROTECTION_UNSPECIFIED_SYSTEM_ID && initData.m_systemId != m_systemId) {

Is this right? Can you only bail out here if you have WebM?

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:89
> +    String m_systemId;
> +    RefPtr<SharedBuffer> m_initData;

This can actually be called m_payload

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:105
> +	      
m_events.append(GRefPtr<GstEvent>(static_cast<GstEvent*>(g_value_get_boxed(gst_
value_list_get_value(streamEncryptionEventsList, i)))));

Capacity is reverved, you can do a faster uncheckedAppend.

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:110
> +	   if (streamEncryptionAllowedSystems) {
> +	       for (unsigned i = 0; streamEncryptionAllowedSystems[i]; ++i)
> +		  
m_availableSystems.append(streamEncryptionAllowedSystems[i]);

We might reserve initial capacity here and then uncheckedAppend.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:305
> +    const GstStructure* structure = gst_event_get_structure(event);

This can be done where it's used.

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:308
>      case GST_EVENT_CUSTOM_DOWNSTREAM_OOB: {

Could you please add a FIXME referring to bug 191355 for regular downstream
events?

>
Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorG
Streamer.cpp:315
> +	   gst_structure_get(structure, "cdm-instance", G_TYPE_POINTER,
&priv->cdmInstance, nullptr);
> +	   if (!priv->cdmInstance) {
> +	       GST_ERROR_OBJECT(self, "No CDM instance received");
> +	       result = FALSE;
> +	       break;
> +	   }
> +	   GST_DEBUG_OBJECT(self, "received a cdm instance %p",
priv->cdmInstance.get());

For the near future. This should be set as a context in the pipeline and we
should run a need-context message for the cdm-instance