From e9da5f27a4b1e028a9b92a4029022762476d4dc6 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Tue, 11 Sep 2018 15:25:56 +0200 Subject: [PATCH] Reland "Decrease complexity of RtpPacketHistory::GetBestFittingPacket."" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Erik Språng Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#24687} --- modules/rtp_rtcp/source/rtp_packet_history.cc | 43 ++++++--- modules/rtp_rtcp/source/rtp_packet_history.h | 1 + .../source/rtp_packet_history_unittest.cc | 89 +++++++++++++++++++ 3 files changed, 119 insertions(+), 14 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 62f1c824dc..e1374feb57 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/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& 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 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 RtpPacketHistory::GetPacketAndSetSendTime( @@ -186,28 +189,34 @@ std::unique_ptr 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::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(*best_packet); } void RtpPacketHistory::Reset() { packet_history_.clear(); + packet_size_.clear(); start_seqno_.reset(); } @@ -272,6 +281,12 @@ std::unique_ptr 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; } diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index 433da0e7ee..1646ba7c76 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -136,6 +136,7 @@ class RtpPacketHistory { // Map from rtp sequence numbers to stored packet. std::map packet_history_ RTC_GUARDED_BY(lock_); + std::map 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. diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index ab5aeb0be6..85e9eb28cd 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/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 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 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 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 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