Bug 191316 – [EME][GStreamer] Make CDMInstance's available in decryptors, and factor out some EME utility classes.

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Charlie Turner]

[EME][GStreamer] Make CDMInstance's available in decryptors, and factor out some EME utility classes.

[Charlie Turner]

Created attachment 353979 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 353979 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1312
> -        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/WebKitCommonEncryptionDecryptorGStreamer.cpp:305
> +    const GstStructure* structure = gst_event_get_structure(event);

This can be done where it's used.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.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/WebKitCommonEncryptionDecryptorGStreamer.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

[Charlie Turner]

Comment on attachment 353979 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1312
>> -        } 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.

I'm not sure why you wrote this comment on this hunk. I'm also uncomfortable with the way qtdemux will stop informing us of protection events, and the fix for this feels like it should perhaps be made to qtdemux to request context with new protection events, rather than handling them in a second place, which has some other inherent problems, such as knowing in the decrytor whether the demuxer has already caused WebCore to fire onEncrypted by the time we get the events in the decryptor.

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

Whoops, will fix.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1232
>>  {
> 
> 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.

Will add the assert.

>> 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?

This is a bit hairy, and I don't like the lack of information we're getting from GStreamer here. During the refactoring, my understanding of the numerous places we were checking GST_PROTECTION_UNSPECIFIED_SYSTEM_ID was just so we can correctly communicate the init data type in EME spec speak. This method is providing an init data builder function, for concatenating init datas. The idea of the check is not to allow users to append payloads from WebM streams when they've already appended payloads from MP4 streams (cenc). It's more of an assertion, and I can't see how it would happen, but the previous code was making these checks, so I tried to keep them. It does occur to me that I should extend the check in the case that the new init data is from MP4 and we've already appending something from WebM...

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

Ah yes, good catch.

>> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:110
>> +                m_availableSystems.append(streamEncryptionAllowedSystems[i]);
> 
> We might reserve initial capacity here and then uncheckedAppend.

Since we don't have a size upfront in this case, I opted to just use plain appends. We'd have to traverse the streamEncryptionAllowedSystems to reserveInitialCapacity.

>> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:308
>>      case GST_EVENT_CUSTOM_DOWNSTREAM_OOB: {
> 
> Could you please add a FIXME referring to bug 191355 for regular downstream events?

I'll add it, still don't completely understand the failure case here however.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #3)
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1312
> >> -        } 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.
> 
> I'm not sure why you wrote this comment on this hunk.

Because it would be the other part of the puzzle. Decryptors send events thru this message and we would handle them here.

> I'm also uncomfortable
> with the way qtdemux will stop informing us of protection events, and the
> fix for this feels like it should perhaps be made to qtdemux to request
> context with new protection events, rather than handling them in a second
> place, which has some other inherent problems, such as knowing in the
> decrytor whether the demuxer has already caused WebCore to fire onEncrypted
> by the time we get the events in the decryptor.

It has nothing unconfortable. Sending the events down the pipeline and handle them in the decryptors would be the logical way. The demuxer selects the decryptor and forwards the events to it. The context query is a way to comply with the W3C requirement of knowing the system in advance and context queries run only when the context is unknown. Our context here is the key system which is already known the second time. Running the events here is actually a necessary side effect of requesting a CDMInstance. Actually, should we have the CDM instance in advance, we should just tell the demuxer and bail out, then wait ofr the events to come and handle them at the decryptors.

> 
> >> 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?
> 
> This is a bit hairy, and I don't like the lack of information we're getting
> from GStreamer here. During the refactoring, my understanding of the
> numerous places we were checking GST_PROTECTION_UNSPECIFIED_SYSTEM_ID was
> just so we can correctly communicate the init data type in EME spec speak.
> This method is providing an init data builder function, for concatenating
> init datas. The idea of the check is not to allow users to append payloads
> from WebM streams when they've already appended payloads from MP4 streams
> (cenc). It's more of an assertion, and I can't see how it would happen, but
> the previous code was making these checks, so I tried to keep them. It does
> occur to me that I should extend the check in the case that the new init
> data is from MP4 and we've already appending something from WebM...

Yes, I understand the purpose. My concern is that bail out condition is right. If you tell me it is, then we good.


> >> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h:110
> >> +                m_availableSystems.append(streamEncryptionAllowedSystems[i]);
> > 
> > We might reserve initial capacity here and then uncheckedAppend.
> 
> Since we don't have a size upfront in this case, I opted to just use plain
> appends. We'd have to traverse the streamEncryptionAllowedSystems to
> reserveInitialCapacity.

Ok.

> >> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:308
> >>      case GST_EVENT_CUSTOM_DOWNSTREAM_OOB: {
> > 
> > Could you please add a FIXME referring to bug 191355 for regular downstream events?
> 
> I'll add it, still don't completely understand the failure case here however.

I think I commented it above. Let me know if you need further clarification.

[Charlie Turner]

Created attachment 354338 [details]
Patch

[Charlie Turner]

I hit some interesting test failures that guided a new bit of functionality in encountering init data in this new patch. Specifically initializationDataEncountered now has a flag to stop it checking if the system ids are different from the CDM. Since we can get a CDM before we've seen any init datas, and we must always fire an encrypted event, even if the init datas are of no use to JS.

[EWS Watchlist]

Comment on attachment 354338 [details]
Patch

Attachment 354338 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9924296

New failing tests:
http/wpt/css/css-animations/start-animation-001.html

[EWS Watchlist]

Created attachment 354351 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Xabier Rodríguez Calvar]

Comment on attachment 354338 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +        gstreamer, since it was not clear what that code was doing unless

Nit: GStreamer

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:391
> +            // FIXME: This is not not firing for clearkey tests, we
> +            // always receive zero available decryptors, so in turn
> +            // the m_handledProtectionEvents is useless, since it
> +            // never contains anything...

The idea behind this is that you keep track of which evens we handle here and which events we handle in the decryptor (when they come back to us thru the handleProtectionEvent logic that is not in used anymore). Considering that that part of the puzzle is currently missing and if we get it back we might do it in a different way, you can remove this and see if anything breaks.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1244
> +    if (!isFirstEncounter && isInitDataForDifferentSystem(initData)) {

This and the former behavior (my fault and probably what is driving us here) are out of the spec. https://www.w3.org/TR/encrypted-media/#initdata-encountered says that we should always fire those events when we find the data in the container so I am now in favor of removing this checks and firing everything we find. Can you please try without this code and see if anything breaks? If nothing breaks then I would remove this check. We might  even be able of getting rid of the UUIDs when handling init datas at this level.

[Charlie Turner]

Created attachment 354548 [details]
Patch

[Charlie Turner]

Indeed, a lot of those checks were not necessary, at least insofar as no tests break when they are removed, so I've done that now.

[Xabier Rodríguez Calvar]

Comment on attachment 354548 [details]
Patch

Patch is code ready, but I think you need to rebase, probably because of me . Sorry for that.

[Charlie Turner]

Created attachment 354665 [details]
Patch for landing

[Charlie Turner]

Created attachment 354666 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354666 [details]
Patch for landing

Rejecting attachment 354666 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 354666, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=354666&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=191316&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 354666 from bug 191316.
Fetching: https://bugs.webkit.org/attachment.cgi?id=354666
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 7 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/ChangeLog.rej
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h
patching file Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.h
patching file Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp
patching file Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9972633

[Charlie Turner]

Created attachment 354667 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354667 [details]
Patch for landing

Clearing flags on attachment: 354667

Committed r238131: <https://trac.webkit.org/changeset/238131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.