rtpvp8depay: Send lost events when marker bit is missing

This means the previous frame was incomplete.

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

diff --git a/gst/rtp/gstrtpvp8depay.c b/gst/rtp/gstrtpvp8depay.c
index 6fe65a3..40ce1e0 100644 (file)
--- a/gst/rtp/gstrtpvp8depay.c
+++ b/gst/rtp/gstrtpvp8depay.c
@@ -80,6 +80,7 @@ gst_rtp_vp8_depay_init (GstRtpVP8Depay * self)
   self->adapter = gst_adapter_new ();
   self->started = FALSE;
   self->wait_for_keyframe = DEFAULT_WAIT_FOR_KEYFRAME;
+  self->last_pushed_was_lost_event = FALSE;
 }
 
 static void
@@ -187,10 +188,37 @@ send_last_lost_event (GstRtpVP8Depay * self)
         ->packet_lost (GST_RTP_BASE_DEPAYLOAD_CAST (self),
         self->last_lost_event);
     gst_event_replace (&self->last_lost_event, NULL);
+    self->last_pushed_was_lost_event = TRUE;
   }
 }
 
 static void
+send_new_lost_event (GstRtpVP8Depay * self, GstClockTime timestamp,
+    guint new_picture_id, const gchar * reason)
+{
+  GstEvent *event;
+
+  if (!GST_CLOCK_TIME_IS_VALID (timestamp)) {
+    GST_WARNING_OBJECT (self, "Can't create lost event with invalid timestmap");
+    return;
+  }
+
+  event = gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM,
+      gst_structure_new ("GstRTPPacketLost",
+          "timestamp", G_TYPE_UINT64, timestamp,
+          "duration", G_TYPE_UINT64, 0, NULL));
+
+  GST_DEBUG_OBJECT (self, "Pushing lost event "
+      "(picids 0x%x 0x%x, reason \"%s\"): %" GST_PTR_FORMAT,
+      self->last_picture_id, new_picture_id, reason, event);
+
+  GST_RTP_BASE_DEPAYLOAD_CLASS (gst_rtp_vp8_depay_parent_class)
+      ->packet_lost (GST_RTP_BASE_DEPAYLOAD_CAST (self), event);
+
+  gst_event_unref (event);
+}
+
+static void
 send_last_lost_event_if_needed (GstRtpVP8Depay * self, guint new_picture_id)
 {
   if (self->last_picture_id == PICTURE_ID_NONE)
@@ -236,6 +264,10 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp)
   guint hdrsize = 1;
   guint picture_id = PICTURE_ID_NONE;
   guint size = gst_rtp_buffer_get_payload_len (rtp);
+  guint s_bit;
+  guint part_id;
+  gboolean frame_start;
+  gboolean sent_lost_event = FALSE;
 
   if (G_UNLIKELY (GST_BUFFER_IS_DISCONT (rtp->buffer))) {
     GST_DEBUG_OBJECT (self, "Discontinuity, flushing adapter");
@@ -251,6 +283,10 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp)
     goto too_small;
 
   data = gst_rtp_buffer_get_payload (rtp);
+
+  s_bit = (data[0] >> 4) & 0x1;
+  part_id = (data[0] >> 0) & 0x7;
+
   /* Check X optional header */
   if ((data[0] & 0x80) != 0) {
     hdrsize++;
@@ -275,18 +311,30 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp)
     if ((data[1] & 0x20) != 0 || (data[1] & 0x10) != 0)
       hdrsize++;
   }
-  GST_LOG_OBJECT (depay, "hdrsize %u, size %u, picture id 0x%x", hdrsize,
-      size, picture_id);
+  GST_LOG_OBJECT (depay,
+      "hdrsize %u, size %u, picture id 0x%x, s %u, part_id %u", hdrsize, size,
+      picture_id, s_bit, part_id);
   if (G_UNLIKELY (hdrsize >= size))
     goto too_small;
 
-  if (G_UNLIKELY (!self->started)) {
-    /* Check if this is the start of a VP8 frame, otherwise bail */
-    /* S=1 and PartID= 0 */
-    if ((data[0] & 0x17) != 0x10) {
+  frame_start = (s_bit == 1) && (part_id == 0);
+  if (frame_start) {
+    if (G_UNLIKELY (self->started)) {
+      GST_DEBUG_OBJECT (depay, "Incomplete frame, flushing adapter");
+      gst_adapter_clear (self->adapter);
+      self->started = FALSE;
+
+      send_new_lost_event (self, GST_BUFFER_PTS (rtp->buffer), picture_id,
+          "Incomplete frame detected");
+      sent_lost_event = TRUE;
+    }
+  }
+
+  if (!self->started) {
+    if (G_UNLIKELY (!frame_start)) {
       GST_DEBUG_OBJECT (depay,
-          "The frame is missing the first packets, ignoring the packet");
-      if (self->stop_lost_events) {
+          "The frame is missing the first packet, ignoring the packet");
+      if (self->stop_lost_events && !sent_lost_event) {
         send_last_lost_event (self);
         self->stop_lost_events = FALSE;
       }
@@ -295,7 +343,7 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp)
 
     GST_LOG_OBJECT (depay, "Found the start of the frame");
 
-    if (self->stop_lost_events) {
+    if (self->stop_lost_events && !sent_lost_event) {
       send_last_lost_event_if_needed (self, picture_id);
       self->stop_lost_events = FALSE;
     }
@@ -373,6 +421,9 @@ gst_rtp_vp8_depay_process (GstRTPBaseDepayload * depay, GstRTPBuffer * rtp)
 
     if (picture_id != PICTURE_ID_NONE)
       self->stop_lost_events = TRUE;
+
+    self->last_pushed_was_lost_event = FALSE;
+
     return out;
   }
 
@@ -441,6 +492,8 @@ gst_rtp_vp8_depay_packet_lost (GstRTPBaseDepayload * depay, GstEvent * event)
   GstRtpVP8Depay *self = GST_RTP_VP8_DEPAY (depay);
   const GstStructure *s;
   gboolean might_have_been_fec;
+  gboolean unref_event = FALSE;
+  gboolean ret;
 
   s = gst_event_get_structure (event);
 
@@ -453,16 +506,30 @@ gst_rtp_vp8_depay_packet_lost (GstRTPBaseDepayload * depay, GstEvent * event)
       return TRUE;
     }
   } else if (self->last_picture_id != PICTURE_ID_NONE) {
-    GstStructure *s = gst_event_writable_structure (self->last_lost_event);
+    GstStructure *s;
+
+    if (!gst_event_is_writable (event)) {
+      event = gst_event_copy (event);
+      unref_event = TRUE;
+    }
+
+    s = gst_event_writable_structure (event);
 
     /* We are currently processing a picture, let's make sure the
      * base depayloader doesn't drop this lost event */
     gst_structure_remove_field (s, "might-have-been-fec");
   }
 
-  return
+  self->last_pushed_was_lost_event = TRUE;
+
+  ret =
       GST_RTP_BASE_DEPAYLOAD_CLASS
       (gst_rtp_vp8_depay_parent_class)->packet_lost (depay, event);
+
+  if (unref_event)
+    gst_event_unref (event);
+
+  return ret;
 }
 
 gboolean

diff --git a/gst/rtp/gstrtpvp8depay.h b/gst/rtp/gstrtpvp8depay.h
index c53ccaa..5582d63 100644 (file)
--- a/gst/rtp/gstrtpvp8depay.h
+++ b/gst/rtp/gstrtpvp8depay.h
@@ -70,6 +70,7 @@ struct _GstRtpVP8Depay
   guint last_picture_id;
 
   gboolean wait_for_keyframe;
+  gboolean last_pushed_was_lost_event;
 };
 
 GType gst_rtp_vp8_depay_get_type (void);

diff --git a/tests/check/elements/rtpvp8.c b/tests/check/elements/rtpvp8.c
index 4411289..939c028 100644 (file)
--- a/tests/check/elements/rtpvp8.c
+++ b/tests/check/elements/rtpvp8.c
@@ -48,8 +48,8 @@ static guint8 intra_nopicid_seqnum0[] = {
 };
 
 static GstBuffer *
-create_rtp_vp8_buffer (guint seqnum, guint picid, gint picid_bits,
-    GstClockTime buf_pts)
+create_rtp_vp8_buffer_full (guint seqnum, guint picid, gint picid_bits,
+    GstClockTime buf_pts, gboolean s_bit, gboolean marker_bit)
 {
   GstBuffer *ret;
   guint8 *packet;
@@ -74,11 +74,29 @@ create_rtp_vp8_buffer (guint seqnum, guint picid, gint picid_bits,
   packet[2] = (seqnum >> 8) & 0xff;
   packet[3] = (seqnum >> 0) & 0xff;
 
+  if (marker_bit)
+    packet[1] |= 0x80;
+  else
+    packet[1] &= ~0x80;
+
+  if (s_bit)
+    packet[12] |= 0x10;
+  else
+    packet[12] &= ~0x10;
+
   ret = gst_buffer_new_wrapped (packet, size);
   GST_BUFFER_PTS (ret) = buf_pts;
   return ret;
 }
 
+static GstBuffer *
+create_rtp_vp8_buffer (guint seqnum, guint picid, guint picid_bits,
+    GstClockTime buf_pts)
+{
+  return create_rtp_vp8_buffer_full (seqnum, picid, picid_bits, buf_pts, TRUE,
+      TRUE);
+}
+
 static void
 add_vp8_meta (GstBuffer * buffer, gboolean use_temporal_scaling,
     gboolean layer_sync, guint layer_id, guint tl0picidx)
@@ -502,6 +520,15 @@ typedef struct _DepayGapEventTestData
   gint picid_bits;
 } DepayGapEventTestData;
 
+typedef struct
+{
+  gint seq_num;
+  gint picid;
+  guint buffer_type;
+  gboolean s_bit;
+  gboolean marker_bit;
+} DepayGapEventTestDataFull;
+
 static void
 test_depay_gap_event_base (const DepayGapEventTestData * data,
     gboolean send_lost_event, gboolean expect_gap_event)
@@ -571,6 +598,153 @@ GST_START_TEST (test_depay_stop_gap_events)
 
 GST_END_TEST;
 
+GST_START_TEST (test_depay_send_gap_event_when_marker_bit_missing_and_picid_gap)
+{
+  gboolean send_lost_event = __i__ == 0;
+  GstClockTime pts = 0;
+  gint seqnum = 100;
+  gint i;
+  GstEvent *event;
+
+  GstHarness *h = gst_harness_new_parse ("rtpvp8depay");
+  gst_harness_set_src_caps_str (h, RTP_VP8_CAPS_STR);
+
+  /* Push a complete frame to avoid depayloader to suppress gap events */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 23,
+              7, pts, TRUE, TRUE)));
+  pts += 33 * GST_MSECOND;
+  seqnum++;
+
+  for (i = 0; i < 3; i++)
+    gst_event_unref (gst_harness_pull_event (h));
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  /* Push packet with start bit set, but no marker bit */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 24,
+              7, pts, TRUE, FALSE)));
+  pts += 33 * GST_MSECOND;
+  seqnum++;
+
+  if (send_lost_event) {
+    gst_harness_push_event (h,
+        gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM,
+            gst_structure_new ("GstRTPPacketLost", "timestamp", G_TYPE_UINT64,
+                pts, "duration", G_TYPE_UINT64, 33 * GST_MSECOND, NULL)));
+    pts += 33 * GST_MSECOND;
+    seqnum++;
+  }
+
+  /* Push packet with gap in picid */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 26,
+              7, pts, TRUE, TRUE)));
+
+  /* Expect only 2 output frames since the one frame was incomplete */
+  fail_unless_equals_int (2, gst_harness_buffers_received (h));
+
+  /* There should be a gap event, either triggered by the loss or the picid
+   * gap */
+
+  /* Making sure the GAP event was pushed downstream */
+  event = gst_harness_pull_event (h);
+  fail_unless_equals_string ("gap",
+      gst_event_type_get_name (GST_EVENT_TYPE (event)));
+  gst_event_unref (event);
+
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+GST_START_TEST
+    (test_depay_send_gap_event_when_marker_bit_missing_and_no_picid_gap) {
+  GstClockTime pts = 0;
+  gint seqnum = 100;
+  gint i;
+  GstEvent *event;
+
+  GstHarness *h = gst_harness_new_parse ("rtpvp8depay");
+  gst_harness_set_src_caps_str (h, RTP_VP8_CAPS_STR);
+
+  /* Push a complete frame to avoid depayloader to suppress gap events */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 23,
+              7, pts, TRUE, TRUE)));
+  pts += 33 * GST_MSECOND;
+  seqnum++;
+
+  for (i = 0; i < 3; i++)
+    gst_event_unref (gst_harness_pull_event (h));
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  /* Push packet with start bit set, but no marker bit */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 24,
+              7, pts, TRUE, FALSE)));
+  pts += 33 * GST_MSECOND;
+  seqnum++;
+
+  /* Push packet for next picid, without having sent a packet with marker bit
+   * foor the previous picid */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (seqnum, 25,
+              7, pts, TRUE, TRUE)));
+
+  /* Expect only 2 output frames since one was incomplete */
+  fail_unless_equals_int (2, gst_harness_buffers_received (h));
+
+  /* Making sure the GAP event was pushed downstream */
+  event = gst_harness_pull_event (h);
+  gst_event_unref (event);
+
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_depay_no_gap_event_when_partial_frames_with_no_picid_gap)
+{
+  gint i;
+  GstClockTime pts = 0;
+  GstHarness *h = gst_harness_new ("rtpvp8depay");
+  gst_harness_set_src_caps_str (h, RTP_VP8_CAPS_STR);
+
+  /* start with complete frame to make sure depayloader will not drop
+   * potential gap events */
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (100, 24,
+              7, pts, TRUE, TRUE)));
+  fail_unless_equals_int (1, gst_harness_buffers_received (h));
+
+  /* drop setup events to more easily check for gap events */
+  for (i = 0; i < 3; i++)
+    gst_event_unref (gst_harness_pull_event (h));
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  /* Next frame is split in two packets */
+  pts += 33 * GST_MSECOND;
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (101, 25,
+              7, pts, TRUE, FALSE)));
+  fail_unless_equals_int (GST_FLOW_OK,
+      gst_harness_push (h, create_rtp_vp8_buffer_full (102, 25,
+              7, pts, FALSE, TRUE)));
+  fail_unless_equals_int (2, gst_harness_buffers_received (h));
+
+  /* there must be no gap events */
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
+
 /* Packet loss + lost picture ids */
 static const DepayGapEventTestData resend_gap_event_test_data[][2] = {
   /* 7bit picture ids */
@@ -617,6 +791,12 @@ rtpvp8_suite (void)
       G_N_ELEMENTS (stop_gap_events_test_data));
   tcase_add_loop_test (tc_chain, test_depay_resend_gap_event, 0,
       G_N_ELEMENTS (resend_gap_event_test_data));
+  tcase_add_loop_test (tc_chain,
+      test_depay_send_gap_event_when_marker_bit_missing_and_picid_gap, 0, 2);
+  tcase_add_test (tc_chain,
+      test_depay_send_gap_event_when_marker_bit_missing_and_no_picid_gap);
+  tcase_add_test (tc_chain,
+      test_depay_no_gap_event_when_partial_frames_with_no_picid_gap);
 
   return s;
 }