Add new padding mode to RtpPacketHistory

Instead of using most recent, or most "valuable" packets for padding, use most recent large packet.
The large packet for padding is not culled when acked by the receiver.
The most recent large packet is kept where  payload size + 100bytes > currently stored.

Bug: webrtc:15201
Change-Id: I510735b757f99460c477b575061963d2b69016e4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/306521
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Erik Språng <sprang@google.com>
Auto-Submit: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40146}

modules/rtp_rtcp/source/rtp_packet_history.cc

@@ -11,8 +11,10 @@
 #include "modules/rtp_rtcp/source/rtp_packet_history.h"
 
 #include <algorithm>
+#include <cstdint>
 #include <limits>
 #include <memory>
+#include <optional>
 #include <utility>
 
 #include "modules/include/module_common_types_public.h"
@@ -23,6 +25,13 @@
 
 namespace webrtc {
 
+namespace {
+
+constexpr size_t kOldPayloadPaddingSizeHysteresis = 100;
+constexpr uint16_t kMaxOldPayloadPaddingSequenceNumber = 1 << 13;
+
+}  // namespace
+
 RtpPacketHistory::StoredPacket::StoredPacket(
     std::unique_ptr<RtpPacketToSend> packet,
     Timestamp send_time,
@@ -67,9 +76,9 @@ bool RtpPacketHistory::MoreUseful::operator()(StoredPacket* lhs,
   return lhs->insert_order() > rhs->insert_order();
 }
 
-RtpPacketHistory::RtpPacketHistory(Clock* clock, bool enable_padding_prio)
+RtpPacketHistory::RtpPacketHistory(Clock* clock, PaddingMode padding_mode)
     : clock_(clock),
-      enable_padding_prio_(enable_padding_prio),
+      padding_mode_(padding_mode),
       number_to_store_(0),
       mode_(StorageMode::kDisabled),
       rtt_(TimeDelta::MinusInfinity()),
@@ -142,10 +151,21 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr<RtpPacketToSend> packet,
   RTC_DCHECK_LT(packet_index, packet_history_.size());
   RTC_DCHECK(packet_history_[packet_index].packet_ == nullptr);
 
+  if (padding_mode_ == PaddingMode::kRecentLargePacket) {
+    if ((!large_payload_packet_ ||
+         packet->payload_size() + kOldPayloadPaddingSizeHysteresis >
+             large_payload_packet_->payload_size() ||
+         IsNewerSequenceNumber(packet->SequenceNumber(),
+                               large_payload_packet_->SequenceNumber() +
+                                   kMaxOldPayloadPaddingSequenceNumber))) {
+      large_payload_packet_.emplace(*packet);
+    }
+  }
+
   packet_history_[packet_index] =
       StoredPacket(std::move(packet), send_time, packets_inserted_++);
 
-  if (enable_padding_prio_) {
+  if (padding_priority_enabled()) {
     if (padding_priority_.size() >= kMaxPaddingHistory - 1) {
       padding_priority_.erase(std::prev(padding_priority_.end()));
     }
@@ -211,8 +231,8 @@ void RtpPacketHistory::MarkPacketAsSent(uint16_t sequence_number) {
   // transmission count.
   packet->set_send_time(clock_->CurrentTime());
   packet->pending_transmission_ = false;
-  packet->IncrementTimesRetransmitted(enable_padding_prio_ ? &padding_priority_
-                                                           : nullptr);
+  packet->IncrementTimesRetransmitted(
+      padding_priority_enabled() ? &padding_priority_ : nullptr);
 }
 
 bool RtpPacketHistory::GetPacketState(uint16_t sequence_number) const {
@@ -265,12 +285,16 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket(
   if (mode_ == StorageMode::kDisabled) {
     return nullptr;
   }
+  if (padding_mode_ == PaddingMode::kRecentLargePacket &&
+      large_payload_packet_) {
+    return encapsulate(*large_payload_packet_);
+  }
 
   StoredPacket* best_packet = nullptr;
-  if (enable_padding_prio_ && !padding_priority_.empty()) {
+  if (padding_priority_enabled() && !padding_priority_.empty()) {
     auto best_packet_it = padding_priority_.begin();
     best_packet = *best_packet_it;
-  } else if (!enable_padding_prio_ && !packet_history_.empty()) {
+  } 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) {
@@ -300,7 +324,7 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPayloadPaddingPacket(
 
   best_packet->set_send_time(clock_->CurrentTime());
   best_packet->IncrementTimesRetransmitted(
-      enable_padding_prio_ ? &padding_priority_ : nullptr);
+      padding_priority_enabled() ? &padding_priority_ : nullptr);
 
   return padding_packet;
 }
@@ -326,6 +350,7 @@ void RtpPacketHistory::Clear() {
 void RtpPacketHistory::Reset() {
   packet_history_.clear();
   padding_priority_.clear();
+  large_payload_packet_ = std::nullopt;
 }
 
 void RtpPacketHistory::CullOldPackets() {
@@ -374,7 +399,7 @@ std::unique_ptr<RtpPacketToSend> RtpPacketHistory::RemovePacket(
       std::move(packet_history_[packet_index].packet_);
 
   // Erase from padding priority set, if eligible.
-  if (enable_padding_prio_) {
+  if (padding_mode_ == PaddingMode::kPriority) {
     padding_priority_.erase(&packet_history_[packet_index]);
   }
 
@@ -425,4 +450,8 @@ 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

@@ -22,13 +22,13 @@
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
+#include "modules/rtp_rtcp/source/rtp_packet_to_send.h"
 #include "rtc_base/synchronization/mutex.h"
 #include "rtc_base/thread_annotations.h"
 
 namespace webrtc {
 
 class Clock;
-class RtpPacketToSend;
 
 class RtpPacketHistory {
  public:
@@ -38,6 +38,16 @@ class RtpPacketHistory {
                    // packets as they time out or as signaled as received.
   };
 
+  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.
+  };
+
   // Maximum number of packets we ever allow in the history.
   static constexpr size_t kMaxCapacity = 9600;
   // Maximum number of entries in prioritized queue of padding packets.
@@ -48,7 +58,11 @@ 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* clock, bool enable_padding_prio)
+      : RtpPacketHistory(clock,
+                         enable_padding_prio ? PaddingMode::kPriority
+                                             : PaddingMode::kDefault) {}
+  RtpPacketHistory(Clock* clock, PaddingMode padding_mode);
 
   RtpPacketHistory() = delete;
   RtpPacketHistory(const RtpPacketHistory&) = delete;
@@ -157,6 +171,8 @@ class RtpPacketHistory {
     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
       RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_);
@@ -172,7 +188,7 @@ class RtpPacketHistory {
       RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   Clock* const clock_;
-  const bool enable_padding_prio_;
+  const PaddingMode padding_mode_;
   mutable Mutex lock_;
   size_t number_to_store_ RTC_GUARDED_BY(lock_);
   StorageMode mode_ RTC_GUARDED_BY(lock_);
@@ -191,6 +207,8 @@ class RtpPacketHistory {
   // Objects from `packet_history_` ordered by "most likely to be useful", used
   // in GetPayloadPaddingPacket().
   PacketPrioritySet padding_priority_ RTC_GUARDED_BY(lock_);
+
+  std::optional<RtpPacketToSend> large_payload_packet_ RTC_GUARDED_BY(lock_);
 };
 }  // namespace webrtc
 #endif  // MODULES_RTP_RTCP_SOURCE_RTP_PACKET_HISTORY_H_

modules/rtp_rtcp/source/rtp_packet_history_unittest.cc

@@ -10,9 +10,13 @@
 
 #include "modules/rtp_rtcp/source/rtp_packet_history.h"
 
+#include <cstdint>
+#include <limits>
 #include <memory>
 #include <utility>
 
+#include "api/units/time_delta.h"
+#include "api/units/timestamp.h"
 #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"
@@ -28,11 +32,28 @@ constexpr uint16_t kStartSeqNum = 65534u;
 uint16_t To16u(size_t sequence_number) {
   return static_cast<uint16_t>(sequence_number & 0xFFFF);
 }
-}  // namespace
 
 using StorageMode = RtpPacketHistory::StorageMode;
 
-class RtpPacketHistoryTest : public ::testing::TestWithParam<bool> {
+using ::testing::AllOf;
+using ::testing::Pointee;
+using ::testing::Property;
+
+std::unique_ptr<RtpPacketToSend> CreatePacket(
+    uint16_t seq_num,
+    Timestamp capture_time = Timestamp::Zero()) {
+  // Payload, ssrc, timestamp and extensions are irrelevant for this tests.
+  std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(nullptr));
+  packet->SetSequenceNumber(seq_num);
+  packet->set_capture_time(capture_time);
+  packet->set_allow_retransmission(true);
+  return packet;
+}
+
+}  // namespace
+
+class RtpPacketHistoryTest
+    : public ::testing::TestWithParam<RtpPacketHistory::PaddingMode> {
  protected:
   RtpPacketHistoryTest()
       : fake_clock_(123456),
@@ -42,12 +63,7 @@ class RtpPacketHistoryTest : public ::testing::TestWithParam<bool> {
   RtpPacketHistory hist_;
 
   std::unique_ptr<RtpPacketToSend> CreateRtpPacket(uint16_t seq_num) {
-    // Payload, ssrc, timestamp and extensions are irrelevant for this tests.
-    std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(nullptr));
-    packet->SetSequenceNumber(seq_num);
-    packet->set_capture_time(fake_clock_.CurrentTime());
-    packet->set_allow_retransmission(true);
-    return packet;
+    return CreatePacket(seq_num, fake_clock_.CurrentTime());
   }
 };
 
@@ -229,9 +245,8 @@ TEST_P(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) {
 }
 
 TEST_P(RtpPacketHistoryTest, RemovesLowestPrioPaddingWhenAtMaxCapacity) {
-  if (!GetParam()) {
-    // Padding prioritization is off, ignore this test.
-    return;
+  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
@@ -516,9 +531,8 @@ TEST_P(RtpPacketHistoryTest, DontRemovePendingTransmissions) {
 }
 
 TEST_P(RtpPacketHistoryTest, PrioritizedPayloadPadding) {
-  if (!GetParam()) {
-    // Padding prioritization is off, ignore this test.
-    return;
+  if (GetParam() != RtpPacketHistory::PaddingMode::kPriority) {
+    GTEST_SKIP() << "Padding prioritization required for this test";
   }
 
   hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1);
@@ -566,7 +580,12 @@ TEST_P(RtpPacketHistoryTest, NoPendingPacketAsPadding) {
 
   // If packet is pending retransmission, don't try to use it as padding.
   hist_.GetPacketAndMarkAsPending(kStartSeqNum);
+  if (GetParam() != RtpPacketHistory::PaddingMode::kRecentLargePacket) {
     EXPECT_EQ(nullptr, hist_.GetPayloadPaddingPacket());
+  } else {
+    // We do allow sending the same packet multiple times in this mode.
+    EXPECT_NE(nullptr, hist_.GetPayloadPaddingPacket());
+  }
 
   // Market it as no longer pending, should be usable as padding again.
   hist_.MarkPacketAsSent(kStartSeqNum);
@@ -633,9 +652,8 @@ TEST_P(RtpPacketHistoryTest, OutOfOrderInsertRemoval) {
 }
 
 TEST_P(RtpPacketHistoryTest, UsesLastPacketAsPaddingWithPrioOff) {
-  if (GetParam()) {
-    // Padding prioritization is enabled, ignore this test.
-    return;
+  if (GetParam() != RtpPacketHistory::PaddingMode::kDefault) {
+    GTEST_SKIP() << "Default padding prioritization required for this test";
   }
 
   const size_t kHistorySize = 10;
@@ -675,7 +693,100 @@ TEST_P(RtpPacketHistoryTest, UsesLastPacketAsPaddingWithPrioOff) {
   EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr);
 }
 
-INSTANTIATE_TEST_SUITE_P(WithAndWithoutPaddingPrio,
+INSTANTIATE_TEST_SUITE_P(
+    WithAndWithoutPaddingPrio,
     RtpPacketHistoryTest,
-                         ::testing::Bool());
+    ::testing::Values(RtpPacketHistory::PaddingMode::kDefault,
+                      RtpPacketHistory::PaddingMode::kPriority,
+                      RtpPacketHistory::PaddingMode::kRecentLargePacket));
+
+TEST(RtpPacketHistoryRecentLargePacketMode,
+     GetPayloadPaddingPacketAfterCullWithAcksReturnOldPacket) {
+  SimulatedClock fake_clock(1234);
+  RtpPacketHistory history(&fake_clock,
+                           RtpPacketHistory::PaddingMode::kRecentLargePacket);
+
+  history.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
+  std::unique_ptr<RtpPacketToSend> packet = CreatePacket(kStartSeqNum);
+  packet->SetPayloadSize(1000);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+  fake_clock.AdvanceTimeMilliseconds(33);
+  history.CullAcknowledgedPackets(std::vector<uint16_t>{kStartSeqNum});
+
+  EXPECT_THAT(
+      history.GetPayloadPaddingPacket(),
+      Pointee(AllOf(Property(&RtpPacketToSend::SequenceNumber, kStartSeqNum),
+                    (Property(&RtpPacketToSend::payload_size, 1000)))));
+}
+
+TEST(RtpPacketHistoryRecentLargePacketMode,
+     GetPayloadPaddingPacketIgnoreSmallRecentPackets) {
+  SimulatedClock fake_clock(1234);
+  RtpPacketHistory history(&fake_clock,
+                           RtpPacketHistory::PaddingMode::kRecentLargePacket);
+  history.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
+  std::unique_ptr<RtpPacketToSend> packet = CreatePacket(kStartSeqNum);
+  packet->SetPayloadSize(1000);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+  packet = CreatePacket(kStartSeqNum + 1);
+  packet->SetPayloadSize(100);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+
+  EXPECT_THAT(
+      history.GetPayloadPaddingPacket(),
+      Pointee(AllOf(Property(&RtpPacketToSend::SequenceNumber, kStartSeqNum),
+                    Property(&RtpPacketToSend::payload_size, 1000))));
+}
+
+TEST(RtpPacketHistoryRecentLargePacketMode,
+     GetPayloadPaddingPacketReturnsRecentPacketIfSizeNearMax) {
+  SimulatedClock fake_clock(1234);
+  RtpPacketHistory history(&fake_clock,
+                           RtpPacketHistory::PaddingMode::kRecentLargePacket);
+  history.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
+  std::unique_ptr<RtpPacketToSend> packet = CreatePacket(kStartSeqNum);
+  packet->SetPayloadSize(1000);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+  packet = CreatePacket(kStartSeqNum + 1);
+  packet->SetPayloadSize(950);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+
+  EXPECT_THAT(history.GetPayloadPaddingPacket(),
+              (Pointee(AllOf(
+                  Property(&RtpPacketToSend::SequenceNumber, kStartSeqNum + 1),
+                  Property(&RtpPacketToSend::payload_size, 950)))));
+}
+
+TEST(RtpPacketHistoryRecentLargePacketMode,
+     GetPayloadPaddingPacketReturnsLastPacketAfterLargeSequenceNumberGap) {
+  SimulatedClock fake_clock(1234);
+  RtpPacketHistory history(&fake_clock,
+                           RtpPacketHistory::PaddingMode::kRecentLargePacket);
+  history.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10);
+  uint16_t sequence_number = std::numeric_limits<uint16_t>::max() - 50;
+  std::unique_ptr<RtpPacketToSend> packet = CreatePacket(sequence_number);
+  packet->SetPayloadSize(1000);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+  ASSERT_THAT(
+      history.GetPayloadPaddingPacket(),
+      Pointee(Property(&RtpPacketToSend::SequenceNumber, sequence_number)));
+
+  // A long time pass... and potentially many small packets are injected, or
+  // timestamp jumps.
+  sequence_number = 1 << 13;
+  packet = CreatePacket(sequence_number);
+  packet->SetPayloadSize(100);
+  history.PutRtpPacket(std::move(packet),
+                       /*send_time=*/fake_clock.CurrentTime());
+  EXPECT_THAT(
+      history.GetPayloadPaddingPacket(),
+      Pointee(Property(&RtpPacketToSend::SequenceNumber, sequence_number)));
+}
+
 }  // namespace webrtc

modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -44,7 +44,7 @@ const int64_t kDefaultExpectedRetransmissionTimeMs = 125;
 
 ModuleRtpRtcpImpl::RtpSenderContext::RtpSenderContext(
     const RtpRtcpInterface::Configuration& config)
-    : packet_history(config.clock, config.enable_rtx_padding_prioritization),
+    : packet_history(config.clock, RtpPacketHistory::PaddingMode::kPriority),
       sequencer_(config.local_media_ssrc,
                  config.rtx_send_ssrc,
                  /*require_marker_before_media_padding=*/!config.audio,

modules/rtp_rtcp/source/rtp_rtcp_impl2.cc

@@ -25,6 +25,7 @@
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/dlrr.h"
+#include "modules/rtp_rtcp/source/rtp_packet_history.h"
 #include "modules/rtp_rtcp/source/rtp_rtcp_config.h"
 #include "modules/rtp_rtcp/source/time_util.h"
 #include "rtc_base/checks.h"
@@ -51,11 +52,20 @@ RTCPSender::Configuration AddRtcpSendEvaluationCallback(
   return config;
 }
 
+RtpPacketHistory::PaddingMode GetPaddingMode(
+    const FieldTrialsView* field_trials) {
+  if (field_trials &&
+      field_trials->IsEnabled("WebRTC-PaddingMode-RecentLargePacket")) {
+    return RtpPacketHistory::PaddingMode::kRecentLargePacket;
+  }
+  return RtpPacketHistory::PaddingMode::kPriority;
+}
+
 }  // namespace
 
 ModuleRtpRtcpImpl2::RtpSenderContext::RtpSenderContext(
     const RtpRtcpInterface::Configuration& config)
-    : packet_history(config.clock, config.enable_rtx_padding_prioritization),
+    : packet_history(config.clock, GetPaddingMode(config.field_trials)),
       sequencer(config.local_media_ssrc,
                 config.rtx_send_ssrc,
                 /*require_marker_before_media_padding=*/!config.audio,

modules/rtp_rtcp/source/rtp_rtcp_interface.h

@@ -132,13 +132,6 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface {
 
     bool need_rtp_packet_infos = false;
 
-    // If true, the RTP packet history will select RTX packets based on
-    // heuristics such as send time, retransmission count etc, in order to
-    // make padding potentially more useful.
-    // If false, the last packet will always be picked. This may reduce CPU
-    // overhead.
-    bool enable_rtx_padding_prioritization = true;
-
     // Estimate RTT as non-sender as described in
     // https://tools.ietf.org/html/rfc3611#section-4.4 and #section-4.5
     bool non_sender_rtt_measurement = false;

modules/rtp_rtcp/source/rtp_sender_unittest.cc

@@ -147,7 +147,7 @@ class RtpSenderTest : public ::testing::Test {
 
   void CreateSender(const RtpRtcpInterface::Configuration& config) {
     packet_history_ = std::make_unique<RtpPacketHistory>(
-        config.clock, config.enable_rtx_padding_prioritization);
+        config.clock, RtpPacketHistory::PaddingMode::kPriority);
     sequencer_.emplace(kSsrc, kRtxSsrc,
                        /*require_marker_before_media_padding=*/!config.audio,
                        clock_);