Remember lost packets between RTCP feedback reports

The idea is to reduce the risk of calculating a packet as lost if a
packet is reordered between two feedback reports.
It works as long as the recevied feedback does not complete an
observation.

Bug: webrtc:42222865 b/349765923
Change-Id: Iaf1595e624f546951baf3998d161f4cd1d5d491b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/355942
Reviewed-by: Diep Bui <diepbp@google.com>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42562}

modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc

@@ -52,37 +52,6 @@ bool IsValid(Timestamp timestamp) {
 
 double ToKiloBytes(DataSize datasize) { return datasize.bytes() / 1000.0; }
 
-struct PacketResultsSummary {
-  int num_packets = 0;
-  int num_lost_packets = 0;
-  DataSize total_size = DataSize::Zero();
-  DataSize lost_size = DataSize::Zero();
-  Timestamp first_send_time = Timestamp::PlusInfinity();
-  Timestamp last_send_time = Timestamp::MinusInfinity();
-};
-
-// Returns a `PacketResultsSummary` where `first_send_time` is `PlusInfinity,
-// and `last_send_time` is `MinusInfinity`, if `packet_results` is empty.
-PacketResultsSummary GetPacketResultsSummary(
-    rtc::ArrayView<const PacketResult> packet_results) {
-  PacketResultsSummary packet_results_summary;
-
-  packet_results_summary.num_packets = packet_results.size();
-  for (const PacketResult& packet : packet_results) {
-    if (!packet.IsReceived()) {
-      packet_results_summary.num_lost_packets++;
-      packet_results_summary.lost_size += packet.sent_packet.size;
-    }
-    packet_results_summary.total_size += packet.sent_packet.size;
-    packet_results_summary.first_send_time = std::min(
-        packet_results_summary.first_send_time, packet.sent_packet.send_time);
-    packet_results_summary.last_send_time = std::max(
-        packet_results_summary.last_send_time, packet.sent_packet.send_time);
-  }
-
-  return packet_results_summary;
-}
-
 double GetLossProbability(double inherent_loss,
                           DataRate loss_limited_bandwidth,
                           DataRate sending_rate) {
@@ -173,7 +142,6 @@ LossBasedBweV2::Result LossBasedBweV2::GetLossBasedResult() const {
                                       : DataRate::PlusInfinity(),
             .state = LossBasedState::kDelayBasedEstimate};
   }
-
   return loss_based_result_;
 }
 
@@ -1140,22 +1108,27 @@ bool LossBasedBweV2::PushBackObservation(
     return false;
   }
 
-  PacketResultsSummary packet_results_summary =
+  partial_observation_.num_packets += packet_results.size();
-      GetPacketResultsSummary(packet_results);
+  Timestamp last_send_time = Timestamp::MinusInfinity();
-
+  Timestamp first_send_time = Timestamp::PlusInfinity();
-  partial_observation_.num_packets += packet_results_summary.num_packets;
+  for (const PacketResult& packet : packet_results) {
-  partial_observation_.num_lost_packets +=
+    if (packet.IsReceived()) {
-      packet_results_summary.num_lost_packets;
+      partial_observation_.lost_packets.erase(
-  partial_observation_.size += packet_results_summary.total_size;
+          packet.sent_packet.sequence_number);
-  partial_observation_.lost_size += packet_results_summary.lost_size;
+    } else {
+      partial_observation_.lost_packets.emplace(
+          packet.sent_packet.sequence_number, packet.sent_packet.size);
+    }
+    partial_observation_.size += packet.sent_packet.size;
+    last_send_time = std::max(last_send_time, packet.sent_packet.send_time);
+    first_send_time = std::min(first_send_time, packet.sent_packet.send_time);
+  }
 
   // This is the first packet report we have received.
   if (!IsValid(last_send_time_most_recent_observation_)) {
-    last_send_time_most_recent_observation_ =
+    last_send_time_most_recent_observation_ = first_send_time;
-        packet_results_summary.first_send_time;
   }
 
-  const Timestamp last_send_time = packet_results_summary.last_send_time;
   const TimeDelta observation_duration =
       last_send_time - last_send_time_most_recent_observation_;
   // Too small to be meaningful.
@@ -1168,12 +1141,14 @@ bool LossBasedBweV2::PushBackObservation(
 
   Observation observation;
   observation.num_packets = partial_observation_.num_packets;
-  observation.num_lost_packets = partial_observation_.num_lost_packets;
+  observation.num_lost_packets = partial_observation_.lost_packets.size();
   observation.num_received_packets =
       observation.num_packets - observation.num_lost_packets;
   observation.sending_rate =
       GetSendingRate(partial_observation_.size / observation_duration);
-  observation.lost_size = partial_observation_.lost_size;
+  for (auto const& [key, packet_size] : partial_observation_.lost_packets) {
+    observation.lost_size += packet_size;
+  }
   observation.size = partial_observation_.size;
   observation.id = num_observations_++;
   observations_[observation.id % config_->observation_window_size] =

modules/congestion_controller/goog_cc/loss_based_bwe_v2.h

@@ -11,6 +11,8 @@
 #ifndef MODULES_CONGESTION_CONTROLLER_GOOG_CC_LOSS_BASED_BWE_V2_H_
 #define MODULES_CONGESTION_CONTROLLER_GOOG_CC_LOSS_BASED_BWE_V2_H_
 
+#include <cstdint>
+#include <unordered_map>
 #include <vector>
 
 #include "absl/types/optional.h"
@@ -147,9 +149,8 @@ class LossBasedBweV2 {
 
   struct PartialObservation {
     int num_packets = 0;
-    int num_lost_packets = 0;
+    std::unordered_map<int64_t, DataSize> lost_packets;
     DataSize size = DataSize::Zero();
-    DataSize lost_size = DataSize::Zero();
   };
 
   struct PaddingInfo {

modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc

@@ -10,6 +10,7 @@
 
 #include "modules/congestion_controller/goog_cc/loss_based_bwe_v2.h"
 
+#include <cstdint>
 #include <string>
 #include <vector>
 
@@ -91,6 +92,10 @@ class LossBasedBweV2Test : public ::testing::TestWithParam<bool> {
   std::vector<PacketResult> CreatePacketResultsWithReceivedPackets(
       Timestamp first_packet_timestamp) {
     std::vector<PacketResult> enough_feedback(2);
+    enough_feedback[0].sent_packet.sequence_number =
+        transport_sequence_number_++;
+    enough_feedback[1].sent_packet.sequence_number =
+        transport_sequence_number_++;
     enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize);
     enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize);
     enough_feedback[0].sent_packet.send_time = first_packet_timestamp;
@@ -107,8 +112,9 @@ class LossBasedBweV2Test : public ::testing::TestWithParam<bool> {
       Timestamp first_packet_timestamp,
       DataSize lost_packet_size = DataSize::Bytes(kPacketSize)) {
     std::vector<PacketResult> enough_feedback(10);
-    enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize);
     for (unsigned i = 0; i < enough_feedback.size(); ++i) {
+      enough_feedback[i].sent_packet.sequence_number =
+          transport_sequence_number_++;
       enough_feedback[i].sent_packet.size = DataSize::Bytes(kPacketSize);
       enough_feedback[i].sent_packet.send_time =
           first_packet_timestamp +
@@ -125,6 +131,10 @@ class LossBasedBweV2Test : public ::testing::TestWithParam<bool> {
   std::vector<PacketResult> CreatePacketResultsWith50pPacketLossRate(
       Timestamp first_packet_timestamp) {
     std::vector<PacketResult> enough_feedback(2);
+    enough_feedback[0].sent_packet.sequence_number =
+        transport_sequence_number_++;
+    enough_feedback[1].sent_packet.sequence_number =
+        transport_sequence_number_++;
     enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize);
     enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize);
     enough_feedback[0].sent_packet.send_time = first_packet_timestamp;
@@ -139,6 +149,10 @@ class LossBasedBweV2Test : public ::testing::TestWithParam<bool> {
   std::vector<PacketResult> CreatePacketResultsWith100pLossRate(
       Timestamp first_packet_timestamp) {
     std::vector<PacketResult> enough_feedback(2);
+    enough_feedback[0].sent_packet.sequence_number =
+        transport_sequence_number_++;
+    enough_feedback[1].sent_packet.sequence_number =
+        transport_sequence_number_++;
     enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize);
     enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize);
     enough_feedback[0].sent_packet.send_time = first_packet_timestamp;
@@ -148,6 +162,9 @@ class LossBasedBweV2Test : public ::testing::TestWithParam<bool> {
     enough_feedback[1].receive_time = Timestamp::PlusInfinity();
     return enough_feedback;
   }
+
+ private:
+  int64_t transport_sequence_number_ = 0;
 };
 
 TEST_F(LossBasedBweV2Test, EnabledWhenGivenValidConfigurationValues) {
@@ -1812,5 +1829,61 @@ TEST_F(LossBasedBweV2Test, PaceAtLossBasedEstimate) {
   EXPECT_TRUE(loss_based_bandwidth_estimator.PaceAtLossBasedEstimate());
 }
 
+TEST_F(LossBasedBweV2Test,
+       EstimateDoesNotBackOffDueToPacketReorderingBetweenFeedback) {
+  ExplicitKeyValueConfig key_value_config(ShortObservationConfig(""));
+  LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config);
+  const DataRate kStartBitrate = DataRate::KilobitsPerSec(2500);
+  loss_based_bandwidth_estimator.SetBandwidthEstimate(kStartBitrate);
+
+  std::vector<PacketResult> feedback_1(3);
+  feedback_1[0].sent_packet.sequence_number = 1;
+  feedback_1[0].sent_packet.size = DataSize::Bytes(kPacketSize);
+  feedback_1[0].sent_packet.send_time = Timestamp::Zero();
+  feedback_1[0].receive_time =
+      feedback_1[0].sent_packet.send_time + TimeDelta::Millis(10);
+  feedback_1[1].sent_packet.sequence_number = 2;
+  feedback_1[1].sent_packet.size = DataSize::Bytes(kPacketSize);
+  feedback_1[1].sent_packet.send_time = Timestamp::Zero();
+  // Lost or reordered
+  feedback_1[1].receive_time = Timestamp::PlusInfinity();
+
+  feedback_1[2].sent_packet.sequence_number = 3;
+  feedback_1[2].sent_packet.size = DataSize::Bytes(kPacketSize);
+  feedback_1[2].sent_packet.send_time = Timestamp::Zero();
+  feedback_1[2].receive_time =
+      feedback_1[2].sent_packet.send_time + TimeDelta::Millis(10);
+
+  std::vector<PacketResult> feedback_2(3);
+  feedback_2[0].sent_packet.sequence_number = 2;
+  feedback_2[0].sent_packet.size = DataSize::Bytes(kPacketSize);
+  feedback_2[0].sent_packet.send_time = Timestamp::Zero();
+  feedback_2[0].receive_time =
+      feedback_1[0].sent_packet.send_time + TimeDelta::Millis(10);
+  feedback_2[1].sent_packet.sequence_number = 4;
+  feedback_2[1].sent_packet.size = DataSize::Bytes(kPacketSize);
+  feedback_2[1].sent_packet.send_time =
+      Timestamp::Zero() + kObservationDurationLowerBound;
+  feedback_2[1].receive_time =
+      feedback_2[1].sent_packet.send_time + TimeDelta::Millis(10);
+  feedback_2[2].sent_packet.sequence_number = 5;
+  feedback_2[2].sent_packet.size = DataSize::Bytes(kPacketSize);
+  feedback_2[2].sent_packet.send_time = Timestamp::Zero();
+  feedback_2[2].receive_time =
+      feedback_2[2].sent_packet.send_time + TimeDelta::Millis(10);
+
+  loss_based_bandwidth_estimator.UpdateBandwidthEstimate(
+      feedback_1,
+      /*delay_based_estimate=*/kStartBitrate,
+      /*in_alr=*/false);
+  loss_based_bandwidth_estimator.UpdateBandwidthEstimate(
+      feedback_2,
+      /*delay_based_estimate=*/kStartBitrate,
+      /*in_alr=*/false);
+  EXPECT_EQ(
+      loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate,
+      kStartBitrate);
+}
+
 }  // namespace
 }  // namespace webrtc

modules/congestion_controller/rtp/transport_feedback_adapter.cc

@@ -253,9 +253,10 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner(
       });
 
   if (failed_lookups > 0) {
-    RTC_LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups
+    RTC_LOG(LS_WARNING)
-                        << " packet" << (failed_lookups > 1 ? "s" : "")
+        << "Failed to lookup send time for " << failed_lookups << " packet"
-                        << ". Send time history too small?";
+        << (failed_lookups > 1 ? "s" : "")
+        << ". Packets reordered or send time history too small?";
   }
   if (ignored > 0) {
     RTC_LOG(LS_INFO) << "Ignoring " << ignored