From 2ee43d6daaa3083dab3bd4df58326cd654b79daf Mon Sep 17 00:00:00 2001 From: Per K Date: Mon, 14 Oct 2024 15:44:23 +0000 Subject: [PATCH] Callback to NetworkStateEstimateObserver before NetworkLinkRtcpObserver If RTCP compound message is received with both these messages, NetworkStateEstimator should be invoked before NetworkLinkRtcpObserver since remote network state estimate may set limits on the BWE calculated from the transport feedback. Bug: webrtc:42220808 Change-Id: Ieac9c1d7d9c28e690351bcf1d8125c9e0099f962 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/365583 Reviewed-by: Danil Chapovalov Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#43239} --- modules/rtp_rtcp/BUILD.gn | 1 + .../mock_network_state_estimator_observer.h | 28 ++++++++++ modules/rtp_rtcp/source/rtcp_receiver.cc | 13 +++-- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 55 +++++++++++++++++++ 4 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 modules/rtp_rtcp/mocks/mock_network_state_estimator_observer.h diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index c0318eb650..494caaa811 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn @@ -502,6 +502,7 @@ rtc_library("mock_rtp_rtcp") { testonly = true public = [ "mocks/mock_network_link_rtcp_observer.h", + "mocks/mock_network_state_estimator_observer.h", "mocks/mock_recovered_packet_receiver.h", "mocks/mock_rtcp_rtt_stats.h", "mocks/mock_rtp_rtcp.h", diff --git a/modules/rtp_rtcp/mocks/mock_network_state_estimator_observer.h b/modules/rtp_rtcp/mocks/mock_network_state_estimator_observer.h new file mode 100644 index 0000000000..8c6ea45185 --- /dev/null +++ b/modules/rtp_rtcp/mocks/mock_network_state_estimator_observer.h @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2024 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#ifndef MODULES_RTP_RTCP_MOCKS_MOCK_NETWORK_STATE_ESTIMATOR_OBSERVER_H_ +#define MODULES_RTP_RTCP_MOCKS_MOCK_NETWORK_STATE_ESTIMATOR_OBSERVER_H_ + +#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "test/gmock.h" + +namespace webrtc { + +class MockNetworkStateEstimateObserver : public NetworkStateEstimateObserver { + public: + MOCK_METHOD(void, + OnRemoteNetworkEstimate, + (NetworkStateEstimate estimate), + (override)); +}; + +} // namespace webrtc + +#endif // MODULES_RTP_RTCP_MOCKS_MOCK_NETWORK_STATE_ESTIMATOR_OBSERVER_H_ diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 8732b3772c..10f9d87730 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -1163,6 +1163,13 @@ void RTCPReceiver::TriggerCallbacksFromRtcpPacket( loss_notification->decodability_flag()); } } + // Network state estimate should be applied before other feedback since it may + // affect how other feedback is handled. + if (network_state_estimate_observer_ && + packet_information.network_state_estimate) { + network_state_estimate_observer_->OnRemoteNetworkEstimate( + *packet_information.network_state_estimate); + } if (network_link_rtcp_observer_) { Timestamp now = env_.clock().CurrentTime(); @@ -1194,12 +1201,6 @@ void RTCPReceiver::TriggerCallbacksFromRtcpPacket( packet_information.report_block_datas); } - if (network_state_estimate_observer_ && - packet_information.network_state_estimate) { - network_state_estimate_observer_->OnRemoteNetworkEstimate( - *packet_information.network_state_estimate); - } - if (bitrate_allocation_observer_ && packet_information.target_bitrate_allocation) { bitrate_allocation_observer_->OnBitrateAllocationUpdated( diff --git a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 31e5d343ff..b01c23c310 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -22,6 +22,7 @@ #include "absl/strings/string_view.h" #include "api/array_view.h" #include "api/environment/environment_factory.h" +#include "api/transport/network_types.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" @@ -32,6 +33,7 @@ #include "modules/rtp_rtcp/include/rtcp_statistics.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/mocks/mock_network_link_rtcp_observer.h" +#include "modules/rtp_rtcp/mocks/mock_network_state_estimator_observer.h" #include "modules/rtp_rtcp/source/byte_io.h" #include "modules/rtp_rtcp/source/ntp_time_util.h" #include "modules/rtp_rtcp/source/rtcp_packet/app.h" @@ -46,6 +48,7 @@ #include "modules/rtp_rtcp/source/rtcp_packet/rapid_resync_request.h" #include "modules/rtp_rtcp/source/rtcp_packet/receiver_report.h" #include "modules/rtp_rtcp/source/rtcp_packet/remb.h" +#include "modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h" #include "modules/rtp_rtcp/source/rtcp_packet/rtpfb.h" #include "modules/rtp_rtcp/source/rtcp_packet/sdes.h" #include "modules/rtp_rtcp/source/rtcp_packet/sender_report.h" @@ -168,12 +171,14 @@ struct ReceiverMocks { StrictMock bitrate_allocation_observer; StrictMock rtp_rtcp_impl; NiceMock network_link_rtcp_observer; + NiceMock network_state_estimate_observer; RtpRtcpInterface::Configuration config = { .receiver_only = false, .intra_frame_callback = &intra_frame_observer, .rtcp_loss_notification_observer = &rtcp_loss_notification_observer, .network_link_rtcp_observer = &network_link_rtcp_observer, + .network_state_estimate_observer = &network_state_estimate_observer, .bitrate_allocation_observer = &bitrate_allocation_observer, .rtcp_packet_type_counter_observer = &packet_type_counter_observer, .rtcp_report_interval_ms = kRtcpIntervalMs, @@ -1761,6 +1766,56 @@ TEST(RtcpReceiverTest, NotifiesNetworkLinkObserverOnCongestionControlFeedback) { receiver.IncomingPacket(packet.Build()); } +TEST(RtcpReceiverTest, + NotifiesNetworkStateEstimateObserverOnRemoteNetworkEstimate) { + ReceiverMocks mocks; + RTCPReceiver receiver = Create(mocks); + receiver.SetRemoteSSRC(kSenderSsrc); + + NetworkStateEstimate estimate; + estimate.link_capacity_lower = DataRate::BitsPerSec(1000); + estimate.link_capacity_upper = DataRate::BitsPerSec(10000); + rtcp::RemoteEstimate remote_estimate; + remote_estimate.SetEstimate(estimate); + + EXPECT_CALL(mocks.network_state_estimate_observer, + OnRemoteNetworkEstimate( + AllOf(Field(&NetworkStateEstimate::link_capacity_lower, + DataRate::BitsPerSec(1000)), + Field(&NetworkStateEstimate::link_capacity_upper, + DataRate::BitsPerSec(10000))))); + + receiver.IncomingPacket(remote_estimate.Build()); +} + +TEST(RtcpReceiverTest, + NotifiesNetworkStateEstimateObserverBeforeNetworkLinkObserver) { + ReceiverMocks mocks; + RTCPReceiver receiver = Create(mocks); + receiver.SetRemoteSSRC(kSenderSsrc); + + NetworkStateEstimate estimate; + estimate.link_capacity_lower = DataRate::BitsPerSec(1000); + estimate.link_capacity_upper = DataRate::BitsPerSec(10000); + std::unique_ptr remote_estimate = + std::make_unique(); + remote_estimate->SetEstimate(estimate); + std::unique_ptr feedback_packet = + std::make_unique(); + feedback_packet->SetMediaSsrc(mocks.config.local_media_ssrc); + feedback_packet->SetSenderSsrc(kSenderSsrc); + feedback_packet->SetBase(123, Timestamp::Millis(1)); + feedback_packet->AddReceivedPacket(123, Timestamp::Millis(1)); + rtcp::CompoundPacket compound; + compound.Append(std::move(remote_estimate)); + compound.Append(std::move(feedback_packet)); + + InSequence s; + EXPECT_CALL(mocks.network_state_estimate_observer, OnRemoteNetworkEstimate); + EXPECT_CALL(mocks.network_link_rtcp_observer, OnTransportFeedback); + receiver.IncomingPacket(compound.Build()); +} + TEST(RtcpReceiverTest, HandlesInvalidCongestionControlFeedback) { ReceiverMocks mocks; mocks.field_trials = "WebRTC-RFC8888CongestionControlFeedback/Enabled/";