Use SeqNumUnwapper to handle reordering of first packets

Fixed todo by replacing SequenceNumberUnwrapper with updated class
SeqNumUnwrapper that correctly handles reordering of early packets.

Bug: webrtc:10263
Change-Id: Iffd93db924fee132d35752996b8d29acbb315d24
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130498
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Commit-Queue: Johannes Kron <kron@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27417}

modules/remote_bitrate_estimator/BUILD.gn

@@ -50,6 +50,7 @@ rtc_static_library("remote_bitrate_estimator") {
     "../../modules/rtp_rtcp:rtp_rtcp_format",
     "../../rtc_base:checks",
     "../../rtc_base:rtc_base_approved",
+    "../../rtc_base:rtc_numerics",
     "../../rtc_base:safe_minmax",
     "../../rtc_base/experiments:field_trial_parser",
     "../../system_wrappers",

modules/remote_bitrate_estimator/remote_estimator_proxy.cc

@@ -42,7 +42,6 @@ RemoteEstimatorProxy::RemoteEstimatorProxy(
       last_process_time_ms_(-1),
       media_ssrc_(0),
       feedback_packet_count_(0),
-      window_start_seq_(-1),
       send_interval_ms_(kDefaultSendIntervalMs),
       send_feedback_on_request_only_(false) {}
 
@@ -125,40 +124,27 @@ void RemoteEstimatorProxy::OnPacketArrival(
     return;
   }
 
-  // TODO(holmer): We should handle a backwards wrap here if the first
-  // sequence number was small and the new sequence number is large. The
-  // SequenceNumberUnwrapper doesn't do this, so we should replace this with
-  // calls to IsNewerSequenceNumber instead.
   int64_t seq = unwrapper_.Unwrap(sequence_number);
-  if (window_start_seq_ != -1 && seq > window_start_seq_ + 0xFFFF / 2) {
-    RTC_LOG(LS_WARNING) << "Skipping this sequence number (" << sequence_number
-                        << ") since it likely is reordered, but the unwrapper"
-                           "failed to handle it. Feedback window starts at "
-                        << window_start_seq_ << ".";
-    return;
-  }
 
   if (send_feedback_on_request_only_) {
     // Remove old packet arrival times.
     auto clear_to_it =
         packet_arrival_times_.lower_bound(seq - kMaxNumberOfPackets);
     packet_arrival_times_.erase(packet_arrival_times_.begin(), clear_to_it);
-  } else if (packet_arrival_times_.lower_bound(window_start_seq_) ==
-             packet_arrival_times_.end()) {
-    // Start new feedback packet, cull old packets.
-    for (auto it = packet_arrival_times_.begin();
-         it != packet_arrival_times_.end() && it->first < seq &&
-         arrival_time - it->second >= kBackWindowMs;) {
-      auto delete_it = it;
-      ++it;
-      packet_arrival_times_.erase(delete_it);
+  } else {
+    if (periodic_window_start_seq_ &&
+        packet_arrival_times_.lower_bound(*periodic_window_start_seq_) ==
+            packet_arrival_times_.end()) {
+      // Start new feedback packet, cull old packets.
+      for (auto it = packet_arrival_times_.begin();
+           it != packet_arrival_times_.end() && it->first < seq &&
+           arrival_time - it->second >= kBackWindowMs;) {
+        it = packet_arrival_times_.erase(it);
+      }
+    }
+    if (!periodic_window_start_seq_ || seq < *periodic_window_start_seq_) {
+      periodic_window_start_seq_ = seq;
     }
-  }
-
-  if (window_start_seq_ == -1) {
-    window_start_seq_ = sequence_number;
-  } else if (seq < window_start_seq_) {
-    window_start_seq_ = seq;
   }
 
   // We are only interested in the first time a packet is received.
@@ -174,16 +160,20 @@ void RemoteEstimatorProxy::OnPacketArrival(
 }
 
 void RemoteEstimatorProxy::SendPeriodicFeedbacks() {
-  // |window_start_seq_| is the first sequence number to include in the current
-  // feedback packet. Some older may still be in the map, in case a reordering
-  // happens and we need to retransmit them.
+  // |periodic_window_start_seq_| is the first sequence number to include in the
+  // current feedback packet. Some older may still be in the map, in case a
+  // reordering happens and we need to retransmit them.
+  if (!periodic_window_start_seq_)
+    return;
+
   for (auto begin_iterator =
-           packet_arrival_times_.lower_bound(window_start_seq_);
+           packet_arrival_times_.lower_bound(*periodic_window_start_seq_);
        begin_iterator != packet_arrival_times_.cend();
-       begin_iterator = packet_arrival_times_.lower_bound(window_start_seq_)) {
+       begin_iterator =
+           packet_arrival_times_.lower_bound(*periodic_window_start_seq_)) {
     rtcp::TransportFeedback feedback_packet;
-    window_start_seq_ = BuildFeedbackPacket(
-        feedback_packet_count_++, media_ssrc_, window_start_seq_,
+    periodic_window_start_seq_ = BuildFeedbackPacket(
+        feedback_packet_count_++, media_ssrc_, *periodic_window_start_seq_,
         begin_iterator, packet_arrival_times_.cend(), &feedback_packet);
 
     RTC_DCHECK(feedback_sender_ != nullptr);
@@ -208,11 +198,9 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest(
       packet_arrival_times_.lower_bound(first_sequence_number);
   auto end_iterator = packet_arrival_times_.upper_bound(sequence_number);
 
-  // window_start_seq must be updated to make sure that we detect incorrectly
-  // unwrapped sequence_numbers in OnPacketArrival().
-  window_start_seq_ = BuildFeedbackPacket(feedback_packet_count_++, media_ssrc_,
-                                          first_sequence_number, begin_iterator,
-                                          end_iterator, &feedback_packet);
+  BuildFeedbackPacket(feedback_packet_count_++, media_ssrc_,
+                      first_sequence_number, begin_iterator, end_iterator,
+                      &feedback_packet);
 
   // Clear up to the first packet that is included in this feedback packet.
   packet_arrival_times_.erase(packet_arrival_times_.begin(), begin_iterator);

modules/remote_bitrate_estimator/remote_estimator_proxy.h

@@ -14,9 +14,9 @@
 #include <map>
 #include <vector>
 
-#include "modules/include/module_common_types.h"
 #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 #include "rtc_base/critical_section.h"
+#include "rtc_base/numerics/sequence_number_util.h"
 
 namespace webrtc {
 
@@ -81,8 +81,8 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
 
   uint32_t media_ssrc_ RTC_GUARDED_BY(&lock_);
   uint8_t feedback_packet_count_ RTC_GUARDED_BY(&lock_);
-  SequenceNumberUnwrapper unwrapper_ RTC_GUARDED_BY(&lock_);
-  int64_t window_start_seq_ RTC_GUARDED_BY(&lock_);
+  SeqNumUnwrapper<uint16_t> unwrapper_ RTC_GUARDED_BY(&lock_);
+  absl::optional<int64_t> periodic_window_start_seq_ RTC_GUARDED_BY(&lock_);
   // Map unwrapped seq -> time.
   std::map<int64_t, int64_t> packet_arrival_times_ RTC_GUARDED_BY(&lock_);
   int64_t send_interval_ms_ RTC_GUARDED_BY(&lock_);

modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc

@@ -196,18 +196,19 @@ TEST_F(RemoteEstimatorProxyTest, SendsFragmentedFeedback) {
   Process();
 }
 
-TEST_F(RemoteEstimatorProxyTest, GracefullyHandlesReorderingAndWrap) {
+TEST_F(RemoteEstimatorProxyTest, HandlesReorderingAndWrap) {
   const int64_t kDeltaMs = 1000;
   const uint16_t kLargeSeq = 62762;
   IncomingPacket(kBaseSeq, kBaseTimeMs);
   IncomingPacket(kLargeSeq, kBaseTimeMs + kDeltaMs);
 
   EXPECT_CALL(router_, SendTransportFeedback(_))
-      .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) {
-        EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence());
+      .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) {
+        EXPECT_EQ(kLargeSeq, feedback_packet->GetBaseSequence());
         EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc());
 
-        EXPECT_THAT(TimestampsMs(*feedback_packet), ElementsAre(kBaseTimeMs));
+        EXPECT_THAT(TimestampsMs(*feedback_packet),
+                    ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs));
         return true;
       }));