GNOME Bugzilla – Bug 753614
qtdemux: PIFF box parsing support
Last modified: 2016-04-02 17:03:43 UTC
The PIFF box stores informations necessary for content decryption. The demuxer could parse this box and and update its crypto_info accordingly.
Created attachment 309251 [details] [review] pach
Review of attachment 309251 [details] [review]: I'll try a different approach for this by building a self-contained parser (like the sidx parser in dash/) in the smooth-streaming plugin. I'll also add parsing support for the MS other 2 MS-specific custom boxes (tfxd and tfrf).
Comment on attachment 309251 [details] [review] pach Turns out this belongs to qtdemux and not in mssdemux because qtdemux is the one splitting the fragment in samples (with an encryption box attached as buffer metadata).
Review of attachment 309251 [details] [review]: Some minor comments below. Are there any public samples on this protection scheme? ::: gst/isomp4/qtdemux.c @@ +2414,3 @@ + + structure = gst_caps_get_structure (stream->caps, 0); + g_assert (gst_structure_has_name (structure, "application/x-cenc")); This doesn't look like a programming error. A malicious file could be crafted to have a piff box on a regular non-ecnrypted file. I'd just use a warning here and abort parsing the piff. @@ +2511,3 @@ + GstBuffer *box; + + properties = qtdemux_get_cenc_sample_properties (qtdemux, stream, i); Is sample here the number of samples on the stream or is this a different meaning for sample? Creating 1 structure per sample seems heavy. Perhaps use an array of custom structs? @@ +2539,3 @@ }; + + static const guint8 piff_uuid[] = { This not really a piff box, but a piff_sample_encryption_uuid. piff seems to be the ftyp of the file.
Review of attachment 309251 [details] [review]: Thanks for the review, I'll upload a rebased patch. ::: gst/isomp4/qtdemux.c @@ +2511,3 @@ + GstBuffer *box; + + } sample_count (this patch needs a rebase, in git master the name is cenc_aux_sample_count) is the number of samples in the track fragment. Not sure to understand your question about using one structure per sample. Something similar is done in qtdemux_parse_cenc_aux_info().
Created attachment 316590 [details] [review] qtdemux: PIFF box detection and minimal parsing support The PIFF data is stored in a custom UUID box which is parsed and the crypto_info of the element is updated accordingly. This allows downstream decryptors to process and decrypt the protected content.
Comment on attachment 316590 [details] [review] qtdemux: PIFF box detection and minimal parsing support After revising this patch I think the full box attachment to each sample properties structure can be avoided.
Created attachment 320690 [details] [review] qtdemux: PIFF box detection and parsing support The PIFF data is stored in a custom UUID box which is parsed and the crypto_info of the element is updated accordingly. This allows downstream decryptors to process and decrypt the protected content.
Created attachment 321634 [details] [review] qtdemux: PIFF box detection and parsing support The PIFF data is stored in a custom UUID box which is parsed and the crypto_info of the element is updated accordingly. This allows downstream decryptors to process and decrypt the protected content. This new iteration fixes two buffer leaks.
Any reason not to commit this now?
If you ask me, then my answer is no :)
Pushed with some minor cosmetic changes, thanks! commit fd7964e7462544a0c120ebf1d4c8e4b0174a1518 Author: Philippe Normand <philn@igalia.com> Date: Fri Jul 10 09:44:15 2015 +0200 qtdemux: PIFF box detection and parsing support The PIFF data is stored in a custom UUID box which is parsed and the crypto_info of the element is updated accordingly. This allows downstream decryptors to process and decrypt the protected content. https://bugzilla.gnome.org/show_bug.cgi?id=753614
Created attachment 325228 [details] [review] cosmetic changes (at least 99% of them, I added a newline after the variable declaration block somewhere and also didn't let the GNode * declarations wrap into a second line.)