From 21f2fc9c73518392aaf30359b1eac5d18c576931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Tue, 16 Jul 2019 21:09:14 +0200 Subject: [PATCH] Remove the non-useful rtx payload padding option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL removes the field trial left in place as a kill-switch in case there were any regressions related to selecting payload padding based on the likelihood of being useful instead of matching size. It also removes the functionality that was only enabled with the kill-switch active. The feature has been default-on since June 23rd 2019: https://webrtc.googlesource.com/src.git/+/214f54365ec210db76218a35ead66c9ce23e068e Since we have not observed any issues, let's clean this code up. Bug: webrtc:8975 Change-Id: I7f49fe354227b3f6566a250332e56b6d70fe2f09 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145821 Commit-Queue: Erik Språng Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/heads/master@{#28616} --- modules/rtp_rtcp/source/rtp_packet_history.cc | 67 ----------- modules/rtp_rtcp/source/rtp_packet_history.h | 7 -- .../source/rtp_packet_history_unittest.cc | 110 ------------------ modules/rtp_rtcp/source/rtp_sender.cc | 14 +-- modules/rtp_rtcp/source/rtp_sender.h | 6 - .../rtp_rtcp/source/rtp_sender_unittest.cc | 105 ----------------- 6 files changed, 2 insertions(+), 307 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index 6f48b25444..8bcdfb93fc 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -21,16 +21,6 @@ #include "system_wrappers/include/clock.h" namespace webrtc { -namespace { -// Utility function to get the absolute difference in size between the provided -// target size and the size of packet. -size_t SizeDiff(size_t packet_size, size_t size) { - if (packet_size > size) { - return packet_size - size; - } - return size - packet_size; -} -} // namespace constexpr size_t RtpPacketHistory::kMaxCapacity; constexpr int64_t RtpPacketHistory::kMinPacketDurationMs; @@ -150,16 +140,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, : 0)); RTC_DCHECK(it.second) << "Failed to insert packet in history."; StoredPacket& stored_packet = it.first->second; - if (stored_packet.packet_) { - // It is an error if this happen. But it can happen if the sequence numbers - // for some reason restart without that the history has been reset. - auto size_iterator = packet_size_.find(stored_packet.packet_->size()); - if (size_iterator != packet_size_.end() && - size_iterator->second == stored_packet.packet_->SequenceNumber()) { - packet_size_.erase(size_iterator); - } - } - if (stored_packet.packet_->capture_time_ms() <= 0) { stored_packet.packet_->set_capture_time_ms(now_ms); } @@ -170,7 +150,6 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, // Store the sequence number of the last send packet with this size. if (type != StorageType::kDontRetransmit) { - packet_size_[stored_packet.packet_->size()] = rtp_seq_no; auto it = padding_priority_.insert(&stored_packet); RTC_DCHECK(it.second) << "Failed to insert packet into prio set."; } @@ -317,45 +296,6 @@ bool RtpPacketHistory::VerifyRtt(const RtpPacketHistory::StoredPacket& packet, return true; } -std::unique_ptr RtpPacketHistory::GetBestFittingPacket( - size_t packet_length) const { - rtc::CritScope cs(&lock_); - if (packet_size_.empty()) { - return nullptr; - } - - auto size_iter_upper = packet_size_.upper_bound(packet_length); - auto size_iter_lower = size_iter_upper; - if (size_iter_upper == packet_size_.end()) { - --size_iter_upper; - } - if (size_iter_lower != packet_size_.begin()) { - --size_iter_lower; - } - const size_t upper_bound_diff = - SizeDiff(size_iter_upper->first, packet_length); - const size_t lower_bound_diff = - SizeDiff(size_iter_lower->first, packet_length); - - const uint16_t seq_no = upper_bound_diff < lower_bound_diff - ? size_iter_upper->second - : size_iter_lower->second; - auto history_it = packet_history_.find(seq_no); - if (history_it == packet_history_.end()) { - RTC_LOG(LS_ERROR) << "Can't find packet in history with seq_no" << seq_no; - RTC_DCHECK(false); - return nullptr; - } - if (!history_it->second.packet_) { - RTC_LOG(LS_ERROR) << "Packet pointer is null in history for seq_no" - << seq_no; - RTC_DCHECK(false); - return nullptr; - } - RtpPacketToSend* best_packet = history_it->second.packet_.get(); - return absl::make_unique(*best_packet); -} - std::unique_ptr RtpPacketHistory::GetPayloadPaddingPacket() { // Default implementation always just returns a copy of the packet. return GetPayloadPaddingPacket([](const RtpPacketToSend& packet) { @@ -423,7 +363,6 @@ bool RtpPacketHistory::SetPendingTransmission(uint16_t sequence_number) { void RtpPacketHistory::Reset() { packet_history_.clear(); - packet_size_.clear(); padding_priority_.clear(); start_seqno_.reset(); } @@ -508,12 +447,6 @@ std::unique_ptr RtpPacketHistory::RemovePacket( } } - auto size_iterator = packet_size_.find(rtp_packet->size()); - if (size_iterator != packet_size_.end() && - size_iterator->second == rtp_packet->SequenceNumber()) { - packet_size_.erase(size_iterator); - } - return rtp_packet; } diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index a0d42827be..ca4ab3ddc6 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -108,11 +108,6 @@ class RtpPacketHistory { // current state for packet, and never updates internal state. absl::optional GetPacketState(uint16_t sequence_number) const; - // Get the packet (if any) from the history, with size closest to - // |packet_size|. The exact size of the packet is not guaranteed. - std::unique_ptr GetBestFittingPacket( - size_t packet_size) const; - // Get the packet (if any) from the history, that is deemed most likely to // the remote side. This is calculated from heuristics such as packet age // and times retransmitted. Updated the send time of the packet, so is not @@ -203,8 +198,6 @@ class RtpPacketHistory { // Map from rtp sequence numbers to stored packet. std::map packet_history_ RTC_GUARDED_BY(lock_); - // Map from packet size to sequence number. - std::map packet_size_ RTC_GUARDED_BY(lock_); // Total number of packets with StorageType::kAllowsRetransmission inserted. uint64_t retransmittable_packets_inserted_ 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 345ad7a6bc..b801ae83f3 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -471,116 +471,6 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } -TEST_F(RtpPacketHistoryTest, GetBestFittingPacket) { - const size_t kTargetSize = 500; - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); - - // Add three packets of various sizes. - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - packet->SetPayloadSize(kTargetSize); - const size_t target_packet_size = packet->size(); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - packet = CreateRtpPacket(To16u(kStartSeqNum + 1)); - packet->SetPayloadSize(kTargetSize - 1); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - packet = CreateRtpPacket(To16u(kStartSeqNum + 2)); - packet->SetPayloadSize(kTargetSize + 1); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - - EXPECT_EQ(target_packet_size, - hist_.GetBestFittingPacket(target_packet_size)->size()); -} - -TEST_F(RtpPacketHistoryTest, - GetBestFittingPacketReturnsNextPacketWhenBestPacketHasBeenCulled) { - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - packet->SetPayloadSize(50); - const size_t target_packet_size = packet->size(); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - - packet = hist_.GetBestFittingPacket(target_packet_size + 2); - ASSERT_THAT(packet, ::testing::NotNull()); - - // Send the packet and advance time past where packet expires. - ASSERT_THAT(hist_.GetPacketAndSetSendTime(kStartSeqNum), - ::testing::NotNull()); - fake_clock_.AdvanceTimeMilliseconds( - RtpPacketHistory::kPacketCullingDelayFactor * - RtpPacketHistory::kMinPacketDurationMs); - - packet = CreateRtpPacket(To16u(kStartSeqNum + 1)); - packet->SetPayloadSize(100); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - ASSERT_FALSE(hist_.GetPacketState(kStartSeqNum)); - - auto best_packet = hist_.GetBestFittingPacket(target_packet_size + 2); - ASSERT_THAT(best_packet, ::testing::NotNull()); - EXPECT_EQ(best_packet->SequenceNumber(), To16u(kStartSeqNum + 1)); -} - -TEST_F(RtpPacketHistoryTest, GetBestFittingPacketReturnLastPacketWhenSameSize) { - const size_t kTargetSize = 500; - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); - - // Add two packets of same size. - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - packet->SetPayloadSize(kTargetSize); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - packet = CreateRtpPacket(To16u(kStartSeqNum + 1)); - packet->SetPayloadSize(kTargetSize); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - - auto best_packet = hist_.GetBestFittingPacket(123); - ASSERT_THAT(best_packet, ::testing::NotNull()); - EXPECT_EQ(best_packet->SequenceNumber(), To16u(kStartSeqNum + 1)); -} - -TEST_F(RtpPacketHistoryTest, - GetBestFittingPacketReturnsPacketWithSmallestDiff) { - const size_t kTargetSize = 500; - hist_.SetStorePacketsStatus(StorageMode::kStore, 10); - - // Add two packets of very different size. - std::unique_ptr small_packet = CreateRtpPacket(kStartSeqNum); - small_packet->SetPayloadSize(kTargetSize); - hist_.PutRtpPacket(std::move(small_packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - - auto large_packet = CreateRtpPacket(To16u(kStartSeqNum + 1)); - large_packet->SetPayloadSize(kTargetSize * 2); - hist_.PutRtpPacket(std::move(large_packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); - - ASSERT_THAT(hist_.GetBestFittingPacket(kTargetSize), ::testing::NotNull()); - EXPECT_EQ(hist_.GetBestFittingPacket(kTargetSize)->SequenceNumber(), - kStartSeqNum); - - ASSERT_THAT(hist_.GetBestFittingPacket(kTargetSize * 2), - ::testing::NotNull()); - EXPECT_EQ(hist_.GetBestFittingPacket(kTargetSize * 2)->SequenceNumber(), - To16u(kStartSeqNum + 1)); -} - -TEST_F(RtpPacketHistoryTest, - GetBestFittingPacketIgnoresNoneRetransmitablePackets) { - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - packet->SetPayloadSize(50); - hist_.PutRtpPacket(std::move(packet), kDontRetransmit, - fake_clock_.TimeInMilliseconds()); - EXPECT_THAT(hist_.GetBestFittingPacket(50), ::testing::IsNull()); - EXPECT_THAT(hist_.GetPacketAndSetSendTime(kStartSeqNum), - ::testing::NotNull()); -} - TEST_F(RtpPacketHistoryTest, CullWithAcks) { const int64_t kPacketLifetime = RtpPacketHistory::kMinPacketDurationMs * RtpPacketHistory::kPacketCullingDelayFactor; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index a2cad52c53..b762e60e88 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -201,9 +201,6 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) legacy_packet_history_storage_mode_( IsEnabled("WebRTC-UseRtpPacketHistoryLegacyStorageMode", config.field_trials)), - payload_padding_prefer_useful_packets_( - !IsDisabled("WebRTC-PayloadPadding-UseMostUsefulPacket", - config.field_trials)), pacer_legacy_packet_referencing_( !IsDisabled("WebRTC-Pacer-LegacyPacketReferencing", config.field_trials)) { @@ -293,9 +290,6 @@ RTPSender::RTPSender( legacy_packet_history_storage_mode_( field_trials.Lookup("WebRTC-UseRtpPacketHistoryLegacyStorageMode") .find("Enabled") == 0), - payload_padding_prefer_useful_packets_( - field_trials.Lookup("WebRTC-PayloadPadding-UseMostUsefulPacket") - .find("Disabled") != 0), pacer_legacy_packet_referencing_( field_trials.Lookup("WebRTC-Pacer-LegacyPacketReferencing") .find("Disabled") != 0) { @@ -441,12 +435,8 @@ size_t RTPSender::TrySendRedundantPayloads(size_t bytes_to_send, int bytes_left = static_cast(bytes_to_send); while (bytes_left >= kMinPayloadPaddingBytes) { - std::unique_ptr packet; - if (payload_padding_prefer_useful_packets_) { - packet = packet_history_.GetPayloadPaddingPacket(); - } else { - packet = packet_history_.GetBestFittingPacket(bytes_left); - } + std::unique_ptr packet = + packet_history_.GetPayloadPaddingPacket(); if (!packet) break; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 032e65cc54..823710e89b 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -320,12 +320,6 @@ class RTPSender { const bool send_side_bwe_with_overhead_; const bool legacy_packet_history_storage_mode_; - // Set by field trial "WebRTC-PayloadPadding-UseMostUsefulPacket". If set - // to "Enabled" this field will be true and - // packet_history_.GetPayloadPaddingPacket() will be called instead of - // packet_history_.GetBestFittingPacket() in TrySendRedundantPayloads(). - const bool payload_padding_prefer_useful_packets_; - // If true, PacedSender should only reference packets as in legacy mode. // If false, PacedSender may have direct ownership of RtpPacketToSend objects. // Defaults to true, will be changed to default false soon. diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index c9121c8e11..75c418fc97 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1229,8 +1229,6 @@ TEST_P(RtpSenderTest, OnSendPacketNotUpdatedWithoutSeqNumAllocator) { EXPECT_EQ(1, transport_.packets_sent()); } -// TODO(bugs.webrtc.org/8975): Remove this test when non-useful padding is -// removed. TEST_P(RtpSenderTest, SendRedundantPayloads) { if (!GetParam().pacer_references_packets) { // If PacedSender owns the RTP packets, GeneratePadding() family of methods @@ -1238,109 +1236,6 @@ TEST_P(RtpSenderTest, SendRedundantPayloads) { return; } - test::ScopedFieldTrials field_trials( - "WebRTC-PayloadPadding-UseMostUsefulPacket/Disabled/"); - MockTransport transport; - RtpRtcp::Configuration config; - config.clock = &fake_clock_; - config.outgoing_transport = &transport; - config.paced_sender = &mock_paced_sender_; - config.media_send_ssrc = kSsrc; - config.rtx_send_ssrc = kRtxSsrc; - config.event_log = &mock_rtc_event_log_; - config.retransmission_rate_limiter = &retransmission_rate_limiter_; - rtp_sender_ = absl::make_unique(config); - - rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); - - uint16_t seq_num = kSeqNum; - rtp_sender_->SetStorePacketsStatus(true, 10); - int32_t rtp_header_len = kRtpHeaderSize; - EXPECT_EQ( - 0, rtp_sender_->RegisterRtpHeaderExtension(kRtpExtensionAbsoluteSendTime, - kAbsoluteSendTimeExtensionId)); - rtp_header_len += 4; // 4 bytes extension. - rtp_header_len += 4; // 4 extra bytes common to all extension headers. - - rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); - - const size_t kNumPayloadSizes = 10; - const size_t kPayloadSizes[kNumPayloadSizes] = {500, 550, 600, 650, 700, - 750, 800, 850, 900, 950}; - // Expect all packets go through the pacer. - EXPECT_CALL(mock_rtc_event_log_, - LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) - .Times(kNumPayloadSizes); - - // Send 10 packets of increasing size. - for (size_t i = 0; i < kNumPayloadSizes; ++i) { - int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); - - EXPECT_CALL(transport, SendRtp(_, _, _)).WillOnce(::testing::Return(true)); - - if (GetParam().pacer_references_packets) { - EXPECT_CALL(mock_paced_sender_, InsertPacket(_, kSsrc, seq_num, _, _, _)); - SendPacket(capture_time_ms, kPayloadSizes[i]); - rtp_sender_->TimeToSendPacket(kSsrc, seq_num, - fake_clock_.TimeInMilliseconds(), false, - PacedPacketInfo()); - } else { - EXPECT_CALL( - mock_paced_sender_, - EnqueuePacket(AllOf( - Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc)), - Pointee(Property(&RtpPacketToSend::SequenceNumber, seq_num))))); - auto packet = SendPacket(capture_time_ms, kPayloadSizes[i]); - packet->set_packet_type(RtpPacketToSend::Type::kVideo); - rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); - } - - ++seq_num; - fake_clock_.AdvanceTimeMilliseconds(33); - } - - EXPECT_CALL(mock_rtc_event_log_, - LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) - .Times(AtLeast(4)); - - // The amount of padding to send it too small to send a payload packet. - EXPECT_CALL(transport, SendRtp(_, kMaxPaddingSize + rtp_header_len, _)) - .WillOnce(Return(true)); - EXPECT_EQ(kMaxPaddingSize, - rtp_sender_->TimeToSendPadding(49, PacedPacketInfo())); - - PacketOptions options; - EXPECT_CALL(transport, - SendRtp(_, kPayloadSizes[0] + rtp_header_len + kRtxHeaderSize, _)) - .WillOnce(DoAll(SaveArg<2>(&options), Return(true))); - EXPECT_EQ(kPayloadSizes[0], - rtp_sender_->TimeToSendPadding(500, PacedPacketInfo())); - EXPECT_TRUE(options.is_retransmit); - - EXPECT_CALL(transport, SendRtp(_, - kPayloadSizes[kNumPayloadSizes - 1] + - rtp_header_len + kRtxHeaderSize, - _)) - .WillOnce(Return(true)); - - options.is_retransmit = false; - EXPECT_CALL(transport, SendRtp(_, kMaxPaddingSize + rtp_header_len, _)) - .WillOnce(DoAll(SaveArg<2>(&options), Return(true))); - EXPECT_EQ(kPayloadSizes[kNumPayloadSizes - 1] + kMaxPaddingSize, - rtp_sender_->TimeToSendPadding(999, PacedPacketInfo())); - EXPECT_FALSE(options.is_retransmit); -} - -TEST_P(RtpSenderTest, SendRedundantPayloadsUsefulPadding) { - if (!GetParam().pacer_references_packets) { - // If PacedSender owns the RTP packets, GeneratePadding() family of methods - // will be called instead and this test makes no sense. - return; - } - - test::ScopedFieldTrials field_trials( - "WebRTC-PayloadPadding-UseMostUsefulPacket/Enabled/"); MockTransport transport; RtpRtcp::Configuration config; config.clock = &fake_clock_;