Reland "Decrease complexity of RtpPacketHistory::GetBestFittingPacket.""

This reverts commit 49b2c3c4c43359bc86d8510d29d117f3d7a621a3.

Original CL description:
Decrease complexity of RtpPacketHistory::GetBestFittingPacket.
Use a map of packet sizes in RtpPacketHistory instead of looping through the whole history for every call

patch set 1 contains the initial submit from https://webrtc-review.googlesource.com/c/src/+/98882
new patch sets contains the modification.

The problem with the initial submit was the assumption that packets are removed
from history in the same order as they are added which is not always true.

Bug: webrtc:9731
Change-Id: Ic2c8905a0f47287fc46e53f41a019a4c69c3dd8e
Reviewed-on: https://webrtc-review.googlesource.com/99460
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24687}

modules/rtp_rtcp/source/rtp_packet_history.cc

@@ -27,8 +27,7 @@ constexpr size_t kMinPacketRequestBytes = 50;
 
 // Utility function to get the absolute difference in size between the provided
 // target size and the size of packet.
-size_t SizeDiff(const std::unique_ptr<RtpPacketToSend>& packet, size_t size) {
-  size_t packet_size = packet->size();
+size_t SizeDiff(size_t packet_size, size_t size) {
   if (packet_size > size) {
     return packet_size - size;
   }
@@ -110,6 +109,10 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr<RtpPacketToSend> packet,
   if (!start_seqno_) {
     start_seqno_ = rtp_seq_no;
   }
+  // Store the sequence number of the last send packet with this size.
+  if (type != StorageType::kDontRetransmit) {
+    packet_size_[stored_packet.packet->size()] = rtp_seq_no;
+  }
 }
 
 std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPacketAndSetSendTime(
@@ -186,28 +189,34 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetBestFittingPacket(
     size_t packet_length) const {
   // TODO(sprang): Make this smarter, taking retransmit count etc into account.
   rtc::CritScope cs(&lock_);
-  if (packet_length < kMinPacketRequestBytes || packet_history_.empty()) {
+  if (packet_length < kMinPacketRequestBytes || packet_size_.empty()) {
     return nullptr;
   }
 
-  size_t min_diff = std::numeric_limits<size_t>::max();
-  RtpPacketToSend* best_packet = nullptr;
-  for (auto& it : packet_history_) {
-    size_t diff = SizeDiff(it.second.packet, packet_length);
-    if (!min_diff || diff < min_diff) {
-      min_diff = diff;
-      best_packet = it.second.packet.get();
-      if (diff == 0) {
-        break;
-      }
-    }
+  auto size_iter_upper = packet_size_.upper_bound(packet_length);
+  auto size_iter_lower = size_iter_upper;
+  if (size_iter_upper == packet_size_.end()) {
+    --size_iter_upper;
   }
+  if (size_iter_lower != packet_size_.begin()) {
+    --size_iter_lower;
+  }
+  const size_t upper_bound_diff =
+      SizeDiff(size_iter_upper->first, packet_length);
+  const size_t lower_bound_diff =
+      SizeDiff(size_iter_lower->first, packet_length);
 
+  const uint16_t seq_no = upper_bound_diff < lower_bound_diff
+                              ? size_iter_upper->second
+                              : size_iter_lower->second;
+  RtpPacketToSend* best_packet =
+      packet_history_.find(seq_no)->second.packet.get();
   return absl::make_unique<RtpPacketToSend>(*best_packet);
 }
 
 void RtpPacketHistory::Reset() {
   packet_history_.clear();
+  packet_size_.clear();
   start_seqno_.reset();
 }
 
@@ -272,6 +281,12 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::RemovePacket(
     start_seqno_.reset();
   }
 
+  auto size_iterator = packet_size_.find(rtp_packet->size());
+  if (size_iterator != packet_size_.end() &&
+      size_iterator->second == rtp_packet->SequenceNumber()) {
+    packet_size_.erase(size_iterator);
+  }
+
   return rtp_packet;
 }
 

modules/rtp_rtcp/source/rtp_packet_history.h

@@ -136,6 +136,7 @@ class RtpPacketHistory {
 
   // Map from rtp sequence numbers to stored packet.
   std::map<uint16_t, StoredPacket> packet_history_ RTC_GUARDED_BY(lock_);
+  std::map<size_t, uint16_t> packet_size_ RTC_GUARDED_BY(lock_);
 
   // The earliest packet in the history. This might not be the lowest sequence
   // number, in case there is a wraparound.

modules/rtp_rtcp/source/rtp_packet_history_unittest.cc

@@ -16,6 +16,7 @@
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "modules/rtp_rtcp/source/rtp_packet_to_send.h"
 #include "system_wrappers/include/clock.h"
+#include "test/gmock.h"
 #include "test/gtest.h"
 
 namespace webrtc {
@@ -488,4 +489,92 @@ TEST_F(RtpPacketHistoryTest, GetBestFittingPacket) {
   EXPECT_EQ(target_packet_size,
             hist_.GetBestFittingPacket(target_packet_size)->size());
 }
+
+TEST_F(RtpPacketHistoryTest,
+       GetBestFittingPacketReturnsNextPacketWhenBestPacketHasBeenCulled) {
+  hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
+  std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
+  packet->SetPayloadSize(50);
+  const size_t target_packet_size = packet->size();
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+
+  packet = hist_.GetBestFittingPacket(target_packet_size + 2);
+  ASSERT_THAT(packet, ::testing::NotNull());
+
+  // Send the packet and advance time past where packet expires.
+  ASSERT_THAT(hist_.GetPacketAndSetSendTime(kStartSeqNum, false),
+              ::testing::NotNull());
+  fake_clock_.AdvanceTimeMilliseconds(
+      RtpPacketHistory::kPacketCullingDelayFactor *
+      RtpPacketHistory::kMinPacketDurationMs);
+
+  packet = CreateRtpPacket(kStartSeqNum + 1);
+  packet->SetPayloadSize(100);
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+  ASSERT_FALSE(hist_.GetPacketState(kStartSeqNum, false));
+
+  auto best_packet = hist_.GetBestFittingPacket(target_packet_size + 2);
+  ASSERT_THAT(best_packet, ::testing::NotNull());
+  EXPECT_EQ(best_packet->SequenceNumber(), kStartSeqNum + 1);
+}
+
+TEST_F(RtpPacketHistoryTest, GetBestFittingPacketReturnLastPacketWhenSameSize) {
+  const size_t kTargetSize = 500;
+  hist_.SetStorePacketsStatus(StorageMode::kStore, 10);
+
+  // Add two packets of same size.
+  std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
+  packet->SetPayloadSize(kTargetSize);
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+  packet = CreateRtpPacket(kStartSeqNum + 1);
+  packet->SetPayloadSize(kTargetSize);
+  hist_.PutRtpPacket(std::move(packet), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+
+  auto best_packet = hist_.GetBestFittingPacket(123);
+  ASSERT_THAT(best_packet, ::testing::NotNull());
+  EXPECT_EQ(best_packet->SequenceNumber(), kStartSeqNum + 1);
+}
+
+TEST_F(RtpPacketHistoryTest,
+       GetBestFittingPacketReturnsPacketWithSmallestDiff) {
+  const size_t kTargetSize = 500;
+  hist_.SetStorePacketsStatus(StorageMode::kStore, 10);
+
+  // Add two packets of very different size.
+  std::unique_ptr<RtpPacketToSend> small_packet = CreateRtpPacket(kStartSeqNum);
+  small_packet->SetPayloadSize(kTargetSize);
+  hist_.PutRtpPacket(std::move(small_packet), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+
+  auto large_packet = CreateRtpPacket(kStartSeqNum + 1);
+  large_packet->SetPayloadSize(kTargetSize * 2);
+  hist_.PutRtpPacket(std::move(large_packet), kAllowRetransmission,
+                     fake_clock_.TimeInMilliseconds());
+
+  ASSERT_THAT(hist_.GetBestFittingPacket(kTargetSize), ::testing::NotNull());
+  EXPECT_EQ(hist_.GetBestFittingPacket(kTargetSize)->SequenceNumber(),
+            kStartSeqNum);
+
+  ASSERT_THAT(hist_.GetBestFittingPacket(kTargetSize * 2),
+              ::testing::NotNull());
+  EXPECT_EQ(hist_.GetBestFittingPacket(kTargetSize * 2)->SequenceNumber(),
+            kStartSeqNum + 1);
+}
+
+TEST_F(RtpPacketHistoryTest,
+       GetBestFittingPacketIgnoresNoneRetransmitablePackets) {
+  hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
+  std::unique_ptr<RtpPacketToSend> packet = CreateRtpPacket(kStartSeqNum);
+  packet->SetPayloadSize(50);
+  hist_.PutRtpPacket(std::move(packet), kDontRetransmit,
+                     fake_clock_.TimeInMilliseconds());
+  EXPECT_THAT(hist_.GetBestFittingPacket(50), ::testing::IsNull());
+  EXPECT_THAT(hist_.GetPacketAndSetSendTime(kStartSeqNum, false),
+              ::testing::NotNull());
+}
+
 }  // namespace webrtc