diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 253e9862be..d63d8032df 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -362,6 +362,11 @@ bool RtpPacketHistory::SetPendingTransmission(uint16_t sequence_number) { return true; } +void RtpPacketHistory::Clear() { + rtc::CritScope cs(&lock_); + Reset(); +} + void RtpPacketHistory::Reset() { packet_history_.clear(); padding_priority_.clear(); diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index e477b45593..54c774e663 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -129,6 +129,10 @@ class RtpPacketHistory { // Returns true if status was set, false if packet was not found. bool SetPendingTransmission(uint16_t sequence_number); + // Remove all pending packets from the history, but keep storage mode and + // capacity. + void Clear(); + private: struct MoreUseful; class StoredPacket; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 870cc2a869..69066b8342 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -1417,16 +1417,21 @@ uint32_t RTPSender::TimestampOffset() const { } void RTPSender::SetSSRC(uint32_t ssrc) { - // This is configured via the API. - rtc::CritScope lock(&send_critsect_); + { + rtc::CritScope lock(&send_critsect_); + if (ssrc_ == ssrc) { + return; // Since it's the same SSRC, don't reset anything. + } - if (ssrc_ == ssrc) { - return; // Since it's same ssrc, don't reset anything. - } - ssrc_.emplace(ssrc); - if (!sequence_number_forced_) { - sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber); + ssrc_.emplace(ssrc); + if (!sequence_number_forced_) { + sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber); + } } + + // Clear RTP packet history, since any packets there belong to the old SSRC + // and they may conflict with packets from the new one. + packet_history_.Clear(); } uint32_t RTPSender::SSRC() const { @@ -1459,9 +1464,21 @@ void RTPSender::SetCsrcs(const std::vector& csrcs) { } void RTPSender::SetSequenceNumber(uint16_t seq) { - rtc::CritScope lock(&send_critsect_); - sequence_number_forced_ = true; - sequence_number_ = seq; + bool updated_sequence_number = false; + { + rtc::CritScope lock(&send_critsect_); + sequence_number_forced_ = true; + if (sequence_number_ != seq) { + updated_sequence_number = true; + } + sequence_number_ = seq; + } + + if (updated_sequence_number) { + // Sequence number series has been reset to a new value, clear RTP packet + // history, since any packets there may conflict with new ones. + packet_history_.Clear(); + } } uint16_t RTPSender::SequenceNumber() const { diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index ec12780756..a93a1a6415 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -2708,6 +2708,63 @@ TEST_P(RtpSenderTest, SetsCaptureTimeAndPopulatesTransmissionOffset) { EXPECT_EQ(*transmission_time_extension, 2 * kOffsetMs * kTimestampTicksPerMs); } +TEST_P(RtpSenderTestWithoutPacer, ClearHistoryOnSsrcChange) { + const int64_t kRtt = 10; + + rtp_sender_->SetSendingMediaStatus(true); + rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); + rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); + rtp_sender_->SetStorePacketsStatus(true, 10); + rtp_sender_->SetRtt(kRtt); + rtp_sender_->SetSSRC(kSsrc); + + // Send a packet and record its sequence numbers. + SendGenericPacket(); + ASSERT_EQ(1u, transport_.sent_packets_.size()); + const uint16_t packet_seqence_number = + transport_.sent_packets_.back().SequenceNumber(); + + // Advance time and make sure it can be retransmitted, even if we try to set + // the ssrc the what it already is. + rtp_sender_->SetSSRC(kSsrc); + fake_clock_.AdvanceTimeMilliseconds(kRtt); + EXPECT_GT(rtp_sender_->ReSendPacket(packet_seqence_number), 0); + + // Change the SSRC, then move the time and try to retransmit again. The old + // packet should now be gone. + rtp_sender_->SetSSRC(kSsrc + 1); + fake_clock_.AdvanceTimeMilliseconds(kRtt); + EXPECT_EQ(rtp_sender_->ReSendPacket(packet_seqence_number), 0); +} + +TEST_P(RtpSenderTestWithoutPacer, ClearHistoryOnSequenceNumberCange) { + const int64_t kRtt = 10; + + rtp_sender_->SetSendingMediaStatus(true); + rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); + rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); + rtp_sender_->SetStorePacketsStatus(true, 10); + rtp_sender_->SetRtt(kRtt); + + // Send a packet and record its sequence numbers. + SendGenericPacket(); + ASSERT_EQ(1u, transport_.sent_packets_.size()); + const uint16_t packet_seqence_number = + transport_.sent_packets_.back().SequenceNumber(); + + // Advance time and make sure it can be retransmitted, even if we try to set + // the ssrc the what it already is. + rtp_sender_->SetSequenceNumber(rtp_sender_->SequenceNumber()); + fake_clock_.AdvanceTimeMilliseconds(kRtt); + EXPECT_GT(rtp_sender_->ReSendPacket(packet_seqence_number), 0); + + // Change the sequence number, then move the time and try to retransmit again. + // The old packet should now be gone. + rtp_sender_->SetSequenceNumber(rtp_sender_->SequenceNumber() - 1); + fake_clock_.AdvanceTimeMilliseconds(kRtt); + EXPECT_EQ(rtp_sender_->ReSendPacket(packet_seqence_number), 0); +} + INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead, RtpSenderTest, ::testing::Values(TestConfig{false, false},