From 7e619f7e8371445171fc1dc7f8ddc4fdf0db5acf Mon Sep 17 00:00:00 2001
From: Edward Hervey <edward@centricular.com>
Date: Tue, 4 Apr 2023 09:21:47 +0200
Subject: [PATCH] twcc: Better handle duplicate packets

The previous code would only check if two packets in a row were duplicates. If
not (i.e. a packet is a duplicate of a packet received slightly before) the code
would generate completely bogus FCI because it assumes there were no duplicates
present in the array.

In order to be efficient, just store all received packets and remove the
duplicates just before the FCI is generated once the array of observations have
been sorted by seqnum.

Fixes TWCC usage with moderate to high packet duplication.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4328>
---
 .../gst-plugins-good/gst/rtpmanager/rtptwcc.c | 34 +++++++++++++------
 .../tests/check/elements/rtpsession.c         |  2 +-
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/subprojects/gst-plugins-good/gst/rtpmanager/rtptwcc.c b/subprojects/gst-plugins-good/gst/rtpmanager/rtptwcc.c
index ba8b5a854f..2b642bff6b 100644
--- a/subprojects/gst-plugins-good/gst/rtpmanager/rtptwcc.c
+++ b/subprojects/gst-plugins-good/gst/rtpmanager/rtptwcc.c
@@ -598,6 +598,20 @@ rtp_twcc_manager_add_fci (RTPTWCCManager * twcc, GstRTCPPacket * packet)
 
   g_array_sort (twcc->recv_packets, _twcc_seqnum_sort);
 
+  /* Quick scan to remove duplicates */
+  prev = &g_array_index (twcc->recv_packets, RecvPacket, 0);
+  for (i = 1; i < twcc->recv_packets->len;) {
+    RecvPacket *cur = &g_array_index (twcc->recv_packets, RecvPacket, i);
+
+    if (prev->seqnum == cur->seqnum) {
+      GST_DEBUG ("Removing duplicate packet #%u", cur->seqnum);
+      g_array_remove_index (twcc->recv_packets, i);
+    } else {
+      prev = cur;
+      i += 1;
+    }
+  }
+
   /* get first and last packet */
   first = &g_array_index (twcc->recv_packets, RecvPacket, 0);
   last =
@@ -740,6 +754,11 @@ _many_packets_some_lost (RTPTWCCManager * twcc, guint16 seqnum)
   first = &g_array_index (twcc->recv_packets, RecvPacket, 0);
   packet_count = seqnum - first->seqnum + 1;
 
+  /* If there are a high number of duplicates, we can't use the following
+   * metrics */
+  if (received_packets > packet_count)
+    return FALSE;
+
   /* check if we lost half of the threshold */
   lost_packets = packet_count - received_packets;
   if (received_packets >= 30 && lost_packets >= 60)
@@ -787,17 +806,6 @@ rtp_twcc_manager_recv_packet (RTPTWCCManager * twcc, RTPPacketInfo * pinfo)
     return FALSE;
   }
 
-  if (twcc->recv_packets->len > 0) {
-    RecvPacket *last = &g_array_index (twcc->recv_packets, RecvPacket,
-        twcc->recv_packets->len - 1);
-
-    diff = gst_rtp_buffer_compare_seqnum (last->seqnum, seqnum);
-    if (diff == 0) {
-      GST_INFO ("Received duplicate packet (%u), dropping", seqnum);
-      return FALSE;
-    }
-  }
-
   /* store the packet for Transport-wide RTCP feedback message */
   recv_packet_init (&packet, seqnum, pinfo);
   g_array_append_val (twcc->recv_packets, packet);
@@ -817,6 +825,8 @@ rtp_twcc_manager_recv_packet (RTPTWCCManager * twcc, RTPPacketInfo * pinfo)
           pinfo->running_time + twcc->feedback_interval;
 
     if (pinfo->running_time >= twcc->next_feedback_send_time) {
+      GST_LOG ("Generating feedback : Exceeded feedback interval %"
+          GST_TIME_FORMAT, GST_TIME_ARGS (twcc->feedback_interval));
       rtp_twcc_manager_create_feedback (twcc);
       send_feedback = TRUE;
 
@@ -824,6 +834,8 @@ rtp_twcc_manager_recv_packet (RTPTWCCManager * twcc, RTPPacketInfo * pinfo)
         twcc->next_feedback_send_time += twcc->feedback_interval;
     }
   } else if (pinfo->marker || _many_packets_some_lost (twcc, seqnum)) {
+    GST_LOG ("Generating feedback because of %s",
+        pinfo->marker ? "marker packet" : "many packets some lost");
     rtp_twcc_manager_create_feedback (twcc);
     send_feedback = TRUE;
 
diff --git a/subprojects/gst-plugins-good/tests/check/elements/rtpsession.c b/subprojects/gst-plugins-good/tests/check/elements/rtpsession.c
index ab3d4e92af..60316fc9e1 100644
--- a/subprojects/gst-plugins-good/tests/check/elements/rtpsession.c
+++ b/subprojects/gst-plugins-good/tests/check/elements/rtpsession.c
@@ -3466,7 +3466,7 @@ GST_START_TEST (test_twcc_duplicate_seqnums)
   TWCCPacket packets[] = {
     {1, 4 * 32 * GST_MSECOND, FALSE},
     {2, 5 * 32 * GST_MSECOND, FALSE},
-    {2, 6 * 32 * GST_MSECOND, FALSE},
+    {1, 6 * 32 * GST_MSECOND, FALSE},
     {3, 7 * 32 * GST_MSECOND, TRUE},
   };