From c62e1b8d10137cc0919451d0eb6b3d7b930de698 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= <sprang@webrtc.org>
Date: Sat, 11 Jun 2022 12:18:47 +0200
Subject: [PATCH] Don't increment transport sequence number on send failures.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Bug: webrtc:14130
Change-Id: Idee794445872f3db8ffae7c3e2cef5e72843ef25
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265640
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37190}
---
 modules/pacing/packet_router.cc          | 13 +++++++--
 modules/pacing/packet_router_unittest.cc | 37 ++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc
index 4254a636d3..a09f191bbd 100644
--- a/modules/pacing/packet_router.cc
+++ b/modules/pacing/packet_router.cc
@@ -143,8 +143,11 @@ void PacketRouter::SendPacket(std::unique_ptr<RtpPacketToSend> packet,
   MutexLock lock(&modules_mutex_);
   // With the new pacer code path, transport sequence numbers are only set here,
   // on the pacer thread. Therefore we don't need atomics/synchronization.
-  if (packet->HasExtension<TransportSequenceNumber>()) {
-    packet->SetExtension<TransportSequenceNumber>((++transport_seq_) & 0xFFFF);
+  bool assign_transport_sequence_number =
+      packet->HasExtension<TransportSequenceNumber>();
+  if (assign_transport_sequence_number) {
+    packet->SetExtension<TransportSequenceNumber>((transport_seq_ + 1) &
+                                                  0xFFFF);
   }
 
   uint32_t ssrc = packet->Ssrc();
@@ -163,6 +166,12 @@ void PacketRouter::SendPacket(std::unique_ptr<RtpPacketToSend> packet,
     return;
   }
 
+  // Sending succeeded.
+
+  if (assign_transport_sequence_number) {
+    ++transport_seq_;
+  }
+
   if (rtp_module->SupportsRtxPayloadPadding()) {
     // This is now the last module to send media, and has the desired
     // properties needed for payload based padding. Cache it for later use.
diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc
index 77fe5f9f8d..fc26922850 100644
--- a/modules/pacing/packet_router_unittest.cc
+++ b/modules/pacing/packet_router_unittest.cc
@@ -399,6 +399,43 @@ TEST_F(PacketRouterTest, SendPacketAssignsTransportSequenceNumbers) {
   packet_router_.RemoveSendRtpModule(&rtp_2);
 }
 
+TEST_F(PacketRouterTest, DoesNotIncrementTransportSequenceNumberOnSendFailure) {
+  NiceMock<MockRtpRtcpInterface> rtp;
+  constexpr uint32_t kSsrc = 1234;
+  ON_CALL(rtp, SSRC).WillByDefault(Return(kSsrc));
+  packet_router_.AddSendRtpModule(&rtp, false);
+
+  // Transport sequence numbers start at 1, for historical reasons.
+  const uint16_t kStartTransportSequenceNumber = 1;
+
+  // Build and send a packet - it should be assigned the start sequence number.
+  // Return failure status code to make sure sequence number is not incremented.
+  auto packet = BuildRtpPacket(kSsrc);
+  EXPECT_TRUE(packet->ReserveExtension<TransportSequenceNumber>());
+  EXPECT_CALL(
+      rtp, TrySendPacket(
+               Property(&RtpPacketToSend::GetExtension<TransportSequenceNumber>,
+                        kStartTransportSequenceNumber),
+               _))
+      .WillOnce(Return(false));
+  packet_router_.SendPacket(std::move(packet), PacedPacketInfo());
+
+  // Send another packet, verify transport sequence number is still at the
+  // start state.
+  packet = BuildRtpPacket(kSsrc);
+  EXPECT_TRUE(packet->ReserveExtension<TransportSequenceNumber>());
+
+  EXPECT_CALL(
+      rtp, TrySendPacket(
+               Property(&RtpPacketToSend::GetExtension<TransportSequenceNumber>,
+                        kStartTransportSequenceNumber),
+               _))
+      .WillOnce(Return(true));
+  packet_router_.SendPacket(std::move(packet), PacedPacketInfo());
+
+  packet_router_.RemoveSendRtpModule(&rtp);
+}
+
 #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
 using PacketRouterDeathTest = PacketRouterTest;
 TEST_F(PacketRouterDeathTest, DoubleRegistrationOfSendModuleDisallowed) {