[Bug 705991] Adding support for DASH common encryption to qtdemux and dashdemux

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=705991

A Ashley <bugzilla at ashley-family.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #301698|none                        |needs-work
             status|                            |

--- Comment #126 from A Ashley <bugzilla at ashley-family.net> ---
Review of attachment 301698:
 --> (https://bugzilla.gnome.org/review?bug=705991&attachment=301698)

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;"

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.