Fix filtering of small packets in delay-based BWE
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 <terelius@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Christoffer Rodbro <crodbro@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29804}
diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc
index 2b62891..0a84284 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 @@
} // namespace
+constexpr char BweIgnoreSmallPacketsSettings::kKey[];
+
+BweIgnoreSmallPacketsSettings::BweIgnoreSmallPacketsSettings(
+ const WebRtcKeyValueConfig* key_value_config) {
+ Parser()->Parse(
+ key_value_config->Lookup(BweIgnoreSmallPacketsSettings::kKey));
+}
+
+std::unique_ptr<StructParametersParser>
+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 @@
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 @@
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 @@
// 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<double>(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 a2331b6..0384594 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<StructParametersParser> Parser();
+};
+
class DelayBasedBwe {
public:
struct Result {
@@ -86,6 +102,12 @@
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<InterArrival> inter_arrival_;
std::unique_ptr<DelayIncreaseDetectorInterface> delay_detector_;
@@ -96,7 +118,6 @@
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 130acbf..d8d984e 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<StructParametersParser>
-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 @@
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 @@
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 @@
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 c48fcf0..0f70943 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<StructParametersParser> Parser();
-};
-
class TrendlineEstimator : public DelayIncreaseDetectorInterface {
public:
TrendlineEstimator(const WebRtcKeyValueConfig* key_value_config,
@@ -72,11 +56,6 @@
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_;