From 0ce7de7aa826daed81e2f23c298ac158ff596212 Mon Sep 17 00:00:00 2001 From: Per K Date: Thu, 2 May 2024 13:37:57 +0000 Subject: [PATCH] Remove RtpPacketHistory::PaddingMode::kPriority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit And cleanup WebRTC-PaddingMode-RecentLargePacket Bug: webrtc:42225520 Change-Id: If84588d9dbd5767c14174ae62a7f6d8284b8ef4a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/349621 Reviewed-by: Erik Språng Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#42327} --- experiments/field_trials.py | 3 - modules/rtp_rtcp/source/rtp_packet_history.cc | 60 ++------------ modules/rtp_rtcp/source/rtp_packet_history.h | 25 +----- .../source/rtp_packet_history_unittest.cc | 79 +------------------ modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 3 +- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 12 +-- .../source/rtp_sender_egress_unittest.cc | 3 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 2 +- 8 files changed, 16 insertions(+), 171 deletions(-) diff --git a/experiments/field_trials.py b/experiments/field_trials.py index 2c85ab38ea..aaf8b4708d 100755 --- a/experiments/field_trials.py +++ b/experiments/field_trials.py @@ -104,9 +104,6 @@ ACTIVE_FIELD_TRIALS: FrozenSet[FieldTrial] = frozenset([ FieldTrial('WebRTC-Pacer-KeyframeFlushing', 42221435, date(2024, 4, 1)), - FieldTrial('WebRTC-PaddingMode-RecentLargePacket', - 42225520, - date(2024, 4, 1)), FieldTrial('WebRTC-PermuteTlsClientHello', 42225803, date(2024, 7, 1)), diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 1e75e4787e..93bd4ab653 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -46,33 +46,8 @@ RtpPacketHistory::StoredPacket& RtpPacketHistory::StoredPacket::operator=( RtpPacketHistory::StoredPacket&&) = default; RtpPacketHistory::StoredPacket::~StoredPacket() = default; -void RtpPacketHistory::StoredPacket::IncrementTimesRetransmitted( - PacketPrioritySet* priority_set) { - // Check if this StoredPacket is in the priority set. If so, we need to remove - // it before updating `times_retransmitted_` since that is used in sorting, - // and then add it back. - const bool in_priority_set = priority_set && priority_set->erase(this) > 0; +void RtpPacketHistory::StoredPacket::IncrementTimesRetransmitted() { ++times_retransmitted_; - if (in_priority_set) { - auto it = priority_set->insert(this); - RTC_DCHECK(it.second) - << "ERROR: Priority set already contains matching packet! In set: " - "insert order = " - << (*it.first)->insert_order_ - << ", times retransmitted = " << (*it.first)->times_retransmitted_ - << ". Trying to add: insert order = " << insert_order_ - << ", times retransmitted = " << times_retransmitted_; - } -} - -bool RtpPacketHistory::MoreUseful::operator()(StoredPacket* lhs, - StoredPacket* rhs) const { - // Prefer to send packets we haven't already sent as padding. - if (lhs->times_retransmitted() != rhs->times_retransmitted()) { - return lhs->times_retransmitted() < rhs->times_retransmitted(); - } - // All else being equal, prefer newer packets. - return lhs->insert_order() > rhs->insert_order(); } RtpPacketHistory::RtpPacketHistory(Clock* clock, PaddingMode padding_mode) @@ -163,14 +138,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, packet_history_[packet_index] = StoredPacket(std::move(packet), send_time, packets_inserted_++); - - if (padding_priority_enabled()) { - if (padding_priority_.size() >= kMaxPaddingHistory - 1) { - padding_priority_.erase(std::prev(padding_priority_.end())); - } - auto prio_it = padding_priority_.insert(&packet_history_[packet_index]); - RTC_DCHECK(prio_it.second) << "Failed to insert packet into prio set."; - } } std::unique_ptr RtpPacketHistory::GetPacketAndMarkAsPending( @@ -230,8 +197,7 @@ void RtpPacketHistory::MarkPacketAsSent(uint16_t sequence_number) { // transmission count. packet->set_send_time(clock_->CurrentTime()); packet->pending_transmission_ = false; - packet->IncrementTimesRetransmitted( - padding_priority_enabled() ? &padding_priority_ : nullptr); + packet->IncrementTimesRetransmitted(); } bool RtpPacketHistory::GetPacketState(uint16_t sequence_number) const { @@ -290,11 +256,8 @@ std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket( } StoredPacket* best_packet = nullptr; - if (padding_priority_enabled() && !padding_priority_.empty()) { - auto best_packet_it = padding_priority_.begin(); - best_packet = *best_packet_it; - } else if (!padding_priority_enabled() && !packet_history_.empty()) { - // Prioritization not available, pick the last packet. + if (!packet_history_.empty()) { + // Pick the last packet. for (auto it = packet_history_.rbegin(); it != packet_history_.rend(); ++it) { if (it->packet_ != nullptr) { @@ -322,9 +285,7 @@ std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket( } best_packet->set_send_time(clock_->CurrentTime()); - best_packet->IncrementTimesRetransmitted( - padding_priority_enabled() ? &padding_priority_ : nullptr); - + best_packet->IncrementTimesRetransmitted(); return padding_packet; } @@ -348,7 +309,6 @@ void RtpPacketHistory::Clear() { void RtpPacketHistory::Reset() { packet_history_.clear(); - padding_priority_.clear(); large_payload_packet_ = absl::nullopt; } @@ -396,12 +356,6 @@ std::unique_ptr RtpPacketHistory::RemovePacket( // Move the packet out from the StoredPacket container. std::unique_ptr rtp_packet = std::move(packet_history_[packet_index].packet_); - - // Erase from padding priority set, if eligible. - if (padding_mode_ == PaddingMode::kPriority) { - padding_priority_.erase(&packet_history_[packet_index]); - } - if (packet_index == 0) { while (!packet_history_.empty() && packet_history_.front().packet_ == nullptr) { @@ -449,8 +403,4 @@ RtpPacketHistory::StoredPacket* RtpPacketHistory::GetStoredPacket( return &packet_history_[index]; } -bool RtpPacketHistory::padding_priority_enabled() const { - return padding_mode_ == PaddingMode::kPriority; -} - } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index 18310a8bd3..46db9d72fc 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -40,11 +40,8 @@ class RtpPacketHistory { }; enum class PaddingMode { - kDefault, // Last packet stored in the history that has not yet been - // culled. - kPriority, // Selects padding packets based on - // heuristics such as send time, retransmission count etc, in order to - // make padding potentially more useful. + kDefault, // Last packet stored in the history that has not yet been + // culled. kRecentLargePacket // Use the most recent large packet. Packet is kept for // padding even after it has been culled from history. }; @@ -59,10 +56,6 @@ class RtpPacketHistory { // With kStoreAndCull, always remove packets after 3x max(1000ms, 3x rtt). static constexpr int kPacketCullingDelayFactor = 3; - RtpPacketHistory(Clock* clock, bool enable_padding_prio) - : RtpPacketHistory(clock, - enable_padding_prio ? PaddingMode::kPriority - : PaddingMode::kDefault) {} RtpPacketHistory(Clock* clock, PaddingMode padding_mode); RtpPacketHistory() = delete; @@ -130,10 +123,6 @@ class RtpPacketHistory { void Clear(); private: - struct MoreUseful; - class StoredPacket; - using PacketPrioritySet = std::set; - class StoredPacket { public: StoredPacket() = default; @@ -146,7 +135,7 @@ class RtpPacketHistory { uint64_t insert_order() const { return insert_order_; } size_t times_retransmitted() const { return times_retransmitted_; } - void IncrementTimesRetransmitted(PacketPrioritySet* priority_set); + void IncrementTimesRetransmitted(); // The time of last transmission, including retransmissions. Timestamp send_time() const { return send_time_; } @@ -168,11 +157,6 @@ class RtpPacketHistory { // Number of times RE-transmitted, ie excluding the first transmission. size_t times_retransmitted_; }; - struct MoreUseful { - bool operator()(StoredPacket* lhs, StoredPacket* rhs) const; - }; - - bool padding_priority_enabled() const; // Helper method to check if packet has too recently been sent. bool VerifyRtt(const StoredPacket& packet) const @@ -205,9 +189,6 @@ class RtpPacketHistory { // Total number of packets with inserted. uint64_t packets_inserted_ RTC_GUARDED_BY(lock_); - // Objects from `packet_history_` ordered by "most likely to be useful", used - // in GetPayloadPaddingPacket(). - PacketPrioritySet padding_priority_ RTC_GUARDED_BY(lock_); absl::optional large_payload_packet_ RTC_GUARDED_BY(lock_); }; diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index 5019a72296..99d6b0628f 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -244,42 +244,6 @@ TEST_P(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); } -TEST_P(RtpPacketHistoryTest, RemovesLowestPrioPaddingWhenAtMaxCapacity) { - if (GetParam() != RtpPacketHistory::PaddingMode::kPriority) { - GTEST_SKIP() << "Padding prioritization required for this test"; - } - - // Tests the absolute upper bound on number of packets in the prioritized - // set of potential padding packets. - const size_t kMaxNumPackets = RtpPacketHistory::kMaxPaddingHistory; - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets * 2); - hist_.SetRtt(TimeDelta::Millis(1)); - - // Add packets until the max is reached, and then yet another one. - for (size_t i = 0; i < kMaxNumPackets + 1; ++i) { - std::unique_ptr packet = - CreateRtpPacket(To16u(kStartSeqNum + i)); - // Don't mark packets as sent, preventing them from being removed. - hist_.PutRtpPacket(std::move(packet), fake_clock_.CurrentTime()); - } - - // Advance time to allow retransmission/padding. - fake_clock_.AdvanceTimeMilliseconds(1); - - // The oldest packet will be least prioritized and has fallen out of the - // priority set. - for (size_t i = kMaxNumPackets - 1; i > 0; --i) { - auto packet = hist_.GetPayloadPaddingPacket(); - ASSERT_TRUE(packet); - EXPECT_EQ(packet->SequenceNumber(), To16u(kStartSeqNum + i + 1)); - } - - // Wrap around to newest padding packet again. - auto packet = hist_.GetPayloadPaddingPacket(); - ASSERT_TRUE(packet); - EXPECT_EQ(packet->SequenceNumber(), To16u(kStartSeqNum + kMaxNumPackets)); -} - TEST_P(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { // Set size to remove old packets as soon as possible. hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); @@ -530,46 +494,6 @@ TEST_P(RtpPacketHistoryTest, DontRemovePendingTransmissions) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } -TEST_P(RtpPacketHistoryTest, PrioritizedPayloadPadding) { - if (GetParam() != RtpPacketHistory::PaddingMode::kPriority) { - GTEST_SKIP() << "Padding prioritization required for this test"; - } - - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); - - // Add two sent packets, one millisecond apart. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.CurrentTime()); - fake_clock_.AdvanceTimeMilliseconds(1); - - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum + 1), - fake_clock_.CurrentTime()); - fake_clock_.AdvanceTimeMilliseconds(1); - - // Latest packet given equal retransmission count. - EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), - kStartSeqNum + 1); - - // Older packet has lower retransmission count. - EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum); - - // Equal retransmission count again, use newest packet. - EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), - kStartSeqNum + 1); - - // Older packet has lower retransmission count. - EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum); - - // Remove newest packet. - hist_.CullAcknowledgedPackets(std::vector{kStartSeqNum + 1}); - - // Only older packet left. - EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum); - - hist_.CullAcknowledgedPackets(std::vector{kStartSeqNum}); - - EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr); -} - TEST_P(RtpPacketHistoryTest, NoPendingPacketAsPadding) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); @@ -651,7 +575,7 @@ TEST_P(RtpPacketHistoryTest, OutOfOrderInsertRemoval) { } } -TEST_P(RtpPacketHistoryTest, UsesLastPacketAsPaddingWithPrioOff) { +TEST_P(RtpPacketHistoryTest, UsesLastPacketAsPaddingWithDefaultMode) { if (GetParam() != RtpPacketHistory::PaddingMode::kDefault) { GTEST_SKIP() << "Default padding prioritization required for this test"; } @@ -697,7 +621,6 @@ INSTANTIATE_TEST_SUITE_P( WithAndWithoutPaddingPrio, RtpPacketHistoryTest, ::testing::Values(RtpPacketHistory::PaddingMode::kDefault, - RtpPacketHistory::PaddingMode::kPriority, RtpPacketHistory::PaddingMode::kRecentLargePacket)); TEST(RtpPacketHistoryRecentLargePacketMode, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 20862e3c45..182f284062 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -44,7 +44,8 @@ constexpr TimeDelta kDefaultExpectedRetransmissionTime = TimeDelta::Millis(125); ModuleRtpRtcpImpl::RtpSenderContext::RtpSenderContext( const RtpRtcpInterface::Configuration& config) - : packet_history(config.clock, RtpPacketHistory::PaddingMode::kPriority), + : packet_history(config.clock, + RtpPacketHistory::PaddingMode::kRecentLargePacket), sequencer_(config.local_media_ssrc, config.rtx_send_ssrc, /*require_marker_before_media_padding=*/!config.audio, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index 7f0fdb8947..19de899d85 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -51,21 +51,13 @@ RTCPSender::Configuration AddRtcpSendEvaluationCallback( return config; } -RtpPacketHistory::PaddingMode GetPaddingMode( - const FieldTrialsView* field_trials) { - if (!field_trials || - !field_trials->IsDisabled("WebRTC-PaddingMode-RecentLargePacket")) { - return RtpPacketHistory::PaddingMode::kRecentLargePacket; - } - return RtpPacketHistory::PaddingMode::kPriority; -} - } // namespace ModuleRtpRtcpImpl2::RtpSenderContext::RtpSenderContext( TaskQueueBase& worker_queue, const RtpRtcpInterface::Configuration& config) - : packet_history(config.clock, GetPaddingMode(config.field_trials)), + : packet_history(config.clock, + RtpPacketHistory::PaddingMode::kRecentLargePacket), sequencer(config.local_media_ssrc, config.rtx_send_ssrc, /*require_marker_before_media_padding=*/!config.audio, diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index be79601acd..b833b65ed3 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -117,7 +117,8 @@ class RtpSenderEgressTest : public ::testing::Test { : time_controller_(kStartTime), clock_(time_controller_.GetClock()), transport_(&header_extensions_), - packet_history_(clock_, /*enable_rtx_padding_prioritization=*/true), + packet_history_(clock_, + RtpPacketHistory::PaddingMode::kRecentLargePacket), trials_(""), sequence_number_(kStartSequenceNumber) {} diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 269d003deb..3028a88df0 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -156,7 +156,7 @@ class RtpSenderTest : public ::testing::Test { void CreateSender(const RtpRtcpInterface::Configuration& config) { packet_history_ = std::make_unique( - config.clock, RtpPacketHistory::PaddingMode::kPriority); + config.clock, RtpPacketHistory::PaddingMode::kRecentLargePacket); sequencer_.emplace(kSsrc, kRtxSsrc, /*require_marker_before_media_padding=*/!config.audio, clock_);