[webkit-reviews] review denied: [Bug 185245] [EME][GStreamer] Add a handler for GStreamer protection event : [Attachment 339550] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Yacine Bandou
<bandou.yacine at gmail.com>'s request for review:
Bug 185245: [EME][GStreamer] Add a handler for GStreamer protection event
https://bugs.webkit.org/show_bug.cgi?id=185245

Attachment 339550: Patch

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




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

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

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
1288
> +    const gchar* eventKeySystemId = nullptr;
> +    GstBuffer* data = nullptr;
> +    Vector<uint8_t> concatenatedInitDataChunks;
> +
>      if (m_handledProtectionEvents.contains(GST_EVENT_SEQNUM(event))) {
>	   GST_DEBUG_OBJECT(pipeline(), "event %u already handled",
GST_EVENT_SEQNUM(event));
> -	   m_handledProtectionEvents.remove(GST_EVENT_SEQNUM(event));
> -	   if (m_needToResendCredentials) {
> -	       GST_DEBUG_OBJECT(pipeline(), "resending credentials");
> -	       attemptToDecryptWithLocalInstance();
> -	   }
>	   return;
>      }
>  
> -    const gchar* eventKeySystemId = nullptr;
> -    gst_event_parse_protection(event, &eventKeySystemId, nullptr, nullptr);
> -    GST_WARNING("FIXME: unhandled protection event for %s",
eventKeySystemId);
> -    ASSERT_NOT_REACHED();
> +    gst_event_parse_protection(event, &eventKeySystemId, &data, nullptr);
> +    if (m_cdmInstance &&
strcmp(GStreamerEMEUtilities::keySystemToUuid(m_cdmInstance->keySystem()),
eventKeySystemId))
> +	   return;
> +
> +    GstMapInfo mapInfo;
> +    if (!gst_buffer_map(data, &mapInfo, GST_MAP_READ)) {
> +	   GST_WARNING("cannot map %s protection data", eventKeySystemId);
> +	   return;
> +    }
> +
> +    GST_DEBUG("scheduling Protection event for %s with init data size of
%u", eventKeySystemId, mapInfo.size);
> +    GST_MEMDUMP("init datas", mapInfo.data, mapInfo.size);
> +
> +    concatenatedInitDataChunks.append(mapInfo.data, mapInfo.size);
> +    m_handledProtectionEvents.add(GST_EVENT_SEQNUM(event));
> +    gst_buffer_unmap(data, &mapInfo);
> +
> +    RunLoop::main().dispatch([this, eventKeySystemId, initData =
WTFMove(concatenatedInitDataChunks)] {
> +	   GST_DEBUG("scheduling OnEncrypted event for %s with concatenated
init datas size of %" G_GSIZE_FORMAT, eventKeySystemId, initData.size());
> +	   GST_MEMDUMP("init datas", initData.data(), initData.size());
> +	   this->m_player->initializationDataEncountered(ASCIILiteral("cenc"),
ArrayBuffer::create(initData.data(), initData.size()));
> +    });

This is a nice piece of code though it should belong in other patch.

You will need to get rid of some variables that are not needed anymore. I'd
recommend you base your code in
https://github.com/WebPlatformForEmbedded/WPEWebKit/blob/wpe-20170728/Source/We
bCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp#L1441 and
since that code is mine, authorship should be at least shared with me. We'll
find somebody else to do the formal r+.

So summing up, please move this out of this patch and since this patch should
depend on that new one, add the dependency.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1100
> +	   if (appendPipeline && appendPipeline->playerPrivate())
> +	       appendPipeline->playerPrivate()->handleProtectionEvent(event);
> +	   break;

I think it is better to return GST_PAD_PROBE_DROP since the protection event
should die here and be moved to the PlaybackPipeline.