diff --git a/modules/include/module_common_types_public.h b/modules/include/module_common_types_public.h index d4dab40573..345e45ce12 100644 --- a/modules/include/module_common_types_public.h +++ b/modules/include/module_common_types_public.h @@ -83,11 +83,15 @@ class Unwrapper { using SequenceNumberUnwrapper = Unwrapper; using TimestampUnwrapper = Unwrapper; +// NB: Doesn't fulfill strict weak ordering requirements. +// Mustn't be used as std::map Compare function. inline bool IsNewerSequenceNumber(uint16_t sequence_number, uint16_t prev_sequence_number) { return IsNewer(sequence_number, prev_sequence_number); } +// NB: Doesn't fulfill strict weak ordering requirements. +// Mustn't be used as std::map Compare function. inline bool IsNewerTimestamp(uint32_t timestamp, uint32_t prev_timestamp) { return IsNewer(timestamp, prev_timestamp); } diff --git a/modules/remote_bitrate_estimator/test/bwe.cc b/modules/remote_bitrate_estimator/test/bwe.cc index 6120c0f3e1..7833eece97 100644 --- a/modules/remote_bitrate_estimator/test/bwe.cc +++ b/modules/remote_bitrate_estimator/test/bwe.cc @@ -10,6 +10,7 @@ #include "modules/remote_bitrate_estimator/test/bwe.h" +#include #include #include "modules/remote_bitrate_estimator/test/estimators/bbr.h" @@ -18,16 +19,13 @@ #include "modules/remote_bitrate_estimator/test/estimators/send_side.h" #include "modules/remote_bitrate_estimator/test/estimators/tcp.h" #include "rtc_base/constructormagic.h" +#include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/system/fallthrough.h" namespace webrtc { namespace testing { namespace bwe { -// With the assumption that packet loss is lower than 97%, the max gap -// between elements in the set is lower than 0x8000, hence we have a -// total order in the set. For (x,y,z) subset of the LinkedSet, -// (x<=y and y<=z) ==> x<=z so the set can be sorted. const int kSetCapacity = 1000; BweReceiver::BweReceiver(int flow_id) @@ -163,12 +161,7 @@ LossAccount BweReceiver::LinkedSetPacketLossRatio() { return LossAccount(); } - uint16_t oldest_seq_num = received_packets_.OldestSeqNumber(); - uint16_t newest_seq_num = received_packets_.NewestSeqNumber(); - - size_t set_total_packets = - static_cast(newest_seq_num - oldest_seq_num + 1); - + size_t set_total_packets = received_packets_.Range(); size_t set_received_packets = received_packets_.size(); size_t set_lost_packets = set_total_packets - set_received_packets; @@ -195,26 +188,21 @@ float BweReceiver::RecentPacketLossRatio() { // Lowest timestamp limit, oldest one that should be checked. int64_t time_limit_ms = (*node_it)->arrival_time_ms - kPacketLossTimeWindowMs; // Oldest and newest values found within the given time window. - uint16_t oldest_seq_num = (*node_it)->sequence_number; - uint16_t newest_seq_num = oldest_seq_num; + int64_t oldest_seq_num = (*node_it)->unwrapped_sequence_number; + int64_t newest_seq_num = oldest_seq_num; while (node_it != received_packets_.end()) { if ((*node_it)->arrival_time_ms < time_limit_ms) { break; } - uint16_t seq_num = (*node_it)->sequence_number; - if (IsNewerSequenceNumber(seq_num, newest_seq_num)) { - newest_seq_num = seq_num; - } - if (IsNewerSequenceNumber(oldest_seq_num, seq_num)) { - oldest_seq_num = seq_num; - } + int64_t seq_num = (*node_it)->unwrapped_sequence_number; + newest_seq_num = std::max(newest_seq_num, seq_num); + oldest_seq_num = std::min(oldest_seq_num, seq_num); ++node_it; ++number_packets_received; } // Interval width between oldest and newest sequence number. - // There was an overflow if newest_seq_num < oldest_seq_num. - int gap = static_cast(newest_seq_num - oldest_seq_num + 1); + int gap = rtc::dchecked_cast(newest_seq_num - oldest_seq_num + 1); return static_cast(gap - number_packets_received) / gap; } @@ -230,7 +218,9 @@ void LinkedSet::Insert(uint16_t sequence_number, int64_t send_time_ms, int64_t arrival_time_ms, size_t payload_size) { - auto it = map_.find(sequence_number); + auto unwrapped_sequence_number = unwrapper_.Unwrap(sequence_number); + auto it = map_.find(unwrapped_sequence_number); + // Handle duplicate unwrapped sequence number. if (it != map_.end()) { PacketNodeIt node_it = it->second; PacketIdentifierNode* node = *node_it; @@ -238,34 +228,36 @@ void LinkedSet::Insert(uint16_t sequence_number, if (node_it != list_.begin()) { list_.erase(node_it); list_.push_front(node); - map_[sequence_number] = list_.begin(); + map_[unwrapped_sequence_number] = list_.begin(); } } else { if (size() == capacity_) { RemoveTail(); } - UpdateHead(new PacketIdentifierNode(sequence_number, send_time_ms, + UpdateHead(new PacketIdentifierNode(unwrapped_sequence_number, send_time_ms, arrival_time_ms, payload_size)); } } void LinkedSet::Insert(PacketIdentifierNode packet_identifier) { - Insert(packet_identifier.sequence_number, packet_identifier.send_time_ms, - packet_identifier.arrival_time_ms, packet_identifier.payload_size); + Insert(packet_identifier.unwrapped_sequence_number, + packet_identifier.send_time_ms, packet_identifier.arrival_time_ms, + packet_identifier.payload_size); } void LinkedSet::RemoveTail() { - map_.erase(list_.back()->sequence_number); + map_.erase(list_.back()->unwrapped_sequence_number); delete list_.back(); list_.pop_back(); } + void LinkedSet::UpdateHead(PacketIdentifierNode* new_head) { list_.push_front(new_head); - map_[new_head->sequence_number] = list_.begin(); + map_[new_head->unwrapped_sequence_number] = list_.begin(); } void LinkedSet::Erase(PacketNodeIt node_it) { - map_.erase((*node_it)->sequence_number); + map_.erase((*node_it)->unwrapped_sequence_number); delete (*node_it); list_.erase(node_it); } diff --git a/modules/remote_bitrate_estimator/test/bwe.h b/modules/remote_bitrate_estimator/test/bwe.h index 42682aed9a..24385f2595 100644 --- a/modules/remote_bitrate_estimator/test/bwe.h +++ b/modules/remote_bitrate_estimator/test/bwe.h @@ -21,19 +21,12 @@ #include "modules/remote_bitrate_estimator/test/packet.h" #include "rtc_base/constructormagic.h" #include "rtc_base/gtest_prod_util.h" +#include "rtc_base/numerics/sequence_number_util.h" namespace webrtc { namespace testing { namespace bwe { -// Overload map comparator. -class SequenceNumberOlderThan { - public: - bool operator()(uint16_t seq_num_1, uint16_t seq_num_2) const { - return IsNewerSequenceNumber(seq_num_2, seq_num_1); - } -}; - // Holds information for computing global packet loss. struct LossAccount { LossAccount() : num_total(0), num_lost(0) {} @@ -49,16 +42,16 @@ struct LossAccount { // Holds only essential information about packets to be saved for // further use, e.g. for calculating packet loss and receiving rate. struct PacketIdentifierNode { - PacketIdentifierNode(uint16_t sequence_number, + PacketIdentifierNode(int64_t unwrapped_sequence_number, int64_t send_time_ms, int64_t arrival_time_ms, size_t payload_size) - : sequence_number(sequence_number), + : unwrapped_sequence_number(unwrapped_sequence_number), send_time_ms(send_time_ms), arrival_time_ms(arrival_time_ms), payload_size(payload_size) {} - uint16_t sequence_number; + int64_t unwrapped_sequence_number; int64_t send_time_ms; int64_t arrival_time_ms; size_t payload_size; @@ -92,9 +85,10 @@ class LinkedSet { size_t size() const { return list_.size(); } size_t capacity() const { return capacity_; } - uint16_t OldestSeqNumber() const { return empty() ? 0 : map_.begin()->first; } - uint16_t NewestSeqNumber() const { - return empty() ? 0 : map_.rbegin()->first; + // Return size of interval covering current set, i.e.: + // unwrapped newest seq number - unwrapped oldest seq number + 1 + int64_t Range() const { + return empty() ? 0 : map_.rbegin()->first - map_.begin()->first + 1; } void Erase(PacketNodeIt node_it); @@ -105,7 +99,10 @@ class LinkedSet { // Add new element to the front of the list and insert it in the map. void UpdateHead(PacketIdentifierNode* new_head); size_t capacity_; - std::map map_; + // We want to keep track of the current oldest and newest sequence_numbers. + // To get strict weak ordering, we unwrap uint16_t into an int64_t. + SeqNumUnwrapper unwrapper_; + std::map map_; std::list list_; }; diff --git a/modules/remote_bitrate_estimator/test/bwe_unittest.cc b/modules/remote_bitrate_estimator/test/bwe_unittest.cc index 892020c3c4..fd4d4e789c 100644 --- a/modules/remote_bitrate_estimator/test/bwe_unittest.cc +++ b/modules/remote_bitrate_estimator/test/bwe_unittest.cc @@ -33,8 +33,7 @@ class LinkedSetTest : public ::testing::Test { }; TEST_F(LinkedSetTest, EmptySet) { - EXPECT_EQ(linked_set_.OldestSeqNumber(), 0); - EXPECT_EQ(linked_set_.NewestSeqNumber(), 0); + EXPECT_EQ(linked_set_.Range(), 0); } TEST_F(LinkedSetTest, SinglePacket) { @@ -42,8 +41,7 @@ TEST_F(LinkedSetTest, SinglePacket) { // Other parameters don't matter here. linked_set_.Insert(kSeqNumber, 0, 0, 0); - EXPECT_EQ(linked_set_.OldestSeqNumber(), kSeqNumber); - EXPECT_EQ(linked_set_.NewestSeqNumber(), kSeqNumber); + EXPECT_EQ(linked_set_.Range(), 1); } TEST_F(LinkedSetTest, MultiplePackets) { @@ -61,9 +59,8 @@ TEST_F(LinkedSetTest, MultiplePackets) { linked_set_.Insert(static_cast(i), 0, 0, 0); } - // Packets arriving out of order should not affect the following values: - EXPECT_EQ(linked_set_.OldestSeqNumber(), 0); - EXPECT_EQ(linked_set_.NewestSeqNumber(), kNumberPackets - 1); + // Packets arriving out of order should not affect the following value: + EXPECT_EQ(linked_set_.Range(), kNumberPackets); } TEST_F(LinkedSetTest, Overflow) { @@ -75,32 +72,29 @@ TEST_F(LinkedSetTest, Overflow) { linked_set_.Insert(static_cast(i), 0, 0, 0); } - // Packets arriving out of order should not affect the following values: - EXPECT_EQ(linked_set_.OldestSeqNumber(), - static_cast(kFirstSeqNumber)); - EXPECT_EQ(linked_set_.NewestSeqNumber(), - static_cast(kLastSeqNumber)); + // Wrapping shouldn't matter + EXPECT_EQ(linked_set_.Range(), kLastSeqNumber - kFirstSeqNumber + 1); } -class SequenceNumberOlderThanTest : public ::testing::Test { - public: - SequenceNumberOlderThanTest() {} - ~SequenceNumberOlderThanTest() {} +TEST_F(LinkedSetTest, SameSequenceNumbers) { + // Test correct behavior when + // sequence numbers wrap (after 0xFFFF). - protected: - SequenceNumberOlderThan comparator_; -}; + // Choose step such as step*capacity < 0x8000 + // (received packets in a reasonable window) + const int kStep = 0x20; + // Choose iteration such as step*iteration > 0x10000 + // (imply wrap) + const int kIterations = 0x1000; -TEST_F(SequenceNumberOlderThanTest, Operator) { - // Operator()(x, y) returns true <==> y is newer than x. - EXPECT_TRUE(comparator_.operator()(0x0000, 0x0001)); - EXPECT_TRUE(comparator_.operator()(0x0001, 0x1000)); - EXPECT_FALSE(comparator_.operator()(0x0001, 0x0000)); - EXPECT_FALSE(comparator_.operator()(0x0002, 0x0002)); - EXPECT_TRUE(comparator_.operator()(0xFFF6, 0x000A)); - EXPECT_FALSE(comparator_.operator()(0x000A, 0xFFF6)); - EXPECT_TRUE(comparator_.operator()(0x0000, 0x8000)); - EXPECT_FALSE(comparator_.operator()(0x8000, 0x0000)); + int kSeqNumber = 1; + for (int i = 0; i < kIterations; ++i) { + // Other parameters don't matter here. + linked_set_.Insert(static_cast(kSeqNumber), 0, 0, 0); + kSeqNumber += kStep; + } + + EXPECT_EQ(linked_set_.Range(), (kSetCapacity - 1) * kStep + 1); } class LossAccountTest : public ::testing::Test {