rtptwcc: fix seqnum-wrap

Using the proper API to do this is obviously an improvement, and
adding a test for the case of a packet-loss when the seqnum wrap
is also a good idea.

Co-authored-by: Tulio Beloqui <tulio.beloqui@pexip.com>
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/927>

2 files changed, 50 insertions, 3 deletions

diff --git a/gst/rtpmanager/rtptwcc.c b/gst/rtpmanager/rtptwcc.c
index 515c69b23..ad0628abc 100644
--- a/gst/rtpmanager/rtptwcc.c
+++ b/gst/rtpmanager/rtptwcc.c
@@ -596,7 +596,7 @@ rtp_twcc_manager_recv_packet (RTPTWCCManager * twcc,
 {
   gboolean send_feedback = FALSE;
   RecvPacket packet;
-  gint32 diff;
+  gint diff;
 
   /* if this packet would exceed the capacity of our MTU, we create a feedback
      with the current packets, and start over with this one */
@@ -614,8 +614,8 @@ rtp_twcc_manager_recv_packet (RTPTWCCManager * twcc,
   /* check if we are reordered, and treat it as lost if we already sent
      a feedback msg with a higher seqnum. If the diff is huge, treat
      it as a restart of a stream */
-  diff = (gint32) seqnum - (gint32) twcc->expected_recv_seqnum;
-  if (twcc->fb_pkt_count > 0 && diff < 0 && diff > -1000) {
+  diff = gst_rtp_buffer_compare_seqnum (twcc->expected_recv_seqnum, seqnum);
+  if (twcc->fb_pkt_count > 0 && diff < 0) {
     GST_INFO ("Received out of order packet (%u after %u), treating as lost",
         seqnum, twcc->expected_recv_seqnum);
     return FALSE;
diff --git a/tests/check/elements/rtpsession.c b/tests/check/elements/rtpsession.c
index c8eb5bdbc..d18b55bf0 100644
--- a/tests/check/elements/rtpsession.c
+++ b/tests/check/elements/rtpsession.c
@@ -3027,6 +3027,52 @@ GST_START_TEST (test_twcc_seqnum_wrap)
 GST_END_TEST;
 
 
+GST_START_TEST (test_twcc_seqnum_wrap_with_loss)
+{
+  SessionHarness *h = session_harness_new ();
+  GstBuffer *buf;
+
+  TWCCPacket packets[] = {
+    {65534, 0 * 250 * GST_USECOND, TRUE},
+    {1, 3 * 250 * GST_USECOND, TRUE},
+  };
+
+  guint8 exp_fci0[] = {
+    0xff, 0xfe,                 /* base sequence number: 65534 */
+    0x00, 0x01,                 /* packet status count: 2 */
+    0x00, 0x00, 0x00,           /* reference time: 0 */
+    0x00,                       /* feedback packet count: 0 */
+    0x20, 0x01,                 /* packet chunk */
+    0x00,                       /* recv delta: +0:00:00.000000000 */
+    0x00,                       /* padding */
+  };
+
+  guint8 exp_fci1[] = {
+    0x00, 0x01,                 /* base sequence number: 1 */
+    0x00, 0x01,                 /* packet status count: 1 */
+    0x00, 0x00, 0x00,           /* reference time: 0 */
+    0x01,                       /* feedback packet count: 1 */
+    0x20, 0x01,                 /* packet chunk */
+    0x03,                       /* recv delta: +0:00:00.000750000 */
+    0x00,                       /* padding */
+  };
+
+  twcc_push_packets (h, packets);
+
+  buf = session_harness_produce_twcc (h);
+  twcc_verify_fci (buf, exp_fci0);
+  gst_buffer_unref (buf);
+
+  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_double_packets)
 {
   SessionHarness *h = session_harness_new ();
@@ -3956,6 +4002,7 @@ rtpsession_suite (void)
   tcase_add_loop_test (tc_chain, test_twcc_various_gaps, 0, 50);
   tcase_add_test (tc_chain, test_twcc_negative_delta);
   tcase_add_test (tc_chain, test_twcc_seqnum_wrap);
+  tcase_add_test (tc_chain, test_twcc_seqnum_wrap_with_loss);
   tcase_add_test (tc_chain, test_twcc_huge_seqnum_gap);
   tcase_add_test (tc_chain, test_twcc_double_packets);
   tcase_add_test (tc_chain, test_twcc_duplicate_seqnums);