From f2e3e7a25aa34f7af63b0e83970145031ac3bc42 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Fri, 6 Apr 2018 17:16:06 +0200 Subject: [PATCH] Removed observer from probe controller. Replacing observer interface with polling for pending probe clusters. The purpose is to make it easier to reason about and control side effects and to prepare for a similar change in the network controller interface. Bug: webrtc:8415 Change-Id: I8101cfda22e640a8e0fa75f3f6e63876db826a89 Reviewed-on: https://webrtc-review.googlesource.com/66881 Commit-Queue: Sebastian Jansson Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#22775} --- .../goog_cc/goog_cc_network_control.cc | 7 +- .../goog_cc/probe_controller.cc | 13 +- .../goog_cc/probe_controller.h | 11 +- .../goog_cc/probe_controller_unittest.cc | 123 +++++++++--------- .../rtp/send_side_congestion_controller.cc | 1 + ...end_side_congestion_controller_unittest.cc | 1 + 6 files changed, 85 insertions(+), 71 deletions(-) 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 0958935fce..517ef1d69e 100644 --- a/modules/congestion_controller/goog_cc/goog_cc_network_control.cc +++ b/modules/congestion_controller/goog_cc/goog_cc_network_control.cc @@ -109,7 +109,7 @@ GoogCcNetworkController::GoogCcNetworkController( NetworkControllerConfig config) : event_log_(event_log), observer_(observer), - probe_controller_(new ProbeController(observer_)), + probe_controller_(new ProbeController()), bandwidth_estimation_( rtc::MakeUnique(event_log_)), alr_detector_(rtc::MakeUnique()), @@ -167,6 +167,11 @@ void GoogCcNetworkController::OnProcessInterval(ProcessInterval msg) { alr_detector_->GetApplicationLimitedRegionStartTime(); probe_controller_->SetAlrStartTimeMs(start_time_ms); probe_controller_->Process(msg.at_time.ms()); + + for (const ProbeClusterConfig& probe : + probe_controller_->GetAndResetPendingProbes()) { + observer_->OnProbeClusterConfig(probe); + } MaybeTriggerOnNetworkChanged(msg.at_time); } diff --git a/modules/congestion_controller/goog_cc/probe_controller.cc b/modules/congestion_controller/goog_cc/probe_controller.cc index 2f1b5b8061..44836162f8 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/modules/congestion_controller/goog_cc/probe_controller.cc @@ -76,8 +76,7 @@ constexpr char kBweRapidRecoveryExperiment[] = } // namespace -ProbeController::ProbeController(NetworkControllerObserver* observer) - : observer_(observer), enable_periodic_alr_probing_(false) { +ProbeController::ProbeController() : enable_periodic_alr_probing_(false) { Reset(0); in_rapid_recovery_experiment_ = webrtc::field_trial::FindFullName( kBweRapidRecoveryExperiment) == "Enabled"; @@ -287,6 +286,14 @@ void ProbeController::Process(int64_t at_time_ms) { } } +std::vector ProbeController::GetAndResetPendingProbes() { + if (pending_probes_.empty()) + return std::vector(); + std::vector pending_probes; + pending_probes_.swap(pending_probes); + return pending_probes; +} + void ProbeController::InitiateProbing( int64_t now_ms, std::initializer_list bitrates_to_probe, @@ -305,7 +312,7 @@ void ProbeController::InitiateProbing( config.target_data_rate = DataRate::bps(rtc::dchecked_cast(bitrate)); config.target_duration = TimeDelta::ms(kMinProbeDurationMs); config.target_probe_count = kMinProbePacketsSent; - observer_->OnProbeClusterConfig(config); + pending_probes_.push_back(config); } time_last_probing_initiated_ms_ = now_ms; if (probe_further) { diff --git a/modules/congestion_controller/goog_cc/probe_controller.h b/modules/congestion_controller/goog_cc/probe_controller.h index 5649a99b11..6434b0ec8c 100644 --- a/modules/congestion_controller/goog_cc/probe_controller.h +++ b/modules/congestion_controller/goog_cc/probe_controller.h @@ -14,6 +14,7 @@ #include #include +#include #include "api/optional.h" #include "modules/congestion_controller/network_control/include/network_control.h" @@ -29,7 +30,7 @@ namespace webrtc_cc { // bitrate is adjusted by an application. class ProbeController { public: - explicit ProbeController(NetworkControllerObserver* observer); + ProbeController(); ~ProbeController(); void SetBitrates(int64_t min_bitrate_bps, @@ -59,6 +60,8 @@ class ProbeController { void Process(int64_t at_time_ms); + std::vector GetAndResetPendingProbes(); + private: enum class State { // Initial state where no probing has been triggered yet. @@ -74,8 +77,6 @@ class ProbeController { std::initializer_list bitrates_to_probe, bool probe_further); - NetworkControllerObserver* const observer_; - bool network_available_; State state_; int64_t min_bitrate_to_probe_further_bps_; @@ -96,7 +97,9 @@ class ProbeController { int64_t mid_call_probing_bitrate_bps_; int64_t mid_call_probing_succcess_threshold_; - RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(ProbeController); + std::vector pending_probes_; + + RTC_DISALLOW_COPY_AND_ASSIGN(ProbeController); }; } // namespace webrtc_cc diff --git a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc index 89e329bc41..52903acf49 100644 --- a/modules/congestion_controller/goog_cc/probe_controller_unittest.cc +++ b/modules/congestion_controller/goog_cc/probe_controller_unittest.cc @@ -39,23 +39,12 @@ constexpr int kExponentialProbingTimeoutMs = 5000; constexpr int kAlrProbeInterval = 5000; constexpr int kAlrEndedTimeoutMs = 3000; constexpr int kBitrateDropTimeoutMs = 5000; - -inline Matcher DataRateEqBps(int bps) { - return Field(&ProbeClusterConfig::target_data_rate, DataRate::bps(bps)); -} -class MockNetworkControllerObserver : public NetworkControllerObserver { - public: - MOCK_METHOD1(OnCongestionWindow, void(CongestionWindow)); - MOCK_METHOD1(OnPacerConfig, void(PacerConfig)); - MOCK_METHOD1(OnProbeClusterConfig, void(ProbeClusterConfig)); - MOCK_METHOD1(OnTargetTransferRate, void(TargetTransferRate)); -}; } // namespace class ProbeControllerTest : public ::testing::Test { protected: ProbeControllerTest() : clock_(100000000L) { - probe_controller_.reset(new ProbeController(&cluster_handler_)); + probe_controller_.reset(new ProbeController()); } ~ProbeControllerTest() override {} @@ -69,107 +58,110 @@ class ProbeControllerTest : public ::testing::Test { int64_t NowMs() { return clock_.TimeInMilliseconds(); } SimulatedClock clock_; - NiceMock cluster_handler_; std::unique_ptr probe_controller_; }; TEST_F(ProbeControllerTest, InitiatesProbingAtStart) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(AtLeast(2)); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); + EXPECT_GE(probe_controller_->GetAndResetPendingProbes().size(), 2u); } TEST_F(ProbeControllerTest, ProbeOnlyWhenNetworkIsUp) { SetNetworkAvailable(false); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); - - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(AtLeast(2)); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); SetNetworkAvailable(true); + EXPECT_GE(probe_controller_->GetAndResetPendingProbes().size(), 2u); } TEST_F(ProbeControllerTest, InitiatesProbingOnMaxBitrateIncrease) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(AtLeast(2)); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); // Long enough to time out exponential probing. clock_.AdvanceTimeMilliseconds(kExponentialProbingTimeoutMs); probe_controller_->SetEstimatedBitrate(kStartBitrateBps, NowMs()); probe_controller_->Process(NowMs()); + EXPECT_GE(probe_controller_->GetAndResetPendingProbes().size(), 2u); - EXPECT_CALL(cluster_handler_, - OnProbeClusterConfig(DataRateEqBps(kMaxBitrateBps + 100))); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps + 100, NowMs()); + + EXPECT_EQ( + probe_controller_->GetAndResetPendingProbes()[0].target_data_rate.bps(), + kMaxBitrateBps + 100); } TEST_F(ProbeControllerTest, InitiatesProbingOnMaxBitrateIncreaseAtMaxBitrate) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(AtLeast(2)); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); // Long enough to time out exponential probing. clock_.AdvanceTimeMilliseconds(kExponentialProbingTimeoutMs); probe_controller_->SetEstimatedBitrate(kStartBitrateBps, NowMs()); probe_controller_->Process(NowMs()); + EXPECT_GE(probe_controller_->GetAndResetPendingProbes().size(), 2u); probe_controller_->SetEstimatedBitrate(kMaxBitrateBps, NowMs()); - EXPECT_CALL(cluster_handler_, - OnProbeClusterConfig(DataRateEqBps(kMaxBitrateBps + 100))); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps + 100, NowMs()); + EXPECT_EQ( + probe_controller_->GetAndResetPendingProbes()[0].target_data_rate.bps(), + kMaxBitrateBps + 100); } TEST_F(ProbeControllerTest, TestExponentialProbing) { probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); + probe_controller_->GetAndResetPendingProbes(); // Repeated probe should only be sent when estimated bitrate climbs above // 0.7 * 6 * kStartBitrateBps = 1260. - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); probe_controller_->SetEstimatedBitrate(1000, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(DataRateEqBps(2 * 1800))); probe_controller_->SetEstimatedBitrate(1800, NowMs()); + EXPECT_EQ( + probe_controller_->GetAndResetPendingProbes()[0].target_data_rate.bps(), + 2 * 1800); } TEST_F(ProbeControllerTest, TestExponentialProbingTimeout) { probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); - + probe_controller_->GetAndResetPendingProbes(); // Advance far enough to cause a time out in waiting for probing result. clock_.AdvanceTimeMilliseconds(kExponentialProbingTimeoutMs); probe_controller_->Process(NowMs()); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); probe_controller_->SetEstimatedBitrate(1800, NowMs()); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); } TEST_F(ProbeControllerTest, RequestProbeInAlr) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(2); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(DataRateEqBps(0.85 * 500))) - .Times(1); + EXPECT_GE(probe_controller_->GetAndResetPendingProbes().size(), 2u); + probe_controller_->SetAlrStartTimeMs(clock_.TimeInMilliseconds()); clock_.AdvanceTimeMilliseconds(kAlrProbeInterval + 1); probe_controller_->Process(NowMs()); probe_controller_->SetEstimatedBitrate(250, NowMs()); probe_controller_->RequestProbe(NowMs()); + + std::vector probes = + probe_controller_->GetAndResetPendingProbes(); + EXPECT_EQ(probes.size(), 1u); + EXPECT_EQ(probes[0].target_data_rate.bps(), 0.85 * 500); } TEST_F(ProbeControllerTest, RequestProbeWhenAlrEndedRecently) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(2); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(DataRateEqBps(0.85 * 500))) - .Times(1); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 2u); + probe_controller_->SetAlrStartTimeMs(rtc::nullopt); clock_.AdvanceTimeMilliseconds(kAlrProbeInterval + 1); probe_controller_->Process(NowMs()); @@ -177,15 +169,19 @@ TEST_F(ProbeControllerTest, RequestProbeWhenAlrEndedRecently) { probe_controller_->SetAlrEndedTimeMs(clock_.TimeInMilliseconds()); clock_.AdvanceTimeMilliseconds(kAlrEndedTimeoutMs - 1); probe_controller_->RequestProbe(NowMs()); + + std::vector probes = + probe_controller_->GetAndResetPendingProbes(); + EXPECT_EQ(probes.size(), 1u); + EXPECT_EQ(probes[0].target_data_rate.bps(), 0.85 * 500); } TEST_F(ProbeControllerTest, RequestProbeWhenAlrNotEndedRecently) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(2); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 2u); + probe_controller_->SetAlrStartTimeMs(rtc::nullopt); clock_.AdvanceTimeMilliseconds(kAlrProbeInterval + 1); probe_controller_->Process(NowMs()); @@ -193,65 +189,63 @@ TEST_F(ProbeControllerTest, RequestProbeWhenAlrNotEndedRecently) { probe_controller_->SetAlrEndedTimeMs(clock_.TimeInMilliseconds()); clock_.AdvanceTimeMilliseconds(kAlrEndedTimeoutMs + 1); probe_controller_->RequestProbe(NowMs()); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); } TEST_F(ProbeControllerTest, RequestProbeWhenBweDropNotRecent) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(2); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 2u); + probe_controller_->SetAlrStartTimeMs(clock_.TimeInMilliseconds()); clock_.AdvanceTimeMilliseconds(kAlrProbeInterval + 1); probe_controller_->Process(NowMs()); probe_controller_->SetEstimatedBitrate(250, NowMs()); clock_.AdvanceTimeMilliseconds(kBitrateDropTimeoutMs + 1); probe_controller_->RequestProbe(NowMs()); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); } TEST_F(ProbeControllerTest, PeriodicProbing) { - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(2); probe_controller_->EnablePeriodicAlrProbing(true); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 2u); int64_t start_time = clock_.TimeInMilliseconds(); // Expect the controller to send a new probe after 5s has passed. - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(DataRateEqBps(1000))) - .Times(1); probe_controller_->SetAlrStartTimeMs(start_time); clock_.AdvanceTimeMilliseconds(5000); probe_controller_->Process(NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); + + std::vector probes = + probe_controller_->GetAndResetPendingProbes(); + EXPECT_EQ(probes.size(), 1u); + EXPECT_EQ(probes[0].target_data_rate.bps(), 1000); // The following probe should be sent at 10s into ALR. - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); probe_controller_->SetAlrStartTimeMs(start_time); clock_.AdvanceTimeMilliseconds(4000); probe_controller_->Process(NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(1); probe_controller_->SetAlrStartTimeMs(start_time); clock_.AdvanceTimeMilliseconds(1000); probe_controller_->Process(NowMs()); probe_controller_->SetEstimatedBitrate(500, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 1u); } TEST_F(ProbeControllerTest, PeriodicProbingAfterReset) { - NiceMock local_handler; - probe_controller_.reset(new ProbeController(&local_handler)); + probe_controller_.reset(new ProbeController()); int64_t alr_start_time = clock_.TimeInMilliseconds(); probe_controller_->SetAlrStartTimeMs(alr_start_time); - EXPECT_CALL(local_handler, OnProbeClusterConfig(_)).Times(2); probe_controller_->EnablePeriodicAlrProbing(true); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); @@ -259,17 +253,19 @@ TEST_F(ProbeControllerTest, PeriodicProbingAfterReset) { clock_.AdvanceTimeMilliseconds(10000); probe_controller_->Process(NowMs()); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 2u); - EXPECT_CALL(local_handler, OnProbeClusterConfig(_)).Times(2); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps, NowMs()); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 2u); // Make sure we use |kStartBitrateBps| as the estimated bitrate // until SetEstimatedBitrate is called with an updated estimate. clock_.AdvanceTimeMilliseconds(10000); - EXPECT_CALL(local_handler, - OnProbeClusterConfig(DataRateEqBps(kStartBitrateBps * 2))); probe_controller_->Process(NowMs()); + EXPECT_EQ( + probe_controller_->GetAndResetPendingProbes()[0].target_data_rate.bps(), + kStartBitrateBps * 2); } TEST_F(ProbeControllerTest, TestExponentialProbingOverflow) { @@ -277,15 +273,16 @@ TEST_F(ProbeControllerTest, TestExponentialProbingOverflow) { probe_controller_->SetBitrates(kMinBitrateBps, 10 * kMbpsMultiplier, 100 * kMbpsMultiplier, NowMs()); - // Verify that probe bitrate is capped at the specified max bitrate. - EXPECT_CALL(cluster_handler_, - OnProbeClusterConfig(DataRateEqBps(100 * kMbpsMultiplier))); probe_controller_->SetEstimatedBitrate(60 * kMbpsMultiplier, NowMs()); - testing::Mock::VerifyAndClearExpectations(&cluster_handler_); + + // Verify that probe bitrate is capped at the specified max bitrate. + EXPECT_EQ( + probe_controller_->GetAndResetPendingProbes()[2].target_data_rate.bps(), + 100 * kMbpsMultiplier); // Verify that repeated probes aren't sent. - EXPECT_CALL(cluster_handler_, OnProbeClusterConfig(_)).Times(0); probe_controller_->SetEstimatedBitrate(100 * kMbpsMultiplier, NowMs()); + EXPECT_EQ(probe_controller_->GetAndResetPendingProbes().size(), 0u); } } // namespace test diff --git a/modules/congestion_controller/rtp/send_side_congestion_controller.cc b/modules/congestion_controller/rtp/send_side_congestion_controller.cc index 0075919d32..e1defdc140 100644 --- a/modules/congestion_controller/rtp/send_side_congestion_controller.cc +++ b/modules/congestion_controller/rtp/send_side_congestion_controller.cc @@ -348,6 +348,7 @@ void SendSideCongestionController::MaybeCreateControllers() { controller_ = controller_factory_->Create(control_handler_.get(), initial_config_); UpdateStreamsConfig(); + UpdateControllerWithTimeInterval(); StartProcessPeriodicTasks(); } diff --git a/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc b/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc index b002b6d780..56f010f594 100644 --- a/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc +++ b/modules/congestion_controller/rtp/send_side_congestion_controller_unittest.cc @@ -361,6 +361,7 @@ TEST_F(SendSideCongestionControllerTest, ProbeOnRouteChange) { route.local_network_id = 1; controller_->OnNetworkRouteChanged(route, 2 * kInitialBitrateBps, 0, 20 * kInitialBitrateBps); + controller_->Process(); } // Estimated bitrate reduced when the feedbacks arrive with such a long delay,