Ensure ScreamNetworkController update target rate when network available Also implement resetting Scream when network route change. Bug: webrtc:447039083 Change-Id: Ie6e3feb64f99f71bb416a377fa605a232e5fd184 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/413922 Commit-Queue: Per Kjellander <perkj@webrtc.org> Reviewed-by: Diep Bui <diepbp@webrtc.org> Cr-Commit-Position: refs/heads/main@{#45841}
diff --git a/modules/congestion_controller/scream/scream_network_controller.cc b/modules/congestion_controller/scream/scream_network_controller.cc index d8640bb..aa3a35e 100644 --- a/modules/congestion_controller/scream/scream_network_controller.cc +++ b/modules/congestion_controller/scream/scream_network_controller.cc
@@ -17,17 +17,21 @@ #include "api/transport/network_types.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "logging/rtc_event_log/events/rtc_event_bwe_update_delay_based.h" +#include "modules/congestion_controller/scream/scream_v2.h" #include "rtc_base/logging.h" namespace webrtc { +static constexpr DataRate kDefaultStartRate = DataRate::KilobitsPerSec(300); + ScreamNetworkController::ScreamNetworkController(NetworkControllerConfig config) : env_(config.env), - scream_(env_), + scream_(std::make_unique<ScreamV2>(env_)), target_rate_constraints_(config.constraints) { if (config.constraints.min_data_rate.has_value() || config.constraints.max_data_rate.has_value()) { - scream_.SetTargetBitrateConstraints( + scream_->SetTargetBitrateConstraints( config.constraints.min_data_rate.value_or(DataRate::Zero()), config.constraints.max_data_rate.value_or(DataRate::PlusInfinity())); } @@ -35,12 +39,36 @@ NetworkControlUpdate ScreamNetworkController::OnNetworkAvailability( NetworkAvailability msg) { + RTC_LOG(LS_INFO) << " OnNetworkAvailability network_available:" + << msg.network_available; + if (msg.network_available) { + // TODO: bugs.webrtc.org/447037083 - rtt must currently be set on every + // update. But here it is not yet known. + return CreateUpdate( + msg.at_time, + target_rate_constraints_.starting_rate.value_or(kDefaultStartRate), + /*rtt*/ TimeDelta::Zero()); + } return NetworkControlUpdate(); } NetworkControlUpdate ScreamNetworkController::OnNetworkRouteChange( NetworkRouteChange msg) { - return NetworkControlUpdate(); + RTC_LOG(LS_INFO) << " OnNetworkRouteChange, resetting ScreamV2."; + target_rate_constraints_ = msg.constraints; + scream_ = std::make_unique<ScreamV2>(env_); + // TODO: bugs.webrtc.org/447037083 - We should use the minimum rate from + // constraints, REMB and remote network state estimates. + scream_->SetTargetBitrateConstraints( + target_rate_constraints_.min_data_rate.value_or(DataRate::Zero()), + target_rate_constraints_.max_data_rate.value_or( + DataRate::PlusInfinity())); + // TODO: bugs.webrtc.org/447037083 - rtt must currently be set on every + // update. But here it is not yet known. + return CreateUpdate( + msg.at_time, + target_rate_constraints_.starting_rate.value_or(kDefaultStartRate), + /*rtt*/ TimeDelta::Zero()); } NetworkControlUpdate ScreamNetworkController::OnProcessInterval( @@ -85,7 +113,7 @@ // TODO: bugs.webrtc.org/447037083 - We should use the minimum rate from // constraints, REMB and remote network state estimates. - scream_.SetTargetBitrateConstraints( + scream_->SetTargetBitrateConstraints( target_rate_constraints_.min_data_rate.value_or(DataRate::Zero()), target_rate_constraints_.max_data_rate.value_or( DataRate::PlusInfinity())); @@ -107,19 +135,23 @@ NetworkControlUpdate ScreamNetworkController::OnTransportPacketsFeedback( TransportPacketsFeedback msg) { - DataRate target_rate = scream_.OnTransportPacketsFeedback(msg); + DataRate target_rate = scream_->OnTransportPacketsFeedback(msg); // TODO: bugs.webrtc.org/447037083 - Should we add separate event for Scream? env_.event_log().Log(std::make_unique<RtcEventBweUpdateDelayBased>( target_rate.bps(), BandwidthUsage::kBwNormal)); + return CreateUpdate(msg.feedback_time, target_rate, msg.smoothed_rtt); +} +NetworkControlUpdate ScreamNetworkController::CreateUpdate(Timestamp now, + DataRate target_rate, + TimeDelta rtt) { TargetTransferRate target_rate_msg; - target_rate_msg.at_time = msg.feedback_time; + target_rate_msg.at_time = now; target_rate_msg.target_rate = target_rate; - target_rate_msg.network_estimate.at_time = msg.feedback_time; - target_rate_msg.network_estimate.round_trip_time = msg.smoothed_rtt; - + target_rate_msg.network_estimate.at_time = now; + target_rate_msg.network_estimate.round_trip_time = rtt; // TODO: bugs.webrtc.org/447037083 - bwe_period must currently be set but // it seems like it is not used for anything sensible. Try to remove it. target_rate_msg.network_estimate.bwe_period = TimeDelta::Millis(25);
diff --git a/modules/congestion_controller/scream/scream_network_controller.h b/modules/congestion_controller/scream/scream_network_controller.h index ebb1e35..403c05e 100644 --- a/modules/congestion_controller/scream/scream_network_controller.h +++ b/modules/congestion_controller/scream/scream_network_controller.h
@@ -11,10 +11,14 @@ #ifndef MODULES_CONGESTION_CONTROLLER_SCREAM_SCREAM_NETWORK_CONTROLLER_H_ #define MODULES_CONGESTION_CONTROLLER_SCREAM_SCREAM_NETWORK_CONTROLLER_H_ +#include <memory> + #include "api/environment/environment.h" #include "api/transport/network_control.h" #include "api/transport/network_types.h" #include "api/units/data_rate.h" +#include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "modules/congestion_controller/scream/scream_v2.h" namespace webrtc { @@ -40,10 +44,13 @@ NetworkControlUpdate OnNetworkStateEstimate(NetworkStateEstimate) override; private: + NetworkControlUpdate CreateUpdate(Timestamp now, + DataRate target_rate, + TimeDelta rtt); PacerConfig GetPacerConfig(DataRate target_rate) const; Environment env_; - ScreamV2 scream_; + std::unique_ptr<ScreamV2> scream_; TargetRateConstraints target_rate_constraints_; };
diff --git a/modules/congestion_controller/scream/scream_network_controller_unittest.cc b/modules/congestion_controller/scream/scream_network_controller_unittest.cc index 19b8636..33df67e 100644 --- a/modules/congestion_controller/scream/scream_network_controller_unittest.cc +++ b/modules/congestion_controller/scream/scream_network_controller_unittest.cc
@@ -31,6 +31,23 @@ ScreamNetworkController scream_controller(config); } +TEST(ScreamControllerTest, OnNetworkAvailabilityUpdatesTargetRateAndPacerRate) { + SimulatedClock clock(Timestamp::Seconds(1'234)); + Environment env = CreateTestEnvironment({.time = &clock}); + NetworkControllerConfig config(env); + config.constraints.starting_rate = DataRate::KilobitsPerSec(123); + ScreamNetworkController scream_controller(config); + + NetworkControlUpdate update = + scream_controller.OnNetworkAvailability({.network_available = true}); + ASSERT_TRUE(update.has_updates()); + ASSERT_TRUE(update.target_rate.has_value()); + EXPECT_EQ(update.target_rate->target_rate, config.constraints.starting_rate); + ASSERT_TRUE(update.pacer_config); + EXPECT_EQ(update.pacer_config->data_window, + *config.constraints.starting_rate * 1.5 * TimeDelta::Seconds(1)); +} + TEST(ScreamControllerTest, OnTransportPacketsFeedbackUpdatesTargetRateAndPacerRate) { SimulatedClock clock(Timestamp::Seconds(1'234)); @@ -49,12 +66,48 @@ ASSERT_TRUE(update.has_updates()); ASSERT_TRUE(update.target_rate.has_value()); EXPECT_GT(update.target_rate->target_rate, DataRate::KilobitsPerSec(100)); - ASSERT_TRUE(update.pacer_config); EXPECT_EQ(update.pacer_config->data_window, update.target_rate->target_rate * 1.5 * TimeDelta::Seconds(1)); } +TEST(ScreamControllerTest, + OnNetworkRouteChangeResetsScreamAndUpdatesTargetRate) { + SimulatedClock clock(Timestamp::Seconds(1'234)); + Environment env = CreateTestEnvironment({.time = &clock}); + NetworkControllerConfig config(env); + config.constraints.starting_rate = DataRate::KilobitsPerSec(50); + ScreamNetworkController scream_controller(config); + + CcFeedbackGenerator feedback_generator({}); + DataRate send_rate = DataRate::KilobitsPerSec(100); + for (int i = 0; i < 10; ++i) { + TransportPacketsFeedback feedback = + feedback_generator.ProcessUntilNextFeedback(send_rate, clock); + NetworkControlUpdate update = + scream_controller.OnTransportPacketsFeedback(feedback); + if (update.target_rate.has_value()) { + send_rate = update.target_rate->target_rate; + } + } + ASSERT_GT(send_rate, DataRate::KilobitsPerSec(50)); + + NetworkRouteChange route_change; + route_change.constraints.starting_rate = config.constraints.starting_rate = + DataRate::KilobitsPerSec(123); + route_change.at_time = clock.CurrentTime(); + NetworkControlUpdate update = + scream_controller.OnNetworkRouteChange(route_change); + ASSERT_TRUE(update.has_updates()); + ASSERT_TRUE(update.target_rate.has_value()); + EXPECT_EQ(update.target_rate->target_rate, + route_change.constraints.starting_rate); + ASSERT_TRUE(update.pacer_config); + EXPECT_EQ( + update.pacer_config->data_window, + *route_change.constraints.starting_rate * 1.5 * TimeDelta::Seconds(1)); +} + TEST(ScreamControllerTest, TargetRateRampsUptoTargetConstraints) { SimulatedClock clock(Timestamp::Seconds(1'234)); Environment env = CreateTestEnvironment({.time = &clock});