rtptwcc: fixed feedback packet count overflow that allowed late

packets to be processed

Co-authored-by: Havard Graff <havard.graff@gmail.com>
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/927>

2 files changed, 75 insertions, 3 deletions

diff --git a/gst/rtpmanager/rtptwcc.c b/gst/rtpmanager/rtptwcc.c
index 3bb054063..515c69b23 100644
--- a/gst/rtpmanager/rtptwcc.c
+++ b/gst/rtpmanager/rtptwcc.c
@@ -72,7 +72,7 @@ struct _RTPTWCCManager
   guint max_packets_per_rtcp;
   GArray *recv_packets;
 
-  guint8 fb_pkt_count;
+  guint64 fb_pkt_count;
   gint32 last_seqnum;
 
   GArray *sent_packets;
@@ -438,7 +438,7 @@ rtp_twcc_manager_add_fci (RTPTWCCManager * twcc, GstRTCPPacket * packet)
   GST_WRITE_UINT16_BE (header.base_seqnum, first->seqnum);
   GST_WRITE_UINT16_BE (header.packet_count, packet_count);
   GST_WRITE_UINT24_BE (header.base_time, base_time);
-  GST_WRITE_UINT8 (header.fb_pkt_count, twcc->fb_pkt_count);
+  GST_WRITE_UINT8 (header.fb_pkt_count, twcc->fb_pkt_count % G_MAXUINT8);
 
   base_time *= REF_TIME_UNIT;
   ts_rounded = base_time;
@@ -446,7 +446,7 @@ rtp_twcc_manager_add_fci (RTPTWCCManager * twcc, GstRTCPPacket * packet)
   GST_DEBUG ("Created TWCC feedback: base_seqnum: #%u, packet_count: %u, "
       "base_time %" GST_TIME_FORMAT " fb_pkt_count: %u",
       first->seqnum, packet_count, GST_TIME_ARGS (base_time),
-      twcc->fb_pkt_count);
+      twcc->fb_pkt_count % G_MAXUINT8);
 
   twcc->fb_pkt_count++;
   twcc->expected_recv_seqnum = first->seqnum + packet_count;
diff --git a/tests/check/elements/rtpsession.c b/tests/check/elements/rtpsession.c
index 451ae2d29..c8eb5bdbc 100644
--- a/tests/check/elements/rtpsession.c
+++ b/tests/check/elements/rtpsession.c
@@ -3532,6 +3532,77 @@ GST_START_TEST (test_twcc_recv_packets_reordered)
 
 GST_END_TEST;
 
+GST_START_TEST (test_twcc_recv_late_packet_fb_pkt_count_wrap)
+{
+  SessionHarness *h = session_harness_new ();
+  GstBuffer *buf;
+  guint i;
+
+  guint8 exp_fci0[] = {
+    0x01, 0x00,                 /* base sequence number: 256 */
+    0x00, 0x01,                 /* packet status count: 1 */
+    0x00, 0x00, 0x01,           /* reference time: 0 */
+    0x00,                       /* feedback packet count: 00 */
+    /* packet chunks: */
+    0x20, 0x01,                 /* 0 0 1 0 0 0 0 0 | 0 0 0 0 0 0 0 1 */
+    0x00,                       /* 0 recv-delta */
+    0x00,                       /* padding */
+  };
+
+  guint8 exp_fci1[] = {
+    0x01, 0x01,                 /* base sequence number: 257 */
+    0x00, 0x01,                 /* packet status count: 1 */
+    0x00, 0x00, 0x01,           /* reference time: 0 */
+    0x01,                       /* feedback packet count: 0 */
+    /* packet chunks: */
+    0x20, 0x01,                 /* 0 0 1 0 0 0 0 0 | 0 0 0 0 0 0 0 1 */
+    0x01,                       /* 1 recv-delta */
+    0x00,                       /* padding */
+  };
+
+  session_harness_set_twcc_recv_ext_id ((h), TEST_TWCC_EXT_ID);
+
+  /* Push packets to get the feedback packet count wrap limit */
+  for (i = 0; i < 255; i++) {
+    fail_unless_equals_int (GST_FLOW_OK,
+        session_harness_recv_rtp ((h),
+            generate_twcc_recv_buffer (i, i * 250 * GST_USECOND, TRUE)));
+
+    /* ignore the twcc for these ones */
+    gst_buffer_unref (session_harness_produce_twcc (h));
+  }
+
+  /* push pkt #256 to jump ahead and force the overflow */
+  fail_unless_equals_int (GST_FLOW_OK,
+      session_harness_recv_rtp ((h),
+          generate_twcc_recv_buffer (256, 256 * 250 * GST_USECOND, TRUE)));
+
+  /* pkt #255 is late and should be dropped */
+  fail_unless_equals_int (GST_FLOW_OK,
+      session_harness_recv_rtp ((h),
+          generate_twcc_recv_buffer (255, 255 * 250 * GST_USECOND, TRUE)));
+
+  /* push pkt #257 to verify fci is correct */
+  fail_unless_equals_int (GST_FLOW_OK,
+      session_harness_recv_rtp ((h),
+          generate_twcc_recv_buffer (257, 257 * 250 * GST_USECOND, TRUE)));
+
+
+  /* we expect a fci for pkt #256 */
+  buf = session_harness_produce_twcc (h);
+  twcc_verify_fci (buf, exp_fci0);
+  gst_buffer_unref (buf);
+
+  /* and one fci for pkt #257 */
+  buf = session_harness_produce_twcc (h);
+  twcc_verify_fci (buf, exp_fci1);
+  gst_buffer_unref (buf);
+
+  session_harness_free (h);
+}
+
+GST_END_TEST;
+
 GST_START_TEST (test_twcc_recv_rtcp_reordered)
 {
   SessionHarness *send_h = session_harness_new ();
@@ -3894,6 +3965,7 @@ rtpsession_suite (void)
   tcase_add_test (tc_chain, test_twcc_delta_ts_rounding);
   tcase_add_test (tc_chain, test_twcc_double_gap);
   tcase_add_test (tc_chain, test_twcc_recv_packets_reordered);
+  tcase_add_test (tc_chain, test_twcc_recv_late_packet_fb_pkt_count_wrap);
   tcase_add_test (tc_chain, test_twcc_recv_rtcp_reordered);
   tcase_add_test (tc_chain, test_twcc_no_exthdr_in_buffer);
   tcase_add_test (tc_chain, test_twcc_send_and_recv);