From f2fe43b6550e2a8c90d80147e01d326ae74f80d4 Mon Sep 17 00:00:00 2001 From: Filip Hlasek Date: Fri, 6 May 2022 01:35:28 -0700 Subject: [PATCH] Don't round the computed time deltas to nearest ms. Resolving https://bugs.chromium.org/p/webrtc/issues/detail?id=14023 At the moment, in DelayBasedBwe the time deltas are rounded to the nearest millisecond. This change makes sure the numbers are passed as doubles as expected by the TrendlineEstimator. Change-Id: I68882547fb19af0e67e7b5d8de4159083a54d7eb Bug: webrtc:14023 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261320 Reviewed-by: Per Kjellander Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#36806} --- AUTHORS | 1 + .../goog_cc/delay_based_bwe.cc | 3 +- .../goog_cc/delay_based_bwe_unittest.cc | 35 +++++++++++++++++++ .../delay_based_bwe_unittest_helper.cc | 13 +++++-- .../goog_cc/delay_based_bwe_unittest_helper.h | 4 +++ 5 files changed, 52 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index 3e3ece47f4..393c1e1005 100644 --- a/AUTHORS +++ b/AUTHORS @@ -44,6 +44,7 @@ Dirk-Jan C. Binnema Dmitry Lizin Eike Rathke Eric Rescorla, RTFM Inc. +Filip Hlasek Frederik Riedel, Frogg GmbH Giji Gangadharan Graham Yoakum diff --git a/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/modules/congestion_controller/goog_cc/delay_based_bwe.cc index fddeca3135..750c5c60fb 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -193,7 +193,8 @@ void DelayBasedBwe::IncomingPacketFeedback(const PacketResult& packet_feedback, packet_feedback.sent_packet.send_time, packet_feedback.receive_time, at_time, packet_size.bytes(), &send_delta, &recv_delta, &size_delta); - delay_detector_for_packet->Update(recv_delta.ms(), send_delta.ms(), + 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); 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 71b7ee7f90..b7dc6aae47 100644 --- a/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc +++ b/modules/congestion_controller/goog_cc/delay_based_bwe_unittest.cc @@ -214,6 +214,41 @@ TEST_F(DelayBasedBweTest, TestInitialOveruse) { 15000); } +TEST_F(DelayBasedBweTest, TestTimestampPrecisionHandling) { + // This test does some basic checks to make sure that timestamps with higher + // than millisecond precision are handled properly and do not cause any + // problems in the estimator. Specifically, previously reported in + // webrtc:14023 and described in more details there, the rounding to the + // nearest milliseconds caused discrepancy in the accumulated delay. This lead + // to false-positive overuse detection. + // Technical details of the test: + // Send times(ms): 0.000, 9.725, 20.000, 29.725, 40.000, 49.725, ... + // Recv times(ms): 0.500, 10.000, 20.500, 30.000, 40.500, 50.000, ... + // Send deltas(ms): 9.750, 10.250, 9.750, 10.250, 9.750, ... + // Recv deltas(ms): 9.500, 10.500, 9.500, 10.500, 9.500, ... + // There is no delay building up between the send times and the receive times, + // therefore this case should never lead to an overuse detection. However, if + // the time deltas were accidentally rounded to the nearest milliseconds, then + // all the send deltas would be equal to 10ms while some recv deltas would + // round up to 11ms which would lead in a false illusion of delay build up. + uint32_t last_bitrate = bitrate_observer_.latest_bitrate(); + for (int i = 0; i < 1000; ++i) { + clock_.AdvanceTimeMicroseconds(500); + IncomingFeedback(clock_.CurrentTime(), + clock_.CurrentTime() - TimeDelta::Micros(500), 1000, + PacedPacketInfo()); + clock_.AdvanceTimeMicroseconds(9500); + IncomingFeedback(clock_.CurrentTime(), + clock_.CurrentTime() - TimeDelta::Micros(250), 1000, + PacedPacketInfo()); + clock_.AdvanceTimeMicroseconds(10000); + + // The bitrate should never decrease in this test. + EXPECT_LE(last_bitrate, bitrate_observer_.latest_bitrate()); + last_bitrate = bitrate_observer_.latest_bitrate(); + } +} + class DelayBasedBweTestWithBackoffTimeoutExperiment : public DelayBasedBweTest { public: DelayBasedBweTestWithBackoffTimeoutExperiment() 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 6b4cd77de7..bb91d5957d 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 @@ -179,10 +179,17 @@ void DelayBasedBweTest::IncomingFeedback(int64_t arrival_time_ms, size_t payload_size, const PacedPacketInfo& pacing_info) { RTC_CHECK_GE(arrival_time_ms + arrival_time_offset_ms_, 0); + IncomingFeedback(Timestamp::Millis(arrival_time_ms + arrival_time_offset_ms_), + Timestamp::Millis(send_time_ms), payload_size, pacing_info); +} + +void DelayBasedBweTest::IncomingFeedback(Timestamp receive_time, + Timestamp send_time, + size_t payload_size, + const PacedPacketInfo& pacing_info) { PacketResult packet; - packet.receive_time = - Timestamp::Millis(arrival_time_ms + arrival_time_offset_ms_); - packet.sent_packet.send_time = Timestamp::Millis(send_time_ms); + packet.receive_time = receive_time; + packet.sent_packet.send_time = send_time; packet.sent_packet.size = DataSize::Bytes(payload_size); packet.sent_packet.pacing_info = pacing_info; if (packet.sent_packet.pacing_info.probe_cluster_id != 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 474d2970df..a5da4a017d 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 @@ -131,6 +131,10 @@ class DelayBasedBweTest : public ::testing::Test { int64_t send_time_ms, size_t payload_size, const PacedPacketInfo& pacing_info); + void IncomingFeedback(Timestamp receive_time, + Timestamp send_time, + size_t payload_size, + const PacedPacketInfo& pacing_info); // Generates a frame of packets belonging to a stream at a given bitrate and // with a given ssrc. The stream is pushed through a very simple simulated