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 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()) { - packet->SetExtension((++transport_seq_) & 0xFFFF); + bool assign_transport_sequence_number = + packet->HasExtension(); + if (assign_transport_sequence_number) { + packet->SetExtension((transport_seq_ + 1) & + 0xFFFF); } uint32_t ssrc = packet->Ssrc(); @@ -163,6 +166,12 @@ void PacketRouter::SendPacket(std::unique_ptr 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 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()); + EXPECT_CALL( + rtp, TrySendPacket( + Property(&RtpPacketToSend::GetExtension, + 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()); + + EXPECT_CALL( + rtp, TrySendPacket( + Property(&RtpPacketToSend::GetExtension, + 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) {