From 641d59b3379c4d5d74d454f4c96e99ce819a27ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Mon, 30 Mar 2020 10:01:29 +0200 Subject: [PATCH] Add ability to disable padding prioritization. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows trading off some potential media quality for CPU usage. Bug: webrtc:8975 Change-Id: I447a03f596e9e711ba5d7038fe71f27bd80bf795 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/172085 Commit-Queue: Erik Språng Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#30936} --- modules/rtp_rtcp/include/rtp_rtcp.h | 7 + modules/rtp_rtcp/source/rtp_packet_history.cc | 50 +++++-- modules/rtp_rtcp/source/rtp_packet_history.h | 3 +- .../source/rtp_packet_history_unittest.cc | 128 +++++++++++++----- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 2 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 2 +- 6 files changed, 141 insertions(+), 51 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 579b2dfd8e..095688a7d8 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -149,6 +149,13 @@ class RtpRtcp : public Module, 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 = false; + private: RTC_DISALLOW_COPY_AND_ASSIGN(Configuration); }; diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 6a2253cd64..58e971ff1d 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -56,7 +56,7 @@ void RtpPacketHistory::StoredPacket::IncrementTimesRetransmitted( // 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->erase(this) > 0; + const bool in_priority_set = priority_set && priority_set->erase(this) > 0; ++times_retransmitted_; if (in_priority_set) { auto it = priority_set->insert(this); @@ -80,8 +80,9 @@ bool RtpPacketHistory::MoreUseful::operator()(StoredPacket* lhs, return lhs->insert_order() > rhs->insert_order(); } -RtpPacketHistory::RtpPacketHistory(Clock* clock) +RtpPacketHistory::RtpPacketHistory(Clock* clock, bool enable_padding_prio) : clock_(clock), + enable_padding_prio_(enable_padding_prio), number_to_store_(0), mode_(StorageMode::kDisabled), rtt_ms_(-1), @@ -158,11 +159,13 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, packet_history_[packet_index] = StoredPacket(std::move(packet), send_time_ms, packets_inserted_++); - if (padding_priority_.size() >= kMaxPaddingtHistory - 1) { - padding_priority_.erase(std::prev(padding_priority_.end())); + if (enable_padding_prio_) { + if (padding_priority_.size() >= kMaxPaddingtHistory - 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."; } - 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 RtpPacketHistory::GetPacketAndSetSendTime( @@ -183,7 +186,8 @@ std::unique_ptr RtpPacketHistory::GetPacketAndSetSendTime( } if (packet->send_time_ms_) { - packet->IncrementTimesRetransmitted(&padding_priority_); + packet->IncrementTimesRetransmitted( + enable_padding_prio_ ? &padding_priority_ : nullptr); } // Update send-time and mark as no long in pacer queue. @@ -253,7 +257,8 @@ void RtpPacketHistory::MarkPacketAsSent(uint16_t sequence_number) { // transmission count. packet->send_time_ms_ = clock_->TimeInMilliseconds(); packet->pending_transmission_ = false; - packet->IncrementTimesRetransmitted(&padding_priority_); + packet->IncrementTimesRetransmitted(enable_padding_prio_ ? &padding_priority_ + : nullptr); } absl::optional RtpPacketHistory::GetPacketState( @@ -307,12 +312,28 @@ std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket( rtc::FunctionView(const RtpPacketToSend&)> encapsulate) { rtc::CritScope cs(&lock_); - if (mode_ == StorageMode::kDisabled || padding_priority_.empty()) { + if (mode_ == StorageMode::kDisabled) { + return nullptr; + } + + StoredPacket* best_packet = nullptr; + if (enable_padding_prio_ && !padding_priority_.empty()) { + auto best_packet_it = padding_priority_.begin(); + best_packet = *best_packet_it; + } else if (!enable_padding_prio_ && !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) { + best_packet = &(*it); + break; + } + } + } + if (best_packet == nullptr) { return nullptr; } - auto best_packet_it = padding_priority_.begin(); - StoredPacket* best_packet = *best_packet_it; if (best_packet->pending_transmission_) { // Because PacedSender releases it's lock when it calls // GeneratePadding() there is the potential for a race where a new @@ -328,7 +349,8 @@ std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket( } best_packet->send_time_ms_ = clock_->TimeInMilliseconds(); - best_packet->IncrementTimesRetransmitted(&padding_priority_); + best_packet->IncrementTimesRetransmitted( + enable_padding_prio_ ? &padding_priority_ : nullptr); return padding_packet; } @@ -414,7 +436,9 @@ std::unique_ptr RtpPacketHistory::RemovePacket( std::move(packet_history_[packet_index].packet_); // Erase from padding priority set, if eligible. - padding_priority_.erase(&packet_history_[packet_index]); + if (enable_padding_prio_) { + padding_priority_.erase(&packet_history_[packet_index]); + } if (packet_index == 0) { while (!packet_history_.empty() && diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index 9253ede4fa..db25b17a17 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -62,7 +62,7 @@ class RtpPacketHistory { // With kStoreAndCull, always remove packets after 3x max(1000ms, 3x rtt). static constexpr int kPacketCullingDelayFactor = 3; - explicit RtpPacketHistory(Clock* clock); + RtpPacketHistory(Clock* clock, bool enable_padding_prio); ~RtpPacketHistory(); // Set/get storage mode. Note that setting the state will clear the history, @@ -192,6 +192,7 @@ class RtpPacketHistory { const StoredPacket& stored_packet); Clock* const clock_; + const bool enable_padding_prio_; rtc::CriticalSection lock_; size_t number_to_store_ RTC_GUARDED_BY(lock_); StorageMode mode_ 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 fdf64d51bf..2331724397 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -32,9 +32,11 @@ uint16_t To16u(size_t sequence_number) { using StorageMode = RtpPacketHistory::StorageMode; -class RtpPacketHistoryTest : public ::testing::Test { +class RtpPacketHistoryTest : public ::testing::TestWithParam { protected: - RtpPacketHistoryTest() : fake_clock_(123456), hist_(&fake_clock_) {} + RtpPacketHistoryTest() + : fake_clock_(123456), + hist_(&fake_clock_, /*enable_padding_prio=*/GetParam()) {} SimulatedClock fake_clock_; RtpPacketHistory hist_; @@ -49,7 +51,7 @@ class RtpPacketHistoryTest : public ::testing::Test { } }; -TEST_F(RtpPacketHistoryTest, SetStoreStatus) { +TEST_P(RtpPacketHistoryTest, SetStoreStatus) { EXPECT_EQ(StorageMode::kDisabled, hist_.GetStorageMode()); hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); EXPECT_EQ(StorageMode::kStoreAndCull, hist_.GetStorageMode()); @@ -59,7 +61,7 @@ TEST_F(RtpPacketHistoryTest, SetStoreStatus) { EXPECT_EQ(StorageMode::kDisabled, hist_.GetStorageMode()); } -TEST_F(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { +TEST_P(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); // Store a packet, but with send-time. It should then not be removed. hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), absl::nullopt); @@ -70,7 +72,7 @@ TEST_F(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, StartSeqResetAfterReset) { +TEST_P(RtpPacketHistoryTest, StartSeqResetAfterReset) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); // Store a packet, but with send-time. It should then not be removed. hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), absl::nullopt); @@ -96,7 +98,7 @@ TEST_F(RtpPacketHistoryTest, StartSeqResetAfterReset) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); } -TEST_F(RtpPacketHistoryTest, NoStoreStatus) { +TEST_P(RtpPacketHistoryTest, NoStoreStatus) { EXPECT_EQ(StorageMode::kDisabled, hist_.GetStorageMode()); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); hist_.PutRtpPacket(std::move(packet), absl::nullopt); @@ -104,12 +106,12 @@ TEST_F(RtpPacketHistoryTest, NoStoreStatus) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, GetRtpPacket_NotStored) { +TEST_P(RtpPacketHistoryTest, GetRtpPacket_NotStored) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); EXPECT_FALSE(hist_.GetPacketState(0)); } -TEST_F(RtpPacketHistoryTest, PutRtpPacket) { +TEST_P(RtpPacketHistoryTest, PutRtpPacket) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); @@ -118,7 +120,7 @@ TEST_F(RtpPacketHistoryTest, PutRtpPacket) { EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, GetRtpPacket) { +TEST_P(RtpPacketHistoryTest, GetRtpPacket) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); int64_t capture_time_ms = 1; std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); @@ -133,7 +135,7 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) { EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms()); } -TEST_F(RtpPacketHistoryTest, PacketStateIsCorrect) { +TEST_P(RtpPacketHistoryTest, PacketStateIsCorrect) { const uint32_t kSsrc = 92384762; const int64_t kRttMs = 100; hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); @@ -164,7 +166,7 @@ TEST_F(RtpPacketHistoryTest, PacketStateIsCorrect) { EXPECT_EQ(state->times_retransmitted, 1u); } -TEST_F(RtpPacketHistoryTest, MinResendTimeWithPacer) { +TEST_P(RtpPacketHistoryTest, MinResendTimeWithPacer) { static const int64_t kMinRetransmitIntervalMs = 100; hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); @@ -205,7 +207,7 @@ TEST_F(RtpPacketHistoryTest, MinResendTimeWithPacer) { EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { +TEST_P(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { static const int64_t kMinRetransmitIntervalMs = 100; hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); @@ -231,7 +233,7 @@ TEST_F(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { +TEST_P(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { const size_t kMaxNumPackets = 10; hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); @@ -262,7 +264,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); } -TEST_F(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { +TEST_P(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { // Tests the absolute upper bound on number of stored packets. Don't allow // storing more than this, even if packets have not yet been sent. const size_t kMaxNumPackets = RtpPacketHistory::kMaxCapacity; @@ -290,7 +292,12 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); } -TEST_F(RtpPacketHistoryTest, RemovesLowestPrioPaddingWhenAtMaxCapacity) { +TEST_P(RtpPacketHistoryTest, RemovesLowestPrioPaddingWhenAtMaxCapacity) { + if (!GetParam()) { + // Padding prioritization is off, ignore this test. + return; + } + // Tests the absolute upper bound on number of packets in the prioritized // set of potential padding packets. const size_t kMaxNumPackets = RtpPacketHistory::kMaxPaddingtHistory; @@ -322,7 +329,7 @@ TEST_F(RtpPacketHistoryTest, RemovesLowestPrioPaddingWhenAtMaxCapacity) { EXPECT_EQ(packet->SequenceNumber(), To16u(kStartSeqNum + kMaxNumPackets)); } -TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { +TEST_P(RtpPacketHistoryTest, DontRemoveUnsentPackets) { const size_t kMaxNumPackets = 10; hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); @@ -355,7 +362,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); } -TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { +TEST_P(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { // Set size to remove old packets as soon as possible. hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); @@ -380,7 +387,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); } -TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPacketsHighRtt) { +TEST_P(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPacketsHighRtt) { const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; const int64_t kPacketTimeoutMs = kRttMs * RtpPacketHistory::kMinPacketDurationRtt; @@ -409,7 +416,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPacketsHighRtt) { EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); } -TEST_F(RtpPacketHistoryTest, RemovesOldWithCulling) { +TEST_P(RtpPacketHistoryTest, RemovesOldWithCulling) { const size_t kMaxNumPackets = 10; // Enable culling. Even without feedback, this can trigger early removal. hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); @@ -432,7 +439,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCulling) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) { +TEST_P(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) { const size_t kMaxNumPackets = 10; const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; // Enable culling. Even without feedback, this can trigger early removal. @@ -458,7 +465,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, CullWithAcks) { +TEST_P(RtpPacketHistoryTest, CullWithAcks) { const int64_t kPacketLifetime = RtpPacketHistory::kMinPacketDurationMs * RtpPacketHistory::kPacketCullingDelayFactor; @@ -511,7 +518,7 @@ TEST_F(RtpPacketHistoryTest, CullWithAcks) { EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 2)).has_value()); } -TEST_F(RtpPacketHistoryTest, SetsPendingTransmissionState) { +TEST_P(RtpPacketHistoryTest, SetsPendingTransmissionState) { const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; hist_.SetRtt(kRttMs); @@ -553,7 +560,7 @@ TEST_F(RtpPacketHistoryTest, SetsPendingTransmissionState) { EXPECT_FALSE(packet_state->pending_transmission); } -TEST_F(RtpPacketHistoryTest, GetPacketAndSetSent) { +TEST_P(RtpPacketHistoryTest, GetPacketAndSetSent) { const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; hist_.SetRtt(kRttMs); @@ -580,7 +587,7 @@ TEST_F(RtpPacketHistoryTest, GetPacketAndSetSent) { EXPECT_TRUE(hist_.GetPacketAndMarkAsPending(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulation) { +TEST_P(RtpPacketHistoryTest, GetPacketWithEncapsulation) { const uint32_t kSsrc = 92384762; const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; hist_.SetRtt(kRttMs); @@ -607,7 +614,7 @@ TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulation) { EXPECT_EQ(retransmit_packet->Ssrc(), kSsrc + 1); } -TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulationAbortOnNullptr) { +TEST_P(RtpPacketHistoryTest, GetPacketWithEncapsulationAbortOnNullptr) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), @@ -617,14 +624,14 @@ TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulationAbortOnNullptr) { // not suitable for retransmission (bandwidth exhausted?) so the retransmit is // aborted and the packet is not marked as pending. EXPECT_FALSE(hist_.GetPacketAndMarkAsPending( - kStartSeqNum, [](const RtpPacketToSend& packet) { return nullptr; })); + kStartSeqNum, [](const RtpPacketToSend&) { return nullptr; })); // New try, this time getting the packet should work, and it should not be // blocked due to any pending status. EXPECT_TRUE(hist_.GetPacketAndMarkAsPending(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, DontRemovePendingTransmissions) { +TEST_P(RtpPacketHistoryTest, DontRemovePendingTransmissions) { const int64_t kRttMs = RtpPacketHistory::kMinPacketDurationMs * 2; const int64_t kPacketTimeoutMs = kRttMs * RtpPacketHistory::kMinPacketDurationRtt; @@ -657,7 +664,12 @@ TEST_F(RtpPacketHistoryTest, DontRemovePendingTransmissions) { ASSERT_FALSE(packet_state.has_value()); } -TEST_F(RtpPacketHistoryTest, PrioritizedPayloadPadding) { +TEST_P(RtpPacketHistoryTest, PrioritizedPayloadPadding) { + if (!GetParam()) { + // Padding prioritization is off, ignore this test. + return; + } + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); // Add two sent packets, one millisecond apart. @@ -694,7 +706,7 @@ TEST_F(RtpPacketHistoryTest, PrioritizedPayloadPadding) { EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr); } -TEST_F(RtpPacketHistoryTest, NoPendingPacketAsPadding) { +TEST_P(RtpPacketHistoryTest, NoPendingPacketAsPadding) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), @@ -712,7 +724,7 @@ TEST_F(RtpPacketHistoryTest, NoPendingPacketAsPadding) { EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), kStartSeqNum); } -TEST_F(RtpPacketHistoryTest, PayloadPaddingWithEncapsulation) { +TEST_P(RtpPacketHistoryTest, PayloadPaddingWithEncapsulation) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), @@ -720,9 +732,8 @@ TEST_F(RtpPacketHistoryTest, PayloadPaddingWithEncapsulation) { fake_clock_.AdvanceTimeMilliseconds(1); // Aborted padding. - EXPECT_EQ(nullptr, - hist_.GetPayloadPaddingPacket( - [](const RtpPacketToSend& packet) { return nullptr; })); + EXPECT_EQ(nullptr, hist_.GetPayloadPaddingPacket( + [](const RtpPacketToSend&) { return nullptr; })); // Get copy of packet, but with sequence number modified. auto padding_packet = @@ -735,7 +746,7 @@ TEST_F(RtpPacketHistoryTest, PayloadPaddingWithEncapsulation) { EXPECT_EQ(padding_packet->SequenceNumber(), kStartSeqNum + 1); } -TEST_F(RtpPacketHistoryTest, NackAfterAckIsNoop) { +TEST_P(RtpPacketHistoryTest, NackAfterAckIsNoop) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 2); // Add two sent packets. hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), @@ -749,7 +760,7 @@ TEST_F(RtpPacketHistoryTest, NackAfterAckIsNoop) { EXPECT_EQ(packet.get(), nullptr); } -TEST_F(RtpPacketHistoryTest, OutOfOrderInsertRemoval) { +TEST_P(RtpPacketHistoryTest, OutOfOrderInsertRemoval) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); // Insert packets, out of order, including both forwards and backwards @@ -780,4 +791,51 @@ TEST_F(RtpPacketHistoryTest, OutOfOrderInsertRemoval) { expected_time_offset_ms += 33; } } + +TEST_P(RtpPacketHistoryTest, UsesLastPacketAsPaddingWithPrioOff) { + if (GetParam()) { + // Padding prioritization is enabled, ignore this test. + return; + } + + const size_t kHistorySize = 10; + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kHistorySize); + + EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr); + + for (size_t i = 0; i < kHistorySize; ++i) { + hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + i)), + fake_clock_.TimeInMilliseconds()); + hist_.MarkPacketAsSent(To16u(kStartSeqNum + i)); + fake_clock_.AdvanceTimeMilliseconds(1); + + // Last packet always returned. + EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), + To16u(kStartSeqNum + i)); + EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), + To16u(kStartSeqNum + i)); + EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), + To16u(kStartSeqNum + i)); + } + + // Remove packets from the end, last in the list should be returned. + for (size_t i = kHistorySize - 1; i > 0; --i) { + hist_.CullAcknowledgedPackets( + std::vector{To16u(kStartSeqNum + i)}); + + EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), + To16u(kStartSeqNum + i - 1)); + EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), + To16u(kStartSeqNum + i - 1)); + EXPECT_EQ(hist_.GetPayloadPaddingPacket()->SequenceNumber(), + To16u(kStartSeqNum + i - 1)); + } + + hist_.CullAcknowledgedPackets(std::vector{kStartSeqNum}); + EXPECT_EQ(hist_.GetPayloadPaddingPacket(), nullptr); +} + +INSTANTIATE_TEST_SUITE_P(WithAndWithoutPaddingPrio, + RtpPacketHistoryTest, + ::testing::Bool()); } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 204bd8b2a3..25fa545213 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -40,7 +40,7 @@ const int64_t kDefaultExpectedRetransmissionTimeMs = 125; ModuleRtpRtcpImpl::RtpSenderContext::RtpSenderContext( const RtpRtcp::Configuration& config) - : packet_history(config.clock), + : packet_history(config.clock, config.enable_rtx_padding_prioritization), packet_sender(config, &packet_history), non_paced_sender(&packet_sender), packet_generator( diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index d4a7fa9125..355312cfd4 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -223,7 +223,7 @@ class StreamDataTestCallback : public StreamDataCountersCallback { // wherever possible. struct RtpSenderContext { explicit RtpSenderContext(const RtpRtcp::Configuration& config) - : packet_history_(config.clock), + : packet_history_(config.clock, config.enable_rtx_padding_prioritization), packet_sender_(config, &packet_history_), non_paced_sender_(&packet_sender_), packet_generator_(