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 650416 - [h264parse] Assertion failure: nal_size >=2
[h264parse] Assertion failure: nal_size >=2
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-05-17 18:15 UTC by bens
Modified: 2011-05-23 14:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
I tried GST_DEBUG=5 but had to stop it when the log file hit 7 GB. (205.28 KB, application/x-xz)
2011-05-17 18:15 UTC, bens
  Details
Backtrace with G_DEBUG=fatal_warnings (2.14 KB, text/plain)
2011-05-18 13:26 UTC, bens
  Details
Patch to fix the bug as suggested (1.25 KB, patch)
2011-05-18 22:07 UTC, bens
none Details | Review

Description bens 2011-05-17 18:15:14 UTC
Created attachment 187977 [details]
I tried GST_DEBUG=5 but had to stop it when the log file hit 7 GB.

I ran this pipeline, which until recently was failing due to #650323

./gst-jhbuild run gst-launch-0.10 filesrc location=clip.mts ! mpegtsdemux name=demux ! ac3parse ! queue ! matroskamux name=mux ! filesink location=clip.mkv . demux. ! h264parse ! queue ! mux.

The pipeline runs for a long time, as it should because clip.mts is a 3.5GB AVCHD file, and then fails with

** (gst-launch-0.10:10133): CRITICAL **: gst_h264_parse_process_nal: assertion `nal_size >= 2' failed

Where I come from, an assertion failure is always a bug.

Unfortunately, a 90 MB segment from the beginning of the file appears to be insufficient to trigger the failure, which does not occur until 2.9 GB have been written to clip.mkv.
Comment 1 Sebastian Dröge (slomo) 2011-05-18 12:08:44 UTC
Could you run this with G_DEBUG=fatal_warnings in gdb and get a backtrace of the warning? There are a few places in h264parse where gst_h264_parse_process_nal() might be called with nal_size < 2.
Comment 2 bens 2011-05-18 13:26:25 UTC
Created attachment 188025 [details]
Backtrace with G_DEBUG=fatal_warnings

Backtrace as requested.  As you can see, it looks like the path is from parse_check_valid_frame().
Comment 3 Sebastian Dröge (slomo) 2011-05-18 13:28:05 UTC
Thanks, looks like not only the length is wrong but also the data.
Comment 4 Mark Nauwelaerts 2011-05-18 13:43:24 UTC
Maybe the input data is basically wrong/corrupt.  I seem to recall having put that assertion on at least size 2 mainly based on a valid NAL at least having size 2 (afaik).  Whether or not the latter is actually correct (w.r.t. specs), it is likely not the best thing to be assert'ing on (and better some data sanity check/warning instead).
Comment 5 bens 2011-05-18 14:28:28 UTC
(In reply to comment #4)
> it is likely not the best thing to be assert'ing on (and better some data
> sanity check/warning instead).

That sounds like a great idea.  The input file here is straight off an AVCHD camera, so it's something that gstreamer should probably handle gracefully.  It looks like the assertion happens right at the end of the file, so perhaps the camera truncated its output at a non-spec-compliant location.  As it stands, the assertion prevents me from remuxing the file into a valid output, because the pipeline gets killed before matroskamux has a chance to finalize the .mkv file.
Comment 6 bens 2011-05-18 22:07:37 UTC
Created attachment 188084 [details] [review]
Patch to fix the bug as suggested

This patch fixes the bug, AFAICT.  Rather than remove the assertion in parse_process_nal, it checks it upstream in check_valid_frame.  This makes sense, I think, because it allows us to return FALSE from check_valid_frame if the frame is indeed invalid.  It almost looks like the code was designed to have this check, given the comment above parse_process_nal that "caller guarantees 2 bytes of nal payload".

Needs review.
Comment 7 Mark Nauwelaerts 2011-05-23 13:04:22 UTC
Made some modifications bearing in mind that NALs may or may not be aggregated into AUs.  Also, _process_nal is also called from a few other places, so better also make sure in _process_nal it is not being called with less than expected. 

commit fe2db9d46037b6daad2555051a40963e8d2f5e7d
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Mon May 23 14:41:27 2011 +0200

    h264parse: gracefully handle truncated input NAL units
    
    Rather than assert'ing in such case, emit warning if the length of a NAL unit
    is less than expected 2 and discard it.
    
    Based on patch by Benjamin M. Schwartz <bens@alum.mit.edu>
    
    Fixes #650416.
Comment 8 Mark Nauwelaerts 2011-05-23 14:00:20 UTC
Actually make that ...

commit cce4fd073cd0b53fbf849b5a445a0a56ca3f2c67
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Mon May 23 14:41:27 2011 +0200