After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753614 - qtdemux: PIFF box parsing support
qtdemux: PIFF box parsing support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-14 10:24 UTC by Philippe Normand
Modified: 2016-04-02 17:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pach (8.43 KB, patch)
2015-08-14 10:28 UTC, Philippe Normand
none Details | Review
qtdemux: PIFF box detection and minimal parsing support (8.60 KB, patch)
2015-12-01 12:29 UTC, Philippe Normand
none Details | Review
qtdemux: PIFF box detection and parsing support (9.93 KB, patch)
2016-02-09 09:37 UTC, Philippe Normand
none Details | Review
qtdemux: PIFF box detection and parsing support (9.29 KB, patch)
2016-02-19 08:34 UTC, Philippe Normand
committed Details | Review
cosmetic changes (3.68 KB, patch)
2016-04-02 17:03 UTC, Tim-Philipp Müller
committed Details | Review

Description Philippe Normand 2015-08-14 10:24:22 UTC
The PIFF box stores informations necessary for content decryption. The demuxer could parse this box and and update its crypto_info accordingly.
Comment 1 Philippe Normand 2015-08-14 10:28:09 UTC
Created attachment 309251 [details] [review]
pach
Comment 2 Philippe Normand 2015-09-01 13:21:38 UTC
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 3 Philippe Normand 2015-09-15 07:20:27 UTC
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).
Comment 4 Thiago Sousa Santos 2015-11-18 22:48:33 UTC
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.
Comment 5 Philippe Normand 2015-12-01 12:26:47 UTC
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().
Comment 6 Philippe Normand 2015-12-01 12:29:35 UTC
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 7 Philippe Normand 2016-02-02 16:57:39 UTC
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.
Comment 8 Philippe Normand 2016-02-09 09:37:35 UTC
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.
Comment 9 Philippe Normand 2016-02-19 08:34:39 UTC
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.
Comment 10 Jan Schmidt 2016-04-01 15:12:30 UTC
Any reason not to commit this now?
Comment 11 Philippe Normand 2016-04-02 11:24:41 UTC
If you ask me, then my answer is no :)
Comment 12 Tim-Philipp Müller 2016-04-02 17:02:02 UTC
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
Comment 13 Tim-Philipp Müller 2016-04-02 17:03:43 UTC
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.)