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 796647 - qtdemux: PIFF track encryption box support
qtdemux: PIFF track encryption box support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-06-22 11:01 UTC by Philippe Normand
Modified: 2018-09-25 09:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (6.20 KB, patch)
2018-06-22 11:07 UTC, Philippe Normand
none Details | Review
updated patch (10.90 KB, patch)
2018-08-17 13:01 UTC, Philippe Normand
none Details | Review
updated patch (10.92 KB, patch)
2018-09-15 18:24 UTC, Philippe Normand
committed Details | Review

Description Philippe Normand 2018-06-22 11:01:15 UTC
qtdemux can already parse PIFF sample encryption boxes but in addition to that it should also parse the PIFF track encryption box when a (standard) cenc track encryption box wasn't found.
Comment 1 Philippe Normand 2018-06-22 11:07:11 UTC
Created attachment 372757 [details] [review]
patch
Comment 2 Thibault Saunier 2018-07-09 19:58:06 UTC
Review of attachment 372757 [details] [review]:

Looks generally correct.

::: gst/isomp4/qtdemux.c
@@ +10050,3 @@
+      }
+      tenc_data = (const guint8 *) tenc->data + 12;
+      isEncrypted = QT_UINT24 (tenc_data);

Should have been is_encrypted, not your fault though.

@@ +10076,3 @@
+        return FALSE;
+      }
+      tenc_data = (const guint8 *) tenc->data + 16 + 12;

Are we garanteed we have enough data here? I do not have the impression

@@ +10084,3 @@
+        isEncrypted = FALSE;
+      } else if (algorithm_id == 1) {
+        /* FIXME: maybe store this in properties? */

We probably want to add the info somewhere/in ss_info->default_properties?
Comment 3 Philippe Normand 2018-08-17 10:53:40 UTC
Review of attachment 372757 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +10076,3 @@
+        return FALSE;
+      }
+      };

I skip 16 bits representing the UUID, and then 12 (8 + 4) representing the version and flags fields. So I think we're good. I can add a test checking tenc_data is still a valid pointer though.

@@ +10084,3 @@
+        isEncrypted = FALSE;
+      } else if (algorithm_id == 1) {
+        GST_ERROR_OBJECT (qtdemux, "schi box does not contain tenc box, "

Yes, actually this code is copy-pasted in 2 places now, I'll refactor this and store the info in default_properties :)
Comment 4 Philippe Normand 2018-08-17 13:01:00 UTC
Created attachment 373379 [details] [review]
updated patch
Comment 5 Philippe Normand 2018-09-15 18:24:22 UTC
Created attachment 373667 [details] [review]
updated patch
Comment 6 Thibault Saunier 2018-09-24 19:04:18 UTC
Review of attachment 373667 [details] [review]:

Sounds good.
Comment 7 Philippe Normand 2018-09-25 09:27:33 UTC
Comment on attachment 373667 [details] [review]
updated patch

commit babf4210f035541a1406698e4992178928f75627 (HEAD -> master, origin/master, origin/HEAD)
Author: Philippe Normand <philn@igalia.com>
Date:   Fri Jun 22 12:05:17 2018 +0100

    qtdemux: PIFF track encryption box support
    
    The PIFF track encryption box is a UUID box containing the default encryption
    values that should be used for PIFF sample encryption.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=796647