Remove RtpPacketHistory::PaddingMode::kPriority
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 <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42327}
diff --git a/experiments/field_trials.py b/experiments/field_trials.py
index 2c85ab3..aaf8b47 100755
--- a/experiments/field_trials.py
+++ b/experiments/field_trials.py
@@ -104,9 +104,6 @@
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 1e75e47..93bd4ab 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&&) = 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 @@
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<RtpPacketToSend> RtpPacketHistory::GetPacketAndMarkAsPending(
@@ -230,8 +197,7 @@
// 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 @@
}
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 @@
}
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::Reset() {
packet_history_.clear();
- padding_priority_.clear();
large_payload_packet_ = absl::nullopt;
}
@@ -396,12 +356,6 @@
// Move the packet out from the StoredPacket container.
std::unique_ptr<RtpPacketToSend> 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 @@
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 18310a8..46db9d7 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history.h
+++ b/modules/rtp_rtcp/source/rtp_packet_history.h
@@ -40,11 +40,8 @@
};
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 @@
// 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 @@
void Clear();
private:
- struct MoreUseful;
- class StoredPacket;
- using PacketPrioritySet = std::set<StoredPacket*, MoreUseful>;
-
class StoredPacket {
public:
StoredPacket() = default;
@@ -146,7 +135,7 @@
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 @@
// 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 @@
// 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<RtpPacketToSend> 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 5019a72..99d6b06 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 @@
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<RtpPacketToSend> 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 @@
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<uint16_t>{kStartSeqNum + 1});
-
- // Only older packet left.
- EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum);
-
- hist_.CullAcknowledgedPackets(std::vector<uint16_t>{kStartSeqNum});
-
- EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr);
-}
-
TEST_P(RtpPacketHistoryTest, NoPendingPacketAsPadding) {
hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1);
@@ -651,7 +575,7 @@
}
}
-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 @@
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 20862e3..182f284 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -44,7 +44,8 @@
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 7f0fdb8..19de899 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
@@ -51,21 +51,13 @@
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 be79601..b833b65 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 @@
: 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 269d003..3028a88 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -156,7 +156,7 @@
void CreateSender(const RtpRtcpInterface::Configuration& config) {
packet_history_ = std::make_unique<RtpPacketHistory>(
- config.clock, RtpPacketHistory::PaddingMode::kPriority);
+ config.clock, RtpPacketHistory::PaddingMode::kRecentLargePacket);
sequencer_.emplace(kSsrc, kRtxSsrc,
/*require_marker_before_media_padding=*/!config.audio,
clock_);
