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: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/1251>

diff --git a/gst-libs/gst/codecparsers/gsth264parser.c b/gst-libs/gst/codecparsers/gsth264parser.c
index 45a4aef..63d96fb 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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);
 }