GNOME Bugzilla – Bug 705991
Adding support for DASH common encryption to qtdemux and dashdemux
Last modified: 2015-09-25 08:21:55 UTC
I would like to add support for encrypted ISOBMFF DASH streams that have been encrypted using the DASH common encryption format (ISO/IEC 23001-7). When a DASH MP4 file is encrypted using DASH common encryption (CENC), the initialisation segment is modified as follows: * The avc1 or avc3 box (path /moov/trak/mdia/minf/stbl/stsd/) gets renamed to “encv” and a protection scheme information box (“sinf”) is added that contains the original data format (e.g. “avc1”) * one or more DRM-specific (“pssh”) boxes are added to the moov box Each fragment is modified as follows: * Each sample in the MDAT is encrypted using AES in CTR mode. Each sample has a mix of encrypted parts and clear-text parts. * A sample encryption box (“senc”) is added that contains the key ID (KID) and the lengths of the encrypted and clear sections for each sample, plus the IV for each sample. To decrypt the stream, a DRM system uses the protection specific boxes, key ID, IV, codec_data and clear/encrypted region information to decrypt each sample. I was thinking of modifying qtdemux to create a pad for each encrypted stream with a new mime type (e.g. video/x-cenc, protectionScheme=”urn:uuid:69f908af-4816-46ea-910c-cd5dcccb0a3a”) plus a sticky message on this pad for the protection specific boxes. Dashdemux would also need to be modified to create a sticky message for any protection specific data in the manifest. As each sample is demultiplexed by qtdemux it would add the clear/encrypted run information, the IV and the KID as metadata attached to the source buffer. A DRM implementation could then use the information attached to each buffer, plus the messages from its sink pad to decrypt the sample and either output the unencrypted data to a source pad or directly display the content (e.g. if the DRM system required a secure pathway to the display). I am interested in the opinion of the gstreamer developers of this approach, and also I would like to find out if anyone else was thinking of implementing DASH CENC. Alex
(In reply to comment #0) > I would like to add support for encrypted ISOBMFF DASH streams that have been > encrypted using the DASH common encryption format (ISO/IEC 23001-7). When a > DASH MP4 file is encrypted using DASH common encryption (CENC), the > initialisation segment is modified as follows: > * The avc1 or avc3 box (path /moov/trak/mdia/minf/stbl/stsd/) gets renamed to > “encv” and a protection scheme information box (“sinf”) is added that contains > the original data format (e.g. “avc1”) > * one or more DRM-specific (“pssh”) boxes are added to the moov box > > Each fragment is modified as follows: > * Each sample in the MDAT is encrypted using AES in CTR mode. Each sample has a > mix of encrypted parts and clear-text parts. > * A sample encryption box (“senc”) is added that contains the key ID (KID) and > the lengths of the encrypted and clear sections for each sample, plus the IV > for each sample. > > To decrypt the stream, a DRM system uses the protection specific boxes, key ID, > IV, codec_data and clear/encrypted region information to decrypt each sample. > > I was thinking of modifying qtdemux to create a pad for each encrypted stream > with a new mime type (e.g. video/x-cenc, > protectionScheme=”urn:uuid:69f908af-4816-46ea-910c-cd5dcccb0a3a”) plus a This makes sense, it is a different type of media. sticky > message on this pad for the protection specific boxes. Dashdemux would also > need to be modified to create a sticky message for any protection specific data > in the manifest. I'm assuming you meant 'event' instead of 'message'. > > As each sample is demultiplexed by qtdemux it would add the clear/encrypted run > information, the IV and the KID as metadata attached to the source buffer. > I haven't read this spec or know much about DRM implementations, so forgive me for naive questions. Is this information changing for every buffer? If not, events could be used here, too. > A DRM implementation could then use the information attached to each buffer, > plus the messages from its sink pad to decrypt the sample and either output the > unencrypted data to a source pad or directly display the content (e.g. if the > DRM system required a secure pathway to the display). I agree to have an element dedicated to the decryption, makes it easier to reuse and debug, also keeps demuxers less complex. About the decrypt+decode+display thing, if the restriction is the use of a special memory, bufferpool allocation could be used to have multiple elements doing smaller tasks instead of a single larger element. > > I am interested in the opinion of the gstreamer developers of this approach, > and also I would like to find out if anyone else was thinking of implementing > DASH CENC. > > Alex
>Is this information changing for every buffer? If not, events could be used here, too. Yes, the IV and clear/encrypted run information is per sample. The key ID does not normally change per sample. A default KID is provided in the MOOV and each fragment can specify one or more KIDs plus a mapping to which samples use these key IDs.
Attaching it to each buffer as a meta sounds correct then. (In reply to comment #0) > I was thinking of modifying qtdemux to create a pad for each encrypted stream > with a new mime type (e.g. video/x-cenc, > protectionScheme=”urn:uuid:69f908af-4816-46ea-910c-cd5dcccb0a3a”) plus a sticky > message on this pad for the protection specific boxes. Dashdemux would also > need to be modified to create a sticky message for any protection specific data > in the manifest. What exactly would this sticky event contain? Couldn't it be part of the caps, like a "codec_data" or "streamheader"? In general sounds like a good plan to move forward with this. Which changes are required to dashdemux? This sounds like it only affects qtdemux?
Yes, hopefully all of the changes would be in qtdemux. However there are ContentProtection elements in the manifest that might need to be exposed to download stream plugins. Somehow the DRM plugin needs to get the protection specific information from the "pssh" box. I don't care if it goes via an event or part of the caps, I'm happy to go with whichever fits better with gstreamer conventions. Each pssh box is opaque as far as the standard is concerned and there can be multiple boxes in the init segment. Each pssh has a system ID that indicates the DRM system that can make use of the data. My reason for suggesting an event rather than caps was because of its size. As the data is opaque we don't know large it is, but expect it to be large enough to hold several key IDs and URLs. Maybe a few kb per box.
Created attachment 256792 [details] [review] Add cenc support Here's an initial patch that extends qtdemux to handle common encryption. It's quite a big patch, so I'll point out a couple of design decisions: 1. Src pad caps For cenc-protected content, the src pads will have the following caps format: [video|audio]/x-cenc, protection-system-id=(string)<URN of protection system>, protection-system-data=(buffer)<protection system private data>, original-caps=(GstCaps)<caps of original unencrypted stream> So the caps structure of the orignal unencrypted stream is nested within the src pad caps; the thinking behind this is that this would make it easy for the downstream cenc decryptor to set it's src caps - all it would have to do is to extract original-caps from its sink pad caps and set that on its src pad. 2. Cenc metadata type I've created a metadata type to attach to the buffers of encrypted media that qtdemux pushes downstream. This is called GstCencMeta, and it includes IV, KID, and information about which bytes in the data are encrypted and which are clear (i.e., all the information needed to decrypt the data in the buffer). Since both qtdemux and cenc decryption elements outside of libgstisomp4 would need to link to the GstCencMeta code, we separated it into its own library in gst-libs/gst/isomp4. (Any thoughts on whether this is the right thing to do, or not?) The code in the patch has a few simplifications/restrictions that can be addressed by future updates: - The cenc spec allows sample properties (IsEncrypted flag, IV_Size and KID) to be set for a group of samples, as specified in the sgpd and sbgp box types. This code does not include support for this feature - it simply uses the default sample properties (contained in the tenc box) for all samples. - According to 14496-12, the sample auxiliary data (which in cenc includes the IVs for each sample) can be stored anywhere in the same file as the sample data itself. This code, however, is a bit more restrictive - it only handles auxiliary information that is contained within the moof box holding the size & offset information of the auxiliary data (i.e., the same moof box that contains the sai[z|o] boxes). - Cenc allows the same protected content to be decrypted via multiple protection systems. The information specific to each protection system is included in a pssh box (so, a piece of content might have a pssh box for Playready, and another for Marlin, etc.). The protection system ID and data that qtdemux sets on its src pad should depend upon which downstream cenc decrypt elements are available on the system, which implies that there will be need to be some caps negotiation; but as a simple initial implementation this code just chooses the first protection system that it's parsed and uses that to set the src pad caps. Bugs: The one issue that I'm aware of occurs only when qtdemux is given a single file to play containing multiple movie fragments, rather than being passed separate fragments (as would happen in the case of DASH). The code maintains cenc metadata for all the samples in a single fragment; when a new moof box is parsed it will overwrite its currently stored cenc metadata with that from the new fragment. For the mode of operation where fragments are being pushed (e.g., the DASH case), this works fine; however, when qtdemux plays a single file it parses the next moof box just prior to outputting the last sample of the current fragment, with the effect that when it does then output the last sample of the current fragment it will actually attach the cenc metadata of the last sample of the next fragment (since it's just overwritten all of the cenc sample metadata with that of the new fragment).
An example DASH CENC decrypt plugin is available from github: https://github.com/asrashley/gst-cencdec It is not linked with any DRM system, you simply provide a key as a parameter to the element. I am not sure if the future of this code should be a stand-alone plugin, that people can use as a starting point for their DRM-linked implementation, or if this code should become a GstBaseDecrypt that DRM implementer's extend to provide the key ID to key functionality.
Created attachment 258525 [details] [review] Add cenc support to gst-plugins-base Based on a conversation with Wim at the GStreamer conference last week, I've moved the cenc metadata code out of gst-plugins-good and into gst-plugins-base. This patch adds the cenc code to gst-plugins-base.
Created attachment 258526 [details] [review] Add cenc support to qtdemux Modifies the original patch to remove the cenc Metadata code, which has now been moved to gst-plugins-base.
Review of attachment 258525 [details] [review]: Please add a pkg-config file in the pkgconfig directory ::: gst-libs/gst/cenc/Makefile.am @@ +45,3 @@ + -I$(top_srcdir)/gst-libs \ + --add-include-path=`PKG_CONFIG_PATH="$(GST_PKG_CONFIG_PATH)" $(PKG_CONFIG) --variable=girdir gstreamer-@GST_API_VERSION@` \ + --add-include-path=`PKG_CONFIG_PATH="$(GST_PKG_CONFIG_PATH)" $(PKG_CONFIG) --variable=girdir gstreamer-base-@GST_API_VERSION@` \ You don't actually need base @@ +51,3 @@ + --libtool="$(top_builddir)/libtool" \ + --pkg gstreamer-@GST_API_VERSION@ \ + --pkg gstreamer-base-@GST_API_VERSION@ \ Or here @@ +73,3 @@ + --includedir=$(builddir) \ + --includedir=`PKG_CONFIG_PATH="$(GST_PKG_CONFIG_PATH)" $(PKG_CONFIG) --variable=girdir gstreamer-@GST_API_VERSION@` \ + --includedir=`PKG_CONFIG_PATH="$(GST_PKG_CONFIG_PATH)" $(PKG_CONFIG) --variable=girdir gstreamer-base-@GST_API_VERSION@` \ Or here ::: gst-libs/gst/cenc/cenc.h @@ +28,3 @@ +typedef struct _CencSubsampleInfo CencSubsampleInfo; +typedef struct _CencSampleCryptoInfo CencSampleCryptoInfo; +typedef struct _CencSampleProperties CencSampleProperties; These should all be in the Gst namespace @@ +37,3 @@ + * Holds information about a subsample of a cenc-protected sample. + */ +struct _CencSubsampleInfo SubSample maybe? @@ +57,3 @@ + GBytes *iv; + guint16 n_subsamples; + GArray *subsample_info; These should probably all get boxed types and ref/unref functions at least. Maybe make them miniobjects? Also some padding in the structs might be sensible :) ::: gst-libs/gst/cenc/gstcencmeta.c @@ +16,3 @@ + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, + * Boston, MA 02110-1301, USA. + */ Please add documentation here what this is all about and how it is used in which use cases
Review of attachment 258526 [details] [review]: Just skipped over this patch, some comments ::: gst/isomp4/qtdemux.c @@ +6149,3 @@ + + case FOURCC_soun: + enc_caps = gst_caps_new_simple ("audio/x-cenc", Why differentiate between audio and video here? Just make it application/x-cenc? @@ +6152,3 @@ + "protection-system-id", G_TYPE_STRING, sys->id, + "protection-system-data", GST_TYPE_BUFFER, sys->data, + "original-caps", GST_TYPE_CAPS, stream->original_caps, NULL); I think some unit tests for caps-in-caps would be useful to have in core now :)
(In reply to comment #9) Thanks for the comments, Sebastian. Could you clarify a couple of things: [...] > @@ +37,3 @@ > + * Holds information about a subsample of a cenc-protected sample. > + */ > +struct _CencSubsampleInfo > > SubSample maybe? Are you suggesting calling it GstCencSubSampleInfo or GstCencSubSample? > @@ +57,3 @@ > + GBytes *iv; > + guint16 n_subsamples; > + GArray *subsample_info; > > These should probably all get boxed types and ref/unref functions at least. > Maybe make them miniobjects? Also some padding in the structs might be sensible > :) Do you mean that you think all of the typedef'd structs in cenc.h (GstCencSampleProperties/GstCencSampleCryptoInfo/GstCencSubSampleInfo) should be made into boxed types? [...]
(In reply to comment #11) > > @@ +37,3 @@ > > + * Holds information about a subsample of a cenc-protected sample. > > + */ > > +struct _CencSubsampleInfo > > > > SubSample maybe? > > Are you suggesting calling it GstCencSubSampleInfo or GstCencSubSample? GstCencSubSampleInfo, I just meant the upper-case S for sample :) Seems more consistent with everything else. > > @@ +57,3 @@ > > + GBytes *iv; > > + guint16 n_subsamples; > > + GArray *subsample_info; > > > > These should probably all get boxed types and ref/unref functions at least. > > Maybe make them miniobjects? Also some padding in the structs might be sensible > > :) > > Do you mean that you think all of the typedef'd structs in cenc.h > (GstCencSampleProperties/GstCencSampleCryptoInfo/GstCencSubSampleInfo) should > be made into boxed types? Yes, as those structures are public API and inside a library it must be possible to use them from bindings. And GObjectIntrospection wants boxed types for them.
(In reply to comment #12) OK - thanks :)
Created attachment 259650 [details] [review] Add cenc support to gst-plugins-base Updates earlier gst-plugins-base patch in line with Sebastian's comments. - Cenc structs are now boxed types inheriting from GstMiniObject. - Cenc pkg-config files added. - Gtkdoc documentation added to heads of *.c files. - Dependency on base removed from Makefile.am
Created attachment 259651 [details] [review] Add cenc support to qtdemux Updates earlier qtdemux patch in line with changes to gst-plugins-base patch and Sebastian's comments.
Created attachment 259731 [details] [review] Add cenc support to gst-plugins-base Updated gst-plugins-base patch so that it now produces gtk-doc HTML output.
Created attachment 259772 [details] [review] qtdemux: add support for common encryption Slightly updated patch 1. Uses G_*_FORMAT macros for 32/64-bit compilation 2. Replaced one use of gst_caps_take() where the caps was running out of refs
Review of attachment 259772 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +1897,3 @@ + } + g_array_free (qtdemux->cenc_protection_systems_info, TRUE); + qtdemux->cenc_protection_systems_info = NULL; This whole block not so small and is duplicated form the _dispose(), can you make it into a function or something? @@ +6175,3 @@ + + /* As an initial implementation, we simply choose the first + * available protection system. */ I'm a bit concerned that we're ignoring the case where there is more than one PSSH box in the file. We should design something that works if we don't have a decoder for the first PSSH, but only for the second. I think we should design that in before we fix the caps.
Review of attachment 259772 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +1898,3 @@ + g_array_free (qtdemux->cenc_protection_systems_info, TRUE); + qtdemux->cenc_protection_systems_info = NULL; + } This section is not super small and duplicated, can you make it into a separate function?h @@ +6175,3 @@ + + /* As an initial implementation, we simply choose the first + * available protection system. */ I'm a bit concerned that we're ignoring the case where there is more than one PSSH box in the file. We should design something that works if we don't have a decrypter for the first PSSH, but only for the second. I think we should design that in before we're stuck with these caps until 2.x.
(In reply to comment #20) [...] > This section is not super small and duplicated, can you make it into a separate > function?h Yes, sure - I'll do that. [...] > I'm a bit concerned that we're ignoring the case where there is more than one > PSSH box in the file. We should design something that works if we don't have a > decrypter for the first PSSH, but only for the second. I think we should design > that in before we're stuck with these caps until 2.x. I agree - the intention was not ultimately to ignore this, only that it seemed complex enough that it would take a fair amount of time to implement, and I wanted to share the code done so far to get some feedback on the overall approach. It's worth saying that the patch currently does parse and store the information from all PSSH boxes in the file; it's just that, when it comes to choosing which one to use when setting the src pad caps, it picks the first one as an initial quick solution (one that allowed testing of the code with suitable content). I'm currently trying to figure out how to implement something that will work with autoplugging. I guess what we want is for qtdemux to have some initial unfixed caps set on its src pads that include an array of values for the protection-system-id, one per PSSH box, e.g., application/x-cenc, protection-system-id={A, B, C} ...such that the autoplugger can then select the right cenc decrypter element from the registry (one that has sink pad caps application/x-cenc, protection-system-id=B, for instance) and link the two. Following that there would be some caps negotiation resulting in the caps on qtdemux's src pad being fixated, etc. However, I'm currently struggling to work out how to implement this in such a way that it works with decodebin. It doesn't look like qtdemux currently does any caps negotiation - it just figures out the caps of the traks within the input file and sets these fixed caps on its src pads. The process of how decodebin links a new demuxer src pad with unfixed caps to a downstream element, and how the caps negotiation code in qtdemux would then subsequently be triggered, is a little bit of a mystery to me at the moment... Any thoughts or guidance would be welcome! :)
It seems that the way to do multiple systems will probably be like multiple languages, just expose one pad for each system, and then stops sending on any pad where a not-linked is received (and restart sending if a reconfigure event is). See discussion on bug #711849
(In reply to comment #22) The case of multiple protection systems is a bit different from that of multiple audio streams: in the multiple audio stream case there are a number of different streams of data (i.e., a number of different audio tracks), one of which is selected for presentation; in the case of multiple protection systems using cenc, however, there aren't multiple tracks of data but a single encrypted track that can be decrypted via any one of the protection systems for which there is a pssh box. All we really need is a way for a suitable cenc decrypter element to be autoplugged after qtdemux. Do you see any particular problems with the solution I outlined above, in which an array of protection-system-ids is used in the src pad caps in order to autoplug a suitable decrypter element?
I agree, this is completely different from e.g. multiple language streams. You have one encrypted streams, but multiple ways to get the keys from different content protection systems (e.g. playready, fairplay, widevine, ...). I think the proposal with the protection-system-ids could make sense, however it won't work during autoplugging. Because when qtdemux has to decide on a caps, downstream does not exist yet. Can't we just put all possible systems in the caps with their corresponding boxes, and then let decoding figure out any possible element that can handle at least one of the systems and autoplug that? This could be done by having caps like: demuxer: application/x-cenc,protection-system-id-1=TRUE,protection-system-psshbox-1=XXXXX,protection-system-id-2=TRUE,protection-system-psshbox-2=XXXXX cenc decrypter: application/x-cenc,protection-system-id-1=TRUE Not exactly beautiful though :)
I'm not sure how having multiple pads that happen to send exactly the same data (but with different caps) is different from having multiple pads that send different data? I understand that DASH allows having separate streams (files) with different DRM systems, so exposing them as different pads is also more generic. I think having protection-system-id-34e5db32-8625-47cd-ba06-68fca0655a72=TRUE in the caps is really ugly.
(In reply to comment #25) > I'm not sure how having multiple pads that happen to send exactly the same data > (but with different caps) is different from having multiple pads that send > different data? I understand that DASH allows having separate streams (files) > with different DRM systems, so exposing them as different pads is also more > generic. DASH supports that? I think not even EME supports that, so unlikely that it's going to be used much ;) However for that case DASH needs a similar selection mechanism like for multiple audio streams in separate files. Just something separate for DRM here, where it somehow selects the appropiate file based on what DRM support is available on the system or $magic. > I think having protection-system-id-34e5db32-8625-47cd-ba06-68fca0655a72=TRUE > in the caps is really ugly. I agree. However if you expose the *same* stream on multiple pads you have a lot of overhead, decodebin would not know that it shouldn't plug decryptors for all of them (which then talk to the DRM system, which probably complains then that only one copy per user is allowed ;) ), and playbin would consider multiple decrypted streams as different streams.
(In reply to comment #26) So it looks like there are problems with both approaches. :) A couple of questions: (i) Is it only qtdemux that must set fixed src caps before the autoplugger can link downstream elements, or is this generally true of all demuxers? (ii) If it's generally true of all demuxers, is there a reason that would make it fundamentally impossible for an autoplugger to link downstream elements to a demuxer src pad that has unfixed caps, or is it just that this hasn't been implemented in decodebin? I guess what I'm getting at is whether the real issue is with the autoplugger, and whether that's where we need to be looking for a solution; the above two approaches both seem to be working around the fact that the autoplugger can't link downstream elements to src pad caps of qtdemux that are as-yet unfixed. (Apologies if I've misunderstood caps/autoplugging!)
Yes, that's generally true. The reason for that is that without fixed caps the autoplugger can't know what the actual caps will be in the end, so it might plug the wrong element. If you have unfixed caps "video/x-h264; video/mpeg" for example, what would you do?
(In reply to comment #28) Could that scenario ever occur? Surely, when it creates a new src pad, a demuxer would know from the information in the container the exact type of the media that will be output?
Sure, take the example "video/mpeg,mpegversion={2,4}" then. That's syntactically the same as what you proposed in comment 21 The problem with unfixed caps is that it semantically means a "any one of these" (it's either MPEG2 or MPEG4), while you want a "all of these" (it's both playready and fairplay that can handle this stream).
Created attachment 264272 [details] [review] Add cenc support to gst-plugins-base Updated patch for gst-plugins-base. - Unit tests added for cenc library. - Cenc MiniObjects have modified constructors and new accessor functions. - GstCencSubSampleInfo reverted to GstCencSubsampleInfo. This is to align with the naming scheme used in ISO/IEC 23001-7, in which `subsample' is used as a single, non-hyphenated word.
Created attachment 264274 [details] [review] Add cenc support to qtdemux Updated patch for gst-plugins-good. - Updated to reflect changes in gst-plugins-base cenc library. - Added new function that frees protection systems info, as suggested by Olivier abive. - Changed src pad caps format to not use nested GstCaps. When I tested the caps-in-caps approach, I found it didn't work with gst_caps_intersect/gst_caps_subset, and it didn't look like it would be straightforward to remedy this. Therefore, I've changed the src pad caps to the following: application/x-cenc, [original media fields], protection-system-id=(string)<URN of protection system>, protection-system-data=(buffer)<protection system private data>, original-media-type=(string)<video/x-h264|audio/mpeg|...> In other words, the cenc-specific fields are now in the same structure as the original media fields, and a field has been added called original-media-type, which holds the name of the original media caps.
(In reply to comment #32) > When I tested the caps-in-caps approach, I found it didn't work with > gst_caps_intersect/gst_caps_subset, and it didn't look like it would be > straightforward to remedy this. I wonder if we shouldn't fix that in the core.
I thought we were using it somewhere already, because I was trying to prevent allowing caps-in-caps at some point ;)
Storing caps in caps is working, but all the operations on caps are not including the inner caps. That said, I think the approach here is acceptable too, fixing caps-in-caps would be nicer though :)
mssdemux uses caps in caps FWIW, but we can't handle more than 2 levels of caps-in-caps because we use "" for marking the beginning/end of caps when serializing/deserializing.
We've been having a think about the different solutions suggested above for how to support ISOBMFF streams that can be decrypted using any one of a number of protection systems (i.e., the case where the stream contains multiple pssh boxes), and would like to suggest an alternative solution: In essence, the alternative is that qtdemux includes in its src pad caps the protection system IDs from all pssh boxes contained in the input stream, and leaves the task of choosing which of these systems to use to a downstream cenc element. There would be some flexibility around how this downstream cenc selection/decryption is implemented; for example: (i) The downstream element responsible for choosing which system to use could be an "autocencdecrypt" element, which will scan the registry and internally instantiate a suitable concrete decrypter element based upon the IDs in the caps, in the same way that autovideosink instantiates a suitable video sink based on its input caps. In this approach, the cenc decrypters for different protection systems would also be separate elements. (ii) The downstream element responsible for choosing which system to use could also itself perform decryption for all supported protection systems by making calls to the appropriate DRM system libraries. In this approach, there would be only a single cenc element that handles both selection and decryption (for all supported protection systems). As to how qtdemux would include the IDs of multiple protection systems in its src caps, I'd suggest that all the IDs are joined together into a single delimiter-separated string field. In other words, the protection-system-id field in the caps format suggested in previous comments would be renamed protection-system-identifiers; if there is a single pssh box, this field would contain the ID of the protection system identified in that pssh box, e.g., protection-system-identifiers=(string)"urn:uuid:9a04f079-9840-4286-ab92-e65be0885f95"; if there are multiple pssh boxes, their IDs would be combined in a single string, with the individual IDs delimited by a special character (such as the pipe character, '|'), e.g., protection-system-identifiers=(string)"urn:uuid:9a04f079-9840-4286-ab92-e65be0885f95|urn:uuid:9a27dd82-fde2-4725-8cbc-4234aa06ec09". Hopefully this would give us something that will enable multi-DRM cenc support in the vast majority of realistic use-cases, while still maintaining a reasonably clean caps format, and avoiding the overhead of having qtdemux expose multiple src pads. Would this be an acceptable solution?
(In reply to comment #37) > We've been having a think about the different solutions suggested above for how > to support ISOBMFF streams that can be decrypted using any one of a number of > protection systems (i.e., the case where the stream contains multiple pssh > boxes), and would like to suggest an alternative solution: > > In essence, the alternative is that qtdemux includes in its src pad caps the > protection system IDs from all pssh boxes contained in the input stream, and > leaves the task of choosing which of these systems to use to a downstream cenc > element. > > There would be some flexibility around how this downstream cenc > selection/decryption is implemented; for example: > (i) The downstream element responsible for choosing which system to use could > be an "autocencdecrypt" element, which will scan the registry and internally > instantiate a suitable concrete decrypter element based upon the IDs in the > caps, in the same way that autovideosink instantiates a suitable video sink > based on its input caps. In this approach, the cenc decrypters for different > protection systems would also be separate elements. That sounds acceptable IMHO. I personally still prefer the "protection-system-id-34e5db32-8625-47cd-ba06-68fca0655a72=TRUE"-approach but this seems ok too. This element should be inside GStreamer though and not provided externally. Externally only the specific decrypter elements would be provided. > (ii) The downstream element responsible for choosing which system to use could > also itself perform decryption for all supported protection systems by making > calls to the appropriate DRM system libraries. In this approach, there would be > only a single cenc element that handles both selection and decryption (for all > supported protection systems). Note that this would break negotiation. It would claim to support all protection systems, would be autoplugged because of that and then explode later if it does not support this specific system. Such an element will break addition of new cenc elements via plugins and should not be done that way IMHO. Also it has no advantage IMHO and just makes everything more complicated :) > As to how qtdemux would include the IDs of multiple protection systems in its > src caps, I'd suggest that all the IDs are joined together into a single > delimiter-separated string field. Why not just use a GstValueArray of strings?
(In reply to comment #38) [...] > > Why not just use a GstValueArray of strings? Ah, sorry - I was assuming that arrays in caps were similar to lists, in that they make the caps unfixed. But if that's not the case, then yes - that would be a better approach.
(In reply to comment #39) > (In reply to comment #38) > [...] > > > > Why not just use a GstValueArray of strings? > > Ah, sorry - I was assuming that arrays in caps were similar to lists, in that > they make the caps unfixed. But if that's not the case, then yes - that would > be a better approach. GstValueArray is fixed, GstValueList is not. Confusing, I know. I always have to look up which one is which each time too ;)
Also see bug #720780 about DTCP/IP support. We should try to use the same overall design for both as it's basically the same problem.
@slomo: The difference is that here you know the decrypted caps before the decrypter and not in DTCP/IP.
(In reply to comment #42) > @slomo: The difference is that here you know the decrypted caps before the > decrypter and not in DTCP/IP. I know, that's why it should just get a simple gst_type_find_helper_for_buffer() call in the DTCP/IP decrypter, everything else is not a good idea (as discussed before, I thought you agreed?). And then we're having exactly the same case with CENC and DTCP/IP.
(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #38) > > [...] > > > > > > Why not just use a GstValueArray of strings? > > > > Ah, sorry - I was assuming that arrays in caps were similar to lists, in that > > they make the caps unfixed. But if that's not the case, then yes - that would > > be a better approach. > > GstValueArray is fixed, GstValueList is not. Confusing, I know. I always have > to look up which one is which each time too ;) How does a GstValueArray work with intersecting? If qtdemux outputs: "application/x-cenc, encryption-scheme={A, B, C}" and my system has a decrypter with: "application/x-cenc, encryption-scheme=A" Does it work? Or will it only work with the autodecrypter that has all possible systems and does everything underneath?
(In reply to comment #44) > How does a GstValueArray work with intersecting? > > If qtdemux outputs: > > "application/x-cenc, encryption-scheme={A, B, C}" > > and my system has a decrypter with: > > "application/x-cenc, encryption-scheme=A" > > Does it work? Or will it only work with the autodecrypter that has all possible > systems and does everything underneath? That's a GstValueList, and there it would would intersect if your element handles any of the encryption schemes but unfortunately a GstValueList is not a fixed value. GstValueArray is e.g. encryption-scheme=<A, B, C> and considered fixed. And your element is only compatible with that if it has exactly <A, B, C> (and in that order!) on the caps. But the idea here is that there is a generic cenc autoplugger that has just "application/x-cenc" on the caps and then internally autoplugs a cenc decryptor that handles the encryption-scheme. IMHO not as nice conceptionally as the other solution that would use the caps as they're supposed to be used (and requires ugly caps, see above)... but still an acceptable compromise.
(In reply to comment #45) > (In reply to comment #44) > > > How does a GstValueArray work with intersecting? > > > > If qtdemux outputs: > > > > "application/x-cenc, encryption-scheme={A, B, C}" > > > > and my system has a decrypter with: > > > > "application/x-cenc, encryption-scheme=A" > > > > Does it work? Or will it only work with the autodecrypter that has all possible > > systems and does everything underneath? > > That's a GstValueList, and there it would would intersect if your element > handles any of the encryption schemes but unfortunately a GstValueList is not a > fixed value. > > GstValueArray is e.g. encryption-scheme=<A, B, C> and considered fixed. And > your element is only compatible with that if it has exactly <A, B, C> (and in > that order!) on the caps. > > But the idea here is that there is a generic cenc autoplugger that has just > "application/x-cenc" on the caps and then internally autoplugs a cenc decryptor > that handles the encryption-scheme. > > > IMHO not as nice conceptionally as the other solution that would use the caps > as they're supposed to be used (and requires ugly caps, see above)... but still > an acceptable compromise. I don't like this very much. The only way I can see this being flexible enough is if this cencdecrypt offers some kind of API to register decrypting modules (something similar to typefind) and as we don't need typefind-mp4 element we would never have to write a cenc-A-decrypt element. If this is the case, I think it is an ok solution. OTOH if we want to go with different elements and autoplugging just like regular decoding, we would need a new kind of GstCaps field. A GstSimultaneousList (maybe GstQuanticList? :) that would always be fixed with multiple values and would intersect* still resulting in a fixed caps. I haven't thought deeply on the pros and cons of those options, but I guess we need to either go typefind-like or autoplugging, mixing those 2 options can lead to inconsistencies in the future. * intersect of src caps cenc="A, B, C" with cenc="A, D" should still be "A, B, C" to keep the caps consistent with what's in it. I hope this doesn't break much of our caps logic/math :)
(In reply to comment #46) > I don't like this very much. The only way I can see this being flexible enough > is if this cencdecrypt offers some kind of API to register decrypting modules > (something similar to typefind) and as we don't need typefind-mp4 element we > would never have to write a cenc-A-decrypt element. If this is the case, I > think it is an ok solution. I don't like it that much either if that helps, but the other solution is not so great too ;) The idea here would be that this cencdecrypt element *understands* the special meaning of the encryption-scheme field. It would the look for an element of the right classification that matches *any* of the encryption-schemes inside that field, and modify the caps accordingly. It wouldn't be normal autoplugging as in decodebin but with some additional knowledge about the caps. Of course this is not nice because we break here the generic way of how caps work. A GstSimultaneousList or similar thing seems like an incredible amount of new complexity and confusion, as you can already see from your intersection example :) If we want to do it right with the way how caps work generically, I would vote for just having "encryption-scheme-A=TRUE, encryption-scheme-B=TRUE" fields in the caps. Not much uglier than codec_data in caps when transformed to a string ;)
(In reply to comment #47) [...] > > If we want to do it right with the way how caps work generically, I would vote > for just having "encryption-scheme-A=TRUE, encryption-scheme-B=TRUE" fields in > the caps. Not much uglier than codec_data in caps when transformed to a string > ;) OK - I think you've convinced me that this is a better approach. :) I'll get on and implement that.
Created attachment 267723 [details] [review] Add cenc support to gst-plugins-base Updated patch that now includes events and associated functions to notify downstream decrypter elements of protection system specific data that is extracted by qtdemux/dashdemux. This system-specific data is found in pssh boxes, which can be contained in both moov and moof boxes, and also in DASH MPDs. Two events are defined: one wraps initial pssh data, found in moov boxes (or DASH MPDs), the other wraps pssh data from movie fragments. This allows for key rotation, and also for protection systems that use a "root & leaf" licence model.
Created attachment 267725 [details] [review] Add cenc support to qtdemux Updated patch with following changes: - Now implements the caps format suggested by Sebastian above, i.e., includes "protection-system-id-A=TRUE, protection-system-id-B=TRUE" in src pad caps. - Now pushes pssh data downstream using the new events added to the latest gst-plugins-base patch.
Review of attachment 267723 [details] [review]: Just skipped over the patch, some first comments below :) ::: docs/libs/Makefile.am @@ +60,3 @@ $(top_builddir)/gst-libs/gst/allocators/libgstallocators-@GST_API_VERSION@.la \ $(top_builddir)/gst-libs/gst/audio/libgstaudio-@GST_API_VERSION@.la \ + $(top_builddir)/gst-libs/gst/cenc/libgstcenc-@GST_API_VERSION@.la \ I think to get started it would make sense to get this into gst-plugins-bad first until we're really sure about the API. And then have elements use that for some time. Also maybe we can have a "drm" library, that contains both the cenc and the dtcp-ip stuff... and whatever else comes in the future? ::: gst-libs/gst/cenc/gstcencevent.h @@ +33,3 @@ +#define GST_EVENT_CENC_PSSH_INIT GST_EVENT_MAKE_TYPE(330, \ + GST_EVENT_TYPE_DOWNSTREAM | GST_EVENT_TYPE_SERIALIZED | \ + GST_EVENT_TYPE_STICKY | GST_EVENT_TYPE_STICKY_MULTI) Wouldn't it be better to have these events as metas on the buffers to which they apply? Also you can't make custom events like this, you have to use the generic "custom downstream" event type and then your custom element would only have a different name in the structure. See how the force-keyunit element for example is implemented Also wouldn't the PSSH_INIT be possible to just put into the caps (because there is only one, right?), and the PSSH one as a meta (because there can be many, one per buffer in the worst case)?
(In reply to comment #51) > I think to get started it would make sense to get this into gst-plugins-bad > first until we're really sure about the API. And then have elements use that > for some time. > > > Also maybe we can have a "drm" library, that contains both the cenc and the > dtcp-ip stuff... and whatever else comes in the future? I don't particularly mind where it's located. But would there be any problems linking qtdemux to it if it's in gst-plugins-bad? > Wouldn't it be better to have these events as metas on the buffers to which > they apply? One of the reasons for choosing events is that you can have initial pssh information in DASH MPDs as well as in ISOBMFF files. So it seemed simpler to pass this information downstream using events, which might originate from dashdemux or from qtdemux. > Also you can't make custom events like this, you have to use the > generic "custom downstream" event type and then your custom element would only > have a different name in the structure. See how the force-keyunit element for > example is implemented Ah, OK. The behaviour I was seeking was for the most recent initial pssh event for each system-ID to be stuck to the src pad, and, similarly, the most recent non-initial pssh event for each system ID to be stuck to the src pad. That's why I went for two separate event types, setting the structure name of each event to GstCencEvent-<system UUID>. But I guess we could get the same behaviour if they are both CUSTOM_DOWNSTREAM_STICKY events, with the initial events named GstCencEventPsshInit-<system UUID> and the non-initial events named GstCencEventPssh-<system UUID>. That would hopefully give the same sticky behaviour, while allowing downstream elements to distinguish initial and non-initial pssh data. > Also wouldn't the PSSH_INIT be possible to just put into the caps (because > there is only one, right?), and the PSSH one as a meta (because there can be > many, one per buffer in the worst case)? First of all, there's one per pssh box in the media, so if you took that route you might have multiple pssh boxes in the caps (which may or may not be an problem - I guess the main drawback would be potentially lots of data in the caps). Secondly, there's not necessarily only one initial pssh event. In the case of DASH streams that have separate initialisation segments for all representations there will be new initial pssh information parsed from the moov of the initialisation segment each time the client switches to a different representation. If the initial pssh information is carried in the caps, then I guess in this scenario you'd need to send new caps events each time the representation changes (there's probably already a new caps event sent in this scenario on representation switches, so maybe this wouldn't be too much of an issue.) However, wouldn't it be more consistent to pass pssh data via the same mechanism, regardless of whether it's initial pssh data or not, and regardless of whether it originates from dashdemux or qtdemux?
(In reply to comment #52) > (In reply to comment #51) > > I think to get started it would make sense to get this into gst-plugins-bad > > first until we're really sure about the API. And then have elements use that > > for some time. > > > > > > Also maybe we can have a "drm" library, that contains both the cenc and the > > dtcp-ip stuff... and whatever else comes in the future? > > I don't particularly mind where it's located. But would there be any problems > linking qtdemux to it if it's in gst-plugins-bad? Indeed it would... you would need to have an internal copy for that in qtdemux then. Reason for suggesting to put it into bad first is that it would allow integrating this much faster, while still allowing some changes to the API. I don't think we will otherwise get this integrated before 1.4. > The behaviour I was seeking was for the most recent initial pssh event for each > system-ID to be stuck to the src pad, and, similarly, the most recent > non-initial pssh event for each system ID to be stuck to the src pad. That's > why I went for two separate event types, setting the structure name of each > event to GstCencEvent-<system UUID>. > > But I guess we could get the same behaviour if they are both > CUSTOM_DOWNSTREAM_STICKY events, with the initial events named > GstCencEventPsshInit-<system UUID> and the non-initial events named > GstCencEventPssh-<system UUID>. That would hopefully give the same sticky > behaviour, while allowing downstream elements to distinguish initial and > non-initial pssh data. Yes, that should work. > > Also wouldn't the PSSH_INIT be possible to just put into the caps (because > > there is only one, right?), and the PSSH one as a meta (because there can be > > many, one per buffer in the worst case)? > > First of all, there's one per pssh box in the media, so if you took that route > you might have multiple pssh boxes in the caps (which may or may not be an > problem - I guess the main drawback would be potentially lots of data in the > caps). > > Secondly, there's not necessarily only one initial pssh event. In the case of > DASH streams that have separate initialisation segments for all representations > there will be new initial pssh information parsed from the moov of the > initialisation segment each time the client switches to a different > representation. If the initial pssh information is carried in the caps, then I > guess in this scenario you'd need to send new caps events each time the > representation changes (there's probably already a new caps event sent in this > scenario on representation switches, so maybe this wouldn't be too much of an > issue.) > > However, wouldn't it be more consistent to pass pssh data via the same > mechanism, regardless of whether it's initial pssh data or not, and regardless > of whether it originates from dashdemux or qtdemux? Yes, I think that makes sense then.
(In reply to comment #53) [...] > > Indeed it would... you would need to have an internal copy for that in qtdemux > then. To clarify: are you suggesting copying all the cenc files into gst-plugins-good/gst/isomp4 and adding the *.c files to libgstisomp4_la_SOURCES? Cenc decrypter elements would also need to link against this code; would they need their own copy of the cenc header files and then link against libgstisomp4.so? > Reason for suggesting to put it into bad first is that it would allow > integrating this much faster, while still allowing some changes to the API. I > don't think we will otherwise get this integrated before 1.4. Agree that it would be good to take a route that leads to a quicker integration. [...] > > Yes, I think that makes sense then. OK, great. Thanks :)
(In reply to comment #54) > (In reply to comment #53) > [...] > > > > Indeed it would... you would need to have an internal copy for that in qtdemux > > then. > > To clarify: are you suggesting copying all the cenc files into > gst-plugins-good/gst/isomp4 and adding the *.c files to > libgstisomp4_la_SOURCES? > > Cenc decrypter elements would also need to link against this code; would they > need their own copy of the cenc header files and then link against > libgstisomp4.so? Not really sure :) This will all be ugly in various ways as the interfaces need to be kept in sync between all copies...
(In reply to comment #55) [...] > > Not really sure :) This will all be ugly in various ways as the interfaces need > to be kept in sync between all copies... In order to get cenc support integrated before 1.4, the cenc changes to qtdemux will also need to be integrated. Given that getting changes accepted into gst-plugins-good is already necessary, would it be an option to add the cenc/drm library code to gst-plugins-good/gst-libs? Or would that imply that the API is already fixed?
Sebastian?
(In reply to comment #56) > (In reply to comment #55) > [...] > > > > Not really sure :) This will all be ugly in various ways as the interfaces need > > to be kept in sync between all copies... > > In order to get cenc support integrated before 1.4, the cenc changes to qtdemux > will also need to be integrated. Given that getting changes accepted into > gst-plugins-good is already necessary, would it be an option to add the > cenc/drm library code to gst-plugins-good/gst-libs? Or would that imply that > the API is already fixed? It would imply that, yes.
Given that qtdemux is the problem, would it be an option to have effectively a 'qtdemux-cenc' in plugins-bad, which tracks qtdemux code but adds support for cenc? If this is done, and cenclib is also put in plugins-bad, then all the changes would be restricted to plugins-bad.
Can't we just put API unstable bits in -base instead, like we have plugins marked experimental plugins in -good, duplicating the demuxer will means we can't auto-plug it.
Erm not, both are not really a good idea. It's already annoying enough for people if we change unstable API in -bad, now just imagine how that would be if we do the same in -base. And copying such a large thing like qtdemux is not really nice either... but we could give it a higher rank than the one in -good to make it autopluggable.
I guess that leaves us the option is skipping 1.4 and merging this early in 1.5.
What are the timescales involved (i.e., when is 1.4 being released and when would 1.6 be released)?
Created attachment 269568 [details] [review] Add cenc support to gst-plugins-base Updated cenc library patch. - Now uses CUSTOM_DOWNSTREAM_STICKY events for passing pssh data, rather than defining new cenc event types.
Created attachment 269569 [details] [review] Add cenc support to qtdemux Updated patch for qtdemux. Fixes bug in which profile, level & codec_data weren't being included in caps for encrypted audio streams.
So, to summarise the above discussions, there are two general approaches: 1. Do something a bit ugly in order to include this in 1.4. 2. Wait until after 1.4 release and merge early in the 1.5 window. A couple of questions re. each approach: Approach #1: Are there actually any acceptable "quick & dirty" options that would allow us to get this in for 1.4? Even if we did find a solution to the issue of where to put cenclib, would the changes to qtdemux itself have a chance of being accepted for 1.4? Approach #2: Would GStreamer be happy to merge the changes into -good and -base soon after the 1.4 release, or would we need to go via -bad first, with the same linkage issues as discussed above? What is the timeframe for the 1.6 release?
I think this should wait until after 1.4. Timeframe for 1.6 is ~September 2014.
Created attachment 276467 [details] [review] re-based qtdemux patch to current git head I have re-based Chris' qtdemux patch to current git head as the current version attached to this ticket no longer applies cleanly.
Created attachment 282887 [details] [review] Add cenc support to gst-plugins-base Updated patch for gst-plugins-base, which adds support for passing arbitrary protection system specific information from ContentProtection elements within MPEG DASH MPDs. The format of the data in a ContectProtection element is not necessarily a cenc pssh box; therefore, the mechanism for passing protection system specific information via events has been updated to reflect this. Specifically: - The boolean indicating the origin of the information contained in an event has been replaced with an enumerated type that indicates whether its origin was a moov box, a moof box, or a DASH MPD. - Use of the term "pssh" has been replaced with the more generic "pssi" (Protection System Specific Information), so that the format of the information contained in an event is not implied to be a pssh box.
Created attachment 282890 [details] [review] Add cenc support to qtdemux Updated patch for gst-plugins-good. Adds support for cenc protection system information pushed in events from upstream elements, e.g., from dashdemux.
Created attachment 282891 [details] [review] add support for generating PSSI events from ContentProtection elements This patch adds support for ContentProtection elements in the MPD. If a ContentProtection element is present in an AdaptationSet element, send PSSI events (using libgstcenc from gst-plugins-base) on the source pad, so that qtdemux can use this information to correctly generate its source caps for DASH CENC encrypted streams. This allows qtdemux to support CENC encrypted DASH streams where the content protection specific information is carried in the MPD file rather than in pssh boxes in the initialisation segments.
Created attachment 285736 [details] [review] Add cenc support to qtdemux Rebased qtdemux patch on current git head, as previous patch no longer applies cleanly.
Created attachment 287081 [details] [review] add support for generating PSSI events from ContentProtection elements re-base of patch 282891 to current head of git master
Created attachment 287434 [details] [review] Add cenc support to qtdemux Rebases qtdemux patch to apply cleanly on current -good head.
We've created a CENC-encrypted DASH stream to allow the functionality in these patches to be tested. The URL of the MPD is http://test-media.youview.co.uk/ondemand/bbb/avc3/1/client_manifest.mpd The content keys needed to decrypt the audio and video representations are hosted on the same server[1]; they're simply stored in the clear as hex strings. The URL of the appropriate key (audio or video) has been placed in the data field of a PSSH box within the initialisation segments of the audio and video representations; in order to acquire the decryption key for a particular content ID, a CENC decrypter element simply needs to make an HTTP GET for the corresponding key URL, as parsed from the PSSH box. We've provided a suitable decrypter element that does this on GitHub (see below). In order to play this stream, you'll need to: 1. Apply the latest patches from this ticket to plugins-base (currently patch 282887) and qtdemux (currently patch 287434) and rebuild. 2. Grab a copy of the CENC decrypter plugin, build and install it; you can get this as follows: git clone -b clearkey https://github.com/asrashley/gst-cencdec.git 3. Play as normal: gst-launch-1.0 playbin uri=http://test-media.youview.co.uk/ondemand/bbb/avc3/1/client_manifest.mpd Note: the decrypter element mentioned above is called cencdec-clearkey, and it outputs debug messages to a category named "cencdec"; so if you're interested in seeing more of what the decrypter is doing, you can turn up the debug level on cencdec. [1] The URLs for the audio and video keys are: http://test-media.youview.co.uk/ondemand/bbb/avc3/1/keys/audio0 http://test-media.youview.co.uk/ondemand/bbb/avc3/1/keys/video0
Created attachment 288133 [details] [review] Add cenc support to qtdemux Updated patch for qtdemux. Now handles representations in which the number of samples in each segment varies.
Created attachment 295278 [details] [review] qtdemux: add support for ISOBMFF Common Encryption Rebased to master
Review of attachment 282887 [details] [review]: ::: gst-libs/gst/cenc/gstcencevent.c @@ +95,3 @@ + event_prefix = "GstCencEventPssiMpd"; + break; + } Can you elaborate on why we need to have this? Can't we reduce all options to a single way of storing the system-id and the key? Does the client need to differentiate or know where did the decryption information was?
As I mentioned in person, I don't think this should be pushed in this form. I think we need to take a step back and re-evaluate how we want to do this, and do it a bit more generically ideally.
Review of attachment 282887 [details] [review]: Problems in "uninstalled" set-up ::: pkgconfig/gstreamer-cenc-uninstalled.pc.in @@ +12,3 @@ +Version: @VERSION@ +Requires: gstreamer-@GST_API_VERSION@ +Libs: @abs_top_builddir@/gst-libs/gst/video/libgstcenc-@GST_API_VERSION@.la s/video/cenc/
Review of attachment 295278 [details] [review]: Problems in "uninstalled" set-up. ::: gst/isomp4/Makefile.am @@ +11,3 @@ -lgsttag-@GST_API_VERSION@ \ -lgstpbutils-@GST_API_VERSION@ \ + -lgstcenc-@GST_API_VERSION@ \ This didn't work for me in the "uninstalled" set-up so I had to add `AG_GST_PKG_CHECK_MODULES(GST_CENC, gstreamer-cenc-1.0)` in `configure.ac` and then use `$(GST_CENC_CFLAGS)` and `$(GST_CENC_LIBS)` in this makefile.
(In reply to comment #78) > Review of attachment 282887 [details] [review]: > > ::: gst-libs/gst/cenc/gstcencevent.c > @@ +95,3 @@ > + event_prefix = "GstCencEventPssiMpd"; > + break; > + } > > Can you elaborate on why we need to have this? Can't we reduce all options to a > single way of storing the system-id and the key? > > Does the client need to differentiate or know where did the decryption > information was? The problem is that some DRM systems use a different encoding of their DRM data in the MPD than data in a PSSH box. For example a certain fish related DRM system uses an XML representation in the MPD and a binary representation in PSSH boxes. The down-stream decryption element needs to know the source of the PSSI data, so that it knows which parser to use.
(In reply to comment #79) > As I mentioned in person, I don't think this should be pushed in this form. I > think we need to take a step back and re-evaluate how we want to do this, and > do it a bit more generically ideally. Without any concrete counter proposal, it's hard to have a meaningful discussion. Are you hinting that we should be using an upstream query to find PSSI data, rather than pushing it as sticky events? Or are you unhappy about the use of protection-system-id-xxxxx in the caps? As you can see from the length of this ticket, we've been back and forth on this and this approach seemed the least worst solution. Or are you unhappy about its name being specific to DASH CENC? The design supports PIFF as well as CENC, so we could rename it to something more generic.
Yes I know, sorry. Coming up with a counter proposal will take some time. I just don't think this is ready to land in its current form. I'm unhappy about a few things. I think it could be made more generic not dash cenc-specific. I think there might be a better solution about for the protection-ids. Also some implementation details (but those are easy to fix). This was just my way of saying that I don't think this should land in 1.6, as much as we all may want it to.
(In reply to comment #84) > I don't think this should land in 1.6, as much as we all may want it to. The same thing was said about 1.4. Apart from me rebasing the code every so often, there doesn't seem to be have been much progress. I would like to work out a way we can land this feature at some point.
I think what this needs to move forward is one of the GStreamer developers putting a considerable amount of time into reviewing this and reading alternative specifications solving the same problem, checking what libav does, etc. Especially I think this somehow needs to be unified with that DTCP-IP patch that is also in Bugzilla. In the end both are solving the same problem. Alex, maybe you can come to the hackfest in London this March and we can spend there an afternoon going through this and putting together a plan about how to move it forward?
Created attachment 296262 [details] [review] qtdemux: add support for ISOBMFF Common Encryption This patch replaces, patch 295278 as it fails to apply cleanly due to whitespace errors in the commit.
Created attachment 296271 [details] [review] dashdemux: add support for generating PSSI events from ContentProtection elements Re-based using adaptivedemux base class
Created attachment 296320 [details] [review] qtdemux: add support for ISOBMFF Common Encryption Doh! Uploaded wrong version of patch. Ignore 296262, it won't compile. Sorry.
Created attachment 296614 [details] [review] dashdemux: add support for generating PSSI events from ContentProtection elements Replacement of previous version of the dashdemux patch, as it did not apply cleanly on a fresh clone of the GStreamer repo.
How will this work from the point of view of the developer? Say for instance, some web rendering engine that needs to implement EME and DASH decryption support... How will the decryption keys be passed to the decoders? Can the new event be used for that?
(In reply to Philippe Normand from comment #91) > How will this work from the point of view of the developer? > > Say for instance, some web rendering engine that needs to implement EME and > DASH decryption support... When using MSE+EME, the implementation of DASH is in the HTML application. The HTML engine is being fed with the fragments that are requested by the JavaScript code of the HTML app. The HTML engine doesn't know (or care) if the stream is DASH, MSS or some other ABR technology. > How will the decryption keys be passed to the > decoders? > > Can the new event be used for that? I think the new Protection event should be able to provide that feature. The EME implementation would generate a Protection event with the binary blob it was provided via the EME API. The downstream decryption element would know how to handle this binary blob. In the case where DASH is implemented purely within the GStreamer world, Protection events are generated by dashdemux and qtdemux to provide the information required by the DASH CENC decryption element.
Created attachment 300676 [details] [review] Adds a new Protection event to GStreamer core In order for a decrypter element to decrypt media protected using a specific protection system, it first needs all the protection system specific information necessary (E.g. information on how to acquire the decryption keys) for that stream. The GST_EVENT_PROTECTION defined in this patch enables this information to be passed from elements that extract it (e.g. qtdemux, dashdemux) to elements that use it (E.g. a decryption element).
Created attachment 300677 [details] [review] Adds a new Decryptor klass to elementfactory An element that performs decryption does not naturally fit within any of the existing element factory class types. It is useful to be able to easily get a list of all elements that support decryption so that a union can be computed between the protection systems that have a supported decryptor and the allowed protection systems for a particular stream. This patch adds a new GST_ELEMENT_FACTORY_TYPE_DECRYPTOR and its associated string identifier "Decryptor". It also adds GST_ELEMENT_FACTORY_TYPE_DECRYPTOR to GST_ELEMENT_FACTORY_TYPE_DECODABLE so that uridecodebin can auto-plug a decryption element.
Created attachment 300678 [details] [review] Creates a GstProtectionMeta object that can be attached to buffers of encrypted samples In order to support some types of protected streams (such as those protected using DASH Common Encryption) some per-buffer information needs to be passed between elements. This patch adds a GstMeta type called GstProtectionMeta that allows protection specific information to be added to a GstBuffer. An example of its usage is qtdemux providing information to each output sample that enables a downstream element to decrypt it. This patch also adds a unit test for GstProtectionMeta that creates GstProtectionMeta adds and removes it from a buffer and performs some simple reference count checks.
Created attachment 300679 [details] [review] Utility function to select a Decryptor element This patch adds a gst_protection_factory_check function that takes an array of identifiers and searches the register for a decryption element that supports one or more of the supplied identifiers. If multiple elements are found, the one with the highest rank is selected. This patch also adds a unit test for the gst_protection_select_system function that adds a fake Decryptor element to the registry and then checks that it can correctly be selected by the utility function.
Created attachment 300680 [details] [review] qtdemux: add support for ISOBMFF Common Encryption This patch adds support for ISOBMFF Common Encryption (cenc), as defined in ISO/IEC 23001-7. It uses a GstProtection event to pass the contents of PSSH boxes to downstream decryptor elements and attaches GstProtectionMeta to each sample.
Created attachment 300681 [details] [review] dashdemux: add support for generating Protection events from ContentProtection elements If a ContentProtection element is present in an AdaptationSet element, send Protection events on the source pad, so that qtdemux can use this information to correctly generate its source caps for DASH CENC encrypted streams. This allows qtdemux to support CENC encrypted DASH streams where the content protection specific information is carried in the MPD file rather than in pssh boxes in the initialisation segments. This patch adds a new function to the adaptivedemux base class to allow a GstEvent to be queued for a stream. The queue of events are sent the next time a buffer is pushed for that stream.
I have just attached a bunch of patches to this ticket that implement the new API that we designed at the last GStreamer hackfest. There is a GST_ELEMENT_FACTORY_TYPE_DECRYPTOR klass that can be used by elements that implement decryption. There is a new Protection event that can be used to carry protection specific information between elements. I have kept the utility library in gst-plugins-base that defines a GstProtectionMeta type and a utility function that uses the element factory to find a supported decryptor. At the hackfest we discussed putting these somewhere else to remove the need to make a new library. I wasn't sure of a good location, so I kept it where it was. The GstProtectionMeta uses a GstStructure to carry all the protection specific information, rather than the previous design that used a DASH specific C structure. There is an example implementation of a decryption element on the "gst-api-v2" branch of gst-cencdec [https://github.com/asrashley/gst-cencdec/tree/gst-api-v2].
(In reply to Krzysztof Konopko from comment #81) > Review of attachment 295278 [details] [review] [review]: > > Problems in "uninstalled" set-up. > > ::: gst/isomp4/Makefile.am > @@ +11,3 @@ > -lgsttag-@GST_API_VERSION@ \ > -lgstpbutils-@GST_API_VERSION@ \ > + -lgstcenc-@GST_API_VERSION@ \ > > This didn't work for me in the "uninstalled" set-up so I had to add > `AG_GST_PKG_CHECK_MODULES(GST_CENC, gstreamer-cenc-1.0)` in `configure.ac` > and then use `$(GST_CENC_CFLAGS)` and `$(GST_CENC_LIBS)` in this makefile. Thanks Kris I think the problem is actually in the pkgconfig files in gst-plugins-base that were not including the new library, which caused pkg-config to fail for other packages that needed it.
Created attachment 301551 [details] [review] add GstProtectionMeta to support protected content This patch fixes an error in patch300678 that caused the gstprotection library to not be correctly included in the pkg-config for gstreamer-plugins-base.
Created attachment 301554 [details] [review] gstprotection: Add utility function to select a supported protection system Fixes commit message of patch300679 that had the wrong function name in the commit message.
Comment on attachment 300677 [details] [review] Adds a new Decryptor klass to elementfactory Pushed this already, with a minor change (used "Decryptor" instead of the KLASS define from the header in the code for consistency with the rest of the code and nicer readability), also added a matching ENCRYPTOR define. commit b8f5a7ba45215a7b37e424150ae987b8a13c3d8d Author: Alex Ashley <bugzilla@ashley-family.net> Date: Mon Mar 16 13:11:59 2015 +0000 elementfactory: add DECRYPTOR class defines
Comment on attachment 300676 [details] [review] Adds a new Protection event to GStreamer core The event stuff looks mostly fine. I have a question though: the event is of type MULTI_STICKY, presumably so we can have multiple types of protection data sticky per pad/stream, e.g. some global data plus something that gets updated on a regular basis. What parallelism will we require in practice though? Only different origins, or do we expect there may be a need for events for different system_ids to co-exist for the same stream? I think it would be nicer if the system_id string and the origin string were added to the event GstStructure explicitly as fields as well. That would also simplify the parsing code in parse_protection() and would allow you to return a pointer to the string without making a copy (which is nicer for the caller).
Comment on attachment 301554 [details] [review] gstprotection: Add utility function to select a supported protection system This patch I'm a bit confused about. There's this whole GstProtection interface, but I don't see it used anywhere. Is it still needed? (I assume you modelled it on GstURI)
Comment on attachment 301551 [details] [review] add GstProtectionMeta to support protected content I don't think we need a new micro library just for a meta and a utility function, so 90% of this can just go away IMHO. I'd say just rename gstprotectionmeta.[ch] to gstprotection.[ch] and drop that into gst/ in GStreamer core.
(In reply to Tim-Philipp Müller from comment #104) > Comment on attachment 300676 [details] [review] [review] > Adds a new Protection event to GStreamer core > > The event stuff looks mostly fine. I have a question though: the event is of > type MULTI_STICKY, presumably so we can have multiple types of protection > data sticky per pad/stream, e.g. some global data plus something that gets > updated on a regular basis. What parallelism will we require in practice > though? Only different origins, or do we expect there may be a need for > events for different system_ids to co-exist for the same stream? > At the moment, dashdemux pushes an event per protection system ID. One option would be for dashdemux to use the utility function to select a system ID and only push one protection event per pad. If the event was not MULTI_STICKY, would a downstream element still be able to get events from multiple upstream elements? > I think it would be nicer if the system_id string and the origin string were > added to the event GstStructure explicitly as fields as well. That would > also simplify the parsing code in parse_protection() and would allow you to > return a pointer to the string without making a copy (which is nicer for the > caller). Yes, I wondered about doing that. I think it would also make is safer because it removes the possibility of characters in the origin string causing the tokeniser to go wrong. Is there a convention about transfer-full vs transfer-none for the event parsing functions?
(In reply to Tim-Philipp Müller from comment #105) > Comment on attachment 301554 [details] [review] [review] > gstprotection: Add utility function to select a supported protection system > > This patch I'm a bit confused about. There's this whole GstProtection > interface, but I don't see it used anywhere. Is it still needed? (I assume > you modelled it on GstURI) No the interface is not needed. It's a demonstration of my lack of GStreamer knowledge! Essentially I was following other code to create a GType and g_once to configure the debug category. Would it be better to move the utility function to gstelementfactory?
(In reply to Tim-Philipp Müller from comment #106) > Comment on attachment 301551 [details] [review] [review] > add GstProtectionMeta to support protected content > > I don't think we need a new micro library just for a meta and a utility > function, so 90% of this can just go away IMHO. > This is another area where my lack of GStreamer knowledge gets in the way. It was not obvious if there is a pattern regarding GstMeta types. I can see a few defined in individual elements, but it wasn't obvious how to create a generic one. > I'd say just rename gstprotectionmeta.[ch] to gstprotection.[ch] and drop > that into gst/ in GStreamer core. That would be easy for me to do, if you think that's the more acceptable way to define a new GstMeta type.
> If the event was not MULTI_STICKY, would a downstream element still be able > to get events from multiple upstream elements? Yes, event stickiness is not really about that though. Imagine the demuxer sends out events before the decryptor plugin was linked. Now that decryptor gets linked afterwards. It will receive all sticky events that have accumulated on the peer pad. If it's a normal sticky event, the decryptor would only receive one single event (the last one sent, as each new one would replace the previous one). If it's a MULTI_STICKY event, the decryptor would receive all sticky events with unique structure names (and every event with the same structure name would replace the previous one on the pad). So I think MULTI_STICKY is what we want, since we probably need to prime the decryptor with multiple pieces of protection info from various origins, not just whatever was the last one. I guess my question was more about the multiple system IDs. The current implementation would be the most future proof one though, and we can still make the demuxer send only events for one system if we want to. > Is there a convention about transfer-full > vs transfer-none for the event parsing functions? Not really, it's a bit inconsistent unfortunately, but I'd say 'transfer none' is more common (see parse_caps or parse_tags). I would opt for all transfer-none where possible. It's nicer for the caller, and taking a ref / making a copy for stuff that needs to be kept around is easy enough.
A meta is just like any other API really. Where it goes depends on a bunch of things. If it's generic it might go into core, if it's audio/video specific it would go into -base, etc. There are no firm rules really. Just like the protection event, we have kept the meta very generic, so I think it's fine to go into core. The utility function can also go into gstprotection.c IMO.
Possibly we could add the stream-id/group-id from the stream-start event in each sticky protection event, so stale ones can be ignored?
Or possibly just use the same event seqnum.
Created attachment 301696 [details] [review] event: Create a new Protection event
Created attachment 301697 [details] [review] Creates gstprotection in GStreamer core This patch adds a GstMeta type called GstProtectionMeta that allows protection specific information to be added to a GstBuffer. An example of its usage is qtdemux providing information to each output sample that enables a downstream element to decrypt it. This patch adds a utility function to select a supported protection system from the installed Decryption elements found in the registry. The gst_protection_select_system function that takes an array of identifiers and searches the registry for a element of klass Decryptor that supports one or more of the supplied identifiers. If multiple elements are found, the one with the highest rank is selected.
Created attachment 301698 [details] [review] qtdemux: add support for ISOBMFF Common Encryption
Created attachment 301699 [details] [review] dashdemux: add support for generating Protection events from ContentProtection elements
Comment on attachment 301697 [details] [review] Creates gstprotection in GStreamer core Looks good overall, thanks. >+GType >+gst_protection_meta_api_get_type (void) >+{ >+ static volatile GType type; >+ static const gchar *tags[] = { "memory", NULL }; Don't think "memory" is right here. Maybe we want a "protection" tag, but for now I'd just leave tags as { NULL }; >+gst_protection_meta_init (GstMeta * meta, gpointer params, GstBuffer * buffer) >+gst_protection_meta_free (GstMeta * meta, GstBuffer * buffer) I think those two should be static functions and not be exposed in the header. >+/** >+ * gst_buffer_add_protection_meta: >+ * @buffer: #GstBuffer holding an encrypted sample, to which protection metadata >+ * should be added. >+ * @info: (allow-none): a #GstStructure holding cryptographic Should also have a (transfer full) annotation. Why is it ok for info to be NULL, is that useful for anything? Please also add * Since: 1.6 markers at the end of all gtk-doc chunks for new API.
Comment on attachment 301696 [details] [review] event: Create a new Protection event Also looks good, just a few minor niggles: >+GstEvent * >+gst_event_new_protection (const gchar * system_id, >+ GstBuffer * data, const gchar * origin) >+{ >+ ... >+ event_name = >+ g_strconcat ("GstProtectionEvent-", >+ origin ? origin : "", "-", system_id, NULL); If origin is NULL the event string contains "--", is that on purpose? >+void >+gst_event_parse_protection (GstEvent * event, const gchar ** system_id, >+ GstBuffer ** data, const gchar ** origin) >+{ >+ ... >+ g_return_if_fail (GST_EVENT_TYPE (event) == GST_EVENT_PROTECTION); >+ >+ s = gst_event_get_structure (event); >+ name = gst_structure_get_name (s); >+ g_return_if_fail (g_str_has_prefix (name, "GstProtectionEvent-")); I don't think this name check is needed here, the EVENT_TYPE check should be enough. >+ if (origin) >+ *origin = gst_structure_get_string (s, "origin"); >+ >+ if (system_id) >+ *system_id = gst_structure_get_string (s, "system_id"); >+ >+ if (data) { >+ const GValue *value = gst_structure_get_value (s, "data"); >+ g_return_if_fail (GST_VALUE_HOLDS_BUFFER (value)); Don't think this HOLDS_BUFFER check is needed here. We created the event after all. (People can do crazy things of course, but we don't have to worry about that) > GST_EVENT_SINK_MESSAGE = GST_EVENT_MAKE_TYPE (100, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY) | FLAG(STICKY_MULTI)), > GST_EVENT_EOS = GST_EVENT_MAKE_TYPE (110, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY)), > GST_EVENT_TOC = GST_EVENT_MAKE_TYPE (120, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY) | FLAG(STICKY_MULTI)), >+ GST_EVENT_PROTECTION = GST_EVENT_MAKE_TYPE (130, >+ FLAG (DOWNSTREAM) | FLAG (SERIALIZED) | FLAG (STICKY) | >+ FLAG (STICKY_MULTI)), Please align your addition with that of the other events. And don't forget to add 'Since: 1.6' markers to the gtk-doc chunks.
(In reply to Tim-Philipp Müller from comment #119) > Comment on attachment 301696 [details] [review] [review] > event: Create a new Protection event > > Also looks good, just a few minor niggles: > > > > GST_EVENT_SINK_MESSAGE = GST_EVENT_MAKE_TYPE (100, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY) | FLAG(STICKY_MULTI)), > > GST_EVENT_EOS = GST_EVENT_MAKE_TYPE (110, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY)), > > GST_EVENT_TOC = GST_EVENT_MAKE_TYPE (120, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY) | FLAG(STICKY_MULTI)), > >+ GST_EVENT_PROTECTION = GST_EVENT_MAKE_TYPE (130, > >+ FLAG (DOWNSTREAM) | FLAG (SERIALIZED) | FLAG (STICKY) | > >+ FLAG (STICKY_MULTI)), > > Please align your addition with that of the other events. > I've tried, but the gst-indent commit hook is stopping me from making a commit with the format used by the other events.
Oh, don't run gst-indent on header files, just on .c files. Header files are free-form :)
Created attachment 301851 [details] [review] event: Create a new Protection event In order for a decrypter element to decrypt media protected using a specific protection system, it first needs all the protection system specific information necessary (E.g. information on how to acquire the decryption keys) for that stream. The GST_EVENT_PROTECTION defined in this patch enables this information to be passed from elements that extract it (e.g. qtdemux, dashdemux) to elements that use it (E.g. a decrypter element).
Created attachment 301853 [details] [review] Creates gstprotection in GStreamer core In order to support some types of protected streams (such as those protected using DASH Common Encryption) some per-buffer information needs to be passed between elements. This patch adds a GstMeta type called GstProtectionMeta that allows protection specific information to be added to a GstBuffer. An example of its usage is qtdemux providing information to each output sample that enables a downstream element to decrypt it.
Comment on attachment 301853 [details] [review] Creates gstprotection in GStreamer core Pushed, with minor changes: (transfer-full) -> (transfer full) ; GST_DEBUG strings don't need a newline at the end; added some gratuitious newlines in the code. Also removed the "If the sample is not encrypted, @info may be %NULL." from the gst_buffer_add_protection_meta() docs to make it consistent with the function guards. Having said that, perhaps it does make sense to allow a NULL info, but one could just as well signal an unencrypted sample with the absence of the protection meta, no? Either way would work for me if you think it makes something easier or is more elegant.
(In reply to Olivier Crête from comment #112) > Possibly we could add the stream-id/group-id from the > stream-start event in each sticky protection event, > so stale ones can be ignored? > Or possibly just use the same event seqnum. Maybe, I guess we can still add that when we find that we need that? I'm not a fan of the seqnum referencing (in general). I think STREAM_START events should generally clear all existing sticky events, but that's probably a separate discussion. In any case, I think we can punt this question for now and don't need to block on it, and it's a problem with other sticky events such as tags as well.
Review of attachment 301698 [details] [review]: It might seem slightly odd to review my own patch, but there are aspects I want to highlight and hopefully gather some suggestions for improvement. ::: gst/isomp4/qtdemux.c @@ +2125,3 @@ + gst_event_parse_protection (event, &system_id, NULL, NULL); + GST_DEBUG_OBJECT (demux, "system_id: %s", system_id); + gst_qtdemux_append_protection_system_id (demux, system_id); Combine in to one GST_DEBUG_OBJECT message? @@ +3220,3 @@ + return FALSE; + } + buf = gst_buffer_new_wrapped (data, n_subsamples * 6); Should there be an option that causes the whole CencSampleAuxiliaryDataFormat structure + a sample number to be placed in the GstStructure, instead of just the BytesOfClearData and BytesOfEncryptedData for the sample? I don't think we can always send the whole CencSampleAuxiliaryDataFormat structure because there are some use cases that require this data to be serialised between qtdemux and the decrypt element. Serialising the entire CencSampleAuxiliaryDataFormat on each sample would be rather costly when only a few bytes of it are needed for each sample. @@ +3380,3 @@ + if (G_UNLIKELY (!qtdemux_parse_saio (qtdemux, stream, &saio_data, + &info_type, &info_type_parameter, &offset))) + GST_ERROR_OBJECT (qtdemux, "failed to parse saio box"); Should there be a "goto fail" if parsing the saio fails? If it's not a fatal error, perhaps the message should be a GST_WARNING_OBJECT? @@ +4892,3 @@ + does not require the data to be copied when attached as Meta to a buffer */ + crypto_info = gst_structure_copy (crypto_info); + GST_LOG_OBJECT (qtdemux, "attaching cenc metadata [%u]", index); Making a copy of the crypto_info for each sample seems wasteful. I tried using a GPtrArray of GstStructure but that segfaults when g_ptr_array_free() is called. Maybe because GstStructure does not have its own reference counting? @@ +8221,3 @@ + if (G_UNLIKELY (!stream->protected)) { + GST_INFO_OBJECT (qtdemux, "stream is not protected"); + return FALSE; Is this an error condition? If not, it should be "return TRUE;"
Created attachment 304512 [details] [review] qtdemux: support for cenc auxiliary info parsing outside of moof box When the cenc aux info index is out of moof boundaries, keep track of it and parse the beginning of the mdat box, before the first sample.
(In reply to Philippe Normand from comment #127) > Created attachment 304512 [details] [review] [review] > qtdemux: support for cenc auxiliary info parsing outside of moof box > > When the cenc aux info index is out of moof boundaries, keep track of > it and parse the beginning of the mdat box, before the first sample. I'm not totally sure this is the right way to do but it worked for the sample I have here. Feedback welcome :)
Created attachment 307010 [details] [review] qtdemux: add support for ISOBMFF Common Encryption Add support for ISOBMFF Common Encryption (cenc), as defined in ISO/IEC 23001-7. It uses a GstProtection event to pass the contents of PSSH boxes to downstream decryptor elements and attached GstProtectionMeta to each sample. Updated to git master 1.5.2-53-g297c166 on 7/07/2015
Created attachment 307012 [details] [review] dashdemux: add support for generating Protection events from ContentProtection elements If a ContentProtection element is present in an AdaptationSet element, send Protection events on the source pad, so that qtdemux can use this information to correctly generate its source caps for DASH CENC encrypted streams. This allows qtdemux to support CENC encrypted DASH streams where the content protection specific information is carried in the MPD file rather than in pssh boxes in the initialisation segments. Rebased to master 1.5.2-53-g297c166 on 7/07/2015
Comment on attachment 307012 [details] [review] dashdemux: add support for generating Protection events from ContentProtection elements >+ if (cp->schemeIdUri && g_str_has_prefix (cp->schemeIdUri, "urn:uuid:")) { >+ pssi_len = g_utf8_strlen (cp->value, -1); >+ pssi = gst_buffer_new_wrapped (g_memdup (cp->value, pssi_len), pssi_len); >+ GST_LOG_OBJECT (stream, "Queuing Protection event on source pad"); >+ event = gst_event_new_protection (cp->schemeIdUri + 9, pssi, "dash/mpd"); >+ gst_adaptive_demux_stream_add_event ((GstAdaptiveDemuxStream *) stream, >+ event); >+ gst_buffer_unref (pssi); >+ } Just to confirm, did you use g_utf8_strlen() here on purpose, or because you thought it was the right thing to do with UTF-8 strings? I think it should be a plain strlen(). g_utf8_strlen() returns the number of *characters* in a UTF-8 string, which may be smaller than the length of the string in bytes. strlen() will return the length in bytes. I suspect the string will be ASCII here anyway, so it won't matter.
(In reply to Tim-Philipp Müller from comment #131) > Comment on attachment 307012 [details] [review] [review] > dashdemux: add support for generating Protection events from > ContentProtection elements > > >+ if (cp->schemeIdUri && g_str_has_prefix (cp->schemeIdUri, "urn:uuid:")) { > >+ pssi_len = g_utf8_strlen (cp->value, -1); > >+ pssi = gst_buffer_new_wrapped (g_memdup (cp->value, pssi_len), pssi_len); > >+ GST_LOG_OBJECT (stream, "Queuing Protection event on source pad"); > >+ event = gst_event_new_protection (cp->schemeIdUri + 9, pssi, "dash/mpd"); > >+ gst_adaptive_demux_stream_add_event ((GstAdaptiveDemuxStream *) stream, > >+ event); > >+ gst_buffer_unref (pssi); > >+ } > > Just to confirm, did you use g_utf8_strlen() here on purpose, or because you > thought it was the right thing to do with UTF-8 strings? I think it should > be a plain strlen(). g_utf8_strlen() returns the number of *characters* in a > UTF-8 string, which may be smaller than the length of the string in bytes. > strlen() will return the length in bytes. I suspect the string will be ASCII > here anyway, so it won't matter. I used g_utf8_strlen because the MPD files are UTF-8 encoded, but you are right, what I actually want is the number of bytes, not the number of characters. I have also found another problem, which is that the code assumes that "urn:uuid:" is always lower case, which is not a valid assumption.
Created attachment 307759 [details] [review] dashdemux: add support for generating Protection events from ContentProtection elements If a ContentProtection element is present in an AdaptationSet element, send Protection events on the source pad, so that qtdemux can use this information to correctly generate its source caps for DASH CENC encrypted streams. Rebased to master (1.5.2-123-g410aec7), fixed case-sensitivity bug and switched to using strlen based upon Tim's review.
Alright, pushed this now: commit b4fe1b516be609c1998ded211764c874569a3422 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sat Jul 18 21:18:23 2015 +0100 adaptivedemux: minor clean-up No need for a foreach callback function that's just a few lines of code and is only used once, just do the event pushing inline. https://bugzilla.gnome.org/show_bug.cgi?id=705991 commit 71a1e3669a743daa5affbdb6d73f6949d4965818 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Fri Feb 6 13:22:14 2015 +0000 dashdemux: add support for generating Protection events from ContentProtection elements If a ContentProtection element is present in an AdaptationSet element, send Protection events on the source pad, so that qtdemux can use this information to correctly generate its source caps for DASH CENC encrypted streams. This allows qtdemux to support CENC encrypted DASH streams where the content protection specific information is carried in the MPD file rather than in pssh boxes in the initialisation segments. This commit adds a new function to the adaptivedemux base class to allow a GstEvent to be queued for a stream. The queue of events are sent the next time a buffer is pushed for that stream. https://bugzilla.gnome.org/show_bug.cgi?id=705991 which is basically your patch with minor formatting/indentation changes, and _add_event() renamed to _queue_event() which seems more appropriate. I merged your updates into a modified version of your previous patch, hopefully I didn't mess up somewhere. Please test.
Tested (with qtdemux patch applied) and it works.
(In reply to Philippe Normand from comment #128) > (In reply to Philippe Normand from comment #127) > > Created attachment 304512 [details] [review] [review] [review] > > qtdemux: support for cenc auxiliary info parsing outside of moof box > > > > When the cenc aux info index is out of moof boundaries, keep track of > > it and parse the beginning of the mdat box, before the first sample. > > I'm not totally sure this is the right way to do but it worked for the > sample I have here. Feedback welcome :) I think the DASH CENC specification allows the saio box to point to anywhere in the fragment for the CommonEncryptionSampleAuxiliaryInformation. It might not be right to assume that it is always before the first sample. DVB-DASH mandates that the data must be within the moof box that contains the saio box.
Created attachment 307815 [details] [review] qtdemux: use GPtrArray for crypto_info to avoid GstStructure copies By using a GPtrArray of GstCaps rather than a single GstCaps, the gst_qtdemux_decorate_and_push_buffer() function can avoid having to do a copy of the content protection related GstStructure for each sample. This reduces the memory and CPU consumption of encrypted streams.
Comment on attachment 307815 [details] [review] qtdemux: use GPtrArray for crypto_info to avoid GstStructure copies This doesn't seem to be the right patch ("use GPtrArray for crypto_info to avoid GstStructure copies")? This 'set in one go' should be merged into the previous one.
Comment on attachment 307010 [details] [review] qtdemux: add support for ISOBMFF Common Encryption Comments / Questions: --------------------- - GST_EVENT_PROTECTION: why do we parse these and add them to the system_id list, but then drop the event instead of forwarding it or storing it for later forwarding? If we're going to pick up the info + system_id from the headers later again anyway, we may just as well just drop the event here, no? - in decorate_and_push_buffer(): when you return GST_FLOW_ERROR, the element should post a proper error message on the bus, e.g. with: GST_ELEMENT_ERROR (demux, STREAM, DEMUX, (NULL), ("stream protected using cenc, but no cenc sample information has been found")); - parse_saiz(): if you're going to ignore info_type + info_type_parameter anyway, drop it from the function and don't parse it out. If anyone needs it later, they can add that later, just add a comment that you're skipping those two there. - parse_saiz(): I think you should just return a guint8 * for the info_sizes there, and fill it with sample_count times default_size if they are all default_size. Should make the code a bit easier everywhere, and you save the GArray which is not needed really. The extra overhead should be negligible and it's temporary anyway. /* Parse saiz box (= the sizes of sample auxiliary info), returns array * of sample_count guint8 size values, or NULL on failure */ static guint8 * qtdemux_parse_saiz (GstQTDemux * demux, GstByteReader * br, guint32 * sample_count) - general question: the CFF spec defines things such that there's an 'senc' box and the sample aux info pointed to by 'saiz' and 'saio' is always inside the 'senc'. Isn't this always the case? If yes, we could just parse the 'senc' box into a GstBuffer and attach the whole senc box to each outgoing sample, plus the sample index for that sample. The decryptor can then parse out all the info it needs (IV, sub sample details etc.) itself. Would save a whole lot of code in qtdemux, and I doubt parsing this in the decryptor would be much more code than handling all the various bits from the structure. Am I missing something or am I confused, or would this be possible? - parse_trak(): you move down the stream->caps = qtdemux_video_caps(...), I think this breaks stuff, such as the 'if (palette_data) {' there which adds stuff to the caps. And you moved a 'g_free (palette_data)' down which would free data that's been used/freed in the if block earlier. Style/Nitpicks (not so important): ---------------------------------- - #include <gst/gstprotection.h> not needed, picked up via gst/gst.h - you generally do a lot of nesting, try more early returns/goto out, that makes code easier to follow. Example: in configure_protected_caps(), just do: if (stream->protection_scheme_type != FOURCC_cenc) { GST_ERROR_OBJECT (qtdemux, "unsupported protection scheme"); return FALSE; } and then the main code comes next. - G_LIKELY/UNLIKELY are probably not very useful in code paths that are not performance critical and called rarely, but affect readability - gst_qtdemux_append_protection_system_id(): just return from loop if found, get rid of that existing_id boolean dance (also drop the 'else {' after creating the array, it drops an indentation level and makes for nicer to read code, and performance is not so much an issue here) - protection_scheme_event_queue: embed in structure (GQueue foo_queue) - qtdemux_uuid_bytes_to_string: just use g_strdup_printf() :) - qtdemux_parse_pssh(): no need to check for fourcc_pssh again, since the code that looks up the node presumably does that already... - qtdemux_parse_pssh(): you can assume gst_event_new_*() always succeeds - parse_cenc_aux_info(): no need to check return value of gst_buffer_new_wrapped(), always succeeds - decorate_and_push_buffer(): just do while ((event = g_queue_pop_header (&foo->queue))) { ... } and drop the empty check and the while (queue->length > 0) - configure_protected_caps(): one could just add a NULL pointer to the id array, and then cast (gchar **) protection_system_ids->pdata ;) (but then if you want to add more entries later you'd have to remove the NULL again, but aiui that's not supported anyway since once we made a choice we've made a choice.)
Created attachment 308359 [details] [review] qtdemux: use GPtrArray for crypto_info to avoid GstStructure copies Doh! uploaded wrong patch. Here's the correct one. By using a GPtrArray of GstCaps rather than a single GstCaps, the gst_qtdemux_decorate_and_push_buffer() function can avoid having to do a copy of the content protection related GstStructure for each sample. This reduces the memory and CPU consumption of encrypted streams.
Comment on attachment 308359 [details] [review] qtdemux: use GPtrArray for crypto_info to avoid GstStructure copies Thanks. I was going to say you should merge this into the other patch. First some comments on this though, and I skipped it when reviewing the other patch since I thought you had a fix for it: GstCaps should IMHO only be used for caps, not as some auxiliary helper object I don't entirely understand what you're trying to achieve here. Ok, so structures are not refcounted and you're trying to avoid a structure copy. But what you're doing with _steal_structure() here means the GstCaps in the array are going to be useless and can't be used again (might they ever be used again, e.g. when seeking back?). So why jump through GstCaps hoops in the first place? You might just as well put the GstStructure into the array and then steal it and set the value in the array to NULL. That's basically the same thing you're doing now just without the extra caps objects, no?
(In reply to Tim-Philipp Müller from comment #139) > Comment on attachment 307010 [details] [review] [review] > qtdemux: add support for ISOBMFF Common Encryption > > Comments / Questions: > --------------------- > > - GST_EVENT_PROTECTION: why do we parse these and add them to the system_id > list, but then drop the event instead of forwarding it or storing it for > later forwarding? If we're going to pick up the info + system_id from the > headers later again anyway, we may just as well just drop the event here, > no? The systems IDs can be defined in either the MPD, PSSH boxes, or both. qtdemux needs to combine all of these IDs before it calls gst_protection_select_system(). Passing the event on is a good idea, because in some DRM systems it might contain information required by the decryption element. > > - in decorate_and_push_buffer(): when you return GST_FLOW_ERROR, the element > should post a proper error message on the bus, e.g. with: > > GST_ELEMENT_ERROR (demux, STREAM, DEMUX, (NULL), > ("stream protected using cenc, but no cenc sample information has > been found")); Will do. > > - parse_saiz(): if you're going to ignore info_type + info_type_parameter > anyway, drop it from the function and don't parse it out. If anyone needs > it later, they can add that later, just add a comment that you're skipping > those two there. > Ok. > - parse_saiz(): I think you should just return a guint8 * for the info_sizes > there, and fill it with sample_count times default_size if they are all > default_size. Should make the code a bit easier everywhere, and you save > the GArray which is not needed really. The extra overhead should be > negligible and it's temporary anyway. > > /* Parse saiz box (= the sizes of sample auxiliary info), returns array > * of sample_count guint8 size values, or NULL on failure */ > static guint8 * > qtdemux_parse_saiz (GstQTDemux * demux, GstByteReader * br, guint32 * > sample_count) > > - general question: the CFF spec defines things such that there's an 'senc' > box > and the sample aux info pointed to by 'saiz' and 'saio' is always inside > the > 'senc'. Isn't this always the case? If yes, we could just parse the 'senc' > box into a GstBuffer and attach the whole senc box to each outgoing sample, > plus the sample index for that sample. The decryptor can then parse out all > the info it needs (IV, sub sample details etc.) itself. Would save a whole > lot of code in qtdemux, and I doubt parsing this in the decryptor would be > much more code than handling all the various bits from the structure. > Am I missing something or am I confused, or would this be possible? > The problem with that approach for me is that we don't have the entire pipeline in a single process. The inter-process communication mechanism requires serialising the GstMeta attached to each buffer. If each sample contained the entire senc box, it would get serialised/de-serialised for each sample, which is a big overhead. Also, qtdemux seems like the right place to be parsing MP4 atoms.
Ok, let's forward the protection events then. The main reason I'm asking about the senc box is that the DRM APIs I've dealt with actually want the senc box plus sample index passed, so parsing them seems completely unnecessary in that case. I see your point about the serialisation/deserialisation overhead when doing IPC. However, this looks like a more general optimisation/design issue in your IPC mechanism to me and I don't think we should take it into account when deciding what to do in qtdemux.
(In reply to Tim-Philipp Müller from comment #141) > Comment on attachment 308359 [details] [review] [review] > qtdemux: use GPtrArray for crypto_info to avoid GstStructure copies > > Thanks. I was going to say you should merge this into the other patch. First > some comments on this though, and I skipped it when reviewing the other > patch since I thought you had a fix for it: > > GstCaps should IMHO only be used for caps, not as some auxiliary helper > object > > I don't entirely understand what you're trying to achieve here. Ok, so > structures are not refcounted and you're trying to avoid a structure copy. > But what you're doing with _steal_structure() here means the GstCaps in the > array are going to be useless and can't be used again (might they ever be > used again, e.g. when seeking back?). So why jump through GstCaps hoops in > the first place? You might just as well put the GstStructure into the array > and then steal it and set the value in the array to NULL. That's basically > the same thing you're doing now just without the extra caps objects, no? I did try something like that but was not able to get it working. I think the issue was that the GstStructure pointers were being freed by GPtrArray that had already been passed downstream in a GstMeta. I will have another go, just using a simple GstStructure** array.
Created attachment 308395 [details] [review] qtdemux: add support for ISOBMFF Common Encryption Addresses all the review comments apart from: - protection_scheme_event_queue: embed in structure (GQueue foo_queue) as the intention of that comment is unclear to me.
> Addresses all the review comments apart from: > - protection_scheme_event_queue: embed in structure (GQueue foo_queue) > as the intention of that comment is unclear to me. Instead of GQueue *queue = g_queue_new (); g_queue_foo (queue, ...) g_queue_free (queue); you can do GQueue queue; g_queue_init (&queue); g_queue_foo (&queue, ...) g_queue_clear (&queue); But it's not super-important. So that leaves the crypt_info thing and what to send in the meta (senc box vs. parsed info).
(In reply to Tim-Philipp Müller from comment #139) [...] > > - general question: the CFF spec defines things such that there's an 'senc' > box > and the sample aux info pointed to by 'saiz' and 'saio' is always inside > the > 'senc'. Isn't this always the case? If yes, we could just parse the 'senc' > box into a GstBuffer and attach the whole senc box to each outgoing sample, > plus the sample index for that sample. The decryptor can then parse out all > the info it needs (IV, sub sample details etc.) itself. Would save a whole > lot of code in qtdemux, and I doubt parsing this in the decryptor would be > much more code than handling all the various bits from the structure. > Am I missing something or am I confused, or would this be possible? It's not always the case that the sample aux info is inside a 'senc' box: use of the senc box is optional in both the CENC spec (2nd edition) and the DVB DASH profile. [...]
(In reply to Tim-Philipp Müller from comment #146) > GQueue queue; > g_queue_init (&queue); > g_queue_foo (&queue, ...) > g_queue_clear (&queue); Or, GQueue queue = G_QUEUE_INIT; g_queue_foo... > > But it's not super-important. >
Created attachment 308405 [details] [review] qtdemux: add support for ISOBMFF Common Encryption Another update, replacing GQueue* with GQueue and removing some more if() nesting.
> Or, > > GQueue queue = G_QUEUE_INIT; > g_queue_foo... Thanks, but the context here was GQueue embedded in a struct, so I skipped that one ;)
(In reply to Tim-Philipp Müller from comment #146) > So that leaves the crypt_info thing and what to send in the meta (senc box > vs. parsed info). The crypt_info thing has been changed, so that it no longer uses GstCaps to store the GstStructure. The senc box is optional in ISO DASH, DVB-DASH and DASH-IF. Using the 'saio' and 'saiz' boxes is the more robust method to find the encryption information. Therefore I don't think we should mandate that the code uses the 'senc' box to find the encryption information, regardless of where the parsing occurs.
Comment on attachment 304512 [details] [review] qtdemux: support for cenc auxiliary info parsing outside of moof box This does not apply any longer.
Yes, I'll rework it (hopefully) before or during the hackfest.
(In reply to A Ashley from comment #136) > (In reply to Philippe Normand from comment #128) > > (In reply to Philippe Normand from comment #127) > > > Created attachment 304512 [details] [review] [review] [review] [review] > > > qtdemux: support for cenc auxiliary info parsing outside of moof box > > > > > > When the cenc aux info index is out of moof boundaries, keep track of > > > it and parse the beginning of the mdat box, before the first sample. > > > > I'm not totally sure this is the right way to do but it worked for the > > sample I have here. Feedback welcome :) > > I think the DASH CENC specification allows the saio box to point to anywhere > in the fragment for the CommonEncryptionSampleAuxiliaryInformation. It might > not be right to assume that it is always before the first sample. > Hum ok. Would you by any chance have some sample files where the saio box wouldn't be before the first sample? For this patch I've been using this: https://github.com/youtube/js_mse_eme/blob/master/media/oops_cenc-20121114-145-no-clear-start.mp4
Created attachment 309139 [details] [review] qtdemux: support for cenc auxiliary info parsing outside of moof box Updated patch. If I understood Alex correctly, in the case of DVB-DASH we should still error out if the cenc info is outside of the moof box but I have no idea how to know when we're processing such type of media :(
I don't think we need to be checking that a file that claims conformance to DVB-DASH is actually conformant. Your patch provides qtdemux with support for other CENC profiles. As long as it doesn't break DVB-DASH support, I think your patch is a useful addition.
Created attachment 309451 [details] [review] qtdemux: fix offset calculation when parsing CENC aux info Commit 7d7e54ce6863ff53e188d0276d2651b65082ffdb added support for DASH common encryption, however commit bb336840c0b0b02fa18dc4437ce0ded3d9142801 that went onto master shortly before the CENC commit caused the calculation of the CENC aux info offset to be incorrect. The base_offset was being added if present, but if the base_offset is relative to the start of the moof, the offset was being added twice. The correct approach is to calculate the offset from the start of the moof and use that offset when parsing the CENC aux info.
Ok, so I didn't manage to get that last patch in for 1.6.0 (sorry!), but it's definitely 1.6.1 material. I'd like to close this bug however, and make a new bug for that last remaining patch. Could you attach the patch in the new bug again please Philippe? Thanks!