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