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

[GStreamer (GNOME Bugzilla)]

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

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

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

--- Comment #139 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 307010
  --> https://bugzilla.gnome.org/attachment.cgi?id=307010
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.)

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