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}

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)),

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(
+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;
   ++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<RtpPacketToSend> 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<RtpPacketToSend> 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(
+  packet->IncrementTimesRetransmitted();
-      padding_priority_enabled() ? &padding_priority_ : nullptr);
 }
 
 bool RtpPacketHistory::GetPacketState(uint16_t sequence_number) const {
@@ -290,11 +256,8 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket(
   }
 
   StoredPacket* best_packet = nullptr;
-  if (padding_priority_enabled() && !padding_priority_.empty()) {
+  if (!packet_history_.empty()) {
-    auto best_packet_it = padding_priority_.begin();
+    // Pick the last packet.
-    best_packet = *best_packet_it;
-  } else if (!padding_priority_enabled() && !packet_history_.empty()) {
-    // Prioritization not available, 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<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket(
   }
 
   best_packet->set_send_time(clock_->CurrentTime());
-  best_packet->IncrementTimesRetransmitted(
+  best_packet->IncrementTimesRetransmitted();
-      padding_priority_enabled() ? &padding_priority_ : nullptr);
-
   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<RtpPacketToSend> RtpPacketHistory::RemovePacket(
   // 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 @@ RtpPacketHistory::StoredPacket* RtpPacketHistory::GetStoredPacket(
   return &packet_history_[index];
 }
 
-bool RtpPacketHistory::padding_priority_enabled() const {
-  return padding_mode_ == PaddingMode::kPriority;
-}
-
 }  // namespace webrtc

modules/rtp_rtcp/source/rtp_packet_history.h

@@ -42,9 +42,6 @@ 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.
     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<StoredPacket*, MoreUseful>;
-
   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<RtpPacketToSend> large_payload_packet_ RTC_GUARDED_BY(lock_);
 };

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<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 @@ 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<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, 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,

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,

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,

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) {}
 

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<RtpPacketHistory>(
-        config.clock, RtpPacketHistory::PaddingMode::kPriority);
+        config.clock, RtpPacketHistory::PaddingMode::kRecentLargePacket);
     sequencer_.emplace(kSsrc, kRtxSsrc,
                        /*require_marker_before_media_padding=*/!config.audio,
                        clock_);