From eadeec791a2bd0277ef0ade2d9c2b14bccfff608 Mon Sep 17 00:00:00 2001
From: Stian Selnes <stian@pexip.com>
Date: Tue, 30 Aug 2016 13:48:00 +0200
Subject: [PATCH] rtpbasedepayload: Drop gap events before first buffer

Before a gap event is pushed downstream a segment event must be pushed
since the gap event can cause packet concealment downstream and hence
data flow. Since concealment before receiving any data packets usually
doesn't make any sense, the gap event is not sent downstream.

Alternatively one could generate a default caps and segment event, but
no need to complicate things until it's proven necessary.

https://bugzilla.gnome.org/show_bug.cgi?id=773104
https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/301
---
 gst-libs/gst/rtp/gstrtpbasedepayload.c |  9 +++++
 tests/check/libs/rtpbasedepayload.c    | 56 ++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/gst-libs/gst/rtp/gstrtpbasedepayload.c b/gst-libs/gst/rtp/gstrtpbasedepayload.c
index c9c5204e1c..547619332f 100644
--- a/gst-libs/gst/rtp/gstrtpbasedepayload.c
+++ b/gst-libs/gst/rtp/gstrtpbasedepayload.c
@@ -913,6 +913,15 @@ gst_rtp_base_depayload_packet_lost (GstRTPBaseDepayload * filter,
     return FALSE;
   }
 
+  sevent = gst_pad_get_sticky_event (filter->srcpad, GST_EVENT_SEGMENT, 0);
+  if (G_UNLIKELY (!sevent)) {
+    /* Typically happens if lost event arrives before first buffer */
+    GST_DEBUG_OBJECT (filter,
+        "Ignore packet loss because segment event missing");
+    return FALSE;
+  }
+  gst_event_unref (sevent);
+
   if (!gst_structure_get_boolean (s, "might-have-been-fec",
           &might_have_been_fec) || !might_have_been_fec) {
     /* send GAP event */
diff --git a/tests/check/libs/rtpbasedepayload.c b/tests/check/libs/rtpbasedepayload.c
index 5e6f8a3a93..b133e4e64e 100644
--- a/tests/check/libs/rtpbasedepayload.c
+++ b/tests/check/libs/rtpbasedepayload.c
@@ -934,6 +934,60 @@ GST_START_TEST (rtp_base_depayload_packet_lost_test)
 }
 
 GST_END_TEST
+/* If a lost event is received before the first buffer, the rtp base
+ * depayloader will not send a gap event downstream. Alternatively it should
+ * make sure that stream-start, caps and segment events are sent in correct
+ * order before the gap event so that packet loss concealment can take place
+ * downstream, but this is more complicated and without any real benefit since
+ * concealment before any data is received is not very useful. */
+GST_START_TEST (rtp_base_depayload_packet_lost_before_first_buffer_test)
+{
+  GstHarness *h;
+  GstEvent *event;
+  GstRtpDummyDepay *depay;
+  const GstEventType etype[] = {
+    GST_EVENT_STREAM_START, GST_EVENT_CAPS, GST_EVENT_SEGMENT
+  };
+  gint i;
+
+  depay = rtp_dummy_depay_new ();
+  h = gst_harness_new_with_element (GST_ELEMENT_CAST (depay), "sink", "src");
+  gst_harness_set_src_caps_str (h, "application/x-rtp");
+
+  /* Verify that depayloader has received setup events */
+  for (i = 0; i < 3; i++) {
+    event = gst_pad_get_sticky_event (h->srcpad, etype[i], 0);
+    fail_unless (event != NULL);
+    gst_event_unref (event);
+  }
+
+  /* Send loss event to depayloader */
+  gst_harness_push_event (h, gst_event_new_custom (GST_EVENT_CUSTOM_DOWNSTREAM,
+          gst_structure_new ("GstRTPPacketLost",
+              "seqnum", G_TYPE_UINT, (guint) 0,
+              "timestamp", G_TYPE_UINT64, (guint64) 0,
+              "duration", G_TYPE_UINT64, (guint64) 10 * GST_MSECOND, NULL)));
+
+  /* When a buffer is pushed, an updated (and more accurate) segment event
+   * should aslo be sent. */
+  gst_harness_push (h, gst_rtp_buffer_new_allocate (0, 0, 0));
+
+  /* Verify that setup events are sent before gap event */
+  for (i = 0; i < 3; i++) {
+    fail_unless (event = gst_harness_pull_event (h));
+    fail_unless_equals_int (GST_EVENT_TYPE (event), etype[i]);
+    gst_event_unref (event);
+  }
+  fail_unless_equals_int (gst_harness_events_in_queue (h), 0);
+
+  gst_buffer_unref (gst_harness_pull (h));
+  fail_unless_equals_int (gst_harness_buffers_in_queue (h), 0);
+
+  g_object_unref (depay);
+  gst_harness_teardown (h);
+}
+
+GST_END_TEST;
 /* rtp base depayloader should set DISCONT flag on buffer in case of a large
  * sequence number gap, and it's not set already by upstream. This tests a
  * certain code path where the buffer needs to be made writable to set the
@@ -1341,6 +1395,8 @@ rtp_basepayloading_suite (void)
   tcase_add_test (tc_chain, rtp_base_depayload_without_negotiation_test);
 
   tcase_add_test (tc_chain, rtp_base_depayload_packet_lost_test);
+  tcase_add_test (tc_chain,
+      rtp_base_depayload_packet_lost_before_first_buffer_test);
   tcase_add_test (tc_chain, rtp_base_depayload_seq_discont_test);
 
   tcase_add_test (tc_chain, rtp_base_depayload_repeated_caps_test);