Bug 185245 – [EME][GStreamer] Add a handler for GStreamer protection event

• Component: Media
• Status: RESOLVED FIXED

[Yacine Bandou]

Qtdemux send the protection event when encountered a new PSSH box (encrypted content).
    
The Decryptor is moved from AppendPipeline to PlaybackPipeline (see https://bugs.webkit.org/show_bug.cgi?id=181855),
thus the protection event is no longer handled because the Decryptor is not in the same pipeline as qtdemux.
    
AppendPipeline: httpsrc-->qtdemux-->appsink
PlaybackPipeline: appsrc-->parser--> decryptor-->decoder-->sink
    
In order to handle the event we should dispatch the event from the AppendPipeline to the application.
For that we need this patch in GStreamer (gst-plugins-base) https://bugzilla.gnome.org/show_bug.cgi?id=785531

[Xabier Rodríguez Calvar]

IMHO, as I already said in the GStreamer bug, we can live perfectly without that patch by attaching a probe to the sink pad of the appsink in the append pipeline. I don't know if or when that GStreamer patch will land so I'd go for the probe solution in WK.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #1)
> IMHO, as I already said in the GStreamer bug, we can live perfectly without
> that patch by attaching a probe to the sink pad of the appsink in the append
> pipeline. I don't know if or when that GStreamer patch will land so I'd go
> for the probe solution in WK.

I agree, I'll see how I'll change my patch.

[Yacine Bandou]

Created attachment 339550 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 339550 [details]
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/WebCore/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.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #4)
> Comment on attachment 339550 [details]
> 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;

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

I agree, I'll do it

> 
> 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/
> WebCore/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.

Ok it's noted.
I hesitated to put GST_PAD_PROBE_OK or GST_PAD_PROBE_DROP for the protection event.

[Yacine Bandou]

Created attachment 339713 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 339713 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-1270
> -    ASSERT_NOT_REACHED();

Please, leave this outside the patch. If you need this out of here, create a new bug and set a dependency to this one.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1093
> +    GST_DEBUG("Handling event %s on appendPipeline appsinkPad", GST_EVENT_TYPE_NAME(event));

appendPipeline -> append pipeline

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #7)
> Comment on attachment 339713 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339713&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-1270
> > -    ASSERT_NOT_REACHED();

Just did it for you

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #8)
> Just did it for you

Well, no, I just added the dependency on the tests because they also need this if I am not mistaken.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #7)
> Comment on attachment 339713 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339713&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-1270
> > -    ASSERT_NOT_REACHED();
> 
> Please, leave this outside the patch. If you need this out of here, create a
> new bug and set a dependency to this one.

The most WPT tests will crash without remove this ASSERT, because now we reach this code with this patch.

I do it in the bug 185420.

[Yacine Bandou]

Created attachment 339813 [details]
Patch

[Yacine Bandou]

Created attachment 339948 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339813 [details]
Patch

Clearing flags on attachment: 339813

Committed r231636: <https://trac.webkit.org/changeset/231636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40121157>

[Yacine Bandou]

I noted that you changed the patch. You didn't land the last pushed patch and you landed the patch which has dependency with this bug 185420.

That provoked the crashes of encrypted-media layoutTests in debug mode as i commented in c10.

[Yacine Bandou]

(In reply to Yacine Bandou from comment #16)
> I noted that you changed the patch. You didn't land the last pushed patch
> and you landed the patch which has dependency with this bug 185420.
> 
> That provoked the crashes of encrypted-media layoutTests in debug mode as i
> commented in c10.


Now, to fix these crashes, we should take this patch 185535.

I think there was a misunderstanding, now I implemented the patch that planned in 185420 in an other one 185535, 
in order to avoid all other misunderstanding.


For more detail, here is the history of this patch: 

1. I pushed a patch that adds a probe for GStreamer protection event and adds a handler for this event in MediaPlayerPrivate (Patch 1)

2. Calvaris asked me to split the code in two parts : 
   - A probe to catch the GStreamer protection event
   - A handler of  the event in MediaPlayerPrivate

3. I removed the handler of the event in MediaPlayerPrivate from this patch1  and I replaced it by removing the ASSERT_NOT_REACHED 
   because it is needed in debug mode. (patch 2)
 
4. Calvaris asked me to move the delete of the ASSERT in an other bug

5. I moved the ASSERT removal in the bug 185420 (patch 3)

6. Calvaris prefers to implement the handler of the protection event in bug 185420 instead to just remove the ASSERT. 

7. In order to take more time to implement the patch 185420. I made these two patches independent by removing the call of 
   MediaPlayerPrivate in the probe of the protection event. (Patch 4).

I close the bug 185420

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #16)
> I noted that you changed the patch. You didn't land the last pushed patch
> and you landed the patch which has dependency with this bug 185420.
> 
> That provoked the crashes of encrypted-media layoutTests in debug mode as i
> commented in c10.

Yes, I told you in private about this and asked you about the best way to land these patches to avoid issues, which I followed.