From fc22dfa72798a4c2c1e5dcdf2fe3e211716b82db Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 25 Jul 2022 13:27:59 +0200 Subject: [PATCH] In RemoteEstimatorProxy treat zero arrival time as valid Inserting packet with zero arrival time may trigger inconsistent state in the internal map where packet sometimes treated as received, but sometimes treated as not received. Bug: chromium:1346959 Change-Id: I0809e41a873103dcd62528358e64794c1d3cb28f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269382 Reviewed-by: Philip Eliasson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#37609} --- modules/remote_bitrate_estimator/packet_arrival_map.cc | 9 +++++---- .../remote_bitrate_estimator/packet_arrival_map_test.cc | 8 ++++---- .../remote_bitrate_estimator/remote_estimator_proxy.cc | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/modules/remote_bitrate_estimator/packet_arrival_map.cc b/modules/remote_bitrate_estimator/packet_arrival_map.cc index deeb208c15..09c9e5aed1 100644 --- a/modules/remote_bitrate_estimator/packet_arrival_map.cc +++ b/modules/remote_bitrate_estimator/packet_arrival_map.cc @@ -19,6 +19,7 @@ constexpr size_t PacketArrivalTimeMap::kMaxNumberOfPackets; void PacketArrivalTimeMap::AddPacket(int64_t sequence_number, Timestamp arrival_time) { + RTC_DCHECK_GE(arrival_time, Timestamp::Zero()); if (!has_seen_packet_) { // First packet. has_seen_packet_ = true; @@ -45,7 +46,7 @@ void PacketArrivalTimeMap::AddPacket(int64_t sequence_number, } arrival_times_.insert(arrival_times_.begin(), missing_packets, - Timestamp::Zero()); + Timestamp::MinusInfinity()); arrival_times_[0] = arrival_time; begin_sequence_number_ = sequence_number; return; @@ -64,7 +65,7 @@ void PacketArrivalTimeMap::AddPacket(int64_t sequence_number, // Also trim the buffer to remove leading non-received packets, to // ensure that the buffer only spans received packets. while (packets_to_remove < arrival_times_.size() && - arrival_times_[packets_to_remove] == Timestamp::Zero()) { + arrival_times_[packets_to_remove].IsInfinite()) { ++packets_to_remove; } @@ -81,7 +82,7 @@ void PacketArrivalTimeMap::AddPacket(int64_t sequence_number, size_t missing_gap_packets = pos - arrival_times_.size(); if (missing_gap_packets > 0) { arrival_times_.insert(arrival_times_.end(), missing_gap_packets, - Timestamp::Zero()); + Timestamp::MinusInfinity()); } RTC_DCHECK_EQ(arrival_times_.size(), pos); arrival_times_.push_back(arrival_time); @@ -100,7 +101,7 @@ void PacketArrivalTimeMap::RemoveOldPackets(int64_t sequence_number, bool PacketArrivalTimeMap::has_received(int64_t sequence_number) const { int64_t pos = sequence_number - begin_sequence_number_; if (pos >= 0 && pos < static_cast(arrival_times_.size()) && - arrival_times_[pos] != Timestamp::Zero()) { + arrival_times_[pos].IsFinite()) { return true; } return false; diff --git a/modules/remote_bitrate_estimator/packet_arrival_map_test.cc b/modules/remote_bitrate_estimator/packet_arrival_map_test.cc index de506386ba..c083daa300 100644 --- a/modules/remote_bitrate_estimator/packet_arrival_map_test.cc +++ b/modules/remote_bitrate_estimator/packet_arrival_map_test.cc @@ -43,7 +43,7 @@ TEST(PacketArrivalMapTest, InsertsFirstItemIntoMap) { TEST(PacketArrivalMapTest, InsertsWithGaps) { PacketArrivalTimeMap map; - map.AddPacket(42, Timestamp::Millis(10)); + map.AddPacket(42, Timestamp::Zero()); map.AddPacket(45, Timestamp::Millis(11)); EXPECT_EQ(map.begin_sequence_number(), 42); EXPECT_EQ(map.end_sequence_number(), 46); @@ -55,9 +55,9 @@ TEST(PacketArrivalMapTest, InsertsWithGaps) { EXPECT_TRUE(map.has_received(45)); EXPECT_FALSE(map.has_received(46)); - EXPECT_EQ(map.get(42), Timestamp::Millis(10)); - EXPECT_EQ(map.get(43), Timestamp::Zero()); - EXPECT_EQ(map.get(44), Timestamp::Zero()); + EXPECT_EQ(map.get(42), Timestamp::Zero()); + EXPECT_LT(map.get(43), Timestamp::Zero()); + EXPECT_LT(map.get(44), Timestamp::Zero()); EXPECT_EQ(map.get(45), Timestamp::Millis(11)); EXPECT_EQ(map.clamp(-100), 42); diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 0215b64155..1bb2f5532b 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -292,7 +292,7 @@ RemoteEstimatorProxy::MaybeBuildFeedbackPacket( for (int64_t seq = start_seq; seq < end_seq; ++seq) { Timestamp arrival_time = packet_arrival_times_.get(seq); - if (arrival_time == Timestamp::Zero()) { + if (arrival_time.IsInfinite()) { // Packet not received. continue; }