GNOME Bugzilla – Bug 765275
matroska: Add encrypted content support in matroskademux
Last modified: 2018-10-03 17:32:27 UTC
Patch coming
Created attachment 326357 [details] [review] matroska: implement reading & writing ContentEncryption headers
+Philippe https://bugzilla.gnome.org/show_bug.cgi?id=762915 ?
Comment on attachment 326357 [details] [review] matroska: implement reading & writing ContentEncryption headers This should be integrated with the protection API (GstProtectionMeta, GST_EVENT_PROTECTION), and plug decryptors similar to qtdemux.
Created attachment 326360 [details] [review] matroska: implement reading & writing ContentEncryption headers
*** Bug 762915 has been marked as a duplicate of this bug. ***
I am working to integrate the protection API in matroska demux, like is done in qtdemux. Could you suggest me the output caps for encrypted content in srcpad ? What do you think about this? application/x-matroska-enc, original-media-type:video/x-vp9,....
That sounds terribly wrong to me. If you must change the caps, I think the way forward would be to add a GstCapsFeature called meta:GstProtectionMeta or something like that. That being said, I cannot see any caps change being done in qtdemux.
When the content is protected, qtdemux calls the function gst_qtdemux_configure_protected_caps, which configure the srcpad caps properly. For example: application/x-cenc, original-media-type=(string)video/x-h264,... in case of h264. So, I don't understand when you say that the qtdemux doesn't change the caps
(In reply to y.bandou from comment #8) > When the content is protected, qtdemux calls the function > gst_qtdemux_configure_protected_caps, which configure the srcpad caps > properly. > > For example: > application/x-cenc, original-media-type=(string)video/x-h264,... in case of > h264. > > So, I don't understand when you say that the qtdemux doesn't change the caps Ah ok, I'm sorry. I hadn't noticed this part. I'm not sure then.
*** Bug 784224 has been marked as a duplicate of this bug. ***
Created attachment 354560 [details] [review] matroska: implement reading & writing ContentEncryption headers (reworked) Rework the last patch in order to add later the support for encrypted content with protection API as qtdemux.
Created attachment 354562 [details] [review] matroska: Add support for encrypted content with protection API This patch: 1.Parse the WebM ContentEncryption subelements. 2.Create a GST_PROTECTION event for each ContentEncryption, which will be sent before the first source buffer. The GST_PROTECTION event doesn't contain the "system_id", DRM system id, field because it hasn't been specified neither by Matroska nor by the WebM spec. For this reason we make DRM system id optional in GST_PROTECTION event. See: https://bugzilla.gnome.org/show_bug.cgi?id=784129 3.Parse the protection information of encrypted Block/SimpleBlock, extract the IV and the partitioning format (subsamples). 4.Create the metadata protection for each encrypted Block/SimpleBlock, with those informations: KeyID (extracted from ContentEncryption element), IV and partitioning format. The protection metadata follows the same format as qtdemux.
Do you have any suggestions or comments about this last patch?
How are you testing this? Can you think of a way to unit test it?
One thing I don't particularly like about this patch is that it is webm-specific. Matroska is meant to be more abstract than that. The way I see it, matroska elements should only be dealing with matroska headers, not with the way data is formatted inside a data block (which is defined in the webm spec). Or perhaps they can be dealing with webm-specific stuff, but only when they are working with webm caps; otherwise, it's like you are prohibiting the use of matroska with other encryption mechanisms. In my original patch, the idea was just to parse the matroska headers to find out if the content is encrypted and then leave downstream to interpret the block data. I wrote that in order to interoperate with a proprietary element that did its own encryption/decryption on individual frames (a bit similar to how the webm standard does it, but not the same). Btw, that patch was badly done - it had hardcoded values for the matroska headers, which is why I didn't post it myself at the time for upstreaming...
(In reply to George Kiagiadakis from comment #15) > One thing I don't particularly like about this patch is that it is > webm-specific. Matroska is meant to be more abstract than that. > The way I see it, matroska elements should only be dealing with matroska > headers, not with the way data is formatted inside a data block (which is > defined in the webm spec). Or perhaps they can be dealing with webm-specific > stuff, but only when they are working with webm caps; otherwise, it's like > you are prohibiting the use of matroska with other encryption mechanisms. In the matroskademux description, we have this : "Demuxes Matroska/WebM streams into video/audio/subtitles" It demuxes the webm content thus it should support the webm encrypted specifications like is done in qtdemux with mp4. Currently there is no spec about encrypted content in Matroska, I never saw a mkv encrypted file. > In my original patch, the idea was just to parse the matroska headers to > find out if the content is encrypted and then leave downstream to interpret > the block data. I wrote that in order to interoperate with a proprietary > element that did its own encryption/decryption on individual frames (a bit > similar to how the webm standard does it, but not the same). I think, if you used your own encryption/decryption wich is out of the standard, you would use you proprietary element to demuxe this stream. This Patch follow only the Matroska/WebM spec > Btw, that patch > was badly done - it had hardcoded values for the matroska headers, which is > why I didn't post it myself at the time for upstreaming... Could you give me an example of the hardcode values apart from the WebM spec.
(In reply to George Kiagiadakis from comment #14) > How are you testing this? Can you think of a way to unit test it? I tested this with WebkitForWayland browser in MSE/EME Youtube conformance test. "https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2017.html?&disable_log=true&test_type=encryptedmedia-test" Yes I'll try to write a unit tests. I'll see how was done in qtdemux.
Review of attachment 354560 [details] [review]: This needs some general documentation about how the stream is stored in buffers, what is sent via GstMetas and events, what goes into caps, etc. and how this maps to the Matroska/WebM spec. Otherwise this looks generally OK to me. ::: gst/matroska/matroska-mux.c @@ +2265,3 @@ + + enc.scope = GST_MATROSKA_TRACK_ENCODING_SCOPE_FRAME; + enc.type = GST_MATROSKA_ENCODING_ENCRYPTION; The scope and type should probably also go into the caps so we can extend it later @@ +2267,3 @@ + enc.type = GST_MATROSKA_ENCODING_ENCRYPTION; + enc.enc_algo = content_encryption ? 5 : 0; + enc.sig_algo = content_signing ? 1 : 0; And these should probably map to something more expressive than booleans in the caps to allow for different algos in the future
Review of attachment 354562 [details] [review]: Generally looks OK to me, but this needs to be generalized a bit and like I wrote in the previous comment: it needs to be documented. How does the Matroska/WebM spec map to buffer content, caps, events and GstMetas? ::: gst/matroska/matroska-read-common.c @@ +354,3 @@ + * Set those parsed information into protection info structure @info_protect which + * will be added in protection metadata of the Gstbuffer. + * The subsamples format follows the same pssh box format in Common Encryption spec: This shouldn't really follow CENC but just parse the things from the Matroska container the way it is stored there. It's the job of the decryptor element to make sense of it, and it needs to understand the application/matroska-enc caps anyway. @@ +379,3 @@ + size = *size_out; + /* For more detail see the WbeM's spec https://www.webmproject.org/docs/webm-encryption/#47-signal-byte-format */ + signal_byte = *data++; Please use a GstByteReader here. All the manual byte parsing we do everywhere was already the reason for various CVEs and more harmless but annoying rashes :) @@ +421,3 @@ + /* For more details see https://www.webmproject.org/docs/webm-encryption/#46-subsample-encrypted-block-format */ + subsamples = + g_malloc (nb_subsample * (sizeof (guint16) + sizeof (guint32))); This calculation can overflow on specially crafted files. Also various other calculations here @@ +2970,3 @@ + GST_WARNING_OBJECT (common->sinkpad, + "Unexpected to have ContentEncAESSettings because Content Encryption Algo different to 5-AES"); + ret = gst_ebml_read_skip (ebml); We can probably still parse that and output the encrypted stream, just that downstream will have an unsupported enc algo in the caps and can't handle it. Unless it can handle something else than AES. Or not?
(In reply to Sebastian Dröge (slomo) from comment #18) > Review of attachment 354560 [details] [review] [review]: > > This needs some general documentation about how the stream is stored in > buffers, what is sent via GstMetas and events, what goes into caps, etc. and > how this maps to the Matroska/WebM spec. > > Otherwise this looks generally OK to me. > > ::: gst/matroska/matroska-mux.c > @@ +2265,3 @@ > + > + enc.scope = GST_MATROSKA_TRACK_ENCODING_SCOPE_FRAME; > + enc.type = GST_MATROSKA_ENCODING_ENCRYPTION; > > The scope and type should probably also go into the caps so we can extend it > later > > @@ +2267,3 @@ > + enc.type = GST_MATROSKA_ENCODING_ENCRYPTION; > + enc.enc_algo = content_encryption ? 5 : 0; > + enc.sig_algo = content_signing ? 1 : 0; > > And these should probably map to something more expressive than booleans in > the caps to allow for different algos in the future I agree. I'll rework the patch as soon as I can.
(In reply to Sebastian Dröge (slomo) from comment #19) > Review of attachment 354562 [details] [review] [review]: > > Generally looks OK to me, but this needs to be generalized a bit and like I > wrote in the previous comment: it needs to be documented. How does the > Matroska/WebM spec map to buffer content, caps, events and GstMetas? > > ::: gst/matroska/matroska-read-common.c > @@ +354,3 @@ > + * Set those parsed information into protection info structure > @info_protect which > + * will be added in protection metadata of the Gstbuffer. > + * The subsamples format follows the same pssh box format in Common > Encryption spec: > > This shouldn't really follow CENC but just parse the things from the > Matroska container the way it is stored there. It's the job of the decryptor > element to make sense of it, and it needs to understand the > application/matroska-enc caps anyway. > Yes I agree with you. But I would like to have the same processing in the Decryptor and Parser element when managing the sub-sample map. > @@ +379,3 @@ > + size = *size_out; > + /* For more detail see the WbeM's spec > https://www.webmproject.org/docs/webm-encryption/#47-signal-byte-format */ > + signal_byte = *data++; > > Please use a GstByteReader here. All the manual byte parsing we do > everywhere was already the reason for various CVEs and more harmless but > annoying rashes :) > I agree, I'll do it. > @@ +421,3 @@ > + /* For more details see > https://www.webmproject.org/docs/webm-encryption/#46-subsample-encrypted- > block-format */ > + subsamples = > + g_malloc (nb_subsample * (sizeof (guint16) + sizeof (guint32))); > > This calculation can overflow on specially crafted files. Also various other > calculations here > guint8 nb_part = *data; ... nb_subsample = (nb_part + 2) >> 1; I think nb_subsample can't be great than 127, because nb_part is unsigned byte. > @@ +2970,3 @@ > + GST_WARNING_OBJECT (common->sinkpad, > + "Unexpected to have ContentEncAESSettings because > Content Encryption Algo different to 5-AES"); > + ret = gst_ebml_read_skip (ebml); > > We can probably still parse that and output the encrypted stream, just that > downstream will have an unsupported enc algo in the caps and can't handle > it. Unless it can handle something else than AES. Or not? The Webm support only AES algorithm. The spec: https://www.webmproject.org/docs/container/ The encryption algorithm used. The value '0' means that the contents have not been encrypted but only signed. Predefined values: 1 - DES; 2 - 3DES; 3 - Twofish; 4 - Blowfish; 5 - AES. WebM only supports a value of 5 (AES).
(In reply to y.bandou from comment #21) > > @@ +2970,3 @@ > > + GST_WARNING_OBJECT (common->sinkpad, > > + "Unexpected to have ContentEncAESSettings because > > Content Encryption Algo different to 5-AES"); > > + ret = gst_ebml_read_skip (ebml); > > > > We can probably still parse that and output the encrypted stream, just that > > downstream will have an unsupported enc algo in the caps and can't handle > > it. Unless it can handle something else than AES. Or not? > > The Webm support only AES algorithm. > > The spec: https://www.webmproject.org/docs/container/ > > The encryption algorithm used. The value '0' means that the contents have > not been encrypted but only signed. Predefined values: 1 - DES; 2 - 3DES; 3 > - Twofish; 4 - Blowfish; 5 - AES. WebM only supports a value of 5 (AES). Right, but there's no reason to error out if a different value is found. We could still put it in the caps, and there just won't be a decryptor found then that can handle it. Also for non-WebM all the other values are still valid.
(In reply to Sebastian Dröge (slomo) from comment #22) > (In reply to y.bandou from comment #21) > > > @@ +2970,3 @@ > > > + GST_WARNING_OBJECT (common->sinkpad, > > > + "Unexpected to have ContentEncAESSettings because > > > Content Encryption Algo different to 5-AES"); > > > + ret = gst_ebml_read_skip (ebml); > > > > > > We can probably still parse that and output the encrypted stream, just that > > > downstream will have an unsupported enc algo in the caps and can't handle > > > it. Unless it can handle something else than AES. Or not? > > > > The Webm support only AES algorithm. > > > > The spec: https://www.webmproject.org/docs/container/ > > > > The encryption algorithm used. The value '0' means that the contents have > > not been encrypted but only signed. Predefined values: 1 - DES; 2 - 3DES; 3 > > - Twofish; 4 - Blowfish; 5 - AES. WebM only supports a value of 5 (AES). > > Right, but there's no reason to error out if a different value is found. We > could still put it in the caps, and there just won't be a decryptor found > then that can handle it. Also for non-WebM all the other values are still > valid. It's OK, I'll do it
Created attachment 361953 [details] [review] Add support for encrypted content with protection API (new) New patch with Sebastian's remarks
Created attachment 361954 [details] [review] matroska: implement reading & writing ContentEncryption headers (reworked) New patch with Sebastian remarks
Review of attachment 361954 [details] [review]: ::: gst/matroska/matroska-mux.c @@ +1051,3 @@ + + gst_structure_get_enum (structure, "signature-algorithm", + MATROSKA_TRACK_SIGNATURE_ALGORITHM_TYPE, &signature_algorithm); Shouldn't encrypted content get its own caps type(s)? This looks now like it supports the existing ones with additional fields, but using the existing ones is wrong: the content is encrypted and unusable without previous decryption In the demuxer you use a new caps type: "application/x-matroska-enc" @@ +2334,3 @@ + enc.sig_algo = signature_algorithm; + + g_array_append_val (context->encodings, enc); This code seems to be duplicated for each pad type, maybe move it to a function?
Review of attachment 361954 [details] [review]: Please also mention somewhere how the new caps type works (in the commit message at least): That it's "application/x-matroska-enc", has this and that additional field and otherwise the original fields.
Review of attachment 361953 [details] [review]: It still seems wrong to me to make the DRM system id optional in the protection event, but what can we do. How would an application right now know what to use/do? ::: gst/matroska/matroska-demux.c @@ +380,3 @@ + + if (gst_matroska_parse_protection_meta (&data, &size, info_protect, + &encrypted)) { How do we know that this is to be used, if it's only defined for WebM but not general Matroska? Is there any indication in the file anywhere? Also maybe the "Subsample Encrypted Block" should be passed directly downstream as-is and the decryptor that handles "application/x-matroska-enc" is supposed to handle that? Why would this be extracted at the demuxer level instead of having it handled in the decryptor? ::: gst/matroska/matroska-read-common.c @@ +361,3 @@ +gboolean +gst_matroska_parse_protection_meta (gpointer * data_out, gsize * size_out, + GstStructure * info_protect, gboolean * encrypted) Support for this should probably also be added to matroskamux, or at least it should fail writing streams that require this according to the caps? @@ +543,3 @@ + gst_buffer_unref (buf_sub_sample); + } else { + gst_structure_set (info_protect, "subsample_count", G_TYPE_UINT, 0, NULL); Underscore instead of dash @@ +3061,3 @@ + GST_DEBUG_OBJECT (common->sinkpad, + "ContentEncAESSettings: %" G_GUINT64_FORMAT, num); + enc.enc_CipherMode = num; This new field does not seem to be used anywhere? It should probably also go into the caps? @@ +3100,3 @@ + + context->protection_info = + gst_structure_new ("application/x-cenc", "iv_size", We usually use dashes instead of underscores in structure field names
(In reply to Sebastian Dröge (slomo) from comment #28) > Review of attachment 361953 [details] [review] [review]: > > It still seems wrong to me to make the DRM system id optional in the > protection event, but what can we do. How would an application right now > know what to use/do? I think there is a miss in the Webm spec as we talked in bug 784129. For the moment the application should choose Widevine in case of WebM. In case of EME the JS application can set the DRM to use. > ::: gst/matroska/matroska-demux.c > @@ +380,3 @@ > + > + if (gst_matroska_parse_protection_meta (&data, &size, info_protect, > + &encrypted)) { > > How do we know that this is to be used, if it's only defined for WebM but > not general Matroska? Is there any indication in the file anywhere? > I agree, I'll add a check if it is WebM, I'll use ctx->is_webm > Also maybe the "Subsample Encrypted Block" should be passed directly > downstream as-is and the decryptor that handles "application/x-matroska-enc" > is supposed to handle that? Why would this be extracted at the demuxer level > instead of having it handled in the decryptor? > It is similar to qtdemux, as you suggested in Comment 3. The Subsample map is extracted in the demuxer and put in CENC format in order to have the same handling in the Decryptor for WebM and MP4. For me, the decryptor should handle the stream independently of the codec or the container, so it extracts all necessary information from the protection metadata. > ::: gst/matroska/matroska-read-common.c > @@ +361,3 @@ > +gboolean > +gst_matroska_parse_protection_meta (gpointer * data_out, gsize * size_out, > + GstStructure * info_protect, gboolean * encrypted) > > Support for this should probably also be added to matroskamux, or at least > it should fail writing streams that require this according to the caps? > I'll see, how I add this in matroskamux. > @@ +543,3 @@ > + gst_buffer_unref (buf_sub_sample); > + } else { > + gst_structure_set (info_protect, "subsample_count", G_TYPE_UINT, 0, > NULL); > > Underscore instead of dash OK > > @@ +3061,3 @@ > + GST_DEBUG_OBJECT (common->sinkpad, > + "ContentEncAESSettings: %" G_GUINT64_FORMAT, num); > + enc.enc_CipherMode = num; > > This new field does not seem to be used anywhere? It should probably also go > into the caps? > I'll add it in caps, but currently it supports one value 1-CTR see: https://www.webmproject.org/docs/webm-encryption/ > @@ +3100,3 @@ > + > + context->protection_info = > + gst_structure_new ("application/x-cenc", "iv_size", > > We usually use dashes instead of underscores in structure field names OK
(In reply to Sebastian Dröge (slomo) from comment #26) > Review of attachment 361954 [details] [review] [review]: > > ::: gst/matroska/matroska-mux.c > @@ +1051,3 @@ > + > + gst_structure_get_enum (structure, "signature-algorithm", > + MATROSKA_TRACK_SIGNATURE_ALGORITHM_TYPE, &signature_algorithm); > > Shouldn't encrypted content get its own caps type(s)? This looks now like it > supports the existing ones with additional fields, but using the existing > ones is wrong: the content is encrypted and unusable without previous > decryption > > In the demuxer you use a new caps type: "application/x-matroska-enc" > Yes indeed, By looking closely matroskamux it is not possible to mux an encrypted contents, as you indicated. I'll remove all encrypted content support from matroskamux. I'll only submit the encrypted content support in matroskademux like is done in qtdemux (qtmux doesn't support encrypted content) What do you think? > @@ +2334,3 @@ > + enc.sig_algo = signature_algorithm; > + > + g_array_append_val (context->encodings, enc); > > This code seems to be duplicated for each pad type, maybe move it to a > function?
(In reply to y.bandou from comment #30) > (In reply to Sebastian Dröge (slomo) from comment #26) > > Review of attachment 361954 [details] [review] [review] [review]: > > > > ::: gst/matroska/matroska-mux.c > > @@ +1051,3 @@ > > + > > + gst_structure_get_enum (structure, "signature-algorithm", > > + MATROSKA_TRACK_SIGNATURE_ALGORITHM_TYPE, &signature_algorithm); > > > > Shouldn't encrypted content get its own caps type(s)? This looks now like it > > supports the existing ones with additional fields, but using the existing > > ones is wrong: the content is encrypted and unusable without previous > > decryption > > > > In the demuxer you use a new caps type: "application/x-matroska-enc" > > > Yes indeed, > > By looking closely matroskamux it is not possible to mux an encrypted > contents, as you indicated. > I'll remove all encrypted content support from matroskamux. Why is it impossible? > I'll only submit the encrypted content support in matroskademux like is done > in qtdemux (qtmux doesn't support encrypted content) > > What do you think? Sounds ok
(In reply to Sebastian Dröge (slomo) from comment #31) > (In reply to y.bandou from comment #30) > > (In reply to Sebastian Dröge (slomo) from comment #26) > > > Review of attachment 361954 [details] [review] [review] [review] [review]: > > > > > > ::: gst/matroska/matroska-mux.c > > > @@ +1051,3 @@ > > > + > > > + gst_structure_get_enum (structure, "signature-algorithm", > > > + MATROSKA_TRACK_SIGNATURE_ALGORITHM_TYPE, &signature_algorithm); > > > > > > Shouldn't encrypted content get its own caps type(s)? This looks now like it > > > supports the existing ones with additional fields, but using the existing > > > ones is wrong: the content is encrypted and unusable without previous > > > decryption > > > > > > In the demuxer you use a new caps type: "application/x-matroska-enc" > > > > > Yes indeed, > > > > By looking closely matroskamux it is not possible to mux an encrypted > > contents, as you indicated. > > I'll remove all encrypted content support from matroskamux. > > Why is it impossible? It's very difficult with encrypted content because sometimes we need to read data as for the Dirac codec. see this function "gst_matroska_mux_handle_dirac_packet". > > > I'll only submit the encrypted content support in matroskademux like is done > > in qtdemux (qtmux doesn't support encrypted content) > > > > What do you think? > > Sounds ok Perfect, I'll submit a new patches to add support of encrypted content just in matroskademux without matroskamux and of course with your new remarks. I should change the summary of this bug ? "matroska: implement reading & writing ContentEncryption headers" -> "matroska: Add encrypted content support in matroskademux" Or I'll create a new bug? What do you think ?
Sure
(In reply to Sebastian Dröge (slomo) from comment #28) > Review of attachment 361953 [details] [review] [review]: > > @@ +543,3 @@ > + gst_buffer_unref (buf_sub_sample); > + } else { > + gst_structure_set (info_protect, "subsample_count", G_TYPE_UINT, 0, > NULL); > > Underscore instead of dash > > @@ +3100,3 @@ > + > + context->protection_info = > + gst_structure_new ("application/x-cenc", "iv_size", > > We usually use dashes instead of underscores in structure field names Currently, in qtdemux we use underscore instead of dash in protection event structure. see qtdemux.c:2803, qtdemux.c:9569.
(In reply to y.bandou from comment #29) > (In reply to Sebastian Dröge (slomo) from comment #28) > > Review of attachment 361953 [details] [review] [review] [review]: > > > > It still seems wrong to me to make the DRM system id optional in the > > protection event, but what can we do. How would an application right now > > know what to use/do? > > I think there is a miss in the Webm spec as we talked in bug 784129. > For the moment the application should choose Widevine in case of WebM. > In case of EME the JS application can set the DRM to use. > For now, in order not to modify the protection event API, I'll set Widevine system ID in the protection event for encrypted WebM. What do you think ?
Did you raise this issue with the WebM and/or EME people? It seems like this should be resolved first instead of doing something random here.
(In reply to y.bandou from comment #34) > > We usually use dashes instead of underscores in structure field names > > Currently, in qtdemux we use underscore instead of dash in protection event > structure. That's unfortunate as this makes it part of the API now. Please do the same here then, even if it's wrong. Let's better keep it consistently wrong
(In reply to Sebastian Dröge (slomo) from comment #36) > Did you raise this issue with the WebM and/or EME people? It seems like this > should be resolved first instead of doing something random here. I agree, I just opened an issue for WebM https://bugs.chromium.org/p/webp/issues/detail?id=367 Anyway, I think we need to change the protection event API, because even in the CENC spec, the mp4 file can support multiple DRM like PlayReady, Widevine or ClearKey, while in the protection event API, we can set only one system ID per event. Currently qtdemux sends an event for each systemId supported, and this is a wrong, because, how the application can know how many events will be received before take the decision, what DRM it uses ? see this bug : https://bugzilla.gnome.org/show_bug.cgi?id=770107
qtdemux (and matroskademux) is supposed to select a single one, based on what decryptors are actually available and what was set by the application in the context. But that's already all discussed and handled in that other bug, nothing new for here :) The event does not have to change in any case.
(In reply to Sebastian Dröge (slomo) from comment #39) > qtdemux (and matroskademux) is supposed to select a single one, based on > what decryptors are actually available and what was set by the application > in the context. But that's already all discussed and handled in that other > bug, nothing new for here :) The event does not have to change in any case. Understood, We don't change the API :) I think the patch which allow the application to set a context, it is not yet commited, No?
Created attachment 365211 [details] [review] matroska: Add the WebM encrypted content support in matroskademux. Here is the reworked patch. This patch : 1. Reads the WebM and Matroska ContentEncryption subelements. 2. Creates a GST_PROTECTION event for each ContentEncryption, which will be sent before pushing the first source buffer. The DRM system id field in this event is set to "UNDEFINED" because it isn't specified neither by Matroska nor by the WebM spec. 3. Reads the protection information of encrypted Block/SimpleBlock and extracts the IV and the partitioning format (subsamples). 4. Creates the metadata protection for each encrypted Block/SimpleBlock, with those informations: KeyID (extracted from ContentEncryption element), IV and partitioning format. 5. Adds a new caps for WebM encrypted content named "application/x-webm-enc", with the following new fields: "encryption-algorithm": The encryption algorithm used. values: "None", "DES", "3DES", "Twofish", "Blowfish", "AES". "encoding-scope": The field that describes which Elements have been modified. Values: "frame", "codec-data", "next-content". "cipher-mode": The cipher mode used in the encryption. Values: "None", "CTR".
(In reply to y.bandou from comment #38) > Currently qtdemux sends an event for each systemId supported, and this is a > wrong, because, how the application can know how many events will be > received before take the decision, what DRM it uses ? > see this bug : https://bugzilla.gnome.org/show_bug.cgi?id=770107 There's still a patch pending to land that sends all events before configuring the decryptor in a context query so this is only partially true :)
The WebM team did not respond to my request "https://bugs.chromium.org/p/webm/issues/detail?id=1478". I don't think they will respond. I propose to set the field systemID of the protection event as "UNDEFINED", like in the last submitted patch. What do you think ? Could you review this last patch if you have some time ? Thanks.
(In reply to George Kiagiadakis from comment #14) > How are you testing this? Can you think of a way to unit test it? (In reply to y.bandou from comment #17) > (In reply to George Kiagiadakis from comment #14) > > How are you testing this? Can you think of a way to unit test it? > > I tested this with WebkitForWayland browser in MSE/EME Youtube conformance > test. > > "https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2017. > html?&disable_log=true&test_type=encryptedmedia-test" > > Yes I'll try to write a unit tests. I'll see how was done in qtdemux. Hi Bandou Yacine, Against which version of Webkit was the patch tested ? Do we have a unit test available ?
(In reply to Donia from comment #44) > (In reply to George Kiagiadakis from comment #14) > > How are you testing this? Can you think of a way to unit test it? > > (In reply to y.bandou from comment #17) > > (In reply to George Kiagiadakis from comment #14) > > > How are you testing this? Can you think of a way to unit test it? > > > > I tested this with WebkitForWayland browser in MSE/EME Youtube conformance > > test. > > > > "https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2017. > > html?&disable_log=true&test_type=encryptedmedia-test" > > > > Yes I'll try to write a unit tests. I'll see how was done in qtdemux. > > Hi Bandou Yacine, > Against which version of Webkit was the patch tested ? > Do we have a unit test available ? Hi Donia, To test this patch with webkit WPE port, you need some patches in order to add the support of encrypted webm in webkit GStreamer backend. I didn't push to upstream these patches because they are not yet ready, they work only in our SoftAtHome private branch. I'll try to push these patch ASAP.
(In reply to y.bandou from comment #45) > (In reply to Donia from comment #44) > > (In reply to George Kiagiadakis from comment #14) > > > How are you testing this? Can you think of a way to unit test it? > > > > (In reply to y.bandou from comment #17) > > > (In reply to George Kiagiadakis from comment #14) > > > > How are you testing this? Can you think of a way to unit test it? > > > > > > I tested this with WebkitForWayland browser in MSE/EME Youtube conformance > > > test. > > > > > > "https://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2017. > > > html?&disable_log=true&test_type=encryptedmedia-test" > > > > > > Yes I'll try to write a unit tests. I'll see how was done in qtdemux. > > > > Hi Bandou Yacine, > > Against which version of Webkit was the patch tested ? > > Do we have a unit test available ? > > Hi Donia, > To test this patch with webkit WPE port, you need some patches in order to > add the support of encrypted webm in webkit GStreamer backend. > I didn't push to upstream these patches because they are not yet ready, they > work only in our SoftAtHome private branch. > I'll try to push these patch ASAP. In order to test this patch and play an encrypted WebM content: 1. Take an upstream WebKit "git://git.webkit.org/WebKit.git" 2. Apply the following patches: -https://bugs.webkit.org/attachment.cgi?id=348764 (Add the current patch in Jhbuild) -https://bugs.webkit.org/attachment.cgi?id=327459 -https://bugs.webkit.org/attachment.cgi?id=348768 -https://bugs.webkit.org/attachment.cgi?id=348770 3. Activate MSE and EME like this: --- a/Tools/MiniBrowser/wpe/main.cpp +++ b/Tools/MiniBrowser/wpe/main.cpp @@ -166,6 +166,8 @@ int main(int argc, char *argv[]) "enable-developer-extras", TRUE, "enable-webgl", TRUE, "enable-media-stream", TRUE, + "enable-mediasource", TRUE, + "enable-encrypted-media", TRUE, 4. Build the WPE Port. 5. # weston --socket=wpe & # WAYLAND_DISPLAY=wpe Tools/Scripts/run-minibrowser --wpe "https://singingtree.github.io/Webm-EME-Examples/VideoAndAudio-VideoSubsampleEncrypted.html" For Unit tests see: https://bugs.webkit.org/show_bug.cgi?id=189196 https://bugs.webkit.org/show_bug.cgi?id=189200
Review of attachment 365211 [details] [review]: I have no more comments about the rest, but what I say does impact in what I would wait as a client of this. ::: gst/matroska/matroska-read-common.c @@ +3084,3 @@ + /* system_id field is set to "UNDEFINED" because it isn't specified neither in WebM nor in Matroska spec. */ + event = + gst_event_new_protection ("UNDEFINED", keyId_buf, This seems a bit like a hack to me. What I think it would be best would be to modify this API to allow nil values.
(In reply to Xabier Rodríguez Calvar from comment #47) > Review of attachment 365211 [details] [review] [review]: > > I have no more comments about the rest, but what I say does impact in what I > would wait as a client of this. > > ::: gst/matroska/matroska-read-common.c > @@ +3084,3 @@ > + /* system_id field is set to "UNDEFINED" because it isn't > specified neither in WebM nor in Matroska spec. */ > + event = > + gst_event_new_protection ("UNDEFINED", keyId_buf, > > This seems a bit like a hack to me. What I think it would be best would be > to modify this API to allow nil values. I agree with you, see the comments 38 and 39.
Created attachment 373817 [details] [review] Add the WebM encrypted content support Rebase the last patch with upstream and replace "UNDEFINED" with the new definition has been added in 797231.
Review of attachment 373817 [details] [review]: ::: gst/matroska/matroska-demux.c @@ +416,3 @@ + + } else { + GST_DEBUG ("Adding protection metadata failed"); Shouldn't this be a WARNING?
Review of attachment 373817 [details] [review]: Looks good overall. ::: gst/matroska/matroska-demux.c @@ +174,3 @@ * gst_matroska_demux_subtitle_caps (GstMatroskaTrackSubtitleContext * subtitlecontext, const gchar * codec_id, gpointer data, guint size); +static const gchar *gst_matroska_track_encryption_algorithm_name (gint val); Coding style looks weird here, interesting `gst-indent` accept it. @@ +1524,3 @@ + gst_structure_set (s, "original-media-type", G_TYPE_STRING, + gst_structure_get_name (s), NULL); + gst_structure_set (s, "encryption-algorithm", G_TYPE_STRING, `gst_matroska_track_encryption_algorithm_name` should not return "Nill" as a string but NULL and handle it on the caller side, or a proper default. ::: gst/matroska/matroska-read-common.c @@ +404,3 @@ + + /* Encrypted buffer */ + if (signal_byte & GST_MATROSKA_BLOCK_ENCRYPTED) { I would use an early return here instead.
Created attachment 373833 [details] [review] Add the WebM encrypted content support
Review of attachment 373833 [details] [review]: Looks good to me.