From 9cef11b75e5173cef3995155f5de80f8710fccae Mon Sep 17 00:00:00 2001 From: sergeyu Date: Fri, 2 Dec 2016 11:03:01 -0800 Subject: [PATCH] Fix exponential probing in ProbeController. https://codereview.webrtc.org/2504023002 broke exponential probing. After that change ProbeController stops exponential probes prematurely: it goes to kProbingComplete state if SetEstimatedBitrate() is called with bitrate lower than min_bitrate_to_probe_further_bps_, which always happens with the first pair of probes. As result it wasn't sending repeated probes as it should. This change fixes that issue by moving probe expieration logic to ProbeContoller::Process(). This also ensures that the controller goes to kProbingComplete state as soon as probing timeout expired, without waiting for the next SetEstimatedBitrate() call. BUG=669421 Review-Url: https://codereview.webrtc.org/2546613003 Cr-Commit-Position: refs/heads/master@{#15392} --- .../congestion_controller/probe_controller.cc | 44 +++++++++---------- .../probe_controller_unittest.cc | 12 ++++- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/webrtc/modules/congestion_controller/probe_controller.cc b/webrtc/modules/congestion_controller/probe_controller.cc index 757e2a1626..10573c3029 100644 --- a/webrtc/modules/congestion_controller/probe_controller.cc +++ b/webrtc/modules/congestion_controller/probe_controller.cc @@ -121,28 +121,17 @@ void ProbeController::SetEstimatedBitrate(int bitrate_bps) { int64_t now_ms = clock_->TimeInMilliseconds(); if (state_ == State::kWaitingForProbingResult) { - if ((now_ms - time_last_probing_initiated_ms_) > - kMaxWaitingTimeForProbingResultMs) { - LOG(LS_INFO) << "kWaitingForProbingResult: timeout"; - state_ = State::kProbingComplete; - min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled; - } else { - // Continue probing if probing results indicate channel has greater - // capacity. - LOG(LS_INFO) << "Measured bitrate: " << bitrate_bps - << " Minimum to probe further: " - << min_bitrate_to_probe_further_bps_; - if (min_bitrate_to_probe_further_bps_ != kExponentialProbingDisabled && - bitrate_bps > min_bitrate_to_probe_further_bps_) { - // Double the probing bitrate and expect a minimum of 25% gain to - // continue probing. - InitiateProbing(now_ms, {2 * bitrate_bps}, - bitrate_bps * kRepeatedProbeMinPercentage / 100); - } else { - // Stop exponential probing. - state_ = State::kProbingComplete; - min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled; - } + // Continue probing if probing results indicate channel has greater + // capacity. + LOG(LS_INFO) << "Measured bitrate: " << bitrate_bps + << " Minimum to probe further: " + << min_bitrate_to_probe_further_bps_; + if (min_bitrate_to_probe_further_bps_ != kExponentialProbingDisabled && + bitrate_bps > min_bitrate_to_probe_further_bps_) { + // Double the probing bitrate and expect a minimum of 25% gain to + // continue probing. + InitiateProbing(now_ms, {2 * bitrate_bps}, + bitrate_bps * kRepeatedProbeMinPercentage / 100); } } @@ -182,6 +171,16 @@ void ProbeController::EnablePeriodicAlrProbing(bool enable) { void ProbeController::Process() { rtc::CritScope cs(&critsect_); + int64_t now_ms = clock_->TimeInMilliseconds(); + + if (state_ == State::kWaitingForProbingResult && + (now_ms - time_last_probing_initiated_ms_) > + kMaxWaitingTimeForProbingResultMs) { + LOG(LS_INFO) << "kWaitingForProbingResult: timeout"; + state_ = State::kProbingComplete; + min_bitrate_to_probe_further_bps_ = kExponentialProbingDisabled; + } + if (state_ != State::kProbingComplete || !enable_periodic_alr_probing_) return; @@ -189,7 +188,6 @@ void ProbeController::Process() { rtc::Optional alr_start_time = pacer_->GetApplicationLimitedRegionStartTime(); if (alr_start_time) { - int64_t now_ms = clock_->TimeInMilliseconds(); int64_t next_probe_time_ms = std::max(*alr_start_time, time_last_probing_initiated_ms_) + kAlrPeriodicProbingIntervalMs; diff --git a/webrtc/modules/congestion_controller/probe_controller_unittest.cc b/webrtc/modules/congestion_controller/probe_controller_unittest.cc index 3c43cfebbb..61ca559a53 100644 --- a/webrtc/modules/congestion_controller/probe_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/probe_controller_unittest.cc @@ -72,6 +72,7 @@ TEST_F(ProbeControllerTest, InitiatesProbingOnMaxBitrateIncrease) { // Long enough to time out exponential probing. clock_.AdvanceTimeMilliseconds(kExponentialProbingTimeoutMs); probe_controller_->SetEstimatedBitrate(kStartBitrateBps); + probe_controller_->Process(); EXPECT_CALL(pacer_, CreateProbeCluster(kMaxBitrateBps + 100, _)); probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, @@ -82,6 +83,12 @@ TEST_F(ProbeControllerTest, TestExponentialProbing) { probe_controller_->SetBitrates(kMinBitrateBps, kStartBitrateBps, kMaxBitrateBps); + // Repeated probe should only be sent when estimated bitrate climbs above 4 * + // kStartBitrateBps = 1200. + EXPECT_CALL(pacer_, CreateProbeCluster(_, _)).Times(0); + probe_controller_->SetEstimatedBitrate(1000); + testing::Mock::VerifyAndClearExpectations(&pacer_); + EXPECT_CALL(pacer_, CreateProbeCluster(2 * 1800, _)); probe_controller_->SetEstimatedBitrate(1800); } @@ -92,7 +99,9 @@ TEST_F(ProbeControllerTest, TestExponentialProbingTimeout) { // Advance far enough to cause a time out in waiting for probing result. clock_.AdvanceTimeMilliseconds(kExponentialProbingTimeoutMs); - EXPECT_CALL(pacer_, CreateProbeCluster(2 * 1800, _)).Times(0); + probe_controller_->Process(); + + EXPECT_CALL(pacer_, CreateProbeCluster(_, _)).Times(0); probe_controller_->SetEstimatedBitrate(1800); } @@ -110,6 +119,7 @@ TEST_F(ProbeControllerTest, ProbeAfterEstimateDropInAlr) { .WillRepeatedly( Return(rtc::Optional(clock_.TimeInMilliseconds()))); clock_.AdvanceTimeMilliseconds(kAlrProbeInterval + 1); + probe_controller_->Process(); probe_controller_->SetEstimatedBitrate(50); }