From 6cacef240279981a195021334a9bf87d9f371780 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 24 Jul 2019 14:15:51 +0200 Subject: [PATCH] Reset packet history on ssrc/seqno reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the SSRC of an RTP module is changed at runtime, we may get conflicts with packets already there. Eg: * Put seq# 123 in the history for SSRC 1. * Change the SSRC to 2. * Send a NACK for seq# 123 from SSRC 2. Currently, we will respond with the packet belonging to SSRC 1 (and not if the NACK specifies SSRC 1, to boot). We can gen similar issues if the sequence number is changed, where half frame are left in the buffer. In these cases, the stream is likely being reset so we should just clear the packet history too. Bug: webrtc:10794 Change-Id: I28147c2532cf1c78840d4808c4366d4a647541f7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145729 Commit-Queue: Erik Språng Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/heads/master@{#28658} --- modules/rtp_rtcp/source/rtp_packet_history.cc | 5 ++ modules/rtp_rtcp/source/rtp_packet_history.h | 4 ++ modules/rtp_rtcp/source/rtp_sender.cc | 39 +++++++++---- .../rtp_rtcp/source/rtp_sender_unittest.cc | 57 +++++++++++++++++++ 4 files changed, 94 insertions(+), 11 deletions(-) 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},