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
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.
(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.
Created attachment 339550 [details] Patch
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.
(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.
Created attachment 339713 [details] Patch
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
(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
(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.
(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.
Created attachment 339813 [details] Patch
Created attachment 339948 [details] Patch
Comment on attachment 339813 [details] Patch Clearing flags on attachment: 339813 Committed r231636: <https://trac.webkit.org/changeset/231636>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40121157>
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.
(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
(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.