rtph26*depay: drop FU's without a corresponding start bit

If we have not received a FU with a start bit set, any subsequent FU
data is not useful at all and would result in an invalid stream.

This case is constructed from multiple requirements in
RFC 3984 Section 5.8 and RFC 7798 Section 4.4.3.  Following are excerpts
from RFC 3984 but RFC 7798 contains similar language.

The FU in a single FU case is forbidden:

   A fragmented NAL unit MUST NOT be transmitted in one FU; i.e., the
   Start bit and End bit MUST NOT both be set to one in the same FU
   header.

and dropping is possible:

   If a fragmentation unit is lost, the receiver SHOULD discard all
   following fragmentation units in transmission order corresponding to
   the same fragmented NAL unit.

The jump in seqnum case is supported by this from the specification
instead of implementing the forbidden_zero_bit mangling:

   If a fragmentation unit is lost, the receiver SHOULD discard all
   following fragmentation units in transmission order corresponding to
   the same fragmented NAL unit.

   A receiver in an endpoint or in a MANE MAY aggregate the first n-1
   fragments of a NAL unit to an (incomplete) NAL unit, even if fragment
   n of that NAL unit is not received.  In this case, the
   forbidden_zero_bit of the NAL unit MUST be set to one to indicate a
   syntax violation.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/730>

diff --git a/gst/rtp/gstrtph264depay.c b/gst/rtp/gstrtph264depay.c
index 854cb7c..cc92c9a 100644 (file)
--- a/gst/rtp/gstrtph264depay.c
+++ b/gst/rtp/gstrtph264depay.c
@@ -1035,6 +1035,7 @@ gst_rtp_h264_depay_process (GstRTPBaseDepayload * depayload, GstRTPBuffer * rtp)
     gst_adapter_clear (rtph264depay->adapter);
     rtph264depay->wait_start = TRUE;
     rtph264depay->current_fu_type = 0;
+    rtph264depay->last_fu_seqnum = 0;
   }
 
   {
@@ -1194,6 +1195,7 @@ gst_rtp_h264_depay_process (GstRTPBaseDepayload * depayload, GstRTPBuffer * rtp)
 
           rtph264depay->current_fu_type = nal_unit_type;
           rtph264depay->fu_timestamp = timestamp;
+          rtph264depay->last_fu_seqnum = gst_rtp_buffer_get_seq (rtp);
 
           rtph264depay->wait_start = FALSE;
 
@@ -1221,6 +1223,25 @@ gst_rtp_h264_depay_process (GstRTPBaseDepayload * depayload, GstRTPBuffer * rtp)
           /* and assemble in the adapter */
           gst_adapter_push (rtph264depay->adapter, outbuf);
         } else {
+          if (rtph264depay->current_fu_type == 0) {
+            /* previous FU packet missing start bit? */
+            GST_WARNING_OBJECT (rtph264depay, "missing FU start bit on an "
+                "earlier packet. Dropping.");
+            gst_adapter_clear (rtph264depay->adapter);
+            return NULL;
+          }
+          if (gst_rtp_buffer_compare_seqnum (rtph264depay->last_fu_seqnum,
+                  gst_rtp_buffer_get_seq (rtp)) != 1) {
+            /* jump in sequence numbers within an FU is cause for discarding */
+            GST_WARNING_OBJECT (rtph264depay, "Jump in sequence numbers from "
+                "%u to %u within Fragmentation Unit. Data was lost, dropping "
+                "stored.", rtph264depay->last_fu_seqnum,
+                gst_rtp_buffer_get_seq (rtp));
+            gst_adapter_clear (rtph264depay->adapter);
+            return NULL;
+          }
+          rtph264depay->last_fu_seqnum = gst_rtp_buffer_get_seq (rtp);
+
           /* strip off FU indicator and FU header bytes */
           payload += 2;
           payload_len -= 2;

diff --git a/gst/rtp/gstrtph264depay.h b/gst/rtp/gstrtph264depay.h
index ba41312..2cb2167 100644 (file)
--- a/gst/rtp/gstrtph264depay.h
+++ b/gst/rtp/gstrtph264depay.h
@@ -59,6 +59,7 @@ struct _GstRtpH264Depay
 
   /* Work around broken payloaders wrt. FU-A & FU-B */
   guint8 current_fu_type;
+  guint16 last_fu_seqnum;
   GstClockTime fu_timestamp;
   gboolean fu_marker;
 

diff --git a/gst/rtp/gstrtph265depay.c b/gst/rtp/gstrtph265depay.c
index 384c6bf..38f53e4 100644 (file)
--- a/gst/rtp/gstrtph265depay.c
+++ b/gst/rtp/gstrtph265depay.c
@@ -1259,6 +1259,7 @@ gst_rtp_h265_depay_process (GstRTPBaseDepayload * depayload, GstRTPBuffer * rtp)
     gst_adapter_clear (rtph265depay->adapter);
     rtph265depay->wait_start = TRUE;
     rtph265depay->current_fu_type = 0;
+    rtph265depay->last_fu_seqnum = 0;
   }
 
   {
@@ -1456,6 +1457,7 @@ gst_rtp_h265_depay_process (GstRTPBaseDepayload * depayload, GstRTPBuffer * rtp)
 
           rtph265depay->current_fu_type = nal_unit_type;
           rtph265depay->fu_timestamp = timestamp;
+          rtph265depay->last_fu_seqnum = gst_rtp_buffer_get_seq (rtp);
 
           rtph265depay->wait_start = FALSE;
 
@@ -1493,6 +1495,24 @@ gst_rtp_h265_depay_process (GstRTPBaseDepayload * depayload, GstRTPBuffer * rtp)
           /* and assemble in the adapter */
           gst_adapter_push (rtph265depay->adapter, outbuf);
         } else {
+          if (rtph265depay->current_fu_type == 0) {
+            /* previous FU packet missing start bit? */
+            GST_WARNING_OBJECT (rtph265depay, "missing FU start bit on an "
+                "earlier packet. Dropping.");
+            gst_adapter_clear (rtph265depay->adapter);
+            return NULL;
+          }
+          if (gst_rtp_buffer_compare_seqnum (rtph265depay->last_fu_seqnum,
+                  gst_rtp_buffer_get_seq (rtp)) != 1) {
+            /* jump in sequence numbers within an FU is cause for discarding */
+            GST_WARNING_OBJECT (rtph265depay, "Jump in sequence numbers from "
+                "%u to %u within Fragmentation Unit. Data was lost, dropping "
+                "stored.", rtph265depay->last_fu_seqnum,
+                gst_rtp_buffer_get_seq (rtp));
+            gst_adapter_clear (rtph265depay->adapter);
+            return NULL;
+          }
+          rtph265depay->last_fu_seqnum = gst_rtp_buffer_get_seq (rtp);
 
           GST_DEBUG_OBJECT (rtph265depay,
               "Following part of Fragmentation Unit");

diff --git a/gst/rtp/gstrtph265depay.h b/gst/rtp/gstrtph265depay.h
index cf17694..b3b55ee 100644 (file)
--- a/gst/rtp/gstrtph265depay.h
+++ b/gst/rtp/gstrtph265depay.h
@@ -73,6 +73,7 @@ struct _GstRtpH265Depay
 
   /* Work around broken payloaders wrt. Fragmentation Units */
   guint8 current_fu_type;
+  guint16 last_fu_seqnum;
   GstClockTime fu_timestamp;
   gboolean fu_marker;
 

diff --git a/tests/check/elements/rtph264.c b/tests/check/elements/rtph264.c
index 10c350c..087a371 100644 (file)
--- a/tests/check/elements/rtph264.c
+++ b/tests/check/elements/rtph264.c
@@ -470,6 +470,127 @@ GST_START_TEST (test_rtph264depay_marker_to_flag)
 
 GST_END_TEST;
 
+/* This was generated using pipeline:
+ * gst-launch-1.0 videotestsrc num-buffers=1 pattern=green \
+ *     ! video/x-raw,width=24,height=16 \
+ *     ! openh264enc ! rtph264pay mtu=32 ! fakesink dump=1
+ */
+/* RTP h264_idr FU-A */
+static guint8 rtp_h264_idr_fu_start[] = {
+  0x80, 0x60, 0x5f, 0xd2, 0x20, 0x3b, 0x6e, 0xcf,
+  0x6c, 0x54, 0x21, 0x8d, 0x7c, 0x85, 0xb8, 0x00,
+  0x04, 0x00, 0x00, 0x09, 0xff, 0xff, 0xf8, 0x22,
+  0x8a, 0x00, 0x1f, 0x1c, 0x00, 0x04, 0x1c, 0xe3,
+};
+
+static guint8 rtp_h264_idr_fu_middle[] = {
+  0x80, 0x60, 0x5f, 0xd3, 0x20, 0x3b, 0x6e, 0xcf,
+  0x6c, 0x54, 0x21, 0x8d, 0x7c, 0x05, 0x80, 0x00,
+  0x84, 0xdf, 0xf8, 0x7f, 0xe0, 0x8e, 0x28, 0x00,
+  0x08, 0x37, 0xf8, 0x80, 0x00, 0x20, 0x52, 0x00,
+};
+
+static guint8 rtp_h264_idr_fu_end[] = {
+  0x80, 0xe0, 0x5f, 0xd4, 0x20, 0x3b, 0x6e, 0xcf,
+  0x6c, 0x54, 0x21, 0x8d, 0x7c, 0x45, 0x02, 0x01,
+  0x91, 0x00, 0x00, 0x40, 0xf4, 0x00, 0x04, 0x08,
+  0x30,
+};
+
+GST_START_TEST (test_rtph264depay_fu_a)
+{
+  GstHarness *h = gst_harness_new ("rtph264depay");
+  GstBuffer *buffer;
+  GstRTPBuffer rtp = GST_RTP_BUFFER_INIT;
+  GstFlowReturn ret;
+
+  gst_harness_set_caps_str (h,
+      "application/x-rtp,media=video,clock-rate=90000,encoding-name=H264",
+      "video/x-h264,alignment=au,stream-format=byte-stream");
+
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_start,
+      sizeof (rtp_h264_idr_fu_start));
+  fail_unless (gst_rtp_buffer_map (buffer, GST_MAP_READ, &rtp));
+  gst_rtp_buffer_unmap (&rtp);
+
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+  fail_unless_equals_int (gst_harness_buffers_in_queue (h), 0);
+
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_middle,
+      sizeof (rtp_h264_idr_fu_middle));
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_end, sizeof (rtp_h264_idr_fu_end));
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+
+  fail_unless_equals_int (gst_harness_buffers_in_queue (h), 1);
+
+  buffer = gst_harness_pull (h);
+  fail_unless (GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_MARKER));
+  gst_buffer_unref (buffer);
+
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_rtph264depay_fu_a_missing_start)
+{
+  GstHarness *h = gst_harness_new ("rtph264depay");
+  GstBuffer *buffer;
+  GstRTPBuffer rtp = GST_RTP_BUFFER_INIT;
+  GstFlowReturn ret;
+  guint16 seq;
+
+  gst_harness_set_caps_str (h,
+      "application/x-rtp,media=video,clock-rate=90000,encoding-name=H264",
+      "video/x-h264,alignment=au,stream-format=byte-stream");
+
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_start,
+      sizeof (rtp_h264_idr_fu_start));
+
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_middle,
+      sizeof (rtp_h264_idr_fu_middle));
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_end, sizeof (rtp_h264_idr_fu_end));
+  fail_unless (gst_rtp_buffer_map (buffer, GST_MAP_READ, &rtp));
+  seq = gst_rtp_buffer_get_seq (&rtp);
+  gst_rtp_buffer_unmap (&rtp);
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+  fail_unless_equals_int (gst_harness_buffers_in_queue (h), 1);
+
+  /* A broken sender case seen in the wild where the seqnums are continuous
+   * but only contain a FU with an end-bit, no start-bit */
+  buffer =
+      wrap_static_buffer (rtp_h264_idr_fu_end, sizeof (rtp_h264_idr_fu_end));
+  fail_unless (gst_rtp_buffer_map (buffer, GST_MAP_WRITE, &rtp));
+  gst_rtp_buffer_set_seq (&rtp, ++seq);
+  gst_rtp_buffer_unmap (&rtp);
+  ret = gst_harness_push (h, buffer);
+  fail_unless_equals_int (ret, GST_FLOW_OK);
+
+  fail_unless_equals_int (gst_harness_buffers_in_queue (h), 1);
+
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
 
 /* As GStreamer does not have STAP-A yet, this was extracted from
  * issue #557 provided sample */
@@ -1344,6 +1465,8 @@ rtph264_suite (void)
   tcase_add_test (tc_chain, test_rtph264depay_eos);
   tcase_add_test (tc_chain, test_rtph264depay_marker_to_flag);
   tcase_add_test (tc_chain, test_rtph264depay_stap_a_marker);
+  tcase_add_test (tc_chain, test_rtph264depay_fu_a);
+  tcase_add_test (tc_chain, test_rtph264depay_fu_a_missing_start);
 
   tc_chain = tcase_create ("rtph264pay");
   suite_add_tcase (s, tc_chain);