GNOME Bugzilla – Bug 721384
h264parse: clears keyframe flags when passing through byte-stream AUs (regression)
Last modified: 2014-01-22 12:08:02 UTC
I observed that in 1.2 my code doesn't detect a key frame in an asf file. Video sample : http://www.axis.com/movies/products/q6035e_original_quality.asf Part of the code : In a probe buffer callback : if (GST_BUFFER_FLAG_IS_SET(buffer,GST_BUFFER_FLAG_DELTA_UNIT) != TRUE) g_warning("You will never see this line"); Pipeline : gst-launch-1.0 filesrc location=axis.asf ! asfdemux ! h264parse ! fakesink Additional informations : Works well with matroskademux Worked in 1.0
There doesn't seem to be anything wrong with asfdemux here (this is using 1.2.2 as in debian sid): $ gst-launch-1.0 filesrc location= /home/tpm/samples//misc/721384-axis-q6035e_original_quality.asf ! asfdemux ! video/x-h264 ! fakesink silent=false -v | grep chain | grep -v delta-unit | sed -e 's/.*pts/pts/' -e 's/, duration.*//' pts: 0:00:00.000000000 pts: 0:00:02.066000000 pts: 0:00:04.132000000 pts: 0:00:06.199000000 ... pts: 0:00:26.865000000 pts: 0:00:28.932000000 pts: 0:00:30.998000000 Seems to be a bug in h264parse then: $ gst-launch-1.0 filesrc location=axis.asf ! asfdemux ! h264parse ! fakesink silent=false -v | grep chain | sed -e 's/.*pts/pts/' -e 's/, duration.*flags/ flags/' -e 's/).*//' | head -n 5 pts: 0:00:00.000000000 flags: 00002000 delta-unit pts: 0:00:00.032000000 flags: 00002000 delta-unit pts: 0:00:00.066000000 flags: 00002000 delta-unit pts: 0:00:00.099000000 flags: 00002000 delta-unit pts: 0:00:00.132000000 flags: 00002000 delta-unit
Caused by a fix to #705452 commit 43dcebe2a024547d04cd4d7fac41cc88c6d13dd6 Author: Matthieu Bouron <matthieu.bouron@collabora.com> Date: Tue Aug 20 11:59:34 2013 +0100 h264parse: do not set CAPS and passthrough mode if SPS/PPS have not been parsed https://bugzilla.gnome.org/show_bug.cgi?id=705452
I'm not sure I understand the logics of h264parse, but this commit prevents it from being set on passthrough mode and while not on passthrough it will process all nals and get: gst_h264_parser_parse_slice_hdr: couldn't find associated picture parameter set with id: 0 This goes on repeatedly and it will remove set the delta flag on all buffers, overwriting whatever the demuxer might have set previously. Should the regression commit be fixed so that it goes into passthrough somehow or do we want to make the parse_slice_hdr succeed?
Well, both I would say :) But it should only go in passthrough mode if the caps are complete and the input is properly parsed.
When trying to parse the only PPS in the stream, the h264parser lib fails on the last data to parse: READ_SE_ALLOWED (&nr, pps->second_chroma_qp_index_offset, -12, 12); It seems there is only one bit remaining when it gets to this point and this bit is 0. AFAIK, 0 is not an allowed value for the 'exponential Golomb coding' that is used in NAL units, so that fails. IIRC a value of 0 should be represented with a single bit 1. So the nal_reader_get_ue will return FALSE and the PPS will be discarded. Without parsing this PPS it makes h264parse unable to set keyframe information into outgoing buffers.
See also bug #722240, which adds the same logic as bug #705452 to h265parse.
Created attachment 266802 [details] [review] h264parser: be forgiving when reading second_chroma_qp_index_offset This makes the file play again, but it is a hack. I see 2 options here: 1) this hack (that is very specific to this particular error) to ignore errors when reading the second_chroma_qp_index_offset, which is the last of the fields on the PPS 2) Make the UE/SE read accept 0 as 0 and valid, making a single bit 0 mean the same as a single bit 1. This has a broader range, but also can lead to more side effects I don't know h264 spec enough to decide :)
Maybe check how libav handles this situation. I think in general this patch should be fine, especially if we never use the value of it anywhere... and decoders handle it just fine.
Considering this is the NAL to parse: 68 ce 3c 80 00 00 00 00 01 gstreamer detects it as having a size of 6 bytes, using the final 00 00 01 as the start code for the next NAL and stripping it. 68 ce 3c 80 00 00 libav (that plays this file correctly) strips one more byte than gstreamer: 68 ce 3c 80 00 so the final element that gstreamer complains about and errors out doesn't exist for libav and it remains happy enough to play the file. libav has this: #define STARTCODE_TEST \ if (i + 2 < length && src[i + 1] == 0 && src[i + 2] <= 3) { \ if (src[i + 2] != 3) { \ /* startcode, so we must be past the end */ \ length = i; \ } \ break; \ } which will also consider 00 00 00/01/02 as a start code. I guess this is why it doesn't use that last byte. (could be a bug)
Created attachment 266932 [details] [review] h264parser: remove trailling 0x00 bytes as the spec doesn't allow them So after talking to libav developers and reading the relevant parts of the spec I learned that the last byte of a NAL shall not be 0x00. In byte stream format, there might be padding, so we need to strip any 0x00 from the end of the NAL
Review of attachment 266932 [details] [review]: Makes sense
Pushed to 1.2 commit 040e81564106dae5b451f06999b664301f3b41d9 Author: Thiago Santos <ts.santos@sisa.samsung.com> Date: Mon Jan 20 17:24:54 2014 -0300 h264parser: remove trailling 0x00 bytes as the spec doesn't allow them The spec states that the last byte of a NAL 'shall not' be 0x00 and it is allowed for byte-stream format to add padding 0x00 for alignment. So our parser should strip any trailling 0x00. https://bugzilla.gnome.org/show_bug.cgi?id=721384 and master: commit 4f0fc9a16fe0e340c48107cf62d17f75302d30d0