rtpbasedepayload: condition the sending of gap events

The default implementation for packet loss handling previously
always sent a gap event.

While this is correct as long as we know the packet that was
lost was actually a media packet, with ULPFEC this becomes
a bit more complicated, as we do not know whether the packet
that was lost was a FEC packet, in which case it is better
to not actually send any gap events in the default implementation.

Some payloaders can be more clever about, for example VP8 can
use the picture-id, and the M and S bits to determine whether
the missing packet was inside an encoded frame or outside,
and thus whether if it was a media packet or a FEC packet,
which is why ulpfecdec still lets these lost events go through,
though stripping them of their seqnum, and appending a new
"might-have-been-fec" field to them.

This is all a bit terrible, but necessary to have ULPFEC
integrate properly with the rest of our RTP stack.

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

diff --git a/gst-libs/gst/rtp/gstrtpbasedepayload.c b/gst-libs/gst/rtp/gstrtpbasedepayload.c
index 8773cee2943a1606368917b7067a72637d906fb2..76228b142b0adfa44228ea64192600b50c800979 100644 (file)
--- a/gst-libs/gst/rtp/gstrtpbasedepayload.c
+++ b/gst-libs/gst/rtp/gstrtpbasedepayload.c
@@ -827,6 +827,8 @@ gst_rtp_base_depayload_packet_lost (GstRTPBaseDepayload * filter,
   GstClockTime timestamp, duration;
   GstEvent *sevent;
   const GstStructure *s;
+  gboolean might_have_been_fec;
+  gboolean res = TRUE;
 
   s = gst_event_get_structure (event);
 
@@ -841,10 +843,14 @@ gst_rtp_base_depayload_packet_lost (GstRTPBaseDepayload * filter,
     return FALSE;
   }
 
-  /* send GAP event */
-  sevent = gst_event_new_gap (timestamp, duration);
+  if (!gst_structure_get_boolean (s, "might-have-been-fec",
+          &might_have_been_fec) || !might_have_been_fec) {
+    /* send GAP event */
+    sevent = gst_event_new_gap (timestamp, duration);
+    res = gst_pad_push_event (filter->srcpad, sevent);
+  }
 
-  return gst_pad_push_event (filter->srcpad, sevent);
+  return res;
 }
 
 static GstStateChangeReturn

diff --git a/tests/check/libs/rtpbasedepayload.c b/tests/check/libs/rtpbasedepayload.c
index ed23ca17421856758a66b02efe569a9ccb9705aa..f24b0c46cb7eb9f7251b7f7c4be1a6d49bbd9a40 100644 (file)
--- a/tests/check/libs/rtpbasedepayload.c
+++ b/tests/check/libs/rtpbasedepayload.c
@@ -509,7 +509,8 @@ set_state (State * state, GstState new_state)
 }
 
 static void
-packet_lost (State * state, GstClockTime timestamp, GstClockTime duration)
+packet_lost (State * state, GstClockTime timestamp, GstClockTime duration,
+    gboolean might_have_been_fec)
 {
   GstEvent *event;
   guint seqnum = 0x4243;
@@ -521,6 +522,7 @@ packet_lost (State * state, GstClockTime timestamp, GstClockTime duration)
           "seqnum", G_TYPE_UINT, seqnum,
           "timestamp", G_TYPE_UINT64, timestamp,
           "duration", G_TYPE_UINT64, duration,
+          "might-have-been-fec", G_TYPE_BOOLEAN, might_have_been_fec,
           "late", G_TYPE_BOOLEAN, late, "retry", G_TYPE_UINT, retries, NULL));
 
   fail_unless (gst_pad_push_event (state->srcpad, event));
@@ -869,7 +871,11 @@ GST_START_TEST (rtp_base_depayload_packet_lost_test)
       "pts", 0 * GST_SECOND,
       "rtptime", G_GUINT64_CONSTANT (0x1234), "seq", 0x4242, NULL);
 
-  packet_lost (state, 1 * GST_SECOND, GST_SECOND);
+  packet_lost (state, 1 * GST_SECOND, GST_SECOND, FALSE);
+
+  /* If a packet was lost but we don't know whether it was a FEC packet,
+   * the depayloader should not generate gap events */
+  packet_lost (state, 2 * GST_SECOND, GST_SECOND, TRUE);
 
   push_rtp_buffer (state,
       "pts", 2 * GST_SECOND,