diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index c8d400a985..b3d9b8fecd 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 +#include #include #include +#include #include #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 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 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 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 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 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 diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index 7475a35be3..c278c15e92 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 @@ 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 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 f50541849e..5019a72296 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 +#include #include #include +#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(sequence_number & 0xFFFF); } -} // namespace using StorageMode = RtpPacketHistory::StorageMode; -class RtpPacketHistoryTest : public ::testing::TestWithParam { +using ::testing::AllOf; +using ::testing::Pointee; +using ::testing::Property; + +std::unique_ptr CreatePacket( + uint16_t seq_num, + Timestamp capture_time = Timestamp::Zero()) { + // Payload, ssrc, timestamp and extensions are irrelevant for this tests. + std::unique_ptr 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 { protected: RtpPacketHistoryTest() : fake_clock_(123456), @@ -42,12 +63,7 @@ class RtpPacketHistoryTest : public ::testing::TestWithParam { RtpPacketHistory hist_; std::unique_ptr CreateRtpPacket(uint16_t seq_num) { - // Payload, ssrc, timestamp and extensions are irrelevant for this tests. - std::unique_ptr 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); - 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, 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, - 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 packet = CreatePacket(kStartSeqNum); + packet->SetPayloadSize(1000); + history.PutRtpPacket(std::move(packet), + /*send_time=*/fake_clock.CurrentTime()); + fake_clock.AdvanceTimeMilliseconds(33); + history.CullAcknowledgedPackets(std::vector{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 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 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::max() - 50; + std::unique_ptr 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 a43f788445..6c402640ea 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/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, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index c22479f3c9..546de63eec 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 @@ 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, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index 73c6c4329f..2184f33801 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/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; diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 68e0d4a6c2..61dde0fec6 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/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( - config.clock, config.enable_rtx_padding_prioritization); + config.clock, RtpPacketHistory::PaddingMode::kPriority); sequencer_.emplace(kSsrc, kRtxSsrc, /*require_marker_before_media_padding=*/!config.audio, clock_);