From 6b667a8fe2bbe69c0857c2dd93b66c159c3378f0 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Thu, 13 Jan 2022 12:15:34 +0100 Subject: [PATCH] Clean up expreiment WebRTC-Bwe-NewInterArrivalDelta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The experiment has been per default enabled since https://webrtc.googlesource.com/src/+/62b340545f80baaa449c2159a8e44052c74116c9 submitted 20210914. Bug: webrtc:12269 Change-Id: I730b603f4d0e382758fd4a6df6ccef9d8b76ea82 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/246105 Reviewed-by: Per Kjellander Reviewed-by: Björn Terelius Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#35683} --- .../goog_cc/delay_based_bwe.cc | 66 ++----------------- .../goog_cc/delay_based_bwe.h | 1 - .../goog_cc/delay_based_bwe_unittest.cc | 56 +++++++--------- .../delay_based_bwe_unittest_helper.cc | 7 +- .../goog_cc/delay_based_bwe_unittest_helper.h | 3 +- 5 files changed, 35 insertions(+), 98 deletions(-) diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index 4c5bdb67a0..2ae5441ef4 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -32,21 +32,8 @@ namespace webrtc { namespace { constexpr TimeDelta kStreamTimeOut = TimeDelta::Seconds(2); - -// Used with field trial "WebRTC-Bwe-NewInterArrivalDelta/Enabled/ constexpr TimeDelta kSendTimeGroupLength = TimeDelta::Millis(5); -// Used unless field trial "WebRTC-Bwe-NewInterArrivalDelta/Enabled/" -constexpr int kTimestampGroupLengthMs = 5; -constexpr int kAbsSendTimeFraction = 18; -constexpr int kAbsSendTimeInterArrivalUpshift = 8; -constexpr int kInterArrivalShift = - kAbsSendTimeFraction + kAbsSendTimeInterArrivalUpshift; -constexpr int kTimestampGroupTicks = - (kTimestampGroupLengthMs << kInterArrivalShift) / 1000; -constexpr double kTimestampToMs = - 1000.0 / static_cast(1 << kInterArrivalShift); - // This ssrc is used to fulfill the current API but will be removed // after the API has been changed. constexpr uint32_t kFixedSsrc = 0; @@ -95,9 +82,6 @@ DelayBasedBwe::DelayBasedBwe(const WebRtcKeyValueConfig* key_value_config, prev_bitrate_(DataRate::Zero()), has_once_detected_overuse_(false), prev_state_(BandwidthUsage::kBwNormal), - use_new_inter_arrival_delta_(!absl::StartsWith( - key_value_config->Lookup("WebRTC-Bwe-NewInterArrivalDelta"), - "Disabled")), alr_limited_backoff_enabled_(absl::StartsWith( key_value_config->Lookup("WebRTC-Bwe-AlrLimitedBackoff"), "Enabled")) { @@ -162,17 +146,11 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, // Reset if the stream has timed out. if (last_seen_packet_.IsInfinite() || at_time - last_seen_packet_ > kStreamTimeOut) { - if (use_new_inter_arrival_delta_) { - video_inter_arrival_delta_ = - std::make_unique(kSendTimeGroupLength); - audio_inter_arrival_delta_ = - std::make_unique(kSendTimeGroupLength); - } else { - video_inter_arrival_ = std::make_unique( - kTimestampGroupTicks, kTimestampToMs, true); - audio_inter_arrival_ = std::make_unique( - kTimestampGroupTicks, kTimestampToMs, true); - } + video_inter_arrival_delta_ = + std::make_unique(kSendTimeGroupLength); + audio_inter_arrival_delta_ = + std::make_unique(kSendTimeGroupLength); + video_delay_detector_.reset( new TrendlineEstimator(key_value_config_, network_state_predictor_)); audio_delay_detector_.reset( @@ -203,7 +181,6 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, } DataSize packet_size = packet_feedback.sent_packet.size; - if (use_new_inter_arrival_delta_) { TimeDelta send_delta = TimeDelta::Zero(); TimeDelta recv_delta = TimeDelta::Zero(); int size_delta = 0; @@ -221,39 +198,6 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, packet_feedback.sent_packet.send_time.ms(), packet_feedback.receive_time.ms(), packet_size.bytes(), calculated_deltas); - } else { - InterArrival* inter_arrival_for_packet = - (separate_audio_.enabled && packet_feedback.sent_packet.audio) - ? video_inter_arrival_.get() - : audio_inter_arrival_.get(); - - uint32_t send_time_24bits = - static_cast( - ((static_cast(packet_feedback.sent_packet.send_time.ms()) - << kAbsSendTimeFraction) + - 500) / - 1000) & - 0x00FFFFFF; - // Shift up send time to use the full 32 bits that inter_arrival works with, - // so wrapping works properly. - uint32_t timestamp = send_time_24bits << kAbsSendTimeInterArrivalUpshift; - - uint32_t timestamp_delta = 0; - int64_t recv_delta_ms = 0; - int size_delta = 0; - - bool calculated_deltas = inter_arrival_for_packet->ComputeDeltas( - timestamp, packet_feedback.receive_time.ms(), at_time.ms(), - packet_size.bytes(), ×tamp_delta, &recv_delta_ms, &size_delta); - double send_delta_ms = - (1000.0 * timestamp_delta) / (1 << kInterArrivalShift); - - delay_detector_for_packet->Update( - recv_delta_ms, send_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 85ce6eaa82..7823f77abe 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.h +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.h @@ -127,7 +127,6 @@ class DelayBasedBwe { DataRate prev_bitrate_; bool has_once_detected_overuse_; BandwidthUsage prev_state_; - const bool use_new_inter_arrival_delta_; bool alr_limited_backoff_enabled_; }; 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 0cc6068b6b..71b7ee7f90 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc @@ -28,16 +28,7 @@ const PacedPacketInfo kPacingInfo1(1, kNumProbesCluster1, 4000); constexpr float kTargetUtilizationFraction = 0.95f; } // namespace -INSTANTIATE_TEST_SUITE_P( - , - DelayBasedBweTest, - ::testing::Values("", "WebRTC-Bwe-NewInterArrivalDelta/Disabled/"), - [](::testing::TestParamInfo info) { - return info.param.empty() ? "SafetypedInterArrival" - : "LegacyInterArrival"; - }); - -TEST_P(DelayBasedBweTest, ProbeDetection) { +TEST_F(DelayBasedBweTest, ProbeDetection) { int64_t now_ms = clock_.TimeInMilliseconds(); // First burst sent at 8 * 1000 / 10 = 800 kbps. @@ -59,7 +50,7 @@ TEST_P(DelayBasedBweTest, ProbeDetection) { EXPECT_GT(bitrate_observer_.latest_bitrate(), 1500000u); } -TEST_P(DelayBasedBweTest, ProbeDetectionNonPacedPackets) { +TEST_F(DelayBasedBweTest, ProbeDetectionNonPacedPackets) { int64_t now_ms = clock_.TimeInMilliseconds(); // First burst sent at 8 * 1000 / 10 = 800 kbps, but with every other packet // not being paced which could mess things up. @@ -76,7 +67,7 @@ TEST_P(DelayBasedBweTest, ProbeDetectionNonPacedPackets) { EXPECT_GT(bitrate_observer_.latest_bitrate(), 800000u); } -TEST_P(DelayBasedBweTest, ProbeDetectionFasterArrival) { +TEST_F(DelayBasedBweTest, ProbeDetectionFasterArrival) { int64_t now_ms = clock_.TimeInMilliseconds(); // First burst sent at 8 * 1000 / 10 = 800 kbps. // Arriving at 8 * 1000 / 5 = 1600 kbps. @@ -91,7 +82,7 @@ TEST_P(DelayBasedBweTest, ProbeDetectionFasterArrival) { EXPECT_FALSE(bitrate_observer_.updated()); } -TEST_P(DelayBasedBweTest, ProbeDetectionSlowerArrival) { +TEST_F(DelayBasedBweTest, ProbeDetectionSlowerArrival) { int64_t now_ms = clock_.TimeInMilliseconds(); // First burst sent at 8 * 1000 / 5 = 1600 kbps. // Arriving at 8 * 1000 / 7 = 1142 kbps. @@ -110,7 +101,7 @@ TEST_P(DelayBasedBweTest, ProbeDetectionSlowerArrival) { kTargetUtilizationFraction * 1140000u, 10000u); } -TEST_P(DelayBasedBweTest, ProbeDetectionSlowerArrivalHighBitrate) { +TEST_F(DelayBasedBweTest, ProbeDetectionSlowerArrivalHighBitrate) { int64_t now_ms = clock_.TimeInMilliseconds(); // Burst sent at 8 * 1000 / 1 = 8000 kbps. // Arriving at 8 * 1000 / 2 = 4000 kbps. @@ -129,7 +120,7 @@ TEST_P(DelayBasedBweTest, ProbeDetectionSlowerArrivalHighBitrate) { kTargetUtilizationFraction * 4000000u, 10000u); } -TEST_P(DelayBasedBweTest, GetExpectedBwePeriodMs) { +TEST_F(DelayBasedBweTest, GetExpectedBwePeriodMs) { auto default_interval = bitrate_estimator_->GetExpectedBwePeriod(); EXPECT_GT(default_interval.ms(), 0); CapacityDropTestHelper(1, true, 333, 0); @@ -138,45 +129,45 @@ TEST_P(DelayBasedBweTest, GetExpectedBwePeriodMs) { EXPECT_NE(interval.ms(), default_interval.ms()); } -TEST_P(DelayBasedBweTest, InitialBehavior) { +TEST_F(DelayBasedBweTest, InitialBehavior) { InitialBehaviorTestHelper(730000); } -TEST_P(DelayBasedBweTest, RateIncreaseReordering) { +TEST_F(DelayBasedBweTest, RateIncreaseReordering) { RateIncreaseReorderingTestHelper(730000); } -TEST_P(DelayBasedBweTest, RateIncreaseRtpTimestamps) { +TEST_F(DelayBasedBweTest, RateIncreaseRtpTimestamps) { RateIncreaseRtpTimestampsTestHelper(622); } -TEST_P(DelayBasedBweTest, CapacityDropOneStream) { +TEST_F(DelayBasedBweTest, CapacityDropOneStream) { CapacityDropTestHelper(1, false, 300, 0); } -TEST_P(DelayBasedBweTest, CapacityDropPosOffsetChange) { +TEST_F(DelayBasedBweTest, CapacityDropPosOffsetChange) { CapacityDropTestHelper(1, false, 867, 30000); } -TEST_P(DelayBasedBweTest, CapacityDropNegOffsetChange) { +TEST_F(DelayBasedBweTest, CapacityDropNegOffsetChange) { CapacityDropTestHelper(1, false, 933, -30000); } -TEST_P(DelayBasedBweTest, CapacityDropOneStreamWrap) { +TEST_F(DelayBasedBweTest, CapacityDropOneStreamWrap) { CapacityDropTestHelper(1, true, 333, 0); } -TEST_P(DelayBasedBweTest, TestTimestampGrouping) { +TEST_F(DelayBasedBweTest, TestTimestampGrouping) { TestTimestampGroupingTestHelper(); } -TEST_P(DelayBasedBweTest, TestShortTimeoutAndWrap) { +TEST_F(DelayBasedBweTest, TestShortTimeoutAndWrap) { // Simulate a client leaving and rejoining the call after 35 seconds. This // will make abs send time wrap, so if streams aren't timed out properly // the next 30 seconds of packets will be out of order. TestWrappingHelper(35); } -TEST_P(DelayBasedBweTest, TestLongTimeoutAndWrap) { +TEST_F(DelayBasedBweTest, TestLongTimeoutAndWrap) { // Simulate a client leaving and rejoining the call after some multiple of // 64 seconds later. This will cause a zero difference in abs send times due // to the wrap, but a big difference in arrival time, if streams aren't @@ -184,7 +175,7 @@ TEST_P(DelayBasedBweTest, TestLongTimeoutAndWrap) { TestWrappingHelper(10 * 64); } -TEST_P(DelayBasedBweTest, TestInitialOveruse) { +TEST_F(DelayBasedBweTest, TestInitialOveruse) { const DataRate kStartBitrate = DataRate::KilobitsPerSec(300); const DataRate kInitialCapacity = DataRate::KilobitsPerSec(200); const uint32_t kDummySsrc = 0; @@ -224,16 +215,15 @@ TEST_P(DelayBasedBweTest, TestInitialOveruse) { } class DelayBasedBweTestWithBackoffTimeoutExperiment : public DelayBasedBweTest { + public: + DelayBasedBweTestWithBackoffTimeoutExperiment() + : DelayBasedBweTest( + "WebRTC-BweAimdRateControlConfig/initial_backoff_interval:200ms/") { + } }; -INSTANTIATE_TEST_SUITE_P( - , - DelayBasedBweTestWithBackoffTimeoutExperiment, - ::testing::Values( - "WebRTC-BweAimdRateControlConfig/initial_backoff_interval:200ms/")); - // This test subsumes and improves DelayBasedBweTest.TestInitialOveruse above. -TEST_P(DelayBasedBweTestWithBackoffTimeoutExperiment, TestInitialOveruse) { +TEST_F(DelayBasedBweTestWithBackoffTimeoutExperiment, TestInitialOveruse) { const DataRate kStartBitrate = DataRate::KilobitsPerSec(300); const DataRate kInitialCapacity = DataRate::KilobitsPerSec(200); const uint32_t kDummySsrc = 0; 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 4c72277aa8..3eb0ae38e5 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 @@ -145,8 +145,11 @@ int64_t StreamGenerator::GenerateFrame(std::vector* packets, } } // namespace test -DelayBasedBweTest::DelayBasedBweTest() - : field_trial(std::make_unique(GetParam())), +DelayBasedBweTest::DelayBasedBweTest() : DelayBasedBweTest("") {} + +DelayBasedBweTest::DelayBasedBweTest(const std::string& field_trial_string) + : field_trial( + std::make_unique(field_trial_string)), 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 5fb048b7a4..927cf9b0cb 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 @@ -113,9 +113,10 @@ class StreamGenerator { }; } // namespace test -class DelayBasedBweTest : public ::testing::TestWithParam { +class DelayBasedBweTest : public ::testing::Test { public: DelayBasedBweTest(); + explicit DelayBasedBweTest(const std::string& field_trial_string); ~DelayBasedBweTest() override; protected: