Created attachment 312117 [details] [review]
qtdemux: support for cenc auxiliary info parsing outside of moof box

When the cenc aux info index is out of moof boundaries, keep track of
it and parse the beginning of the mdat box, before the first sample.

Review of attachment 312117 [details] [review]:

Hi Philippe,

We tested the patch using our GStreamer backend (https://github.com/Samsung/ChromiumGStreamerBackend/)
using Google EME tests. So far seems to work fine.

BR, Anton

::: gst/isomp4/qtdemux.c
@@ +3387,3 @@
       if (!saio_node) {
         GST_ERROR_OBJECT (qtdemux, "saiz box without a corresponding saio box");
+        g_free (qtdemux->info_sizes);

It can probably lead to double deletion in gst_qtdemux_dispose.

Right, good catch! I actually fixed that in my branch 2 weeks ago but forgot to update the patch here :)

Created attachment 313249 [details] [review]
qtdemux: support for cenc auxiliary info parsing outside of moof box

When the cenc aux info index is out of moof boundaries, keep track of
it and parse the beginning of the mdat box, before the first sample.

Tried your latest parch and can confirm that it works and there is no memory corruption. :)

Comment on attachment 313249 [details] [review]
qtdemux: support for cenc auxiliary info parsing outside of moof box

Looks fine, but didn't test myself (I saw that others did, of course). Would appreciate a file or stream for my collection though.

Just a few small nitpicks:

>+  qtdemux->cenc_aux_info_offset = 0;
>+  qtdemux->info_sizes = NULL;
>+  qtdemux->sample_count = 0;

Can we make this cenc_aux_info_sizes and cenc_aux_sample_count or somesuch to make it clear this is cenc-related? (esp. for the sample_count member)

 
>+  if (qtdemux->info_sizes) {
>+    g_free (qtdemux->info_sizes);
>+    qtdemux->info_sizes = NULL;
>+  }

g_free() is NULL-safe, don't need the if (same for the dozen other places below).

>+      if (G_UNLIKELY (qtdemux->info_sizes == NULL)) {
>         GST_ERROR_OBJECT (qtdemux, "failed to parse saiz box");
>         goto fail;
>       }

Let's drop the G_UNLIKELY, this is not a performance-critical code path I think? (I realise the original code has it, but if we are changing it we might as well drop it; imho we use this way too often everywhere and the only thing is does is makes code harder to read; this is a pet peeve of mine, feel free to ignore it.)

Created attachment 314789 [details] [review]
qtdemux: support for cenc auxiliary info parsing outside of moof box

As for the sample file:
https://github.com/youtube/js_mse_eme/blob/master/media/oops_cenc-20121114-145-no-clear-start.mp4

qtdemux: support for cenc auxiliary info parsing outside of moof box

When the cenc aux info index is out of moof boundaries, keep track of
it and parse the beginning of the mdat box, before the first sample.

Thanks!


commit 9f0c22e891cb358d847100dd9b560b086268e7c8
Author: Philippe Normand <philn@igalia.com>
Date:   Wed Aug 12 13:35:40 2015 +0200

    qtdemux: support for cenc auxiliary info parsing outside of moof box
    
    When the cenc aux info index is out of moof boundaries, keep track of
    it and parse the beginning of the mdat box, before the first sample.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=755614

I get crashes with this file in master fwiw:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffef3b1700 (LWP 26337)]
0x00007fffef80109c in gst_qtdemux_decorate_and_push_buffer (qtdemux=0x7ffff011a6e0 [GstQTDemux], stream=0x7fffe8005600, buf=0x7fffe8008060, dts=0, pts=41711111, duration=41711111, 
    keyframe=1, position=0, byte_position=6645) at qtdemux.c:4970
4970	    index = stream->sample_index - (stream->n_samples - info->crypto_info->len);
(gdb) print info->crypto_info

And in 1.6.

Which file? https://github.com/youtube/js_mse_eme/blob/master/media/oops_cenc-20121114-145-no-clear-start.mp4 ?

Yes.

Is this a regression?

Doesn't crash here for me, can we get a new bug for that?

Oh it crashes when played as a local file but not via HTTP.