From c793e46856b498ec4c9fb3a6682c95b4e321dba3 Mon Sep 17 00:00:00 2001 From: Per K Date: Thu, 28 Mar 2024 09:29:21 +0000 Subject: [PATCH] Cleanup TransportFeedbackObserver from RtpSenderEgress TransportFeedbackObserver is thus unused from WebRTC except from DEPRECATED_RtpSender Change-Id: Ib308f5331a342a4ec4f7c7cfdf6f76c3c4c1807c Bug: webrtc:15368 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/344721 Commit-Queue: Per Kjellander Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#42012} --- modules/rtp_rtcp/source/rtp_rtcp_interface.h | 5 + modules/rtp_rtcp/source/rtp_sender_egress.cc | 8 +- modules/rtp_rtcp/source/rtp_sender_egress.h | 1 - .../source/rtp_sender_egress_unittest.cc | 144 ------------------ 4 files changed, 8 insertions(+), 150 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_rtcp_interface.h b/modules/rtp_rtcp/source/rtp_rtcp_interface.h index e41a3fde0b..9fbe93fff1 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_interface.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_interface.h @@ -75,6 +75,11 @@ class RtpRtcpInterface : public RtcpFeedbackSenderInterface { NetworkLinkRtcpObserver* network_link_rtcp_observer = nullptr; NetworkStateEstimateObserver* network_state_estimate_observer = nullptr; + + // DEPRECATED, transport_feedback_callback is no longer invoked by the RTP + // module except from DEPRECATED_RtpSenderEgress. + // TODO: bugs.webrtc.org/15368 - Delete once DEPRECATED_RtpSenderEgress is + // deleted. TransportFeedbackObserver* transport_feedback_callback = nullptr; VideoBitrateAllocationObserver* bitrate_allocation_observer = nullptr; RtcpRttStats* rtt_stats = nullptr; diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc index 705e9d548e..665c7ce47e 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -94,7 +94,6 @@ RtpSenderEgress::RtpSenderEgress(const RtpRtcpInterface::Configuration& config, is_audio_(config.audio), need_rtp_packet_infos_(config.need_rtp_packet_infos), fec_generator_(config.fec_generator), - transport_feedback_observer_(config.transport_feedback_callback), send_packet_observer_(config.send_packet_observer), rtp_stats_callback_(config.rtp_stats_callback), bitrate_callback_(config.send_bitrate_observer), @@ -107,6 +106,9 @@ RtpSenderEgress::RtpSenderEgress(const RtpRtcpInterface::Configuration& config, kRtpSequenceNumberMapMaxEntries) : nullptr) { RTC_DCHECK(worker_queue_); + RTC_DCHECK(config.transport_feedback_callback == nullptr) + << "transport_feedback_callback is no longer used and will soon be " + "deleted."; if (bitrate_callback_) { update_task_ = RepeatingTaskHandle::DelayedStart(worker_queue_, kUpdateInterval, [this]() { @@ -269,10 +271,6 @@ void RtpSenderEgress::CompleteSendPacket(const Packet& compound_packet, } else if (packet->transport_sequence_number()) { options.packet_id = *packet->transport_sequence_number(); } - if (options.packet_id >= 0 && transport_feedback_observer_) { - transport_feedback_observer_->OnAddPacket( - RtpPacketSendInfo::From(*packet, pacing_info)); - } if (packet->packet_type() != RtpPacketMediaType::kPadding && packet->packet_type() != RtpPacketMediaType::kRetransmission && diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.h b/modules/rtp_rtcp/source/rtp_sender_egress.h index 7f038f6142..cb8a4dc3b0 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.h +++ b/modules/rtp_rtcp/source/rtp_sender_egress.h @@ -143,7 +143,6 @@ class RtpSenderEgress { absl::optional last_sent_seq_ RTC_GUARDED_BY(worker_queue_); absl::optional last_sent_rtx_seq_ RTC_GUARDED_BY(worker_queue_); - TransportFeedbackObserver* const transport_feedback_observer_; SendPacketObserver* const send_packet_observer_; StreamDataCountersCallback* const rtp_stats_callback_; BitrateStatisticsObserver* const bitrate_callback_; diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index 908df95148..be79601acd 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -65,11 +65,6 @@ class MockSendPacketObserver : public SendPacketObserver { (override)); }; -class MockTransportFeedbackObserver : public TransportFeedbackObserver { - public: - MOCK_METHOD(void, OnAddPacket, (const RtpPacketSendInfo&), (override)); -}; - class MockStreamDataCountersCallback : public StreamDataCountersCallback { public: MOCK_METHOD(void, @@ -141,7 +136,6 @@ class RtpSenderEgressTest : public ::testing::Test { config.event_log = &mock_rtc_event_log_; config.send_packet_observer = &send_packet_observer_; config.rtp_stats_callback = &mock_rtp_stats_callback_; - config.transport_feedback_callback = &feedback_observer_; config.populate_network2_timestamp = false; config.field_trials = &trials_; return config; @@ -173,7 +167,6 @@ class RtpSenderEgressTest : public ::testing::Test { NiceMock mock_rtc_event_log_; NiceMock mock_rtp_stats_callback_; NiceMock send_packet_observer_; - NiceMock feedback_observer_; RtpHeaderExtensionMap header_extensions_; NiceMock transport_; RtpPacketHistory packet_history_; @@ -181,34 +174,6 @@ class RtpSenderEgressTest : public ::testing::Test { uint16_t sequence_number_; }; -TEST_F(RtpSenderEgressTest, TransportFeedbackObserverGetsCorrectByteCount) { - constexpr size_t kRtpOverheadBytesPerPacket = 12 + 8; - constexpr size_t kPayloadSize = 1400; - const uint16_t kTransportSequenceNumber = 17; - - header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, - TransportSequenceNumber::Uri()); - - const size_t expected_bytes = kPayloadSize + kRtpOverheadBytesPerPacket; - - EXPECT_CALL( - feedback_observer_, - OnAddPacket(AllOf( - Field(&RtpPacketSendInfo::media_ssrc, kSsrc), - Field(&RtpPacketSendInfo::transport_sequence_number, - kTransportSequenceNumber), - Field(&RtpPacketSendInfo::rtp_sequence_number, kStartSequenceNumber), - Field(&RtpPacketSendInfo::length, expected_bytes), - Field(&RtpPacketSendInfo::pacing_info, PacedPacketInfo())))); - - std::unique_ptr packet = BuildRtpPacket(); - packet->set_transport_sequence_number(kTransportSequenceNumber); - packet->AllocatePayload(kPayloadSize); - - std::unique_ptr sender = CreateRtpSenderEgress(); - sender->SendPacket(std::move(packet), PacedPacketInfo()); -} - TEST_F(RtpSenderEgressTest, SendsPacketsOneByOneWhenNotBatching) { std::unique_ptr sender = CreateRtpSenderEgress(); EXPECT_CALL(transport_, @@ -926,115 +891,6 @@ TEST_F(RtpSenderEgressTest, SendPacketUpdatesStats) { EXPECT_EQ(rtx_stats.retransmitted.packets, 1u); } -TEST_F(RtpSenderEgressTest, TransportFeedbackObserverWithRetransmission) { - const uint16_t kTransportSequenceNumber = 17; - header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, - TransportSequenceNumber::Uri()); - std::unique_ptr retransmission = BuildRtpPacket(); - retransmission->set_packet_type(RtpPacketMediaType::kRetransmission); - retransmission->set_transport_sequence_number(kTransportSequenceNumber); - retransmission->set_original_ssrc(kSsrc); - uint16_t retransmitted_seq = retransmission->SequenceNumber() - 2; - retransmission->set_retransmitted_sequence_number(retransmitted_seq); - - std::unique_ptr sender = CreateRtpSenderEgress(); - EXPECT_CALL( - feedback_observer_, - OnAddPacket(AllOf( - Field(&RtpPacketSendInfo::media_ssrc, kSsrc), - Field(&RtpPacketSendInfo::rtp_sequence_number, retransmitted_seq), - Field(&RtpPacketSendInfo::transport_sequence_number, - kTransportSequenceNumber)))); - sender->SendPacket(std::move(retransmission), PacedPacketInfo()); -} - -TEST_F(RtpSenderEgressTest, TransportFeedbackObserverWithRtxRetransmission) { - const uint16_t kTransportSequenceNumber = 17; - header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, - TransportSequenceNumber::Uri()); - - std::unique_ptr rtx_retransmission = BuildRtpPacket(); - rtx_retransmission->SetSsrc(kRtxSsrc); - rtx_retransmission->set_transport_sequence_number(kTransportSequenceNumber); - rtx_retransmission->set_original_ssrc(kSsrc); - rtx_retransmission->set_packet_type(RtpPacketMediaType::kRetransmission); - uint16_t rtx_retransmitted_seq = rtx_retransmission->SequenceNumber() - 2; - rtx_retransmission->set_retransmitted_sequence_number(rtx_retransmitted_seq); - - std::unique_ptr sender = CreateRtpSenderEgress(); - EXPECT_CALL( - feedback_observer_, - OnAddPacket(AllOf( - Field(&RtpPacketSendInfo::media_ssrc, kSsrc), - Field(&RtpPacketSendInfo::rtp_sequence_number, rtx_retransmitted_seq), - Field(&RtpPacketSendInfo::transport_sequence_number, - kTransportSequenceNumber)))); - sender->SendPacket(std::move(rtx_retransmission), PacedPacketInfo()); -} - -TEST_F(RtpSenderEgressTest, TransportFeedbackObserverPadding) { - const uint16_t kTransportSequenceNumber = 17; - header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, - TransportSequenceNumber::Uri()); - std::unique_ptr padding = BuildRtpPacket(); - padding->SetPadding(224); - padding->set_packet_type(RtpPacketMediaType::kPadding); - padding->set_transport_sequence_number(kTransportSequenceNumber); - - std::unique_ptr sender = CreateRtpSenderEgress(); - EXPECT_CALL( - feedback_observer_, - OnAddPacket(AllOf(Field(&RtpPacketSendInfo::media_ssrc, absl::nullopt), - Field(&RtpPacketSendInfo::transport_sequence_number, - kTransportSequenceNumber)))); - sender->SendPacket(std::move(padding), PacedPacketInfo()); -} - -TEST_F(RtpSenderEgressTest, TransportFeedbackObserverRtxPadding) { - const uint16_t kTransportSequenceNumber = 17; - header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, - TransportSequenceNumber::Uri()); - - std::unique_ptr rtx_padding = BuildRtpPacket(); - rtx_padding->SetPadding(224); - rtx_padding->SetSsrc(kRtxSsrc); - rtx_padding->set_packet_type(RtpPacketMediaType::kPadding); - rtx_padding->set_transport_sequence_number(kTransportSequenceNumber); - - std::unique_ptr sender = CreateRtpSenderEgress(); - EXPECT_CALL( - feedback_observer_, - OnAddPacket(AllOf(Field(&RtpPacketSendInfo::media_ssrc, absl::nullopt), - Field(&RtpPacketSendInfo::transport_sequence_number, - kTransportSequenceNumber)))); - sender->SendPacket(std::move(rtx_padding), PacedPacketInfo()); -} - -TEST_F(RtpSenderEgressTest, TransportFeedbackObserverFec) { - const uint16_t kTransportSequenceNumber = 17; - header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, - TransportSequenceNumber::Uri()); - - std::unique_ptr fec_packet = BuildRtpPacket(); - fec_packet->SetSsrc(kFlexFecSsrc); - fec_packet->set_packet_type(RtpPacketMediaType::kForwardErrorCorrection); - fec_packet->set_transport_sequence_number(kTransportSequenceNumber); - - const rtc::ArrayView kNoRtpHeaderExtensionSizes; - FlexfecSender flexfec(kFlexfectPayloadType, kFlexFecSsrc, kSsrc, /*mid=*/"", - /*header_extensions=*/{}, kNoRtpHeaderExtensionSizes, - /*rtp_state=*/nullptr, time_controller_.GetClock()); - RtpRtcpInterface::Configuration config = DefaultConfig(); - config.fec_generator = &flexfec; - auto sender = std::make_unique(config, &packet_history_); - EXPECT_CALL( - feedback_observer_, - OnAddPacket(AllOf(Field(&RtpPacketSendInfo::media_ssrc, absl::nullopt), - Field(&RtpPacketSendInfo::transport_sequence_number, - kTransportSequenceNumber)))); - sender->SendPacket(std::move(fec_packet), PacedPacketInfo()); -} - TEST_F(RtpSenderEgressTest, SupportsAbortingRetransmissions) { std::unique_ptr sender = CreateRtpSenderEgress(); packet_history_.SetStorePacketsStatus(