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 755614 - qtdemux: support for cenc auxiliary info parsing outside of moof box
qtdemux: support for cenc auxiliary info parsing outside of moof box
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 1.6.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-25 08:24 UTC by Tim-Philipp Müller
Modified: 2016-02-23 08:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: support for cenc auxiliary info parsing outside of moof box (5.31 KB, patch)
2015-09-25 08:45 UTC, Philippe Normand
none Details | Review
qtdemux: support for cenc auxiliary info parsing outside of moof box (6.18 KB, patch)
2015-10-14 09:32 UTC, Philippe Normand
none Details | Review
qtdemux: support for cenc auxiliary info parsing outside of moof box (6.04 KB, patch)
2015-11-04 08:17 UTC, Philippe Normand
committed Details | Review

Description Tim-Philipp Müller 2015-09-25 08:24:53 UTC
+++ This bug was initially created as a clone of Bug #705991 +++

New bug for a remaining issue: the cenc auxiliary info might be outside of the moof box, we need to parse that in that case as well. Philippe has a patch in the other bug #705991 .
Comment 1 Philippe Normand 2015-09-25 08:45:09 UTC
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.
Comment 2 Anton Obzhirov 2015-10-13 16:11:58 UTC
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.
Comment 3 Philippe Normand 2015-10-13 18:51:21 UTC
Right, good catch! I actually fixed that in my branch 2 weeks ago but forgot to update the patch here :)
Comment 4 Philippe Normand 2015-10-14 09:32:05 UTC
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.
Comment 5 Anton Obzhirov 2015-10-14 10:52:57 UTC
Tried your latest parch and can confirm that it works and there is no memory corruption. :)
Comment 6 Tim-Philipp Müller 2015-11-03 19:50:33 UTC
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.)
Comment 7 Philippe Normand 2015-11-04 08:17:35 UTC
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.
Comment 8 Tim-Philipp Müller 2015-11-05 00:21:53 UTC
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
Comment 9 Tim-Philipp Müller 2016-02-22 22:50:36 UTC
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
Comment 10 Tim-Philipp Müller 2016-02-22 22:51:47 UTC
And in 1.6.
Comment 12 Tim-Philipp Müller 2016-02-23 07:30:25 UTC
Yes.
Comment 13 Sebastian Dröge (slomo) 2016-02-23 08:52:53 UTC
Is this a regression?
Comment 14 Sebastian Dröge (slomo) 2016-02-23 08:56:55 UTC
Doesn't crash here for me, can we get a new bug for that?
Comment 15 Sebastian Dröge (slomo) 2016-02-23 08:58:41 UTC
Oh it crashes when played as a local file but not via HTTP.