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 766177 - qtdemux: Critical errors reported playing mp4 file with only xmp tags
qtdemux: Critical errors reported playing mp4 file with only xmp tags
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other other
: Normal normal
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 779245 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-05-09 14:25 UTC by David Plowman
Modified: 2017-04-19 12:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Contents of the uuid atom from the offending mp4 file (4.44 KB, application/octet-stream)
2016-05-09 14:25 UTC, David Plowman
  Details
Possible patch (1.50 KB, patch)
2016-05-09 15:01 UTC, David Plowman
rejected Details | Review
Log file with some debug (3.48 KB, text/plain)
2016-05-10 08:06 UTC, David Plowman
  Details
qtdemux: rework taglist handling (13.43 KB, patch)
2016-05-31 16:45 UTC, Thiago Sousa Santos
committed Details | Review
Proprosed addition to Thiago's patch (1.14 KB, patch)
2017-01-13 12:57 UTC, David Warman
committed Details | Review
Debug trace from non-writable taglist case (15.54 KB, text/plain)
2017-01-13 13:01 UTC, David Warman
  Details

Description David Plowman 2016-05-09 14:25:18 UTC
Created attachment 327522 [details]
Contents of the uuid atom from the offending mp4 file

Playing a particular mp4 file that seems to contain only xmp tags gives rise to:

GStreamer-CRITICAL **: gst_tag_list_get_scope: assertion 'GST_IS_TAG_LIST (list)' failed

(and may crash if crtical warnings are treated as fatal.)

Unfortunately the file in question nearly 500MB and my usual tools for chopping it down also make the problem go away. However, I've extracted and attached the uuid atom which appears to be where the problematic tags reside.

The call stack from the crash starts

  • #0 raise
    at ../sysdeps/unix/sysv/linux/pt-raise.c line 36
  • #1 _g_log_abort
    at ../../glib-2.44.1/glib/gmessages.c line 315
  • #2 g_logv
    at ../../glib-2.44.1/glib/gmessages.c line 1041
  • #3 g_log
    at ../../glib-2.44.1/glib/gmessages.c line 1079
  • #4 g_return_if_fail_warning
    at ../../glib-2.44.1/glib/gmessages.c line 1088
  • #5 gst_tag_list_get_scope
    at ../../clone/gst/gsttaglist.c line 837
  • #6 qtdemux_handle_xmp_taglist
    at ../../../clone/gst/isomp4/qtdemux.c line 2376
  • #7 gst_qtdemux_process_adapter
    at ../../../clone/gst/isomp4/qtdemux.c line 5782
  • #8 gst_pad_chain_data_unchecked
    at ../../clone/gst/gstpad.c line 4100

I'm actually running 1.6.1 on an embedded platform, however, the code in 1.8.1 looks the same.
Comment 1 David Plowman 2016-05-09 14:30:01 UTC
I believe the problem lies in qtdemux_handle_xmp_taglist where having qtdemux->tag_list NULL appears to cause this problem. Presumably all this means is that the first tags we've encountered are xmp tags?

Anyway, I'm using the following patch locally and it seems to be working fine:

diff --git a/gst/isomp4/qtdemux.c b/gst/isomp4/qtdemux.c
index e9c7a07..236f853 100644
--- a/gst/isomp4/qtdemux.c
+++ b/gst/isomp4/qtdemux.c
@@ -2373,7 +2373,7 @@ qtdemux_handle_xmp_taglist (GstQTDemux * qtdemux, GstTagList * taglist,
 {
   /* Strip out bogus fields */
   if (xmptaglist) {
-    if (gst_tag_list_get_scope (taglist) == GST_TAG_SCOPE_GLOBAL) {
+    if (taglist && gst_tag_list_get_scope (taglist) == GST_TAG_SCOPE_GLOBAL) {
       gst_tag_list_remove_tag (xmptaglist, GST_TAG_VIDEO_CODEC);
       gst_tag_list_remove_tag (xmptaglist, GST_TAG_AUDIO_CODEC);
     } else {
@@ -2383,8 +2383,12 @@ qtdemux_handle_xmp_taglist (GstQTDemux * qtdemux, GstTagList * taglist,
     GST_DEBUG_OBJECT (qtdemux, "Found XMP tags %" GST_PTR_FORMAT, xmptaglist);
 
     /* prioritize native tags using _KEEP mode */
-    gst_tag_list_insert (taglist, xmptaglist, GST_TAG_MERGE_KEEP);
-    gst_tag_list_unref (xmptaglist);
+    if (taglist) {
+      gst_tag_list_insert (taglist, xmptaglist, GST_TAG_MERGE_KEEP);
+      gst_tag_list_unref (xmptaglist);
+    } else {
+      qtdemux->tag_list = xmptaglist;
+    }
   }
 }

Does that seem reasonable? Of course, I'm not totally sure what I can infer about the scope of a tag list that is empty...
Comment 2 Thiago Sousa Santos 2016-05-09 14:34:29 UTC
Perhaps you could get the first few MBs of the file with something like:

head --bytes=2M <file> and check if it still happens? You can try different sizes. But it also might be that the file has metadata at the end.

Then you can attach that file somewhere and send us a link or if it is very small, attach it here.
Comment 3 Thiago Sousa Santos 2016-05-09 14:35:34 UTC
Can you attach your patch in git format-patch mode? Then we can use the proper tools for merging and reviewing. Thanks!
Comment 4 David Plowman 2016-05-09 15:01:39 UTC
Created attachment 327525 [details] [review]
Possible patch
Comment 5 David Plowman 2016-05-09 15:04:26 UTC
Unfortunately the metadata is near the end of the file which makes things a little difficult. Nonetheless, I've attached the patch that I'm using. Thanks for the quick response!!
Comment 6 Thiago Sousa Santos 2016-05-10 02:55:10 UTC
I think the issue is that the taglist is not being created. If you look at the code, at many places it is created, somehow we managed to fail creating it for this file. Perhaps a GST_DEBUG=qtdemux:6 would help us understand why it fails.

Anything else particularly special about this file?
Comment 7 David Plowman 2016-05-10 08:06:54 UTC
Created attachment 327562 [details]
Log file with some debug

Here's a log file with some debug in it. It seems to be calling qtdemux_parse_uuid before it calls qtdemux_parse_tree, which looks to be where it normally expects to make the tag list (the final line of the log where it enters qtdemux_parse_tree was added by me).
Comment 8 Tim-Philipp Müller 2016-05-27 18:17:01 UTC
We should write a little tool that takes an mp4/qt file, and basically zeroes out the mdat atom (or everything but the first N MB of the mdat atom) and zips it up, plus the reverse unpack :)
Comment 9 Thiago Sousa Santos 2016-05-31 16:45:47 UTC
Created attachment 328832 [details] [review]
qtdemux: rework taglist handling

Can you check if this patch fixes the issue for you? This way it won't ignore the xmp metadata just because it hasn't created a taglist yet.
Comment 10 Jan Schmidt 2016-11-16 04:50:43 UTC
David? Did you get a chance to test Thiago's patch against your file?
Comment 11 David Plowman 2016-11-17 16:44:13 UTC
I must have an old version of the file because I had to merge the patch by hand :(
But yes, the file then played fine :)

Thanks Thiago
Comment 12 David Warman 2017-01-13 12:56:20 UTC
Thiago's patch is a much appreciated simplification of the taglist handling, but I have found one problem that it does not yet solve.

This is that gst_demux_push_tags() takes a second reference while pushing the new tag event to send global tags (qtdemux.c:958), so tag_list becomes non-writable.  A subsequent call to qtdemux_handle_xmp_taglist() then fails with
"gst_tag_list_insert: assertion 'gst_tag_list_is_writable (into)' failed"

It seems inefficient to make qtdemux->tag_list writable immediately after the 2nd reference is taken (although perhaps easier to think about), so I have settled for doing so in qtdemux_parse_uuid(), just before the call to qtdemux_handle_xmp_taglist().  This fixes my immediate problem, but am I still wondering if it would be better to keep qtdemux->tag_list always writable?

I'll attach a qtdemux debug trace for the failing condition and a further patch file.
Comment 13 David Warman 2017-01-13 12:57:12 UTC
Created attachment 343429 [details] [review]
Proprosed addition to Thiago's patch
Comment 14 David Warman 2017-01-13 13:01:22 UTC
Created attachment 343430 [details]
Debug trace from non-writable taglist case


I haven't found an easy way for anyone else to reproduce this, but it fails readily for me, so I can test alternative solutions.
Comment 15 Thiago Sousa Santos 2017-01-18 02:26:40 UTC
commit 642331fd7f5b6236049fbd3b56f2f7f8a71ba174
Author: David Warman <dwarman@manglebit.org>
Date:   Fri Jan 13 12:27:40 2017 +0000

    qtdemux: avoid XMP tag parsing fatal error.
    
    qtdemux_handle_xmp_taglist() requires a writable taglist,
    but qtdemux->tag_list can become non-writable, specifically
    after sending global tags (qtdemux.c:958), which adds a
    second reference.  Ensure the list is made writable before
    calling (make_writable will copy the list if necessary).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766177

commit 5bb7ca8a623426cde67a6cf54526bcc6a3fd3395
Author: Thiago Santos <thiagossantos@gmail.com>
Date:   Tue May 31 13:17:45 2016 -0300

    qtdemux: rework taglist handling
    
    Keep taglist around during element existance to avoid having to
    create it at different places before usage. Makes code simpler to handle.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766177
Comment 16 Sebastian Dröge (slomo) 2017-01-18 09:45:56 UTC
Should we consider this for 1.10 too?
Comment 17 Tim-Philipp Müller 2017-04-19 12:34:07 UTC
*** Bug 779245 has been marked as a duplicate of this bug. ***