commit c62e1b8d10137cc0919451d0eb6b3d7b930de698
author Erik Språng <sprang@webrtc.org> Sat Jun 11 10:18:47 2022
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Jun 13 10:01:26 2022
tree b9c0cd252c5de7f867351188df913c8cd5b25391
parent 72b5dbc44b0d01ea189aa888218d7a6b99b4da81

Don't increment transport sequence number on send failures.

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

diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc
index 4254a63..a09f191 100644
--- a/modules/pacing/packet_router.cc
+++ b/modules/pacing/packet_router.cc
@@ -143,8 +143,11 @@
   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 @@
     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.

modules/pacing/packet_router_unittest.cc

diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc
index 77fe5f9..fc26922 100644
--- a/modules/pacing/packet_router_unittest.cc
+++ b/modules/pacing/packet_router_unittest.cc
@@ -399,6 +399,43 @@
   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) {