Bug 189239 – [EME][GStreamer] Add support for WebM encrypted caps "application/x-webm-enc"

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Yacine Bandou]

Add the support of WebM encrypted caps "application/x-webm-enc"
Tests:
 media/encrypted-media/clearKey/clearKey-encrypted-webm-event-mse.html
 media/encrypted-media/clearKey/clearKey-webm-video-playback-mse.html

[Yacine Bandou]

Created attachment 348768 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 348768 [details]
Patch

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

Wrote some comments on https://bugzilla.gnome.org/show_bug.cgi?id=765275 cause I don't like the "UNDEFINED" API.

> Source/WebCore/ChangeLog:11
> +        see https://bugzilla.gnome.org/attachment.cgi?id=365211

Nit: There's a period missing at the end of this line.

[Yacine Bandou]

Created attachment 351539 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 351539 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        in case of WebM, for details, see https://bugzilla.gnome.org/attachment.cgi?id=365211

period at the end.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:179
> +    return (gst_structure_has_name(structure, "application/x-cenc") || gst_structure_has_name(structure, "application/x-webm-enc"));

You don't need the () enclosing the two checks.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1295
> +        if (eventKeySystemUUID == GST_PROTECTION_UNSPECIFIED_SYSTEM_ID)
> +            weakThis->m_player->initializationDataEncountered("webm"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));
> +        else
> +            weakThis->m_player->initializationDataEncountered("cenc"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));

You can use a ? b : c instead of an if.

I have also a concern of what we are considering here: if system id is unspecified then we consider webm, cenc otherwise, which means that cenc needs to enforce a sys id and WebM will never have one. I guess the later is true by per spec but I wonder if that assumtion of cenc is true.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1296
> +

This line should be removed.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:61
> +    "application/x-cenc, original-media-type=(string)audio/mpeg, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID";"
> +    "application/x-webm-enc, original-media-type=(string)video/x-vp8;"
> +    "application/x-webm-enc, original-media-type=(string)video/x-vp9;"));

Can WebM handle cenc? If it can then we have an issue with this.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:165
> +            if (!g_strcmp0(klass->protectionSystemId, GST_PROTECTION_UNSPECIFIED_SYSTEM_ID))
> +                gst_structure_set_name(outgoingStructure.get(), "application/x-webm-enc");
> +            else
> +                gst_structure_set_name(outgoingStructure.get(), "application/x-cenc");

Ditto for a ? b : c.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #4)
> Comment on attachment 351539 [details]
> Patch
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1295
> > +        if (eventKeySystemUUID == GST_PROTECTION_UNSPECIFIED_SYSTEM_ID)
> > +            weakThis->m_player->initializationDataEncountered("webm"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));
> > +        else
> > +            weakThis->m_player->initializationDataEncountered("cenc"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));
> 
> You can use a ? b : c instead of an if.
> 
> I have also a concern of what we are considering here: if system id is
> unspecified then we consider webm, cenc otherwise, which means that cenc
> needs to enforce a sys id and WebM will never have one. I guess the later is
> true by per spec but I wonder if that assumtion of cenc is true.
> 

CENC specifies the system ID and it is mandatory in PSSH Box.


Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:61
> > +    "application/x-cenc, original-media-type=(string)audio/mpeg, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID";"
> > +    "application/x-webm-enc, original-media-type=(string)video/x-vp8;"
> > +    "application/x-webm-enc, original-media-type=(string)video/x-vp9;"));
> 
> Can WebM handle cenc? If it can then we have an issue with this.
>

Why do you say that? It is "x-webm-enc" not "x-webm-cenc".

[Yacine Bandou]

Created attachment 351599 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 351599 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1292
> +        (eventKeySystemUUID == GST_PROTECTION_UNSPECIFIED_SYSTEM_ID) ? weakThis->m_player->initializationDataEncountered("webm"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes())) : weakThis->m_player->initializationDataEncountered("cenc"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));

It would be much simpler to write:

weakThis->m_player->initializationDataEncountered(eventKeySystemUUID == GST_PROTECTION_UNSPECIFIED_SYSTEM_ID) ? "webm"_s : "cenc"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:162
> +            (!g_strcmp0(klass->protectionSystemId, GST_PROTECTION_UNSPECIFIED_SYSTEM_ID)) ? gst_structure_set_name(outgoingStructure.get(), "application/x-webm-enc") : gst_structure_set_name(outgoingStructure.get(), "application/x-cenc");

gst_structure_set_name(outgoingStructure.get(), g_strcmp0(klass->protectionSystemId, GST_PROTECTION_UNSPECIFIED_SYSTEM_ID) ? "application/x-cenc" : "application/x-webm-enc");

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #5)
> Source/WebCore/platform/graphics/gstreamer/eme/
> WebKitClearKeyDecryptorGStreamer.cpp:61
> > > +    "application/x-cenc, original-media-type=(string)audio/mpeg, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID";"
> > > +    "application/x-webm-enc, original-media-type=(string)video/x-vp8;"
> > > +    "application/x-webm-enc, original-media-type=(string)video/x-vp9;"));
> > 
> > Can WebM handle cenc? If it can then we have an issue with this.
> >
> 
> Why do you say that? It is "x-webm-enc" not "x-webm-cenc".

What I mean is that if WebM supports cenc, we should provide a media type for application/x-cenc, original-media-type=(string)video/x-vp8 and vp9.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #8)
> (In reply to Yacine Bandou from comment #5)
> > Source/WebCore/platform/graphics/gstreamer/eme/
> > WebKitClearKeyDecryptorGStreamer.cpp:61
> > > > +    "application/x-cenc, original-media-type=(string)audio/mpeg, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID";"
> > > > +    "application/x-webm-enc, original-media-type=(string)video/x-vp8;"
> > > > +    "application/x-webm-enc, original-media-type=(string)video/x-vp9;"));
> > > 
> > > Can WebM handle cenc? If it can then we have an issue with this.
> > >
> > 
> > Why do you say that? It is "x-webm-enc" not "x-webm-cenc".
> 
> What I mean is that if WebM supports cenc, we should provide a media type
> for application/x-cenc, original-media-type=(string)video/x-vp8 and vp9.

No, it is not the same thing.

For example: The PSSH box is mandatory in CENC protection scheme.

If we set the caps "application/x-cenc, original-media-type=(string)video/x-vp9" for encrypted WebM/VP9. How we can differentiate it from encrypted MP4/VP9 ?
For you information, MP4 supports the VP8 and VP9 codec (http://mp4ra.org/#/codecs).

see also these two links:
https://www.w3.org/TR/eme-stream-webm/
https://www.w3.org/TR/eme-stream-mp4/

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #7)
> Comment on attachment 351599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351599&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:1292
> > +        (eventKeySystemUUID == GST_PROTECTION_UNSPECIFIED_SYSTEM_ID) ? weakThis->m_player->initializationDataEncountered("webm"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes())) : weakThis->m_player->initializationDataEncountered("cenc"_s, ArrayBuffer::create(reinterpret_cast<const uint8_t*>(initData.characters8()), initData.sizeInBytes()));
> 
> It would be much simpler to write:
> 
> weakThis->m_player->initializationDataEncountered(eventKeySystemUUID ==
> GST_PROTECTION_UNSPECIFIED_SYSTEM_ID) ? "webm"_s : "cenc"_s,
> ArrayBuffer::create(reinterpret_cast<const
> uint8_t*>(initData.characters8()), initData.sizeInBytes()));
> 

Absolutely.

[Yacine Bandou]

Created attachment 351765 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Yacine Bandou from comment #9)
> > What I mean is that if WebM supports cenc, we should provide a media type
> > for application/x-cenc, original-media-type=(string)video/x-vp8 and vp9.
> 
> No, it is not the same thing.
> 
> For example: The PSSH box is mandatory in CENC protection scheme.
> 
> If we set the caps "application/x-cenc,
> original-media-type=(string)video/x-vp9" for encrypted WebM/VP9. How we can
> differentiate it from encrypted MP4/VP9 ?
> For you information, MP4 supports the VP8 and VP9 codec
> (http://mp4ra.org/#/codecs).

This is exactly my question, how can we differentiate them. I guess you're assuming that in the case of mp4 with VP8/9 what the demuxer would report would be application/x-cenc, original-media-type=(string)video/mpeg ?

[Xabier Rodríguez Calvar]

Comment on attachment 351765 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:163
> +                (!g_strcmp0(klass->protectionSystemId, GST_PROTECTION_UNSPECIFIED_SYSTEM_ID)) ? "application/x-webm-enc" : "application/x-cenc");

You don't need the () around g_strcmp0.

[Yacine Bandou]

(In reply to Xabier Rodríguez Calvar from comment #12)
> (In reply to Yacine Bandou from comment #9)
> > > What I mean is that if WebM supports cenc, we should provide a media type
> > > for application/x-cenc, original-media-type=(string)video/x-vp8 and vp9.
> > 
> > No, it is not the same thing.
> > 
> > For example: The PSSH box is mandatory in CENC protection scheme.
> > 
> > If we set the caps "application/x-cenc,
> > original-media-type=(string)video/x-vp9" for encrypted WebM/VP9. How we can
> > differentiate it from encrypted MP4/VP9 ?
> > For you information, MP4 supports the VP8 and VP9 codec
> > (http://mp4ra.org/#/codecs).
> 
> This is exactly my question, how can we differentiate them. I guess you're
> assuming that in the case of mp4 with VP8/9 what the demuxer would report
> would be application/x-cenc, original-media-type=(string)video/mpeg ?

Encrypted MP4/VP9 --> qtdemux with src caps "application/x-cenc, original-media-type=(string)video/VP9".
Encrypted WebM/VP9 --> Matroskademux with src caps "application/x-webm-enc, original-media-type=(string)video/VP9".

[Yacine Bandou]

Created attachment 351771 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 351771 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitClearKeyDecryptorGStreamer.cpp:61
>      GST_STATIC_CAPS("application/x-cenc, original-media-type=(string)video/x-h264, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID "; "
> -    "application/x-cenc, original-media-type=(string)audio/mpeg, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID));
> +    "application/x-cenc, original-media-type=(string)audio/mpeg, protection-system=(string)" WEBCORE_GSTREAMER_EME_UTILITIES_CLEARKEY_UUID";"
> +    "application/x-webm-enc, original-media-type=(string)video/x-vp8;"
> +    "application/x-webm-enc, original-media-type=(string)video/x-vp9;"));

(In reply to Yacine Bandou from comment #14)
> Encrypted MP4/VP9 --> qtdemux with src caps "application/x-cenc,
> original-media-type=(string)video/VP9".

Again, why did  you decide to leave outside the question "application/x-cenc, original-media-type=(string)video/VP9"  cause from current code I see it is not supported. Well, anyway, this would not belong to this patch, so I'll r+ it.

[WebKit Commit Bot]

Comment on attachment 351771 [details]
Patch

Clearing flags on attachment: 351771

Committed r236912: <https://trac.webkit.org/changeset/236912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.