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_;