diff --git a/modules/congestion_controller/BUILD.gn b/modules/congestion_controller/BUILD.gn index 22a7f09292..2c00735235 100644 --- a/modules/congestion_controller/BUILD.gn +++ b/modules/congestion_controller/BUILD.gn @@ -19,9 +19,11 @@ rtc_library("congestion_controller") { deps = [ "../../api:rtp_parameters", + "../../api:sequence_checker", "../../api/environment", "../../api/transport:network_control", "../../api/units:data_rate", + "../../api/units:data_size", "../../api/units:time_delta", "../../api/units:timestamp", "../../rtc_base:logging", @@ -29,6 +31,7 @@ rtc_library("congestion_controller") { "../../rtc_base/synchronization:mutex", "../pacing", "../remote_bitrate_estimator", + "../remote_bitrate_estimator:congestion_control_feedback_generator", "../remote_bitrate_estimator:transport_sequence_number_feedback_generator", "../rtp_rtcp:rtp_rtcp_format", "//third_party/abseil-cpp/absl/base:nullability", @@ -45,6 +48,7 @@ if (rtc_include_tests && !build_with_chromium) { ] deps = [ ":congestion_controller", + "../../api:rtp_parameters", "../../api/environment:environment_factory", "../../api/test/network_emulation", "../../api/test/network_emulation:create_cross_traffic", @@ -53,6 +57,7 @@ if (rtc_include_tests && !build_with_chromium) { "../../api/units:time_delta", "../../api/units:timestamp", "../../system_wrappers", + "../../test:explicit_key_value_config", "../../test:test_support", "../../test/scenario", "../pacing", diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h index deb3a3ebba..7dec287660 100644 --- a/modules/congestion_controller/include/receive_side_congestion_controller.h +++ b/modules/congestion_controller/include/receive_side_congestion_controller.h @@ -15,10 +15,12 @@ #include "absl/base/nullability.h" #include "api/environment/environment.h" +#include "api/sequence_checker.h" #include "api/transport/network_control.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "modules/congestion_controller/remb_throttler.h" +#include "modules/remote_bitrate_estimator/congestion_control_feedback_generator.h" #include "modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/synchronization/mutex.h" @@ -42,6 +44,8 @@ class ReceiveSideCongestionController : public CallStatsObserver { ~ReceiveSideCongestionController() override = default; + void EnablSendCongestionControlFeedbackAccordingToRfc8888(); + void OnReceivedPacket(const RtpPacketReceived& packet, MediaType media_type); // Implements CallStatsObserver. @@ -74,8 +78,18 @@ class ReceiveSideCongestionController : public CallStatsObserver { const Environment env_; RembThrottler remb_throttler_; + + // TODO: bugs.webrtc.org/42224904 - Use sequence checker for all usage of + // ReceiveSideCongestionController. At the time of + // writing OnReceivedPacket and MaybeProcess can unfortunately be called on an + // arbitrary thread by external projects. + SequenceChecker sequence_checker_; + + bool send_rfc8888_congestion_feedback_ = false; TransportSequenceNumberFeedbackGenenerator transport_sequence_number_feedback_generator_; + CongestionControlFeedbackGenerator congestion_control_feedback_generator_ + RTC_GUARDED_BY(sequence_checker_); mutable Mutex mutex_; std::unique_ptr rbe_ RTC_GUARDED_BY(mutex_); diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc index c86b277c52..591dba3445 100644 --- a/modules/congestion_controller/receive_side_congestion_controller.cc +++ b/modules/congestion_controller/receive_side_congestion_controller.cc @@ -10,14 +10,18 @@ #include "modules/congestion_controller/include/receive_side_congestion_controller.h" +#include + #include "absl/base/nullability.h" #include "api/environment/environment.h" #include "api/media_types.h" +#include "api/sequence_checker.h" #include "api/units/data_rate.h" -#include "modules/pacing/packet_router.h" -#include "modules/remote_bitrate_estimator/include/bwe_defines.h" +#include "modules/remote_bitrate_estimator/congestion_control_feedback_generator.h" #include "modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h" #include "modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h" +#include "modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h" +#include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "rtc_base/logging.h" namespace webrtc { @@ -77,17 +81,37 @@ ReceiveSideCongestionController::ReceiveSideCongestionController( absl::Nullable network_state_estimator) : env_(env), remb_throttler_(std::move(remb_sender), &env_.clock()), - transport_sequence_number_feedback_generator_(std::move(feedback_sender), + transport_sequence_number_feedback_generator_(feedback_sender, network_state_estimator), + congestion_control_feedback_generator_(env, feedback_sender), rbe_(std::make_unique( env_, &remb_throttler_)), using_absolute_send_time_(false), - packets_since_absolute_send_time_(0) {} + packets_since_absolute_send_time_(0) { + FieldTrialParameter force_send_rfc8888_feedback("force_send", false); + ParseFieldTrial( + {&force_send_rfc8888_feedback}, + env.field_trials().Lookup("WebRTC-RFC8888CongestionControlFeedback")); + if (force_send_rfc8888_feedback) { + EnablSendCongestionControlFeedbackAccordingToRfc8888(); + } +} + +void ReceiveSideCongestionController:: + EnablSendCongestionControlFeedbackAccordingToRfc8888() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + send_rfc8888_congestion_feedback_ = true; +} void ReceiveSideCongestionController::OnReceivedPacket( const RtpPacketReceived& packet, MediaType media_type) { + if (send_rfc8888_congestion_feedback_) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + congestion_control_feedback_generator_.OnReceivedPacket(packet); + return; + } bool has_transport_sequence_number = packet.HasExtension() || packet.HasExtension(); @@ -108,12 +132,20 @@ void ReceiveSideCongestionController::OnReceivedPacket( } void ReceiveSideCongestionController::OnBitrateChanged(int bitrate_bps) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + DataRate send_bandwidth_estimate = DataRate::BitsPerSec(bitrate_bps); transport_sequence_number_feedback_generator_.OnSendBandwidthEstimateChanged( - DataRate::BitsPerSec(bitrate_bps)); + send_bandwidth_estimate); + congestion_control_feedback_generator_.OnSendBandwidthEstimateChanged( + send_bandwidth_estimate); } TimeDelta ReceiveSideCongestionController::MaybeProcess() { Timestamp now = env_.clock().CurrentTime(); + if (send_rfc8888_congestion_feedback_) { + RTC_DCHECK_RUN_ON(&sequence_checker_); + return congestion_control_feedback_generator_.Process(now); + } mutex_.Lock(); TimeDelta time_until_rbe = rbe_->Process(); mutex_.Unlock(); @@ -130,8 +162,11 @@ void ReceiveSideCongestionController::SetMaxDesiredReceiveBitrate( void ReceiveSideCongestionController::SetTransportOverhead( DataSize overhead_per_packet) { + RTC_DCHECK_RUN_ON(&sequence_checker_); transport_sequence_number_feedback_generator_.SetTransportOverhead( overhead_per_packet); + congestion_control_feedback_generator_.SetTransportOverhead( + overhead_per_packet); } } // namespace webrtc diff --git a/modules/congestion_controller/receive_side_congestion_controller_unittest.cc b/modules/congestion_controller/receive_side_congestion_controller_unittest.cc index 5788898439..8681560af3 100644 --- a/modules/congestion_controller/receive_side_congestion_controller_unittest.cc +++ b/modules/congestion_controller/receive_side_congestion_controller_unittest.cc @@ -11,17 +11,18 @@ #include "modules/congestion_controller/include/receive_side_congestion_controller.h" #include "api/environment/environment_factory.h" +#include "api/media_types.h" #include "api/test/network_emulation/create_cross_traffic.h" #include "api/test/network_emulation/cross_traffic.h" #include "api/units/data_rate.h" #include "api/units/data_size.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" -#include "modules/pacing/packet_router.h" #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "system_wrappers/include/clock.h" +#include "test/explicit_key_value_config.h" #include "test/gmock.h" #include "test/gtest.h" #include "test/scenario/scenario.h" @@ -34,6 +35,7 @@ using ::testing::_; using ::testing::AtLeast; using ::testing::ElementsAre; using ::testing::MockFunction; +using ::testing::SizeIs; constexpr DataRate kInitialBitrate = DataRate::BitsPerSec(60'000); @@ -81,6 +83,76 @@ TEST(ReceiveSideCongestionControllerTest, controller.SetMaxDesiredReceiveBitrate(DataRate::BitsPerSec(123)); } +TEST(ReceiveSideCongestionControllerTest, SendsRfc8888FeedbackIfForced) { + test::ExplicitKeyValueConfig field_trials( + "WebRTC-RFC8888CongestionControlFeedback/force_send:true/"); + MockFunction>)> + rtcp_sender; + MockFunction)> remb_sender; + SimulatedClock clock(123456); + ReceiveSideCongestionController controller( + CreateEnvironment(&clock, &field_trials), rtcp_sender.AsStdFunction(), + remb_sender.AsStdFunction(), nullptr); + + EXPECT_CALL(rtcp_sender, Call); + RtpPacketReceived packet; + packet.set_arrival_time(clock.CurrentTime()); + controller.OnReceivedPacket(packet, MediaType::VIDEO); + TimeDelta next_process = controller.MaybeProcess(); + clock.AdvanceTime(next_process); + next_process = controller.MaybeProcess(); +} + +TEST(ReceiveSideCongestionControllerTest, SendsRfc8888FeedbackIfEnabled) { + MockFunction>)> + rtcp_sender; + MockFunction)> remb_sender; + SimulatedClock clock(123456); + ReceiveSideCongestionController controller( + CreateEnvironment(&clock), rtcp_sender.AsStdFunction(), + remb_sender.AsStdFunction(), nullptr); + controller.EnablSendCongestionControlFeedbackAccordingToRfc8888(); + + EXPECT_CALL(rtcp_sender, Call) + .WillOnce( + [&](std::vector> rtcp_packets) { + ASSERT_THAT(rtcp_packets, SizeIs(1)); + rtc::Buffer buffer = rtcp_packets[0]->Build(); + rtcp::CommonHeader header; + EXPECT_TRUE(header.Parse(buffer.data(), buffer.size())); + EXPECT_EQ(header.fmt(), + rtcp::CongestionControlFeedback::kFeedbackMessageType); + }); + + RtpPacketReceived packet; + packet.set_arrival_time(clock.CurrentTime()); + controller.OnReceivedPacket(packet, MediaType::VIDEO); + TimeDelta next_process = controller.MaybeProcess(); + clock.AdvanceTime(next_process); + next_process = controller.MaybeProcess(); +} + +TEST(ReceiveSideCongestionControllerTest, + SendsNoFeedbackIfNotRfcRfc8888EnabledAndNoTransportFeedback) { + MockFunction>)> + rtcp_sender; + MockFunction)> remb_sender; + SimulatedClock clock(123456); + ReceiveSideCongestionController controller( + CreateEnvironment(&clock), rtcp_sender.AsStdFunction(), + remb_sender.AsStdFunction(), nullptr); + + // No Transport feedback is sent because received packet does not have + // transport sequence number rtp header extension. + EXPECT_CALL(rtcp_sender, Call).Times(0); + RtpPacketReceived packet; + packet.set_arrival_time(clock.CurrentTime()); + controller.OnReceivedPacket(packet, MediaType::VIDEO); + TimeDelta next_process = controller.MaybeProcess(); + clock.AdvanceTime(next_process); + next_process = controller.MaybeProcess(); +} + TEST(ReceiveSideCongestionControllerTest, ConvergesToCapacity) { Scenario s("receive_cc_unit/converge"); NetworkSimulationConfig net_conf; diff --git a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h index 7183d5ffec..3a657451c6 100644 --- a/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h +++ b/modules/remote_bitrate_estimator/congestion_control_feedback_generator.h @@ -44,10 +44,9 @@ namespace webrtc { class CongestionControlFeedbackGenerator : public RtpTransportFeedbackGenerator { public: - using RtcpSender = std::function> packets)>; - CongestionControlFeedbackGenerator(const Environment& env, - RtcpSender feedback_sender); + CongestionControlFeedbackGenerator( + const Environment& env, + RtpTransportFeedbackGenerator::RtcpSender feedback_sender); ~CongestionControlFeedbackGenerator() = default; void OnReceivedPacket(const RtpPacketReceived& packet) override; diff --git a/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h b/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h index 724b589a2a..d8356084a3 100644 --- a/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h +++ b/modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h @@ -11,6 +11,9 @@ #ifndef MODULES_REMOTE_BITRATE_ESTIMATOR_RTP_TRANSPORT_FEEDBACK_GENERATOR_H_ #define MODULES_REMOTE_BITRATE_ESTIMATOR_RTP_TRANSPORT_FEEDBACK_GENERATOR_H_ +#include +#include + #include "api/units/data_rate.h" #include "api/units/data_size.h" #include "api/units/time_delta.h" @@ -21,6 +24,10 @@ namespace webrtc { class RtpTransportFeedbackGenerator { public: + // Function intented to be used for sending RTCP messages generated by an + // implementation of this class. + using RtcpSender = std::function> packets)>; virtual ~RtpTransportFeedbackGenerator() = default; virtual void OnReceivedPacket(const RtpPacketReceived& packet) = 0; diff --git a/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h b/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h index 2291031f35..4c2fb31b0a 100644 --- a/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h +++ b/modules/remote_bitrate_estimator/transport_sequence_number_feedback_generator.h @@ -44,12 +44,8 @@ namespace webrtc { class TransportSequenceNumberFeedbackGenenerator : public RtpTransportFeedbackGenerator { public: - // Used for sending transport feedback messages when send side - // BWE is used. - using RtcpSender = std::function> packets)>; TransportSequenceNumberFeedbackGenenerator( - RtcpSender feedback_sender, + RtpTransportFeedbackGenerator::RtcpSender feedback_sender, NetworkStateEstimator* network_state_estimator); ~TransportSequenceNumberFeedbackGenenerator();