Reset RobustThroughputEstimator if recv timestamp jump backwards
Start using RobustThoughputEstimator in DelayBasedBwe test in preparation for making it default.
Experiments has not showed significant metric changes. However, simulations has showed that RobustThroughputEstimator better follow the actually receive rate better. Especially during bursts of sent packets. Code is also simpler.
Bug: webrtc:13402 chromium:1411666
Change-Id: I83cfa1fc15486982b18cc22fbd0752ff59c1c1b4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/317600
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40644}
diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc
index 278dd3e..5a4dbfd 100644
--- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc
+++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc
@@ -123,7 +123,7 @@
TEST_F(DelayBasedBweTest, GetExpectedBwePeriodMs) {
auto default_interval = bitrate_estimator_->GetExpectedBwePeriod();
EXPECT_GT(default_interval.ms(), 0);
- CapacityDropTestHelper(1, true, 333, 0);
+ CapacityDropTestHelper(1, true, 533, 0);
auto interval = bitrate_estimator_->GetExpectedBwePeriod();
EXPECT_GT(interval.ms(), 0);
EXPECT_NE(interval.ms(), default_interval.ms());
@@ -142,11 +142,11 @@
RateIncreaseReorderingTestHelper(730000);
}
TEST_F(DelayBasedBweTest, RateIncreaseRtpTimestamps) {
- RateIncreaseRtpTimestampsTestHelper(622);
+ RateIncreaseRtpTimestampsTestHelper(617);
}
TEST_F(DelayBasedBweTest, CapacityDropOneStream) {
- CapacityDropTestHelper(1, false, 300, 0);
+ CapacityDropTestHelper(1, false, 500, 0);
}
TEST_F(DelayBasedBweTest, CapacityDropPosOffsetChange) {
@@ -158,7 +158,7 @@
}
TEST_F(DelayBasedBweTest, CapacityDropOneStreamWrap) {
- CapacityDropTestHelper(1, true, 333, 0);
+ CapacityDropTestHelper(1, true, 533, 0);
}
TEST_F(DelayBasedBweTest, TestTimestampGrouping) {
@@ -193,19 +193,16 @@
// Needed to initialize the AimdRateControl.
bitrate_estimator_->SetStartBitrate(kStartBitrate);
- // Produce 30 frames (in 1/3 second) and give them to the estimator.
+ // Produce 40 frames (in 1/3 second) and give them to the estimator.
int64_t bitrate_bps = kStartBitrate.bps();
bool seen_overuse = false;
- for (int i = 0; i < 30; ++i) {
+ for (int i = 0; i < 40; ++i) {
bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps);
- // The purpose of this test is to ensure that we back down even if we don't
- // have any acknowledged bitrate estimate yet. Hence, if the test works
- // as expected, we should not have a measured bitrate yet.
- EXPECT_FALSE(acknowledged_bitrate_estimator_->bitrate().has_value());
if (overuse) {
EXPECT_TRUE(bitrate_observer_.updated());
- EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate.bps() / 2,
- 15000);
+ EXPECT_LE(bitrate_observer_.latest_bitrate(), kInitialCapacity.bps());
+ EXPECT_GT(bitrate_observer_.latest_bitrate(),
+ 0.8 * kInitialCapacity.bps());
bitrate_bps = bitrate_observer_.latest_bitrate();
seen_overuse = true;
break;
@@ -215,8 +212,8 @@
}
}
EXPECT_TRUE(seen_overuse);
- EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate.bps() / 2,
- 15000);
+ EXPECT_LE(bitrate_observer_.latest_bitrate(), kInitialCapacity.bps());
+ EXPECT_GT(bitrate_observer_.latest_bitrate(), 0.8 * kInitialCapacity.bps());
}
TEST_F(DelayBasedBweTest, TestTimestampPrecisionHandling) {
@@ -254,61 +251,4 @@
}
}
-class DelayBasedBweTestWithBackoffTimeoutExperiment : public DelayBasedBweTest {
- public:
- DelayBasedBweTestWithBackoffTimeoutExperiment()
- : DelayBasedBweTest(
- "WebRTC-BweAimdRateControlConfig/initial_backoff_interval:200ms/") {
- }
-};
-
-// This test subsumes and improves DelayBasedBweTest.TestInitialOveruse above.
-TEST_F(DelayBasedBweTestWithBackoffTimeoutExperiment, TestInitialOveruse) {
- const DataRate kStartBitrate = DataRate::KilobitsPerSec(300);
- const DataRate kInitialCapacity = DataRate::KilobitsPerSec(200);
- const uint32_t kDummySsrc = 0;
- // High FPS to ensure that we send a lot of packets in a short time.
- const int kFps = 90;
-
- stream_generator_->AddStream(new test::RtpStream(kFps, kStartBitrate.bps()));
- stream_generator_->set_capacity_bps(kInitialCapacity.bps());
-
- // Needed to initialize the AimdRateControl.
- bitrate_estimator_->SetStartBitrate(kStartBitrate);
-
- // Produce 30 frames (in 1/3 second) and give them to the estimator.
- int64_t bitrate_bps = kStartBitrate.bps();
- bool seen_overuse = false;
- for (int frames = 0; frames < 30 && !seen_overuse; ++frames) {
- bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps);
- // The purpose of this test is to ensure that we back down even if we don't
- // have any acknowledged bitrate estimate yet. Hence, if the test works
- // as expected, we should not have a measured bitrate yet.
- EXPECT_FALSE(acknowledged_bitrate_estimator_->bitrate().has_value());
- if (overuse) {
- EXPECT_TRUE(bitrate_observer_.updated());
- EXPECT_NEAR(bitrate_observer_.latest_bitrate(), kStartBitrate.bps() / 2,
- 15000);
- bitrate_bps = bitrate_observer_.latest_bitrate();
- seen_overuse = true;
- } else if (bitrate_observer_.updated()) {
- bitrate_bps = bitrate_observer_.latest_bitrate();
- bitrate_observer_.Reset();
- }
- }
- EXPECT_TRUE(seen_overuse);
- // Continue generating an additional 15 frames (equivalent to 167 ms) and
- // verify that we don't back down further.
- for (int frames = 0; frames < 15 && seen_overuse; ++frames) {
- bool overuse = GenerateAndProcessFrame(kDummySsrc, bitrate_bps);
- EXPECT_FALSE(overuse);
- if (bitrate_observer_.updated()) {
- bitrate_bps = bitrate_observer_.latest_bitrate();
- EXPECT_GE(bitrate_bps, kStartBitrate.bps() / 2 - 15000);
- EXPECT_LE(bitrate_bps, kInitialCapacity.bps() + 15000);
- bitrate_observer_.Reset();
- }
- }
-}
-
} // namespace webrtc
diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc
index 8618a78..2730c5d 100644
--- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc
+++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.cc
@@ -144,11 +144,9 @@
}
} // namespace test
-DelayBasedBweTest::DelayBasedBweTest() : DelayBasedBweTest("") {}
-
-DelayBasedBweTest::DelayBasedBweTest(absl::string_view field_trial_string)
- : field_trial(
- std::make_unique<test::ScopedFieldTrials>(field_trial_string)),
+DelayBasedBweTest::DelayBasedBweTest()
+ : field_trial(std::make_unique<test::ScopedFieldTrials>(
+ "WebRTC-Bwe-RobustThroughputEstimatorSettings/enabled:true/")),
clock_(100000000),
acknowledged_bitrate_estimator_(
AcknowledgedBitrateEstimatorInterface::Create(&field_trial_config_)),
diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h
index d56fe89..4b06173 100644
--- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h
+++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest_helper.h
@@ -118,7 +118,6 @@
class DelayBasedBweTest : public ::testing::Test {
public:
DelayBasedBweTest();
- explicit DelayBasedBweTest(absl::string_view field_trial_string);
~DelayBasedBweTest() override;
protected:
diff --git a/modules/congestion_controller/goog_cc/robust_throughput_estimator.cc b/modules/congestion_controller/goog_cc/robust_throughput_estimator.cc
index 792a93d4..3f66f7f 100644
--- a/modules/congestion_controller/goog_cc/robust_throughput_estimator.cc
+++ b/modules/congestion_controller/goog_cc/robust_throughput_estimator.cc
@@ -20,6 +20,7 @@
#include "api/units/time_delta.h"
#include "api/units/timestamp.h"
#include "rtc_base/checks.h"
+#include "rtc_base/logging.h"
namespace webrtc {
@@ -76,6 +77,16 @@
i > 0 && window_[i].receive_time < window_[i - 1].receive_time; i--) {
std::swap(window_[i], window_[i - 1]);
}
+ constexpr TimeDelta kMaxReorderingTime = TimeDelta::Seconds(1);
+ const TimeDelta receive_delta =
+ (window_.back().receive_time - packet.receive_time);
+ if (receive_delta > kMaxReorderingTime) {
+ RTC_LOG(LS_WARNING)
+ << "Severe packet re-ordering or timestamps offset changed: "
+ << receive_delta;
+ window_.clear();
+ latest_discarded_send_time_ = Timestamp::MinusInfinity();
+ }
}
// Remove old packets.
diff --git a/modules/congestion_controller/goog_cc/robust_throughput_estimator_unittest.cc b/modules/congestion_controller/goog_cc/robust_throughput_estimator_unittest.cc
index 95ac525..9a013aa 100644
--- a/modules/congestion_controller/goog_cc/robust_throughput_estimator_unittest.cc
+++ b/modules/congestion_controller/goog_cc/robust_throughput_estimator_unittest.cc
@@ -381,6 +381,29 @@
0.05 * send_rate.bytes_per_sec<double>()); // Allow 5% error
}
}
+TEST(RobustThroughputEstimatorTest, ResetsIfReceiveClockChangeBackwards) {
+ FeedbackGenerator feedback_generator;
+ RobustThroughputEstimator throughput_estimator(
+ CreateRobustThroughputEstimatorSettings(
+ "WebRTC-Bwe-RobustThroughputEstimatorSettings/"
+ "enabled:true/"));
+ DataRate send_rate(DataRate::BytesPerSec(100000));
+ DataRate recv_rate(DataRate::BytesPerSec(100000));
+
+ std::vector<PacketResult> packet_feedback =
+ feedback_generator.CreateFeedbackVector(20, DataSize::Bytes(1000),
+ send_rate, recv_rate);
+ throughput_estimator.IncomingPacketFeedbackVector(packet_feedback);
+ EXPECT_EQ(throughput_estimator.bitrate(), send_rate);
+
+ feedback_generator.AdvanceReceiveClock(TimeDelta::Seconds(-2));
+ send_rate = DataRate::BytesPerSec(200000);
+ recv_rate = DataRate::BytesPerSec(200000);
+ packet_feedback = feedback_generator.CreateFeedbackVector(
+ 20, DataSize::Bytes(1000), send_rate, recv_rate);
+ throughput_estimator.IncomingPacketFeedbackVector(packet_feedback);
+ EXPECT_EQ(throughput_estimator.bitrate(), send_rate);
+}
TEST(RobustThroughputEstimatorTest, StreamPausedAndResumed) {
FeedbackGenerator feedback_generator;
diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.cc b/modules/remote_bitrate_estimator/aimd_rate_control.cc
index 686ebcf..0fdc53b 100644
--- a/modules/remote_bitrate_estimator/aimd_rate_control.cc
+++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc
@@ -79,22 +79,11 @@
rtt_(kDefaultRtt),
send_side_(send_side),
no_bitrate_increase_in_alr_(
- key_value_config.IsEnabled("WebRTC-DontIncreaseDelayBasedBweInAlr")),
- initial_backoff_interval_("initial_backoff_interval"),
- link_capacity_fix_("link_capacity_fix") {
+ key_value_config.IsEnabled("WebRTC-DontIncreaseDelayBasedBweInAlr")) {
ParseFieldTrial(
{&disable_estimate_bounded_increase_,
&use_current_estimate_as_min_upper_bound_},
key_value_config.Lookup("WebRTC-Bwe-EstimateBoundedIncrease"));
- // E.g
- // WebRTC-BweAimdRateControlConfig/initial_backoff_interval:100ms/
- ParseFieldTrial({&initial_backoff_interval_, &link_capacity_fix_},
- key_value_config.Lookup("WebRTC-BweAimdRateControlConfig"));
- if (initial_backoff_interval_) {
- RTC_LOG(LS_INFO) << "Using aimd rate control with initial back-off interval"
- " "
- << ToString(*initial_backoff_interval_) << ".";
- }
RTC_LOG(LS_INFO) << "Using aimd rate control with back off factor " << beta_;
}
@@ -143,18 +132,9 @@
}
bool AimdRateControl::InitialTimeToReduceFurther(Timestamp at_time) const {
- if (!initial_backoff_interval_) {
- return ValidEstimate() &&
- TimeToReduceFurther(at_time,
- LatestEstimate() / 2 - DataRate::BitsPerSec(1));
- }
- // TODO(terelius): We could use the RTT (clamped to suitable limits) instead
- // of a fixed bitrate_reduction_interval.
- if (time_last_bitrate_decrease_.IsInfinite() ||
- at_time - time_last_bitrate_decrease_ >= *initial_backoff_interval_) {
- return true;
- }
- return false;
+ return ValidEstimate() &&
+ TimeToReduceFurther(at_time,
+ LatestEstimate() / 2 - DataRate::BitsPerSec(1));
}
DataRate AimdRateControl::LatestEstimate() const {
@@ -307,7 +287,7 @@
// Set bit rate to something slightly lower than the measured throughput
// to get rid of any self-induced delay.
decreased_bitrate = estimated_throughput * beta_;
- if (decreased_bitrate > current_bitrate_ && !link_capacity_fix_) {
+ if (decreased_bitrate > current_bitrate_) {
// TODO(terelius): The link_capacity estimate may be based on old
// throughput measurements. Relying on them may lead to unnecessary
// BWE drops.
diff --git a/modules/remote_bitrate_estimator/aimd_rate_control.h b/modules/remote_bitrate_estimator/aimd_rate_control.h
index 95459e3..4efde54 100644
--- a/modules/remote_bitrate_estimator/aimd_rate_control.h
+++ b/modules/remote_bitrate_estimator/aimd_rate_control.h
@@ -108,8 +108,6 @@
FieldTrialParameter<bool> use_current_estimate_as_min_upper_bound_{"c_upper",
false};
absl::optional<DataRate> last_decrease_;
- FieldTrialOptional<TimeDelta> initial_backoff_interval_;
- FieldTrialFlag link_capacity_fix_;
};
} // namespace webrtc
