From dc4c470d7518ceba69a501f149952cfa00052256 Mon Sep 17 00:00:00 2001 From: Nicolas Dufresne Date: Wed, 6 May 2020 22:28:34 -0400 Subject: [PATCH] h264parse: Properly handle 4 bytes start code This will stop stripping four bytes start code. This was fixed and broken again as it was causing the a timestamp shift. We now call gst_base_parse_set_ts_at_offset() with the offset of the first NAL to ensure that fixing a moderatly broken input stream won't affect the timestamps. We also fixes the unit test, removing a comment about the stripping behaviour not being correct. Part-of: --- gst-libs/gst/codecparsers/gsth264parser.c | 9 +++----- gst/videoparsers/gsth264parse.c | 25 ++++------------------ tests/check/elements/h264parse.c | 35 +++++++++++++------------------ 3 files changed, 21 insertions(+), 48 deletions(-) diff --git a/gst-libs/gst/codecparsers/gsth264parser.c b/gst-libs/gst/codecparsers/gsth264parser.c index 45a4aef..63d96fb 100644 --- a/gst-libs/gst/codecparsers/gsth264parser.c +++ b/gst-libs/gst/codecparsers/gsth264parser.c @@ -1452,6 +1452,9 @@ gst_h264_parser_identify_nalu_unchecked (GstH264NalParser * nalparser, nalu->sc_offset = offset + off1; + /* sc might have 2 or 3 0-bytes */ + if (nalu->sc_offset > 0 && data[nalu->sc_offset - 1] == 00) + nalu->sc_offset--; nalu->offset = offset + off1 + 3; nalu->data = (guint8 *) data; @@ -1465,12 +1468,6 @@ gst_h264_parser_identify_nalu_unchecked (GstH264NalParser * nalparser, nalu->valid = TRUE; - /* sc might have 2 or 3 0-bytes */ - if (nalu->sc_offset > 0 && data[nalu->sc_offset - 1] == 00 - && (nalu->type == GST_H264_NAL_SPS || nalu->type == GST_H264_NAL_PPS - || nalu->type == GST_H264_NAL_AU_DELIMITER)) - nalu->sc_offset--; - if (nalu->type == GST_H264_NAL_SEQ_END || nalu->type == GST_H264_NAL_STREAM_END) { GST_DEBUG ("end-of-seq or end-of-stream nal found"); diff --git a/gst/videoparsers/gsth264parse.c b/gst/videoparsers/gsth264parse.c index 6a2c3d3..8f12151 100644 --- a/gst/videoparsers/gsth264parse.c +++ b/gst/videoparsers/gsth264parse.c @@ -1315,7 +1315,6 @@ gst_h264_parse_handle_frame (GstBaseParse * parse, GstH264NalUnit nalu; GstH264ParserResult pres; gint framesize; - GstFlowReturn ret; if (G_UNLIKELY (GST_BUFFER_FLAG_IS_SET (frame->buffer, GST_BUFFER_FLAG_DISCONT))) { @@ -1380,22 +1379,6 @@ gst_h264_parse_handle_frame (GstBaseParse * parse, switch (pres) { case GST_H264_PARSER_OK: if (nalu.sc_offset > 0) { - int i; - gboolean is_filler_data = TRUE; - /* Handle filler data */ - for (i = 0; i < nalu.sc_offset; i++) { - if (data[i] != 0x00) { - is_filler_data = FALSE; - break; - } - } - if (is_filler_data) { - GST_DEBUG_OBJECT (parse, "Dropping filler data %d", nalu.sc_offset); - frame->flags |= GST_BASE_PARSE_FRAME_FLAG_DROP; - gst_buffer_unmap (buffer, &map); - ret = gst_base_parse_finish_frame (parse, frame, nalu.sc_offset); - goto drop; - } *skipsize = nalu.sc_offset; goto skip; } @@ -1410,6 +1393,10 @@ gst_h264_parse_handle_frame (GstBaseParse * parse, ("Error parsing H.264 stream"), ("Invalid H.264 stream")); goto invalid_stream; } + + /* Ensure we use the TS of the first NAL. This avoids broken timestamp in + * the case of a miss-placed filler byte. */ + gst_base_parse_set_ts_at_offset (parse, nalu.offset); } while (TRUE) { @@ -1576,10 +1563,6 @@ out: gst_buffer_unmap (buffer, &map); return GST_FLOW_OK; -drop: - GST_DEBUG_OBJECT (h264parse, "Dropped data"); - return ret; - skip: GST_DEBUG_OBJECT (h264parse, "skipping %d", *skipsize); /* If we are collecting access units, we need to preserve the initial diff --git a/tests/check/elements/h264parse.c b/tests/check/elements/h264parse.c index 2079e2f..92a4af0 100644 --- a/tests/check/elements/h264parse.c +++ b/tests/check/elements/h264parse.c @@ -180,11 +180,9 @@ verify_buffer (buffer_verify_data_s * vdata, GstBuffer * buffer) if (vdata->discard) { /* check separate header NALs */ gint i = vdata->buffer_counter; - guint ofs; gboolean aud; /* SEI with start code prefix with 2 0-bytes */ - ofs = i == 2; aud = i == 0; fail_unless (i <= 3); @@ -196,8 +194,8 @@ verify_buffer (buffer_verify_data_s * vdata, GstBuffer * buffer) } else { i -= 1; - fail_unless (gst_buffer_get_size (buffer) == ctx_headers[i].size - ofs); - fail_unless (gst_buffer_memcmp (buffer, 0, ctx_headers[i].data + ofs, + fail_unless (gst_buffer_get_size (buffer) == ctx_headers[i].size); + fail_unless (gst_buffer_memcmp (buffer, 0, ctx_headers[i].data, gst_buffer_get_size (buffer)) == 0); } } else { @@ -895,11 +893,6 @@ composite_buffer (GstClockTime pts, GstBufferFlags flags, gint count, ...) #define pull_and_check(h, data, pts, flags) \ pull_and_check_full (h, data, sizeof (data), pts, flags) -/* used to check NALs for which the parser removes the first 0x00 byte; - * this parser behavior is a bit broken, so we may remove that in the future */ -#define pull_and_check_skip1byte(h, data, pts, flags) \ - pull_and_check_full (h, data + 1, sizeof (data) - 1, pts, flags) - #define pull_and_drop(h) \ G_STMT_START { \ GstBuffer *b = gst_harness_pull (h); \ @@ -936,29 +929,29 @@ GST_START_TEST (test_parse_sliced_nal_nal) buf = wrap_buffer (h264_idr_slice_1, sizeof (h264_idr_slice_1), 100, 0); fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 1); - pull_and_check_skip1byte (h, h264_idr_slice_1, 100, 0); + pull_and_check (h, h264_idr_slice_1, 100, 0); buf = wrap_buffer (h264_idr_slice_2, sizeof (h264_idr_slice_2), 100, 0); fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 1); - pull_and_check_skip1byte (h, h264_idr_slice_2, -1, 0); + pull_and_check (h, h264_idr_slice_2, -1, 0); buf = wrap_buffer (h264_idr_slice_1, sizeof (h264_idr_slice_1), 200, 0); fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 2); pull_and_check (h, h264_aud, 200, 0); - pull_and_check_skip1byte (h, h264_idr_slice_1, 200, 0); + pull_and_check (h, h264_idr_slice_1, 200, 0); buf = wrap_buffer (h264_idr_slice_2, sizeof (h264_idr_slice_2), 200, 0); fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 1); - pull_and_check_skip1byte (h, h264_idr_slice_2, -1, 0); + pull_and_check (h, h264_idr_slice_2, -1, 0); buf = wrap_buffer (h264_idr_slice_1, sizeof (h264_idr_slice_1), 250, 0); fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 2); pull_and_check (h, h264_aud, 250, 0); - pull_and_check_skip1byte (h, h264_idr_slice_1, 250, 0); + pull_and_check (h, h264_idr_slice_1, 250, 0); /* 1st slice starts a new AU, even though the previous one is incomplete. * DISCONT must also be propagated */ @@ -967,7 +960,7 @@ GST_START_TEST (test_parse_sliced_nal_nal) fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 2); pull_and_check (h, h264_aud, 400, 0); - pull_and_check_skip1byte (h, h264_idr_slice_1, 400, GST_BUFFER_FLAG_DISCONT); + pull_and_check (h, h264_idr_slice_1, 400, GST_BUFFER_FLAG_DISCONT); gst_harness_teardown (h); } @@ -1004,8 +997,8 @@ GST_START_TEST (test_parse_sliced_au_nal) /* 1st slice here doens't have a PTS * because it was present in the first header NAL */ - pull_and_check_skip1byte (h, h264_idr_slice_1, -1, 0); - pull_and_check_skip1byte (h, h264_idr_slice_2, -1, 0); + pull_and_check (h, h264_idr_slice_1, -1, 0); + pull_and_check (h, h264_idr_slice_2, -1, 0); /* new AU. we expect AUD to be inserted and 1st slice to have the same PTS */ buf = composite_buffer (200, 0, 2, @@ -1014,8 +1007,8 @@ GST_START_TEST (test_parse_sliced_au_nal) fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 3); pull_and_check (h, h264_aud, 200, 0); - pull_and_check_skip1byte (h, h264_idr_slice_1, 200, 0); - pull_and_check_skip1byte (h, h264_idr_slice_2, -1, 0); + pull_and_check (h, h264_idr_slice_1, 200, 0); + pull_and_check (h, h264_idr_slice_2, -1, 0); /* DISCONT must be propagated */ buf = composite_buffer (400, GST_BUFFER_FLAG_DISCONT, 2, @@ -1024,8 +1017,8 @@ GST_START_TEST (test_parse_sliced_au_nal) fail_unless_equals_int (gst_harness_push (h, buf), GST_FLOW_OK); fail_unless_equals_int (gst_harness_buffers_in_queue (h), 3); pull_and_check (h, h264_aud, 400, 0); - pull_and_check_skip1byte (h, h264_idr_slice_1, 400, GST_BUFFER_FLAG_DISCONT); - pull_and_check_skip1byte (h, h264_idr_slice_2, -1, 0); + pull_and_check (h, h264_idr_slice_1, 400, GST_BUFFER_FLAG_DISCONT); + pull_and_check (h, h264_idr_slice_2, -1, 0); gst_harness_teardown (h); } -- 2.7.4