From a041a97f630555469a9eac49eb2b51eece8ddf4f Mon Sep 17 00:00:00 2001 From: Per K Date: Fri, 25 Aug 2023 16:17:25 +0200 Subject: [PATCH] Reset RobustThroughputEstimator if recv timestamp jump backwards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#40644} --- .../goog_cc/delay_based_bwe_unittest.cc | 82 +++---------------- .../delay_based_bwe_unittest_helper.cc | 8 +- .../goog_cc/delay_based_bwe_unittest_helper.h | 1 - .../goog_cc/robust_throughput_estimator.cc | 11 +++ .../robust_throughput_estimator_unittest.cc | 23 ++++++ .../aimd_rate_control.cc | 30 ++----- .../aimd_rate_control.h | 2 - 7 files changed, 53 insertions(+), 104 deletions(-) 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 278dd3ea75..5a4dbfdcc0 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, ProbeDetectionSlowerArrivalHighBitrate) { 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 @@ TEST_F(DelayBasedBweTest, RateIncreaseReordering) { 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, CapacityDropNegOffsetChange) { } TEST_F(DelayBasedBweTest, CapacityDropOneStreamWrap) { - CapacityDropTestHelper(1, true, 333, 0); + CapacityDropTestHelper(1, true, 533, 0); } TEST_F(DelayBasedBweTest, TestTimestampGrouping) { @@ -193,19 +193,16 @@ TEST_F(DelayBasedBweTest, TestInitialOveruse) { // 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 @@ TEST_F(DelayBasedBweTest, TestInitialOveruse) { } } 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 @@ TEST_F(DelayBasedBweTest, TestTimestampPrecisionHandling) { } } -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 8618a7814e..2730c5d49b 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 @@ int64_t StreamGenerator::GenerateFrame(std::vector* packets, } } // namespace test -DelayBasedBweTest::DelayBasedBweTest() : DelayBasedBweTest("") {} - -DelayBasedBweTest::DelayBasedBweTest(absl::string_view field_trial_string) - : field_trial( - std::make_unique(field_trial_string)), +DelayBasedBweTest::DelayBasedBweTest() + : field_trial(std::make_unique( + "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 d56fe892d5..4b06173cdb 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 StreamGenerator { 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 792a93d41e..3f66f7fdae 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 @@ void RobustThroughputEstimator::IncomingPacketFeedbackVector( 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 95ac525640..9a013aa6d0 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 @@ TEST(RobustThroughputEstimatorTest, DeepReordering) { 0.05 * send_rate.bytes_per_sec()); // 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 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 686ebcf026..0fdc53b258 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -79,22 +79,11 @@ AimdRateControl::AimdRateControl(const FieldTrialsView& key_value_config, 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::TimeToReduceFurther(Timestamp at_time, } 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 @@ void AimdRateControl::ChangeBitrate(const RateControlInput& input, // 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 95459e3d6a..4efde54410 100644 --- a/modules/remote_bitrate_estimator/aimd_rate_control.h +++ b/modules/remote_bitrate_estimator/aimd_rate_control.h @@ -108,8 +108,6 @@ class AimdRateControl { FieldTrialParameter use_current_estimate_as_min_upper_bound_{"c_upper", false}; absl::optional last_decrease_; - FieldTrialOptional initial_backoff_interval_; - FieldTrialFlag link_capacity_fix_; }; } // namespace webrtc