rtpbasedepayload: look at ssrc before sequence numbers

Doing so prevents us dropping buffers in the rare, but possible, situations,
when the stream changes SSRC and new sequence numbers does not differ
much from the last sequence number from previous SSRC. For example:
ssrc - 0xaaaa 101,102,103,104 ssrc - 0xbbbb 102, 103, 104, 105...
In the scenario above we don't want to drop the first 3 packets of
0xbbbb stream.

https://bugzilla.gnome.org/show_bug.cgi?id=764459

diff --git a/gst-libs/gst/rtp/gstrtpbasedepayload.c b/gst-libs/gst/rtp/gstrtpbasedepayload.c
index 8a313af..20ffcd8 100644 (file)
--- a/gst-libs/gst/rtp/gstrtpbasedepayload.c
+++ b/gst-libs/gst/rtp/gstrtpbasedepayload.c
@@ -46,6 +46,7 @@ struct _GstRTPBaseDepayloadPrivate
   GstClockTime dts;
   GstClockTime duration;
 
+  guint32 last_ssrc;
   guint32 last_seqnum;
   guint32 last_rtptime;
   guint32 next_seqnum;
@@ -356,6 +357,7 @@ gst_rtp_base_depayload_handle_buffer (GstRTPBaseDepayload * filter,
   GstRTPBaseDepayloadPrivate *priv;
   GstFlowReturn ret = GST_FLOW_OK;
   GstBuffer *out_buf;
+  guint32 ssrc;
   guint16 seqnum;
   guint32 rtptime;
   gboolean discont, buf_discont;
@@ -380,6 +382,7 @@ gst_rtp_base_depayload_handle_buffer (GstRTPBaseDepayload * filter,
   priv->dts = GST_BUFFER_DTS (in);
   priv->duration = GST_BUFFER_DURATION (in);
 
+  ssrc = gst_rtp_buffer_get_ssrc (&rtp);
   seqnum = gst_rtp_buffer_get_seq (&rtp);
   rtptime = gst_rtp_buffer_get_timestamp (&rtp);
 
@@ -396,32 +399,40 @@ gst_rtp_base_depayload_handle_buffer (GstRTPBaseDepayload * filter,
    * are strictly increasing, dropping anything that is out of the ordinary. We
    * can only do this when the next_seqnum is known. */
   if (G_LIKELY (priv->next_seqnum != -1)) {
-    gap = gst_rtp_buffer_compare_seqnum (seqnum, priv->next_seqnum);
-
-    /* if we have no gap, all is fine */
-    if (G_UNLIKELY (gap != 0)) {
-      GST_LOG_OBJECT (filter, "got packet %u, expected %u, gap %d", seqnum,
-          priv->next_seqnum, gap);
-      if (gap < 0) {
-        /* seqnum > next_seqnum, we are missing some packets, this is always a
-         * DISCONT. */
-        GST_LOG_OBJECT (filter, "%d missing packets", gap);
-        discont = TRUE;
-      } else {
-        /* seqnum < next_seqnum, we have seen this packet before or the sender
-         * could be restarted. If the packet is not too old, we throw it away as
-         * a duplicate, otherwise we mark discont and continue. 100 misordered
-         * packets is a good threshold. See also RFC 4737. */
-        if (gap < 100)
-          goto dropping;
-
-        GST_LOG_OBJECT (filter,
-            "%d > 100, packet too old, sender likely restarted", gap);
-        discont = TRUE;
+    if (ssrc != priv->last_ssrc) {
+      GST_LOG_OBJECT (filter,
+          "New ssrc %u (current ssrc %u), sender restarted",
+          ssrc, priv->last_ssrc);
+          discont = TRUE;
+    } else {
+      gap = gst_rtp_buffer_compare_seqnum (seqnum, priv->next_seqnum);
+
+      /* if we have no gap, all is fine */
+      if (G_UNLIKELY (gap != 0)) {
+        GST_LOG_OBJECT (filter, "got packet %u, expected %u, gap %d", seqnum,
+            priv->next_seqnum, gap);
+        if (gap < 0) {
+          /* seqnum > next_seqnum, we are missing some packets, this is always a
+           * DISCONT. */
+          GST_LOG_OBJECT (filter, "%d missing packets", gap);
+          discont = TRUE;
+        } else {
+          /* seqnum < next_seqnum, we have seen this packet before or the sender
+           * could be restarted. If the packet is not too old, we throw it away as
+           * a duplicate, otherwise we mark discont and continue. 100 misordered
+           * packets is a good threshold. See also RFC 4737. */
+          if (gap < 100)
+            goto dropping;
+
+          GST_LOG_OBJECT (filter,
+              "%d > 100, packet too old, sender likely restarted", gap);
+          discont = TRUE;
+        }
       }
     }
   }
   priv->next_seqnum = (seqnum + 1) & 0xffff;
+  priv->last_ssrc = ssrc;
 
   if (G_UNLIKELY (discont)) {
     priv->discont = TRUE;

diff --git a/tests/check/libs/rtpbasedepayload.c b/tests/check/libs/rtpbasedepayload.c
index a2c1c9b..63cbc87 100644 (file)
--- a/tests/check/libs/rtpbasedepayload.c
+++ b/tests/check/libs/rtpbasedepayload.c
@@ -730,6 +730,50 @@ GST_START_TEST (rtp_base_depayload_reversed_test)
 }
 
 GST_END_TEST
+/* The same scenario as in rtp_base_depayload_reversed_test
+ * except that SSRC is changed for the 2nd packet that is why
+ * it should not be discarded.
+ */
+GST_START_TEST (rtp_base_depayload_ssrc_changed_test)
+{
+  State *state;
+
+  state = create_depayloader ("application/x-rtp", NULL);
+
+  set_state (state, GST_STATE_PLAYING);
+
+  push_rtp_buffer (state,
+      "pts", 0 * GST_SECOND,
+      "rtptime", G_GUINT64_CONSTANT (0x43214321),
+      "seq", 0x4242, "ssrc", 0xabe2b0b, NULL);
+
+  push_rtp_buffer (state,
+      "pts", 1 * GST_SECOND,
+      "rtptime", G_GUINT64_CONSTANT (0x43214321) + 1 * DEFAULT_CLOCK_RATE,
+      "seq", 0x4242 - 1, "ssrc", 0xcafebabe, NULL);
+
+  set_state (state, GST_STATE_NULL);
+
+  validate_buffers_received (2);
+
+  validate_buffer (0, "pts", 0 * GST_SECOND, "discont", FALSE, NULL);
+
+  validate_buffer (1, "pts", 1 * GST_SECOND, "discont", TRUE, NULL);
+
+  validate_events_received (3);
+
+  validate_event (0, "stream-start", NULL);
+
+  validate_event (1, "caps", "media-type", "application/x-rtp", NULL);
+
+  validate_event (2, "segment",
+      "time", G_GUINT64_CONSTANT (0),
+      "start", G_GUINT64_CONSTANT (0), "stop", G_MAXUINT64, NULL);
+
+  destroy_depayloader (state);
+}
+
+GST_END_TEST
 /* the intent of this test is to push two RTP packets that have reverse sequence
  * numbers that differ significantly. the depayloader will consider RTP packets
  * where the sequence numbers differ by more than 1000 to indicate that the
@@ -1198,6 +1242,7 @@ rtp_basepayloading_suite (void)
   tcase_add_test (tc_chain, rtp_base_depayload_invalid_rtp_packet_test);
   tcase_add_test (tc_chain, rtp_base_depayload_with_gap_test);
   tcase_add_test (tc_chain, rtp_base_depayload_reversed_test);
+  tcase_add_test (tc_chain, rtp_base_depayload_ssrc_changed_test);
   tcase_add_test (tc_chain, rtp_base_depayload_old_reversed_test);
 
   tcase_add_test (tc_chain, rtp_base_depayload_without_negotiation_test);