[RFC8888] Enable ALR probing when Congestion Control Feedback is enabled. When the RFC8888 Congestion Control Feedback (CCFB) experiment is enabled, send-side bandwidth estimation (BWE) is active, but TWCC might not be configured (as feedback is sent via CCFB instead). This CL updates VideoSendStreamImpl to check if CCFB is enabled when deciding whether to apply ALR probing and pacing settings. If CCFB is enabled, we allow ALR probing and queue time limits to be configured, but we do NOT set the pacing factor on the transport controller (as pacing is handled exclusively by the congestion controller in this mode). This CL also refactors the tests to remove the use of the internal configured_pacing_factor_ member, allowing it to be removed from VideoSendStreamImpl. Bug: webrtc:522844909, chromium:523643492 Change-Id: I8920e0551b5385de5426a37c926080d90883ce87 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/481601 Reviewed-by: Erik Språng <sprang@webrtc.org> Commit-Queue: Per Kjellander <perkj@webrtc.org> Reviewed-by: Björn Terelius <terelius@webrtc.org> Cr-Commit-Position: refs/heads/main@{#48000}
diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc index f84c779..0cac69d 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc
@@ -217,8 +217,12 @@ msg.pacer_queue->bytes()); } bandwidth_estimation_.UpdateEstimate(msg.at_time); + // Avoid ALR probing before the first transport feedback is received in + // order to avoid ALR probing if receive side BWE is used. probe_controller_->SetAlrStartTime( - alr_detector_.GetApplicationLimitedRegionStartTime()); + first_transport_feedback_received_ + ? alr_detector_.GetApplicationLimitedRegionStartTime() + : std::nullopt); auto probes = probe_controller_->Process(msg.at_time); update.probe_cluster_configs.insert(update.probe_cluster_configs.end(),
diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 1120e92..fda5f0b 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc
@@ -516,11 +516,6 @@ encoder_bitrate_priority_(encoder_config.bitrate_priority), encoder_av1_priority_bitrate_override_bps_( GetEncoderPriorityBitrate(config_.rtp.payload_name, - env_.field_trials())), - configured_pacing_factor_( - GetConfiguredPacingFactor(config_, - content_type_, - pacing_config_, env_.field_trials())) { RTC_DCHECK_GE(config_.rtp.payload_type, 0); RTC_DCHECK_LE(config_.rtp.payload_type, 127); @@ -533,9 +528,14 @@ std::optional<bool> enable_alr_bw_probing; - // If send-side BWE is enabled, check if we should apply updated probing and - // pacing settings. - if (configured_pacing_factor_) { + bool rfc8888_experiment_enabled = + env_.field_trials().IsEnabled("WebRTC-RFC8888CongestionControlFeedback"); + + // If send-side BWE, or RFC8888 congestion control feedback experiment is + // enabled, check if we should apply updated probing and pacing settings. + std::optional<float> pacing_factor_override = GetConfiguredPacingFactor( + config_, content_type_, pacing_config_, env_.field_trials()); + if (pacing_factor_override.has_value() || rfc8888_experiment_enabled) { std::optional<AlrExperimentSettings> alr_settings = GetAlrSettings(env_.field_trials(), content_type_); int queue_time_limit_ms; @@ -549,6 +549,11 @@ } transport_->SetQueueTimeLimit(queue_time_limit_ms); + if (!rfc8888_experiment_enabled) { + // In the RFC8888 experiment, the pacing factor is decided exclusively in + // the congestion controller, and SetPacingFactor is not allowed. + transport_->SetPacingFactor(*pacing_factor_override); + } } if (config_.periodic_alr_bandwidth_probing) { @@ -559,13 +564,10 @@ transport->EnablePeriodicAlrProbing(*enable_alr_bw_probing); } - if (configured_pacing_factor_) - transport_->SetPacingFactor(*configured_pacing_factor_); - - // Only request rotation at the source when we positively know that the remote - // side doesn't support the rotation extension. This allows us to prepare the - // encoder in the expectation that rotation is supported - which is the common - // case. + // Only request rotation at the source when we positively know that the + // remote side doesn't support the rotation extension. This allows us to + // prepare the encoder in the expectation that rotation is supported - which + // is the common case. bool rotation_applied = absl::c_none_of( config_.rtp.extensions, [](const RtpExtension& extension) { return extension.uri == RtpExtension::kVideoRotationUri; @@ -645,11 +647,6 @@ RTC_DCHECK_RUN_ON(&thread_checker_); rtp_video_sender_->SetCsrcs(csrcs); } - -std::optional<float> VideoSendStreamImpl::GetPacingFactorOverride() const { - return configured_pacing_factor_; -} - void VideoSendStreamImpl::StopPermanentlyAndGetRtpStates( VideoSendStreamImpl::RtpStateMap* rtp_state_map, VideoSendStreamImpl::RtpPayloadStateMap* payload_state_map) {
diff --git a/video/video_send_stream_impl.h b/video/video_send_stream_impl.h index a06272b..bc4cb30 100644 --- a/video/video_send_stream_impl.h +++ b/video/video_send_stream_impl.h
@@ -137,9 +137,6 @@ std::map<uint32_t, RtpPayloadState> GetRtpPayloadStates() const; - const std::optional<float>& configured_pacing_factor() const { - return configured_pacing_factor_; - } private: friend class test::VideoSendStreamPeer; @@ -163,7 +160,6 @@ SendDelayStats& send_delay_stats_; }; - std::optional<float> GetPacingFactorOverride() const; // Implements BitrateAllocatorObserver. uint32_t OnBitrateUpdated(BitrateAllocationUpdate update) override; std::optional<DataRate> GetUsedRate() const override; @@ -260,7 +256,6 @@ }; std::optional<VbaSendContext> video_bitrate_allocation_context_ RTC_GUARDED_BY(thread_checker_); - const std::optional<float> configured_pacing_factor_; }; } // namespace internal } // namespace webrtc
diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index f1c4784..6cbf89f 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc
@@ -772,8 +772,7 @@ config_.rtp.extensions.emplace_back(RtpExtension::kTransportSequenceNumberUri, kId); EXPECT_CALL(transport_controller_, - SetPacingFactor(kAlrProbingExperimentPaceMultiplier)) - .Times(1); + SetPacingFactor(kAlrProbingExperimentPaceMultiplier)); auto vss_impl = CreateVideoSendStreamImpl( TestVideoEncoderConfig(VideoEncoderConfig::ContentType::kScreen), &field_trials); @@ -788,7 +787,68 @@ auto vss_impl = CreateVideoSendStreamImpl( TestVideoEncoderConfig(VideoEncoderConfig::ContentType::kScreen), &field_trials); - EXPECT_CALL(transport_controller_, SetPacingFactor(_)).Times(0); + EXPECT_CALL(transport_controller_, SetPacingFactor).Times(0); + vss_impl->Start(); + vss_impl->Stop(); +} + +TEST_F(VideoSendStreamImplTest, + EnablesAlrProbingAndSetsPacingFactorWithTwccWithoutCcfb) { + auto field_trials = + SetFieldTrial("WebRTC-RFC8888CongestionControlFeedback", "Disabled"); + // We expect EnablePeriodicAlrProbing(true) to be called. + EXPECT_CALL(transport_controller_, EnablePeriodicAlrProbing(true)); + + // We expect SetQueueTimeLimit to be called. + EXPECT_CALL(transport_controller_, SetQueueTimeLimit); + + // We expect SetPacingFactor to be called with the default ALR experiment + // pacing factor (1.0). + EXPECT_CALL(transport_controller_, SetPacingFactor(1.0f)); + + config_.rtp.extensions.emplace_back(RtpExtension::kTransportSequenceNumberUri, + RtpHeaderExtensionId(1)); + + auto vss_impl = CreateVideoSendStreamImpl( + TestVideoEncoderConfig(VideoEncoderConfig::ContentType::kScreen), + &field_trials); + vss_impl->Start(); + vss_impl->Stop(); +} + +TEST_F(VideoSendStreamImplTest, DoesNotConfigurePacingOrAlrWithoutFeedback) { + auto field_trials = + SetFieldTrial("WebRTC-RFC8888CongestionControlFeedback", "Disabled"); + // We expect NONE of the pacing/ALR methods to be called on transport + // controller. + EXPECT_CALL(transport_controller_, EnablePeriodicAlrProbing).Times(0); + EXPECT_CALL(transport_controller_, SetQueueTimeLimit).Times(0); + EXPECT_CALL(transport_controller_, SetPacingFactor).Times(0); + + auto vss_impl = CreateVideoSendStreamImpl( + TestVideoEncoderConfig(VideoEncoderConfig::ContentType::kScreen), + &field_trials); + vss_impl->Start(); + vss_impl->Stop(); +} + +TEST_F(VideoSendStreamImplTest, + EnablesAlrProbingAndSetsQueueTimeLimitWithRfc8888) { + auto field_trials = SetFieldTrial("WebRTC-RFC8888CongestionControlFeedback", + "Enabled,offer:true"); + + // We expect EnablePeriodicAlrProbing(true) to be called. + EXPECT_CALL(transport_controller_, EnablePeriodicAlrProbing(true)); + + // We expect SetQueueTimeLimit to be called. + EXPECT_CALL(transport_controller_, SetQueueTimeLimit); + + // We expect SetPacingFactor NOT to be called. + EXPECT_CALL(transport_controller_, SetPacingFactor).Times(0); + + auto vss_impl = CreateVideoSendStreamImpl( + TestVideoEncoderConfig(VideoEncoderConfig::ContentType::kScreen), + &field_trials); vss_impl->Start(); vss_impl->Stop(); } @@ -797,7 +857,7 @@ auto vss_impl = CreateVideoSendStreamImpl( TestVideoEncoderConfig(VideoEncoderConfig::ContentType::kScreen)); - EXPECT_CALL(transport_controller_, SetPacingFactor(_)).Times(0); + EXPECT_CALL(transport_controller_, SetPacingFactor).Times(0); VideoStreamEncoderInterface::EncoderSink* const sink = static_cast<VideoStreamEncoderInterface::EncoderSink*>(vss_impl.get()); vss_impl->Start();
diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index 45b20f9..0f648a1 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc
@@ -92,7 +92,6 @@ #include "modules/video_coding/svc/scalable_video_controller.h" #include "rtc_base/checks.h" #include "rtc_base/event.h" -#include "rtc_base/experiments/alr_experiment.h" #include "rtc_base/logging.h" #include "rtc_base/network_route.h" #include "rtc_base/rate_limiter.h" @@ -101,7 +100,6 @@ #include "rtc_base/task_queue_for_test.h" #include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" -#include "rtc_base/unique_id_generator.h" #include "test/call_test.h" #include "test/configurable_frame_size_encoder.h" #include "test/create_test_environment.h" @@ -120,24 +118,8 @@ #include "test/video_test_constants.h" #include "video/config/video_encoder_config.h" #include "video/transport_adapter.h" -#include "video/video_send_stream_impl.h" namespace webrtc { -namespace test { -class VideoSendStreamPeer { - public: - explicit VideoSendStreamPeer(VideoSendStream* base_class_stream) - : internal_stream_( - static_cast<internal::VideoSendStreamImpl*>(base_class_stream)) {} - std::optional<float> GetPacingFactorOverride() const { - return internal_stream_->GetPacingFactorOverride(); - } - - private: - internal::VideoSendStreamImpl const* const internal_stream_; -}; -} // namespace test - namespace { constexpr RtpHeaderExtensionId kAbsSendTimeExtensionId(1); constexpr RtpHeaderExtensionId kTimestampOffsetExtensionId(2); @@ -3732,89 +3714,7 @@ RunBaseTest(&test); } -class PacingFactorObserver : public test::SendTest { - public: - PacingFactorObserver(bool configure_send_side, - std::optional<float> expected_pacing_factor) - : test::SendTest(test::VideoTestConstants::kDefaultTimeout), - configure_send_side_(configure_send_side), - expected_pacing_factor_(expected_pacing_factor) {} - void ModifyVideoConfigs( - VideoSendStream::Config* send_config, - std::vector<VideoReceiveStreamInterface::Config>* receive_configs, - VideoEncoderConfig* encoder_config) override { - // Check if send-side bwe extension is already present, and remove it if - // it is not desired. - bool has_send_side = false; - for (auto it = send_config->rtp.extensions.begin(); - it != send_config->rtp.extensions.end(); ++it) { - if (it->uri == RtpExtension::kTransportSequenceNumberUri) { - if (configure_send_side_) { - has_send_side = true; - } else { - send_config->rtp.extensions.erase(it); - } - break; - } - } - - if (configure_send_side_ && !has_send_side) { - UniqueNumberGenerator<int> unique_id_generator; - unique_id_generator.AddKnownId(0); // First valid RTP extension ID is 1. - for (const RtpExtension& extension : send_config->rtp.extensions) { - unique_id_generator.AddKnownId(extension.id.value()); - } - // Want send side, not present by default, so add it. - send_config->rtp.extensions.emplace_back( - RtpExtension::kTransportSequenceNumberUri, - RtpHeaderExtensionId(unique_id_generator.GenerateNumber())); - } - - // ALR only enabled for screenshare. - encoder_config->content_type = VideoEncoderConfig::ContentType::kScreen; - } - - void OnVideoStreamsCreated(VideoSendStream* send_stream, - const std::vector<VideoReceiveStreamInterface*>& - receive_streams) override { - auto internal_send_peer = test::VideoSendStreamPeer(send_stream); - // Video streams created, check that pacing factor is correctly configured. - EXPECT_EQ(expected_pacing_factor_, - internal_send_peer.GetPacingFactorOverride()); - observation_complete_.Set(); - } - - void PerformTest() override { - EXPECT_TRUE(Wait()) << "Timed out while waiting for stream creation."; - } - - private: - const bool configure_send_side_; - const std::optional<float> expected_pacing_factor_; -}; - -constexpr absl::string_view kAlrProbingExperimentValue = "1.0,2875,80,40,-60,3"; -constexpr float kAlrProbingExperimentPaceMultiplier = 1.0f; - -TEST_F(VideoSendStreamTest, AlrConfiguredWhenSendSideOn) { - field_trials().Set( - AlrExperimentSettings::kScreenshareProbingBweExperimentName, - kAlrProbingExperimentValue); - // Send-side bwe on, use pacing factor from `kAlrProbingExperiment` above. - PacingFactorObserver test_with_send_side(true, - kAlrProbingExperimentPaceMultiplier); - RunBaseTest(&test_with_send_side); -} - -TEST_F(VideoSendStreamTest, AlrNotConfiguredWhenSendSideOff) { - field_trials().Set( - AlrExperimentSettings::kScreenshareProbingBweExperimentName, - kAlrProbingExperimentValue); - // Send-side bwe off, use configuration should not be overridden. - PacingFactorObserver test_without_send_side(false, std::nullopt); - RunBaseTest(&test_without_send_side); -} // Test class takes as argument a function pointer to reset the send // stream and call OnVideoStreamsCreated. This is necessary since you cannot @@ -3878,16 +3778,7 @@ if (done_) return; - auto internal_send_peer = test::VideoSendStreamPeer(send_stream_); - float pacing_factor = - internal_send_peer.GetPacingFactorOverride().value_or(0.0f); - float expected_pacing_factor = 1.1; // Strict pacing factor. - VideoSendStream::Stats stats = send_stream_->GetStats(); - if (stats.content_type == VideoContentType::SCREENSHARE) { - expected_pacing_factor = 1.0f; // Currently used pacing factor in ALR. - } - EXPECT_NEAR(expected_pacing_factor, pacing_factor, 1e-6); // Wait until at least kMinPacketsToSend packets to be sent, so that // some frames would be encoded.