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}
diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc
index c8d400a..b3d9b8f 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history.cc
+++ b/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 @@
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 @@
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 @@
// 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 @@
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 @@
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::Reset() {
packet_history_.clear();
padding_priority_.clear();
+ large_payload_packet_ = std::nullopt;
}
void RtpPacketHistory::CullOldPackets() {
@@ -374,7 +399,7 @@
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 @@
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 7475a35..c278c15 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history.h
+++ b/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 @@
// 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 @@
// 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 @@
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 @@
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 @@
// 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_
diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
index f505418..5019a72 100644
--- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
+++ b/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 @@
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 @@
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, 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, 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 @@
// If packet is pending retransmission, don't try to use it as padding.
hist_.GetPacketAndMarkAsPending(kStartSeqNum);
- EXPECT_EQ(nullptr, hist_.GetPayloadPaddingPacket());
+ 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, 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 @@
EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr);
}
-INSTANTIATE_TEST_SUITE_P(WithAndWithoutPaddingPrio,
- RtpPacketHistoryTest,
- ::testing::Bool());
+INSTANTIATE_TEST_SUITE_P(
+ WithAndWithoutPaddingPrio,
+ RtpPacketHistoryTest,
+ ::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
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index a43f788..6c40264 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -44,7 +44,7 @@
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,
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
index c22479f..546de63 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc
+++ b/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 @@
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,
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h
index 73c6c43..2184f33 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h
@@ -132,13 +132,6 @@
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;
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 68e0d4a..61dde0f 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -147,7 +147,7 @@
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_);
