Reset packet history on ssrc/seqno reset

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 <sprang@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28658}

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();

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;

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<uint32_t>& 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 {

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},