From 788ac70c1f8a34bf375b09dee69a4e7d547dc4f9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= <sprang@webrtc.org>
Date: Tue, 23 Jan 2018 08:10:22 -0800
Subject: [PATCH] Don't overwrite RTP packets in history within one second or
 3x RTT.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This prevents us from prematurely overwriting the packets in the history
if the RTT is underestimated.

Bug: webrtc:8766
Change-Id: I042e8ce74cdce2a0451596f4217779fc856b51f4
Reviewed-on: https://webrtc-review.googlesource.com/42960
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21735}
---
 modules/rtp_rtcp/source/rtp_packet_history.cc |  9 ++++-
 .../source/rtp_packet_history_unittest.cc     | 33 ++++++++++++++++---
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc
index 4374ca6e2d..887406ad04 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history.cc
+++ b/modules/rtp_rtcp/source/rtp_packet_history.cc
@@ -22,6 +22,10 @@
 namespace webrtc {
 namespace {
 constexpr size_t kMinPacketRequestBytes = 50;
+// Don't overwrite a packet within one second, or three RTTs, after transmission
+// whichever is larger. Instead try to dynamically expand history.
+constexpr int64_t kMinPacketDurationMs = 1000;
+constexpr int kMinPacketDurationRtt = 3;
 }  // namespace
 constexpr size_t RtpPacketHistory::kMaxCapacity;
 
@@ -85,9 +89,12 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr<RtpPacketToSend> packet,
   // less than 3 round trip times ago, expand the buffer to avoid overwriting
   // valid data.
   StoredPacket* stored_packet = &stored_packets_[prev_index_];
+  int64_t packet_duration_ms =
+      std::max(kMinPacketDurationRtt * rtt_ms_, kMinPacketDurationMs);
   if (stored_packet->packet &&
       (stored_packet->send_time == 0 ||
-       (rtt_ms_ >= 0 && now_ms - stored_packet->send_time <= 3 * rtt_ms_))) {
+       (rtt_ms_ >= 0 &&
+        now_ms - stored_packet->send_time <= packet_duration_ms))) {
     size_t current_size = stored_packets_.size();
     if (current_size < kMaxCapacity) {
       size_t expanded_size = std::max(current_size * 3 / 2, current_size + 1);
diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
index 54ba13721a..cdfd3a0a25 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
@@ -222,7 +222,7 @@ TEST_F(RtpPacketHistoryTest, FullExpansion) {
 
 TEST_F(RtpPacketHistoryTest, DontExpandIfPacketIsOldEnough) {
   const size_t kSendSidePacketHistorySize = 600;
-  const int64_t kRttMs = 100;
+  const int64_t kRttMs = 334;
   hist_.SetStorePacketsStatus(true, kSendSidePacketHistorySize);
   hist_.SetRtt(kRttMs);
 
@@ -241,12 +241,12 @@ TEST_F(RtpPacketHistoryTest, DontExpandIfPacketIsOldEnough) {
   std::unique_ptr<RtpPacketToSend> packet =
       CreateRtpPacket(kSeqNum + kSendSidePacketHistorySize);
   hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, true);
-  EXPECT_FALSE(hist_.GetPacketAndSetSendTime(kSeqNum, kRttMs, true));
+  EXPECT_FALSE(hist_.HasRtpPacket(kSeqNum));
 }
 
 TEST_F(RtpPacketHistoryTest, ExpandIfPacketTooRecentlyTransmitted) {
   const size_t kSendSidePacketHistorySize = 600;
-  const int64_t kRttMs = 100;
+  const int64_t kRttMs = 334;
   hist_.SetStorePacketsStatus(true, kSendSidePacketHistorySize);
   hist_.SetRtt(kRttMs);
 
@@ -265,6 +265,31 @@ TEST_F(RtpPacketHistoryTest, ExpandIfPacketTooRecentlyTransmitted) {
   std::unique_ptr<RtpPacketToSend> packet =
       CreateRtpPacket(kSeqNum + kSendSidePacketHistorySize);
   hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, true);
-  EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kSeqNum, kRttMs, true));
+  EXPECT_TRUE(hist_.HasRtpPacket(kSeqNum));
+}
+
+TEST_F(RtpPacketHistoryTest, ExpandIfPacketTooRecentlyTransmittedOnFastLink) {
+  const size_t kSendSidePacketHistorySize = 600;
+  const int64_t kRttMs = 5;
+  hist_.SetStorePacketsStatus(true, kSendSidePacketHistorySize);
+  hist_.SetRtt(kRttMs);
+
+  // Fill up the buffer with packets.
+  for (size_t i = 0; i < kSendSidePacketHistorySize; ++i) {
+    std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kSeqNum + i);
+    hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, false);
+    EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kSeqNum + i, kRttMs, false));
+  }
+
+  // Move clock forward after expiration time based on RTT, but before
+  // expiration time based on absolute time.
+  fake_clock_.AdvanceTimeMilliseconds(999);
+
+  // Insert a new packet and verify that the old one for this index still
+  // exists - ie the buffer has been expanded.
+  std::unique_ptr<RtpPacketToSend> packet =
+      CreateRtpPacket(kSeqNum + kSendSidePacketHistorySize);
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, true);
+  EXPECT_TRUE(hist_.HasRtpPacket(kSeqNum));
 }
 }  // namespace webrtc