From 22fd5d7455a5d42278958c80664806a77c9ab4bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Thu, 7 Nov 2019 14:21:05 +0100 Subject: [PATCH] Fixes incorrect probe timing check. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a recent CL, a line that puts a lower bound of 0 on time to next probe was omitted: https://webrtc-review.googlesource.com/c/src/+/158841/7/modules/pacing/bitrate_prober.cc#b143 That cause a misinterpretation in https://webrtc-review.googlesource.com/c/src/+/158841/7/modules/pacing/pacing_controller.cc#290 which may lead to probes aborting if the module processing thread sleeps a little too long. Bug: webrtc:10809 Change-Id: I672375fb213782e4e1f2215252f50894d7655f97 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159023 Commit-Queue: Erik Språng Reviewed-by: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#29728} --- modules/pacing/pacing_controller.cc | 6 +----- modules/pacing/pacing_controller_unittest.cc | 14 ++++++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index 4a5eadd86b..2d73247c10 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc @@ -283,11 +283,7 @@ Timestamp PacingController::NextProbeTime() { return probe_time; } - if (probe_time > now) { - return probe_time; - } - - if (probing_send_failure_ || now - probe_time > TimeDelta::ms(1)) { + if (probe_time <= now && probing_send_failure_) { return Timestamp::PlusInfinity(); } diff --git a/modules/pacing/pacing_controller_unittest.cc b/modules/pacing/pacing_controller_unittest.cc index 361be0dc3f..bd2dd1de02 100644 --- a/modules/pacing/pacing_controller_unittest.cc +++ b/modules/pacing/pacing_controller_unittest.cc @@ -1161,14 +1161,16 @@ TEST_F(PacingControllerTest, SkipsProbesWhenProcessIntervalTooLarge) { // We're exactly where we should be for the next probe. EXPECT_TRUE(pacer_->NextProbeTime().IsFinite()); - // Advance to within one millisecond past where the next probe should be sent, - // will still indicate "process immediately". - clock_.AdvanceTime(TimeDelta::us(500)); + FieldTrialBasedConfig field_trial_config; + BitrateProberConfig probing_config(&field_trial_config); + EXPECT_GT(probing_config.max_probe_delay.Get(), TimeDelta::Zero()); + + // Advance to within max probe delay. + clock_.AdvanceTime(probing_config.max_probe_delay.Get()); EXPECT_TRUE(pacer_->NextProbeTime().IsFinite()); - // We've gone more than one millisecond past the time for the next probe - // packet, it will dropped. - clock_.AdvanceTime(TimeDelta::ms(1)); + // Too high probe delay, drop it! + clock_.AdvanceTime(TimeDelta::us(1)); EXPECT_EQ(pacer_->NextProbeTime(), Timestamp::PlusInfinity()); }