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