diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 8bcdfb93fc..0dd322916e 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -113,10 +113,12 @@ void RtpPacketHistory::SetRtt(int64_t rtt_ms) { rtc::CritScope cs(&lock_); RTC_DCHECK_GE(rtt_ms, 0); rtt_ms_ = rtt_ms; - // If kStoreAndCull mode is used, packets will be removed after a timeout + // If storage is not disabled, packets will be removed after a timeout // that depends on the RTT. Changing the RTT may thus cause some packets // become "old" and subject to removal. - CullOldPackets(clock_->TimeInMilliseconds()); + if (mode_ != StorageMode::kDisabled) { + CullOldPackets(clock_->TimeInMilliseconds()); + } } void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, @@ -336,12 +338,14 @@ std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket( void RtpPacketHistory::CullAcknowledgedPackets( rtc::ArrayView sequence_numbers) { rtc::CritScope cs(&lock_); - if (mode_ == StorageMode::kStoreAndCull) { - for (uint16_t sequence_number : sequence_numbers) { - auto stored_packet_it = packet_history_.find(sequence_number); - if (stored_packet_it != packet_history_.end()) { - RemovePacket(stored_packet_it); - } + if (mode_ == StorageMode::kDisabled) { + return; + } + + for (uint16_t sequence_number : sequence_numbers) { + auto stored_packet_it = packet_history_.find(sequence_number); + if (stored_packet_it != packet_history_.end()) { + RemovePacket(stored_packet_it); } } } @@ -393,10 +397,9 @@ void RtpPacketHistory::CullOldPackets(int64_t now_ms) { } if (packet_history_.size() >= number_to_store_ || - (mode_ == StorageMode::kStoreAndCull && - *stored_packet.send_time_ms_ + - (packet_duration_ms * kPacketCullingDelayFactor) <= - now_ms)) { + *stored_packet.send_time_ms_ + + (packet_duration_ms * kPacketCullingDelayFactor) <= + now_ms) { // Too many packets in history, or this packet has timed out. Remove it // and continue. RemovePacket(stored_packet_it); diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index ca4ab3ddc6..e477b45593 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -31,7 +31,6 @@ class RtpPacketHistory { public: enum class StorageMode { kDisabled, // Don't store any packets. - kStore, // Store and keep at least |number_to_store| packets. kStoreAndCull // Store up to |number_to_store| packets, but try to remove // packets as they time out or as signaled as received. }; diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index b801ae83f3..25c1f868f7 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -51,8 +51,8 @@ class RtpPacketHistoryTest : public ::testing::Test { TEST_F(RtpPacketHistoryTest, SetStoreStatus) { EXPECT_EQ(StorageMode::kDisabled, hist_.GetStorageMode()); - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); - EXPECT_EQ(StorageMode::kStore, hist_.GetStorageMode()); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); + EXPECT_EQ(StorageMode::kStoreAndCull, hist_.GetStorageMode()); hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); EXPECT_EQ(StorageMode::kStoreAndCull, hist_.GetStorageMode()); hist_.SetStorePacketsStatus(StorageMode::kDisabled, 0); @@ -60,14 +60,14 @@ TEST_F(RtpPacketHistoryTest, SetStoreStatus) { } TEST_F(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); // Store a packet, but with send-time. It should then not be removed. hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, absl::nullopt); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Changing store status, even to the current one, will clear the history. - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } @@ -109,12 +109,12 @@ TEST_F(RtpPacketHistoryTest, NoStoreStatus) { } TEST_F(RtpPacketHistoryTest, GetRtpPacket_NotStored) { - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); EXPECT_FALSE(hist_.GetPacketState(0)); } TEST_F(RtpPacketHistoryTest, PutRtpPacket) { - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); @@ -123,7 +123,7 @@ TEST_F(RtpPacketHistoryTest, PutRtpPacket) { } TEST_F(RtpPacketHistoryTest, GetRtpPacket) { - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); int64_t capture_time_ms = 1; std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); packet->set_capture_time_ms(capture_time_ms); @@ -138,7 +138,7 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) { } TEST_F(RtpPacketHistoryTest, NoCaptureTime) { - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); fake_clock_.AdvanceTimeMilliseconds(1); int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); @@ -154,7 +154,7 @@ TEST_F(RtpPacketHistoryTest, NoCaptureTime) { } TEST_F(RtpPacketHistoryTest, DontRetransmit) { - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); rtc::CopyOnWriteBuffer buffer = packet->Buffer(); @@ -207,7 +207,7 @@ TEST_F(RtpPacketHistoryTest, PacketStateIsCorrect) { TEST_F(RtpPacketHistoryTest, MinResendTimeWithPacer) { static const int64_t kMinRetransmitIntervalMs = 100; - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); hist_.SetRtt(kMinRetransmitIntervalMs); int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); @@ -248,7 +248,7 @@ TEST_F(RtpPacketHistoryTest, MinResendTimeWithPacer) { TEST_F(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { static const int64_t kMinRetransmitIntervalMs = 100; - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); hist_.SetRtt(kMinRetransmitIntervalMs); int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); @@ -274,7 +274,7 @@ TEST_F(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { TEST_F(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { const size_t kMaxNumPackets = 10; - hist_.SetStorePacketsStatus(StorageMode::kStore, kMaxNumPackets); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); // History does not allow removing packets within kMinPacketDurationMs, // so in order to test capacity, make sure insertion spans this time. @@ -309,7 +309,7 @@ TEST_F(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; - hist_.SetStorePacketsStatus(StorageMode::kStore, + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, RtpPacketHistory::kMaxCapacity); // Add packets until the buffer is full. @@ -336,7 +336,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { const size_t kMaxNumPackets = 10; - hist_.SetStorePacketsStatus(StorageMode::kStore, kMaxNumPackets); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); // Add packets until the buffer is full. for (size_t i = 0; i < kMaxNumPackets; ++i) { @@ -370,7 +370,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { // Set size to remove old packets as soon as possible. - hist_.SetStorePacketsStatus(StorageMode::kStore, 1); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); // Add a packet, marked as send, and advance time to just before removal time. hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, @@ -399,7 +399,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPacketsHighRtt) { kRttMs * RtpPacketHistory::kMinPacketDurationRtt; // Set size to remove old packets as soon as possible. - hist_.SetStorePacketsStatus(StorageMode::kStore, 1); + hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); hist_.SetRtt(kRttMs); // Add a packet, marked as send, and advance time to just before removal time. diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index b762e60e88..66d6b71984 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -198,9 +198,6 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) populate_network2_timestamp_(config.populate_network2_timestamp), send_side_bwe_with_overhead_( IsEnabled("WebRTC-SendSideBwe-WithOverhead", config.field_trials)), - legacy_packet_history_storage_mode_( - IsEnabled("WebRTC-UseRtpPacketHistoryLegacyStorageMode", - config.field_trials)), pacer_legacy_packet_referencing_( !IsDisabled("WebRTC-Pacer-LegacyPacketReferencing", config.field_trials)) { @@ -213,13 +210,9 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) // Store FlexFEC packets in the packet history data structure, so they can // be found when paced. if (flexfec_ssrc_) { - RtpPacketHistory::StorageMode storage_mode = - legacy_packet_history_storage_mode_ - ? RtpPacketHistory::StorageMode::kStore - : RtpPacketHistory::StorageMode::kStoreAndCull; - flexfec_packet_history_.SetStorePacketsStatus( - storage_mode, kMinFlexfecPacketsToStoreForPacing); + RtpPacketHistory::StorageMode::kStoreAndCull, + kMinFlexfecPacketsToStoreForPacing); } } @@ -287,9 +280,6 @@ RTPSender::RTPSender( send_side_bwe_with_overhead_( field_trials.Lookup("WebRTC-SendSideBwe-WithOverhead") .find("Enabled") == 0), - legacy_packet_history_storage_mode_( - field_trials.Lookup("WebRTC-UseRtpPacketHistoryLegacyStorageMode") - .find("Enabled") == 0), pacer_legacy_packet_referencing_( field_trials.Lookup("WebRTC-Pacer-LegacyPacketReferencing") .find("Disabled") != 0) { @@ -302,13 +292,9 @@ RTPSender::RTPSender( // Store FlexFEC packets in the packet history data structure, so they can // be found when paced. if (flexfec_ssrc_) { - RtpPacketHistory::StorageMode storage_mode = - legacy_packet_history_storage_mode_ - ? RtpPacketHistory::StorageMode::kStore - : RtpPacketHistory::StorageMode::kStoreAndCull; - flexfec_packet_history_.SetStorePacketsStatus( - storage_mode, kMinFlexfecPacketsToStoreForPacing); + RtpPacketHistory::StorageMode::kStoreAndCull, + kMinFlexfecPacketsToStoreForPacing); } } @@ -576,15 +562,10 @@ size_t RTPSender::SendPadData(size_t bytes, } void RTPSender::SetStorePacketsStatus(bool enable, uint16_t number_to_store) { - RtpPacketHistory::StorageMode mode; - if (enable) { - mode = legacy_packet_history_storage_mode_ - ? RtpPacketHistory::StorageMode::kStore - : RtpPacketHistory::StorageMode::kStoreAndCull; - } else { - mode = RtpPacketHistory::StorageMode::kDisabled; - } - packet_history_.SetStorePacketsStatus(mode, number_to_store); + packet_history_.SetStorePacketsStatus( + enable ? RtpPacketHistory::StorageMode::kStoreAndCull + : RtpPacketHistory::StorageMode::kDisabled, + number_to_store); } bool RTPSender::StorePackets() const { diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 823710e89b..f79b71d22e 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -318,7 +318,6 @@ class RTPSender { const bool populate_network2_timestamp_; const bool send_side_bwe_with_overhead_; - const bool legacy_packet_history_storage_mode_; // If true, PacedSender should only reference packets as in legacy mode. // If false, PacedSender may have direct ownership of RtpPacketToSend objects.