From f9f49a323c718a345d5c0f12c2b976b56be5efa2 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 25 Jun 2018 17:56:08 +0200 Subject: [PATCH] Removes redundant AlrDetector. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This replaces the old AlrDetector used by the pacer with the one in GoogCC. This reduces the risk of accidentally changing only one version. Note that the pacer instance will be removed when moving over to the task queue based send side congestion controller. Bug: webrtc:8415 Change-Id: Id4b2000ee5a04b94565092c29a84572a7750d2f5 Reviewed-on: https://webrtc-review.googlesource.com/85363 Commit-Queue: Sebastian Jansson Reviewed-by: Björn Terelius Cr-Commit-Position: refs/heads/master@{#23791} --- .../congestion_controller/goog_cc/BUILD.gn | 27 ++- .../goog_cc/alr_detector.cc | 3 +- .../goog_cc/alr_detector.h | 5 - .../send_side_congestion_controller.cc | 1 - modules/pacing/BUILD.gn | 27 ++- modules/pacing/alr_detector.cc | 86 --------- modules/pacing/alr_detector.h | 72 -------- modules/pacing/alr_detector_unittest.cc | 170 ------------------ modules/pacing/paced_sender.cc | 6 +- 9 files changed, 50 insertions(+), 347 deletions(-) delete mode 100644 modules/pacing/alr_detector.cc delete mode 100644 modules/pacing/alr_detector.h delete mode 100644 modules/pacing/alr_detector_unittest.cc diff --git a/modules/congestion_controller/goog_cc/BUILD.gn b/modules/congestion_controller/goog_cc/BUILD.gn index 554a83c966..bab967b399 100644 --- a/modules/congestion_controller/goog_cc/BUILD.gn +++ b/modules/congestion_controller/goog_cc/BUILD.gn @@ -19,8 +19,6 @@ config("bwe_test_logging") { rtc_static_library("goog_cc") { configs += [ ":bwe_test_logging" ] sources = [ - "alr_detector.cc", - "alr_detector.h", "goog_cc_factory.cc", "goog_cc_network_control.cc", "goog_cc_network_control.h", @@ -35,6 +33,7 @@ rtc_static_library("goog_cc") { } deps = [ + ":alr_detector", ":delay_based_bwe", ":estimators", "../..:module_api", @@ -50,13 +49,34 @@ rtc_static_library("goog_cc") { "../../../system_wrappers:field_trial_api", "../../../system_wrappers:metrics_api", "../../bitrate_controller", - "../../pacing", "../../remote_bitrate_estimator", "../../rtp_rtcp:rtp_rtcp_format", "//third_party/abseil-cpp/absl/types:optional", ] } +rtc_source_set("alr_detector") { + sources = [ + "alr_detector.cc", + "alr_detector.h", + ] + if (!build_with_chromium && is_clang) { + # Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163). + suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] + } + deps = [ + "../../..:webrtc_common", + "../../../:typedefs", + "../../../logging:rtc_event_log_api", + "../../../logging:rtc_event_pacing", + "../../../rtc_base:checks", + "../../../rtc_base:rtc_base_approved", + "../../../rtc_base/experiments:alr_experiment", + "../../../system_wrappers:field_trial_api", + "../../pacing:interval_budget", + "//third_party/abseil-cpp/absl/types:optional", + ] +} rtc_source_set("estimators") { configs += [ ":bwe_test_logging" ] sources = [ @@ -138,6 +158,7 @@ if (rtc_include_tests) { "trendline_estimator_unittest.cc", ] deps = [ + ":alr_detector", ":delay_based_bwe", ":estimators", ":goog_cc", diff --git a/modules/congestion_controller/goog_cc/alr_detector.cc b/modules/congestion_controller/goog_cc/alr_detector.cc index 941809d2a4..0a2cc74569 100644 --- a/modules/congestion_controller/goog_cc/alr_detector.cc +++ b/modules/congestion_controller/goog_cc/alr_detector.cc @@ -20,12 +20,12 @@ #include "rtc_base/experiments/alr_experiment.h" #include "rtc_base/format_macros.h" #include "rtc_base/logging.h" +#include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/ptr_util.h" #include "rtc_base/timeutils.h" #include "system_wrappers/include/field_trial.h" namespace webrtc { -namespace webrtc_cc { AlrDetector::AlrDetector() : AlrDetector(nullptr) {} AlrDetector::AlrDetector(RtcEventLog* event_log) @@ -93,5 +93,4 @@ absl::optional AlrDetector::GetApplicationLimitedRegionStartTime() const { return alr_started_time_ms_; } -} // namespace webrtc_cc } // namespace webrtc diff --git a/modules/congestion_controller/goog_cc/alr_detector.h b/modules/congestion_controller/goog_cc/alr_detector.h index 36fb952356..2a99ae8039 100644 --- a/modules/congestion_controller/goog_cc/alr_detector.h +++ b/modules/congestion_controller/goog_cc/alr_detector.h @@ -14,7 +14,6 @@ #include "absl/types/optional.h" #include "common_types.h" // NOLINT(build/include) #include "modules/pacing/interval_budget.h" -#include "modules/pacing/paced_sender.h" #include "rtc_base/rate_statistics.h" #include "typedefs.h" // NOLINT(build/include) @@ -22,8 +21,6 @@ namespace webrtc { class RtcEventLog; -namespace webrtc_cc { - // Application limited region detector is a class that utilizes signals of // elapsed time and bytes sent to estimate whether network traffic is // currently limited by the application's ability to generate traffic. @@ -70,8 +67,6 @@ class AlrDetector { RtcEventLog* event_log_; }; - -} // namespace webrtc_cc } // namespace webrtc #endif // MODULES_CONGESTION_CONTROLLER_GOOG_CC_ALR_DETECTOR_H_ diff --git a/modules/congestion_controller/send_side_congestion_controller.cc b/modules/congestion_controller/send_side_congestion_controller.cc index 3065c8b25b..7b61d0eaf6 100644 --- a/modules/congestion_controller/send_side_congestion_controller.cc +++ b/modules/congestion_controller/send_side_congestion_controller.cc @@ -19,7 +19,6 @@ #include "modules/bitrate_controller/include/bitrate_controller.h" #include "modules/congestion_controller/goog_cc/acknowledged_bitrate_estimator.h" #include "modules/congestion_controller/probe_controller.h" -#include "modules/pacing/alr_detector.h" #include "modules/remote_bitrate_estimator/include/bwe_defines.h" #include "rtc_base/checks.h" #include "rtc_base/format_macros.h" diff --git a/modules/pacing/BUILD.gn b/modules/pacing/BUILD.gn index 2b41eeeade..3de1d4a32d 100644 --- a/modules/pacing/BUILD.gn +++ b/modules/pacing/BUILD.gn @@ -10,12 +10,8 @@ import("../../webrtc.gni") rtc_static_library("pacing") { sources = [ - "alr_detector.cc", - "alr_detector.h", "bitrate_prober.cc", "bitrate_prober.h", - "interval_budget.cc", - "interval_budget.h", "paced_sender.cc", "paced_sender.h", "pacer.h", @@ -35,6 +31,7 @@ rtc_static_library("pacing") { } deps = [ + ":interval_budget", "..:module_api", "../../:typedefs", "../../:webrtc_common", @@ -47,6 +44,7 @@ rtc_static_library("pacing") { "../../system_wrappers", "../../system_wrappers:field_trial_api", "../../system_wrappers:runtime_enabled_features_api", + "../congestion_controller/goog_cc:alr_detector", "../remote_bitrate_estimator", "../rtp_rtcp", "../rtp_rtcp:rtp_rtcp_format", @@ -55,18 +53,37 @@ rtc_static_library("pacing") { ] } +rtc_source_set("interval_budget") { + sources = [ + "interval_budget.cc", + "interval_budget.h", + ] + + if (!build_with_chromium && is_clang) { + # Suppress warnings from the Chromium Clang plugin (bugs.webrtc.org/163). + suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] + } + + deps = [ + # "../../:typedefs", + "../../:webrtc_common", + "../../rtc_base:checks", + "../../rtc_base:rtc_base_approved", + ] +} + if (rtc_include_tests) { rtc_source_set("pacing_unittests") { testonly = true sources = [ - "alr_detector_unittest.cc", "bitrate_prober_unittest.cc", "interval_budget_unittest.cc", "paced_sender_unittest.cc", "packet_router_unittest.cc", ] deps = [ + ":interval_budget", ":pacing", "../../rtc_base:checks", "../../rtc_base:rtc_base_approved", diff --git a/modules/pacing/alr_detector.cc b/modules/pacing/alr_detector.cc deleted file mode 100644 index 1d16a71e67..0000000000 --- a/modules/pacing/alr_detector.cc +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/pacing/alr_detector.h" - -#include -#include -#include - -#include "logging/rtc_event_log/events/rtc_event_alr_state.h" -#include "logging/rtc_event_log/rtc_event_log.h" -#include "rtc_base/checks.h" -#include "rtc_base/experiments/alr_experiment.h" -#include "rtc_base/format_macros.h" -#include "rtc_base/logging.h" -#include "rtc_base/ptr_util.h" -#include "rtc_base/timeutils.h" -#include "system_wrappers/include/field_trial.h" - -namespace webrtc { -AlrDetector::AlrDetector() : AlrDetector(nullptr) {} - -AlrDetector::AlrDetector(RtcEventLog* event_log) - : bandwidth_usage_percent_(kDefaultAlrBandwidthUsagePercent), - alr_start_budget_level_percent_(kDefaultAlrStartBudgetLevelPercent), - alr_stop_budget_level_percent_(kDefaultAlrStopBudgetLevelPercent), - alr_budget_(0, true), - event_log_(event_log) { - RTC_CHECK(AlrExperimentSettings::MaxOneFieldTrialEnabled()); - absl::optional experiment_settings = - AlrExperimentSettings::CreateFromFieldTrial( - AlrExperimentSettings::kScreenshareProbingBweExperimentName); - if (!experiment_settings) { - experiment_settings = AlrExperimentSettings::CreateFromFieldTrial( - AlrExperimentSettings::kStrictPacingAndProbingExperimentName); - } - if (experiment_settings) { - alr_stop_budget_level_percent_ = - experiment_settings->alr_stop_budget_level_percent; - alr_start_budget_level_percent_ = - experiment_settings->alr_start_budget_level_percent; - bandwidth_usage_percent_ = experiment_settings->alr_bandwidth_usage_percent; - } -} - -AlrDetector::~AlrDetector() {} - -void AlrDetector::OnBytesSent(size_t bytes_sent, int64_t delta_time_ms) { - alr_budget_.UseBudget(bytes_sent); - alr_budget_.IncreaseBudget(delta_time_ms); - bool state_changed = false; - if (alr_budget_.budget_level_percent() > alr_start_budget_level_percent_ && - !alr_started_time_ms_) { - alr_started_time_ms_.emplace(rtc::TimeMillis()); - state_changed = true; - } else if (alr_budget_.budget_level_percent() < - alr_stop_budget_level_percent_ && - alr_started_time_ms_) { - state_changed = true; - alr_started_time_ms_.reset(); - } - if (event_log_ && state_changed) { - event_log_->Log( - rtc::MakeUnique(alr_started_time_ms_.has_value())); - } -} - -void AlrDetector::SetEstimatedBitrate(int bitrate_bps) { - RTC_DCHECK(bitrate_bps); - const auto target_rate_kbps = static_cast(bitrate_bps) * - bandwidth_usage_percent_ / (1000 * 100); - alr_budget_.set_target_rate_kbps(rtc::dchecked_cast(target_rate_kbps)); -} - -absl::optional AlrDetector::GetApplicationLimitedRegionStartTime() - const { - return alr_started_time_ms_; -} -} // namespace webrtc diff --git a/modules/pacing/alr_detector.h b/modules/pacing/alr_detector.h deleted file mode 100644 index cffd6c0c1a..0000000000 --- a/modules/pacing/alr_detector.h +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MODULES_PACING_ALR_DETECTOR_H_ -#define MODULES_PACING_ALR_DETECTOR_H_ - -#include "absl/types/optional.h" -#include "common_types.h" // NOLINT(build/include) -#include "modules/pacing/interval_budget.h" -#include "modules/pacing/paced_sender.h" -#include "rtc_base/rate_statistics.h" -#include "typedefs.h" // NOLINT(build/include) - -namespace webrtc { - -class RtcEventLog; - -// Application limited region detector is a class that utilizes signals of -// elapsed time and bytes sent to estimate whether network traffic is -// currently limited by the application's ability to generate traffic. -// -// AlrDetector provides a signal that can be utilized to adjust -// estimate bandwidth. -// Note: This class is not thread-safe. -class AlrDetector { - public: - AlrDetector(); - explicit AlrDetector(RtcEventLog* event_log); - ~AlrDetector(); - - void OnBytesSent(size_t bytes_sent, int64_t delta_time_ms); - - // Set current estimated bandwidth. - void SetEstimatedBitrate(int bitrate_bps); - - // Returns time in milliseconds when the current application-limited region - // started or empty result if the sender is currently not application-limited. - absl::optional GetApplicationLimitedRegionStartTime() const; - - // Sent traffic percentage as a function of network capacity used to determine - // application-limited region. ALR region start when bandwidth usage drops - // below kAlrStartUsagePercent and ends when it raises above - // kAlrEndUsagePercent. NOTE: This is intentionally conservative at the moment - // until BW adjustments of application limited region is fine tuned. - static constexpr int kDefaultAlrBandwidthUsagePercent = 65; - static constexpr int kDefaultAlrStartBudgetLevelPercent = 80; - static constexpr int kDefaultAlrStopBudgetLevelPercent = 50; - - void UpdateBudgetWithElapsedTime(int64_t delta_time_ms); - void UpdateBudgetWithBytesSent(size_t bytes_sent); - - private: - int bandwidth_usage_percent_; - int alr_start_budget_level_percent_; - int alr_stop_budget_level_percent_; - - IntervalBudget alr_budget_; - absl::optional alr_started_time_ms_; - - RtcEventLog* event_log_; -}; - -} // namespace webrtc - -#endif // MODULES_PACING_ALR_DETECTOR_H_ diff --git a/modules/pacing/alr_detector_unittest.cc b/modules/pacing/alr_detector_unittest.cc deleted file mode 100644 index 13eb65f561..0000000000 --- a/modules/pacing/alr_detector_unittest.cc +++ /dev/null @@ -1,170 +0,0 @@ -/* - * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/pacing/alr_detector.h" - -#include "rtc_base/experiments/alr_experiment.h" -#include "test/field_trial.h" -#include "test/gtest.h" - -namespace { - -constexpr int kEstimatedBitrateBps = 300000; - -} // namespace - -namespace webrtc { - -namespace { -class SimulateOutgoingTrafficIn { - public: - explicit SimulateOutgoingTrafficIn(AlrDetector* alr_detector) - : alr_detector_(alr_detector) { - RTC_CHECK(alr_detector_); - } - - SimulateOutgoingTrafficIn& ForTimeMs(int time_ms) { - interval_ms_ = time_ms; - ProduceTraffic(); - return *this; - } - - SimulateOutgoingTrafficIn& AtPercentOfEstimatedBitrate(int usage_percentage) { - usage_percentage_.emplace(usage_percentage); - ProduceTraffic(); - return *this; - } - - private: - void ProduceTraffic() { - if (!interval_ms_ || !usage_percentage_) - return; - const int kTimeStepMs = 10; - for (int t = 0; t < *interval_ms_; t += kTimeStepMs) { - alr_detector_->OnBytesSent(kEstimatedBitrateBps * *usage_percentage_ * - kTimeStepMs / (8 * 100 * 1000), - kTimeStepMs); - } - int remainder_ms = *interval_ms_ % kTimeStepMs; - if (remainder_ms > 0) { - alr_detector_->OnBytesSent(kEstimatedBitrateBps * *usage_percentage_ * - remainder_ms / (8 * 100 * 1000), - kTimeStepMs); - } - } - AlrDetector* const alr_detector_; - absl::optional interval_ms_; - absl::optional usage_percentage_; -}; -} // namespace - -class LegacyAlrDetectorTest : public testing::Test { - public: - void SetUp() override { - alr_detector_.SetEstimatedBitrate(kEstimatedBitrateBps); - } - - protected: - AlrDetector alr_detector_; -}; - -TEST_F(LegacyAlrDetectorTest, AlrDetection) { - // Start in non-ALR state. - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // Stay in non-ALR state when usage is close to 100%. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(1000) - .AtPercentOfEstimatedBitrate(90); - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // Verify that we ALR starts when bitrate drops below 20%. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(1500) - .AtPercentOfEstimatedBitrate(20); - EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // Verify that ALR ends when usage is above 65%. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(4000) - .AtPercentOfEstimatedBitrate(100); - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); -} - -TEST_F(LegacyAlrDetectorTest, ShortSpike) { - // Start in non-ALR state. - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // Verify that we ALR starts when bitrate drops below 20%. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(1000) - .AtPercentOfEstimatedBitrate(20); - EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // Verify that we stay in ALR region even after a short bitrate spike. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(100) - .AtPercentOfEstimatedBitrate(150); - EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // ALR ends when usage is above 65%. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(3000) - .AtPercentOfEstimatedBitrate(100); - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); -} - -TEST_F(LegacyAlrDetectorTest, BandwidthEstimateChanges) { - // Start in non-ALR state. - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // ALR starts when bitrate drops below 20%. - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(1000) - .AtPercentOfEstimatedBitrate(20); - EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - - // When bandwidth estimate drops the detector should stay in ALR mode and quit - // it shortly afterwards as the sender continues sending the same amount of - // traffic. This is necessary to ensure that ProbeController can still react - // to the BWE drop by initiating a new probe. - alr_detector_.SetEstimatedBitrate(kEstimatedBitrateBps / 5); - EXPECT_TRUE(alr_detector_.GetApplicationLimitedRegionStartTime()); - SimulateOutgoingTrafficIn(&alr_detector_) - .ForTimeMs(1000) - .AtPercentOfEstimatedBitrate(50); - EXPECT_FALSE(alr_detector_.GetApplicationLimitedRegionStartTime()); -} - -TEST_F(LegacyAlrDetectorTest, ParseControlFieldTrial) { - webrtc::test::ScopedFieldTrials field_trial( - "WebRTC-ProbingScreenshareBwe/Control/"); - absl::optional parsed_params = - AlrExperimentSettings::CreateFromFieldTrial( - "WebRTC-ProbingScreenshareBwe"); - EXPECT_FALSE(static_cast(parsed_params)); -} - -TEST_F(LegacyAlrDetectorTest, ParseActiveFieldTrial) { - webrtc::test::ScopedFieldTrials field_trial( - "WebRTC-ProbingScreenshareBwe/1.1,2875,85,20,-20,1/"); - absl::optional parsed_params = - AlrExperimentSettings::CreateFromFieldTrial( - "WebRTC-ProbingScreenshareBwe"); - ASSERT_TRUE(static_cast(parsed_params)); - EXPECT_EQ(1.1f, parsed_params->pacing_factor); - EXPECT_EQ(2875, parsed_params->max_paced_queue_time); - EXPECT_EQ(85, parsed_params->alr_bandwidth_usage_percent); - EXPECT_EQ(20, parsed_params->alr_start_budget_level_percent); - EXPECT_EQ(-20, parsed_params->alr_stop_budget_level_percent); - EXPECT_EQ(1, parsed_params->group_id); -} - -} // namespace webrtc diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 53dec05dc6..eda640e426 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc @@ -17,8 +17,8 @@ #include #include +#include "modules/congestion_controller/goog_cc/alr_detector.h" #include "modules/include/module_common_types.h" -#include "modules/pacing/alr_detector.h" #include "modules/pacing/bitrate_prober.h" #include "modules/pacing/interval_budget.h" #include "modules/pacing/round_robin_packet_queue.h" @@ -280,7 +280,7 @@ void PacedSender::Process() { if (packet_counter_ > 0) { PacedPacketInfo pacing_info; size_t bytes_sent = SendPadding(1, pacing_info); - alr_detector_->OnBytesSent(bytes_sent, elapsed_time_ms); + alr_detector_->OnBytesSent(bytes_sent, now_us / 1000); } } } @@ -355,7 +355,7 @@ void PacedSender::Process() { if (!probing_send_failure_) prober_->ProbeSent(clock_->TimeInMilliseconds(), bytes_sent); } - alr_detector_->OnBytesSent(bytes_sent, elapsed_time_ms); + alr_detector_->OnBytesSent(bytes_sent, now_us / 1000); } void PacedSender::ProcessThreadAttached(ProcessThread* process_thread) {