Refactor RemoteEstimatorProxy

As it will be heavily modified in a follow-up change, it's refactored
as a separate commit to make commits small:

 * Extracted code to separate AddPacket and CullOldPackets methods
 * BuildFeedbackPacket now returns a packet. In the next iteration, it
   might not, so it needs to be able to decide when to increment the
   packet sequence number.
 * Documented some existing fields.

The follow-up is in change I3dd58105f9fbfb111f176833cd4aa6b040c0e01d.

Bug: webrtc:12689
Change-Id: I5ad0aee5a7da008f9e209f7c13bf299c12f9d1f3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217581
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33942}

modules/remote_bitrate_estimator/remote_estimator_proxy.cc

@@ -54,6 +54,25 @@ RemoteEstimatorProxy::RemoteEstimatorProxy(
 
 RemoteEstimatorProxy::~RemoteEstimatorProxy() {}
 
+void RemoteEstimatorProxy::AddPacket(int64_t sequence_number,
+                                     int64_t arrival_time_ms) {
+  packet_arrival_times_[sequence_number] = arrival_time_ms;
+}
+
+void RemoteEstimatorProxy::MaybeCullOldPackets(int64_t sequence_number,
+                                               int64_t arrival_time_ms) {
+  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 < sequence_number &&
+         arrival_time_ms - it->second >= send_config_.back_window->ms();) {
+      it = packet_arrival_times_.erase(it);
+    }
+  }
+}
+
 void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms,
                                           size_t payload_size,
                                           const RTPHeader& header) {
@@ -69,16 +88,8 @@ void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms,
     seq = unwrapper_.Unwrap(header.extension.transportSequenceNumber);
 
     if (send_periodic_feedback_) {
-      if (periodic_window_start_seq_ &&
+      MaybeCullOldPackets(seq, arrival_time_ms);
-          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_ms - it->second >= send_config_.back_window->ms();) {
-          it = packet_arrival_times_.erase(it);
-        }
-      }
       if (!periodic_window_start_seq_ || seq < *periodic_window_start_seq_) {
         periodic_window_start_seq_ = seq;
       }
@@ -88,7 +99,7 @@ void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms,
     if (packet_arrival_times_.find(seq) != packet_arrival_times_.end())
       return;
 
-    packet_arrival_times_[seq] = arrival_time_ms;
+    AddPacket(seq, arrival_time_ms);
 
     // Limit the range of sequence numbers to send feedback for.
     auto first_arrival_time_to_keep = packet_arrival_times_.lower_bound(
@@ -204,10 +215,10 @@ void RemoteEstimatorProxy::SendPeriodicFeedbacks() {
        begin_iterator != packet_arrival_times_.cend();
        begin_iterator =
            packet_arrival_times_.lower_bound(*periodic_window_start_seq_)) {
-    auto feedback_packet = std::make_unique<rtcp::TransportFeedback>();
+    auto feedback_packet = BuildFeedbackPacket(
-    periodic_window_start_seq_ = BuildFeedbackPacket(
+        /*include_timestamps=*/true, *periodic_window_start_seq_,
-        feedback_packet_count_++, media_ssrc_, *periodic_window_start_seq_,
+        begin_iterator, packet_arrival_times_.cend(),
-        begin_iterator, packet_arrival_times_.cend(), feedback_packet.get());
+        /*is_periodic_update=*/true);
 
     RTC_DCHECK(feedback_sender_ != nullptr);
 
@@ -231,18 +242,15 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest(
     return;
   }
 
-  auto feedback_packet = std::make_unique<rtcp::TransportFeedback>(
-      feedback_request.include_timestamps);
-
   int64_t first_sequence_number =
       sequence_number - feedback_request.sequence_count + 1;
   auto begin_iterator =
       packet_arrival_times_.lower_bound(first_sequence_number);
   auto end_iterator = packet_arrival_times_.upper_bound(sequence_number);
 
-  BuildFeedbackPacket(feedback_packet_count_++, media_ssrc_,
+  auto feedback_packet = BuildFeedbackPacket(
-                      first_sequence_number, begin_iterator, end_iterator,
+      feedback_request.include_timestamps, first_sequence_number,
-                      feedback_packet.get());
+      begin_iterator, end_iterator, /*is_periodic_update=*/false);
 
   // Clear up to the first packet that is included in this feedback packet.
   packet_arrival_times_.erase(packet_arrival_times_.begin(), begin_iterator);
@@ -253,24 +261,27 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest(
   feedback_sender_(std::move(packets));
 }
 
-int64_t RemoteEstimatorProxy::BuildFeedbackPacket(
+std::unique_ptr<rtcp::TransportFeedback>
-    uint8_t feedback_packet_count,
+RemoteEstimatorProxy::BuildFeedbackPacket(
-    uint32_t media_ssrc,
+    bool include_timestamps,
     int64_t base_sequence_number,
     std::map<int64_t, int64_t>::const_iterator begin_iterator,
     std::map<int64_t, int64_t>::const_iterator end_iterator,
-    rtcp::TransportFeedback* feedback_packet) {
+    bool is_periodic_update) {
   RTC_DCHECK(begin_iterator != end_iterator);
 
+  auto feedback_packet =
+      std::make_unique<rtcp::TransportFeedback>(include_timestamps);
+
   // TODO(sprang): Measure receive times in microseconds and remove the
   // conversions below.
-  feedback_packet->SetMediaSsrc(media_ssrc);
+  feedback_packet->SetMediaSsrc(media_ssrc_);
   // Base sequence number is the expected first sequence number. This is known,
   // but we might not have actually received it, so the base time shall be the
   // time of the first received packet in the feedback.
   feedback_packet->SetBase(static_cast<uint16_t>(base_sequence_number & 0xFFFF),
                            begin_iterator->second * 1000);
-  feedback_packet->SetFeedbackSequenceNumber(feedback_packet_count);
+  feedback_packet->SetFeedbackSequenceNumber(feedback_packet_count_++);
   int64_t next_sequence_number = base_sequence_number;
   for (auto it = begin_iterator; it != end_iterator; ++it) {
     if (!feedback_packet->AddReceivedPacket(
@@ -285,7 +296,10 @@ int64_t RemoteEstimatorProxy::BuildFeedbackPacket(
     }
     next_sequence_number = it->first + 1;
   }
-  return next_sequence_number;
+  if (is_periodic_update) {
+    periodic_window_start_seq_ = next_sequence_number;
+  }
+  return feedback_packet;
 }
 
 }  // namespace webrtc

modules/remote_bitrate_estimator/remote_estimator_proxy.h

@@ -77,19 +77,35 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
 
   static const int kMaxNumberOfPackets;
 
+  // Records the fact that a packet with `sequence_number` arrived at
+  // `arrival_time_ms`.
+  void AddPacket(int64_t sequence_number, int64_t arrival_time_ms)
+      RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
+  void MaybeCullOldPackets(int64_t sequence_number, int64_t arrival_time_ms)
+      RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
   void SendPeriodicFeedbacks() RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
   void SendFeedbackOnRequest(int64_t sequence_number,
                              const FeedbackRequest& feedback_request)
       RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
-  static int64_t BuildFeedbackPacket(
+
-      uint8_t feedback_packet_count,
+  // Returns a Transport Feedback packet with information about as many packets
-      uint32_t media_ssrc,
+  // that has been received between [`begin_iterator`, `end_iterator`) that can
+  // fit in it. If `is_periodic_update`, this represents sending a periodic
+  // feedback message, which will make it update the
+  // `periodic_window_start_seq_` variable with the first packet that was not
+  // included in the feedback packet, so that the next update can continue from
+  // that sequence number.
+  //
+  // `include_timestamps` decide if the returned TransportFeedback should
+  // include timestamps.
+  std::unique_ptr<rtcp::TransportFeedback> BuildFeedbackPacket(
+      bool include_timestamps,
       int64_t base_sequence_number,
       std::map<int64_t, int64_t>::const_iterator
           begin_iterator,  // |begin_iterator| is inclusive.
       std::map<int64_t, int64_t>::const_iterator
           end_iterator,  // |end_iterator| is exclusive.
-      rtcp::TransportFeedback* feedback_packet);
+      bool is_periodic_update) RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
 
   Clock* const clock_;
   const TransportFeedbackSender feedback_sender_;
@@ -103,6 +119,9 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
   uint32_t media_ssrc_ RTC_GUARDED_BY(&lock_);
   uint8_t feedback_packet_count_ RTC_GUARDED_BY(&lock_);
   SeqNumUnwrapper<uint16_t> unwrapper_ RTC_GUARDED_BY(&lock_);
+
+  // The next sequence number that should be the start sequence number during
+  // periodic reporting. Will be absl::nullopt before the first seen packet.
   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_);