From 718601a1f88f87ae542cdfc5538039185c4c3b74 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Fri, 19 May 2023 15:10:55 +0200 Subject: [PATCH] Cleanup RtcpReceiver from passing TransportFeedback via older interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:8239 Change-Id: Ibc289627cc89bda86f3e2c7c0c11d0ec2ae95087 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305783 Reviewed-by: Åsa Persson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40102} --- call/rtp_transport_controller_send.h | 1 - modules/rtp_rtcp/include/rtp_rtcp_defines.h | 7 +-- modules/rtp_rtcp/source/rtcp_receiver.cc | 10 ---- modules/rtp_rtcp/source/rtcp_receiver.h | 1 - .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 50 ++++--------------- .../source/rtp_sender_egress_unittest.cc | 4 -- 6 files changed, 10 insertions(+), 63 deletions(-) diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index 590c95e4cc..1aace1ce65 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -119,7 +119,6 @@ class RtpTransportControllerSend final // Implements TransportFeedbackObserver interface void OnAddPacket(const RtpPacketSendInfo& packet_info) override; - void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override {} // Implements NetworkStateEstimateObserver interface void OnRemoteNetworkEstimate(NetworkStateEstimate estimate) override; diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 36b2f64503..4bdad6e86d 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -232,14 +232,9 @@ class NetworkStateEstimateObserver { class TransportFeedbackObserver { public: - TransportFeedbackObserver() {} - virtual ~TransportFeedbackObserver() {} + virtual ~TransportFeedbackObserver() = default; virtual void OnAddPacket(const RtpPacketSendInfo& packet_info) = 0; - - // TODO(bugs.webrtc.org/8239): Remove this function in favor of receiving - // feedback message via `NetworkLinkRtcpObserver` interface. - virtual void OnTransportFeedback(const rtcp::TransportFeedback& feedback) {} }; // Interface for PacketRouter to send rtcp feedback on behalf of diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 82612f83d4..bf63828ff7 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -149,8 +149,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, rtcp_intra_frame_observer_(config.intra_frame_callback), rtcp_loss_notification_observer_(config.rtcp_loss_notification_observer), network_state_estimate_observer_(config.network_state_estimate_observer), - deprecated_transport_feedback_observer_( - config.transport_feedback_callback), bitrate_allocation_observer_(config.bitrate_allocation_observer), report_interval_(config.rtcp_report_interval_ms > 0 ? TimeDelta::Millis(config.rtcp_report_interval_ms) @@ -180,8 +178,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcpInterface::Configuration& config, rtcp_intra_frame_observer_(config.intra_frame_callback), rtcp_loss_notification_observer_(config.rtcp_loss_notification_observer), network_state_estimate_observer_(config.network_state_estimate_observer), - deprecated_transport_feedback_observer_( - config.transport_feedback_callback), bitrate_allocation_observer_(config.bitrate_allocation_observer), report_interval_(config.rtcp_report_interval_ms > 0 ? TimeDelta::Millis(config.rtcp_report_interval_ms) @@ -1194,12 +1190,6 @@ void RTCPReceiver::TriggerCallbacksFromRtcpPacket( rtp_rtcp_->OnReceivedRtcpReportBlocks(packet_information.report_blocks); } - if (deprecated_transport_feedback_observer_ && - (packet_information.packet_type_flags & kRtcpTransportFeedback)) { - deprecated_transport_feedback_observer_->OnTransportFeedback( - *packet_information.transport_feedback); - } - if (network_state_estimate_observer_ && packet_information.network_state_estimate) { network_state_estimate_observer_->OnRemoteNetworkEstimate( diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index 00987b3825..4453c5724c 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -360,7 +360,6 @@ class RTCPReceiver final { RtcpIntraFrameObserver* const rtcp_intra_frame_observer_; RtcpLossNotificationObserver* const rtcp_loss_notification_observer_; NetworkStateEstimateObserver* const network_state_estimate_observer_; - TransportFeedbackObserver* const deprecated_transport_feedback_observer_; VideoBitrateAllocationObserver* const bitrate_allocation_observer_; const TimeDelta report_interval_; diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 47bd549639..78abe07139 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -100,15 +100,6 @@ class MockReportBlockDataObserverImpl : public ReportBlockDataObserver { MOCK_METHOD(void, OnReportBlockDataUpdated, (ReportBlockData), (override)); }; -class MockTransportFeedbackObserver : public TransportFeedbackObserver { - public: - MOCK_METHOD(void, OnAddPacket, (const RtpPacketSendInfo&), (override)); - MOCK_METHOD(void, - OnTransportFeedback, - (const rtcp::TransportFeedback&), - (override)); -}; - class MockModuleRtpRtcp : public RTCPReceiver::ModuleRtpRtcp { public: MOCK_METHOD(void, SetTmmbn, (std::vector), (override)); @@ -157,7 +148,6 @@ struct ReceiverMocks { StrictMock bandwidth_observer; StrictMock intra_frame_observer; StrictMock rtcp_loss_notification_observer; - StrictMock transport_feedback_observer; StrictMock bitrate_allocation_observer; StrictMock rtp_rtcp_impl; NiceMock network_link_rtcp_observer; @@ -173,7 +163,6 @@ RtpRtcpInterface::Configuration DefaultConfiguration(ReceiverMocks* mocks) { config.intra_frame_callback = &mocks->intra_frame_observer; config.rtcp_loss_notification_observer = &mocks->rtcp_loss_notification_observer; - config.transport_feedback_callback = &mocks->transport_feedback_observer; config.bitrate_allocation_observer = &mocks->bitrate_allocation_observer; config.rtcp_report_interval_ms = kRtcpIntervalMs; config.local_media_ssrc = kReceiverMainSsrc; @@ -556,7 +545,6 @@ TEST(RtcpReceiverTest, NotifiesNetworkLinkObserverOnReportBlocks) { ReceiverMocks mocks; RtpRtcpInterface::Configuration config = DefaultConfiguration(&mocks); config.bandwidth_callback = nullptr; - config.transport_feedback_callback = nullptr; config.network_link_rtcp_observer = &mocks.network_link_rtcp_observer; RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); @@ -1781,30 +1769,10 @@ TEST(RtcpReceiverTest, GetReportBlockDataAfterTwoReportBlocksOfDifferentSsrcs) { report_block_datas[1].extended_highest_sequence_number()); } -TEST(RtcpReceiverTest, ReceivesTransportFeedback) { - ReceiverMocks mocks; - RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl); - receiver.SetRemoteSSRC(kSenderSsrc); - - rtcp::TransportFeedback packet; - packet.SetMediaSsrc(kReceiverMainSsrc); - packet.SetSenderSsrc(kSenderSsrc); - packet.SetBase(1, Timestamp::Millis(1)); - packet.AddReceivedPacket(1, Timestamp::Millis(1)); - - EXPECT_CALL( - mocks.transport_feedback_observer, - OnTransportFeedback(AllOf( - Property(&rtcp::TransportFeedback::media_ssrc, kReceiverMainSsrc), - Property(&rtcp::TransportFeedback::sender_ssrc, kSenderSsrc)))); - receiver.IncomingPacket(packet.Build()); -} - TEST(RtcpReceiverTest, NotifiesNetworkLinkObserverOnTransportFeedback) { ReceiverMocks mocks; RtpRtcpInterface::Configuration config = DefaultConfiguration(&mocks); config.bandwidth_callback = nullptr; - config.transport_feedback_callback = nullptr; config.network_link_rtcp_observer = &mocks.network_link_rtcp_observer; RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); @@ -1831,7 +1799,6 @@ TEST(RtcpReceiverTest, ReceiverMocks mocks; RtpRtcpInterface::Configuration config = DefaultConfiguration(&mocks); config.bandwidth_callback = nullptr; - config.transport_feedback_callback = nullptr; config.network_link_rtcp_observer = &mocks.network_link_rtcp_observer; RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); @@ -1851,7 +1818,6 @@ TEST(RtcpReceiverTest, ReceiverMocks mocks; RtpRtcpInterface::Configuration config = DefaultConfiguration(&mocks); config.bandwidth_callback = nullptr; - config.transport_feedback_callback = nullptr; config.network_link_rtcp_observer = &mocks.network_link_rtcp_observer; RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); @@ -1885,7 +1851,6 @@ TEST(RtcpReceiverTest, NotifiesNetworkLinkObserverOnRemb) { ReceiverMocks mocks; RtpRtcpInterface::Configuration config = DefaultConfiguration(&mocks); config.bandwidth_callback = nullptr; - config.transport_feedback_callback = nullptr; config.network_link_rtcp_observer = &mocks.network_link_rtcp_observer; RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); @@ -1902,7 +1867,10 @@ TEST(RtcpReceiverTest, NotifiesNetworkLinkObserverOnRemb) { TEST(RtcpReceiverTest, HandlesInvalidTransportFeedback) { ReceiverMocks mocks; - RTCPReceiver receiver(DefaultConfiguration(&mocks), &mocks.rtp_rtcp_impl); + RtpRtcpInterface::Configuration config = DefaultConfiguration(&mocks); + config.bandwidth_callback = nullptr; + config.network_link_rtcp_observer = &mocks.network_link_rtcp_observer; + RTCPReceiver receiver(config, &mocks.rtp_rtcp_impl); receiver.SetRemoteSSRC(kSenderSsrc); // Send a compound packet with a TransportFeedback followed by something else. @@ -1912,10 +1880,10 @@ TEST(RtcpReceiverTest, HandlesInvalidTransportFeedback) { packet->SetBase(1, Timestamp::Millis(1)); packet->AddReceivedPacket(1, Timestamp::Millis(1)); - static uint32_t kBitrateBps = 50000; + static constexpr DataRate kBitrate = DataRate::BitsPerSec(50'000); auto remb = std::make_unique(); remb->SetSenderSsrc(kSenderSsrc); - remb->SetBitrateBps(kBitrateBps); + remb->SetBitrateBps(kBitrate.bps()); rtcp::CompoundPacket compound; compound.Append(std::move(packet)); compound.Append(std::move(remb)); @@ -1927,10 +1895,10 @@ TEST(RtcpReceiverTest, HandlesInvalidTransportFeedback) { 42); // Stress no transport feedback is expected. - EXPECT_CALL(mocks.transport_feedback_observer, OnTransportFeedback).Times(0); + EXPECT_CALL(mocks.network_link_rtcp_observer, OnTransportFeedback).Times(0); // But remb should be processed and cause a callback - EXPECT_CALL(mocks.bandwidth_observer, - OnReceivedEstimatedBitrate(kBitrateBps)); + EXPECT_CALL(mocks.network_link_rtcp_observer, + OnReceiverEstimatedMaxBitrate(_, kBitrate)); receiver.IncomingPacket(built_packet); } diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index a31ee37c43..05a079b669 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -63,10 +63,6 @@ class MockSendPacketObserver : public SendPacketObserver { class MockTransportFeedbackObserver : public TransportFeedbackObserver { public: MOCK_METHOD(void, OnAddPacket, (const RtpPacketSendInfo&), (override)); - MOCK_METHOD(void, - OnTransportFeedback, - (const rtcp::TransportFeedback&), - (override)); }; class MockStreamDataCountersCallback : public StreamDataCountersCallback {