From fd0e32a87a4e6a9a6cfeb02fbdcddd6edf4ac9c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Terelius?= Date: Fri, 15 Nov 2019 14:30:58 +0100 Subject: [PATCH] Fix filtering of small packets in delay-based BWE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit crodbro@ found that the previous field trial, which filtered the deltas in the trendline estimator, can increase the noise caused by varying packet sizes. Moving the filtering to the DelayBasedBwe class fixes the issue. To avoid confusion, we've updated the field trial name, so e.g. WebRTC-BweIgnoreSmallPacketsFix/small:200bytes,large:200bytes, fraction_large:0.25,smoothing:0.1/ should be used to enable the feature. Bug: webrtc:10932 Change-Id: If77e83043c37fff909038405f634e541ce41abb8 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159711 Commit-Queue: Björn Terelius Reviewed-by: Sebastian Jansson Reviewed-by: Christoffer Rodbro Cr-Commit-Position: refs/heads/master@{#29804} --- .../goog_cc/delay_based_bwe.cc | 52 ++++++++++++++++--- .../goog_cc/delay_based_bwe.h | 23 +++++++- .../goog_cc/trendline_estimator.cc | 41 +-------------- .../goog_cc/trendline_estimator.h | 21 -------- 4 files changed, 69 insertions(+), 68 deletions(-) diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index 2b62891daa..0a84284572 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -42,6 +42,22 @@ constexpr uint32_t kFixedSsrc = 0; } // namespace +constexpr char BweIgnoreSmallPacketsSettings::kKey[]; + +BweIgnoreSmallPacketsSettings::BweIgnoreSmallPacketsSettings( + const WebRtcKeyValueConfig* key_value_config) { + Parser()->Parse( + key_value_config->Lookup(BweIgnoreSmallPacketsSettings::kKey)); +} + +std::unique_ptr +BweIgnoreSmallPacketsSettings::Parser() { + return StructParametersParser::Create("smoothing", &smoothing_factor, // + "fraction_large", &fraction_large, // + "large", &large_threshold, // + "small", &small_threshold); +} + DelayBasedBwe::Result::Result() : updated(false), probe(false), @@ -63,6 +79,8 @@ DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, NetworkStatePredictor* network_state_predictor) : event_log_(event_log), key_value_config_(key_value_config), + ignore_small_(key_value_config), + fraction_large_packets_(0.5), network_state_predictor_(network_state_predictor), inter_arrival_(), delay_detector_( @@ -75,7 +93,12 @@ DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, prev_state_(BandwidthUsage::kBwNormal), alr_limited_backoff_enabled_( key_value_config->Lookup("WebRTC-Bwe-AlrLimitedBackoff") - .find("Enabled") == 0) {} + .find("Enabled") == 0) { + RTC_LOG(LS_INFO) << "Initialized DelayBasedBwe with field trial " + << ignore_small_.Parser()->Encode() + << " and alr limited backoff " + << (alr_limited_backoff_enabled_ ? "enabled" : "disabled"); +} DelayBasedBwe::~DelayBasedBwe() {} @@ -151,18 +174,33 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, // so wrapping works properly. uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; + // Ignore "small" packets if many/most packets in the call are "large". The + // packet size may have a significant effect on the propagation delay, + // especially at low bandwidths. Variations in packet size will then show up + // as noise in the delay measurement. By default, we include all packets. + DataSize packet_size = packet_feedback.sent_packet.size; + if (!ignore_small_.small_threshold.IsZero()) { + double is_large = + static_cast(packet_size >= ignore_small_.large_threshold); + fraction_large_packets_ += + ignore_small_.smoothing_factor * (is_large - fraction_large_packets_); + if (packet_size <= ignore_small_.small_threshold && + fraction_large_packets_ >= ignore_small_.fraction_large) { + return; + } + } + uint32_t ts_delta = 0; int64_t t_delta = 0; int size_delta = 0; bool calculated_deltas = inter_arrival_->ComputeDeltas( timestamp, packet_feedback.receive_time.ms(), at_time.ms(), - packet_feedback.sent_packet.size.bytes(), &ts_delta, &t_delta, - &size_delta); + packet_size.bytes(), &ts_delta, &t_delta, &size_delta); double ts_delta_ms = (1000.0 * ts_delta) / (1 << kInterArrivalShift); - delay_detector_->Update( - t_delta, ts_delta_ms, packet_feedback.sent_packet.send_time.ms(), - packet_feedback.receive_time.ms(), - packet_feedback.sent_packet.size.bytes(), calculated_deltas); + delay_detector_->Update(t_delta, ts_delta_ms, + packet_feedback.sent_packet.send_time.ms(), + packet_feedback.receive_time.ms(), + packet_size.bytes(), calculated_deltas); } DataRate DelayBasedBwe::TriggerOveruse(Timestamp at_time, diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.h b/modules/congestion_controller/goog_cc/delay_based_bwe.h index a2331b6223..03845949a4 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.h @@ -27,11 +27,27 @@ #include "modules/remote_bitrate_estimator/include/bwe_defines.h" #include "modules/remote_bitrate_estimator/inter_arrival.h" #include "rtc_base/constructor_magic.h" +#include "rtc_base/experiments/struct_parameters_parser.h" #include "rtc_base/race_checker.h" namespace webrtc { class RtcEventLog; +struct BweIgnoreSmallPacketsSettings { + static constexpr char kKey[] = "WebRTC-BweIgnoreSmallPacketsFix"; + + BweIgnoreSmallPacketsSettings() = default; + explicit BweIgnoreSmallPacketsSettings( + const WebRtcKeyValueConfig* key_value_config); + + double smoothing_factor = 0.1; + double fraction_large = 1.0; + DataSize large_threshold = DataSize::Zero(); + DataSize small_threshold = DataSize::Zero(); + + std::unique_ptr Parser(); +}; + class DelayBasedBwe { public: struct Result { @@ -86,6 +102,12 @@ class DelayBasedBwe { rtc::RaceChecker network_race_; RtcEventLog* const event_log_; const WebRtcKeyValueConfig* const key_value_config_; + + // Filtering out small packets. Intention is to base the detection only + // on video packets even if we have TWCC sequence numbers for audio. + BweIgnoreSmallPacketsSettings ignore_small_; + double fraction_large_packets_; + NetworkStatePredictor* network_state_predictor_; std::unique_ptr inter_arrival_; std::unique_ptr delay_detector_; @@ -96,7 +118,6 @@ class DelayBasedBwe { bool has_once_detected_overuse_; BandwidthUsage prev_state_; bool alr_limited_backoff_enabled_; - RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(DelayBasedBwe); }; diff --git a/modules/congestion_controller/goog_cc/trendline_estimator.cc b/modules/congestion_controller/goog_cc/trendline_estimator.cc index 130acbfe85..d8d984ead9 100644 --- a/modules/congestion_controller/goog_cc/trendline_estimator.cc +++ b/modules/congestion_controller/goog_cc/trendline_estimator.cc @@ -25,23 +25,6 @@ namespace webrtc { -constexpr char BweIgnoreSmallPacketsSettings::kKey[]; - -BweIgnoreSmallPacketsSettings::BweIgnoreSmallPacketsSettings( - const WebRtcKeyValueConfig* key_value_config) { - Parser()->Parse( - key_value_config->Lookup(BweIgnoreSmallPacketsSettings::kKey)); -} - -std::unique_ptr -BweIgnoreSmallPacketsSettings::Parser() { - return StructParametersParser::Create( - "smoothing_factor", &smoothing_factor, // - "min_fraction_large_packets", &min_fraction_large_packets, // - "large_packet_size", &large_packet_size, // - "ignored_size", &ignored_size); -} - namespace { // Parameters for linear least squares fit of regression line to noisy data. @@ -102,9 +85,7 @@ constexpr int kDeltaCounterMax = 1000; TrendlineEstimator::TrendlineEstimator( const WebRtcKeyValueConfig* key_value_config, NetworkStatePredictor* network_state_predictor) - : ignore_small_packets_(key_value_config), - fraction_large_packets_(0.5), - window_size_(key_value_config->Lookup(kBweWindowSizeInPacketsExperiment) + : window_size_(key_value_config->Lookup(kBweWindowSizeInPacketsExperiment) .find("Enabled") == 0 ? ReadTrendlineFilterWindowSize(key_value_config) : kDefaultTrendlineWindowSize), @@ -129,8 +110,7 @@ TrendlineEstimator::TrendlineEstimator( network_state_predictor_(network_state_predictor) { RTC_LOG(LS_INFO) << "Using Trendline filter for delay change estimation with window size " - << window_size_ << ", field trial " - << ignore_small_packets_.Parser()->Encode() << " and " + << window_size_ << " and " << (network_state_predictor_ ? "injected" : "no") << " network state predictor"; } @@ -142,23 +122,6 @@ void TrendlineEstimator::UpdateTrendline(double recv_delta_ms, int64_t send_time_ms, int64_t arrival_time_ms, size_t packet_size) { - if (ignore_small_packets_.ignored_size > 0) { - // Process the packet if it is "large" or if all packets in the call are - // "small". The packet size may have a significant effect on the propagation - // delay, especially at low bandwidths. Variations in packet size will then - // show up as noise in the delay measurement. - // By default, we include all packets. - fraction_large_packets_ = - (1 - ignore_small_packets_.smoothing_factor) * fraction_large_packets_ + - ignore_small_packets_.smoothing_factor * - (packet_size >= ignore_small_packets_.large_packet_size); - if (packet_size <= ignore_small_packets_.ignored_size && - fraction_large_packets_ >= - ignore_small_packets_.min_fraction_large_packets) { - return; - } - } - const double delta_ms = recv_delta_ms - send_delta_ms; ++num_of_deltas_; num_of_deltas_ = std::min(num_of_deltas_, kDeltaCounterMax); diff --git a/modules/congestion_controller/goog_cc/trendline_estimator.h b/modules/congestion_controller/goog_cc/trendline_estimator.h index c48fcf0cfa..0f70943fe5 100644 --- a/modules/congestion_controller/goog_cc/trendline_estimator.h +++ b/modules/congestion_controller/goog_cc/trendline_estimator.h @@ -22,25 +22,9 @@ #include "modules/congestion_controller/goog_cc/delay_increase_detector_interface.h" #include "modules/remote_bitrate_estimator/include/bwe_defines.h" #include "rtc_base/constructor_magic.h" -#include "rtc_base/experiments/struct_parameters_parser.h" namespace webrtc { -struct BweIgnoreSmallPacketsSettings { - static constexpr char kKey[] = "WebRTC-BweIgnoreSmallPackets"; - - BweIgnoreSmallPacketsSettings() = default; - explicit BweIgnoreSmallPacketsSettings( - const WebRtcKeyValueConfig* key_value_config); - - double smoothing_factor = 0.1; - double min_fraction_large_packets = 1.0; - unsigned large_packet_size = 0; - unsigned ignored_size = 0; - - std::unique_ptr Parser(); -}; - class TrendlineEstimator : public DelayIncreaseDetectorInterface { public: TrendlineEstimator(const WebRtcKeyValueConfig* key_value_config, @@ -72,11 +56,6 @@ class TrendlineEstimator : public DelayIncreaseDetectorInterface { void UpdateThreshold(double modified_offset, int64_t now_ms); - // Filtering out small packets. (Intention is to base the detection only - // on video packets even if we have TWCC sequence number for audio.) - BweIgnoreSmallPacketsSettings ignore_small_packets_; - double fraction_large_packets_; - // Parameters. const size_t window_size_; const double smoothing_coef_;