GetTransportFeedbackVector return vector with lost packets too, sorted by seq-num

1. GetTransportFeedbackVector will now return a vector which also explicitly states lost packets.
2. The returned vector is unsorted (uses default order - by sequence number). It's up to the users to sort otherwise, if they need a different order.

BUG=None

Review-Url: https://codereview.webrtc.org/2707383006
Cr-Commit-Position: refs/heads/master@{#17114}

webrtc/modules/congestion_controller/delay_based_bwe.cc

@@ -122,6 +122,28 @@ bool ReadMedianSlopeFilterExperimentParameters(size_t* window_size,
   *threshold_gain = kDefaultMedianSlopeThresholdGain;
   return false;
 }
+
+class PacketFeedbackComparator {
+ public:
+  inline bool operator()(const webrtc::PacketFeedback& lhs,
+                         const webrtc::PacketFeedback& rhs) {
+    if (lhs.arrival_time_ms != rhs.arrival_time_ms)
+      return lhs.arrival_time_ms < rhs.arrival_time_ms;
+    if (lhs.send_time_ms != rhs.send_time_ms)
+      return lhs.send_time_ms < rhs.send_time_ms;
+    return lhs.sequence_number < rhs.sequence_number;
+  }
+};
+
+void SortPacketFeedbackVector(const std::vector<webrtc::PacketFeedback>& input,
+                              std::vector<webrtc::PacketFeedback>* output) {
+  auto pred = [](const webrtc::PacketFeedback& packet_feedback) {
+    return packet_feedback.arrival_time_ms !=
+           webrtc::PacketFeedback::kNotReceived;
+  };
+  std::copy_if(input.begin(), input.end(), std::back_inserter(*output), pred);
+  std::sort(output->begin(), output->end(), PacketFeedbackComparator());
+}
 }  // namespace
 
 namespace webrtc {
@@ -255,6 +277,11 @@ DelayBasedBwe::DelayBasedBwe(RtcEventLog* event_log, Clock* clock)
 DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
     const std::vector<PacketFeedback>& packet_feedback_vector) {
   RTC_DCHECK(network_thread_.CalledOnValidThread());
+
+  std::vector<PacketFeedback> sorted_packet_feedback_vector;
+  SortPacketFeedbackVector(packet_feedback_vector,
+                           &sorted_packet_feedback_vector);
+
   if (!uma_recorded_) {
     RTC_HISTOGRAM_ENUMERATION(kBweTypeHistogram,
                               BweNames::kSendSideTransportSeqNum,
@@ -263,7 +290,7 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
   }
   Result aggregated_result;
   bool delayed_feedback = true;
-  for (const auto& packet_feedback : packet_feedback_vector) {
+  for (const auto& packet_feedback : sorted_packet_feedback_vector) {
     if (packet_feedback.send_time_ms < 0)
       continue;
     delayed_feedback = false;
@@ -277,8 +304,8 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector(
     consecutive_delayed_feedbacks_ = 0;
   }
   if (consecutive_delayed_feedbacks_ >= kMaxConsecutiveFailedLookups) {
-    aggregated_result =
-        OnLongFeedbackDelay(packet_feedback_vector.back().arrival_time_ms);
+    aggregated_result = OnLongFeedbackDelay(
+        sorted_packet_feedback_vector.back().arrival_time_ms);
     consecutive_delayed_feedbacks_ = 0;
   }
   return aggregated_result;

webrtc/modules/congestion_controller/transport_feedback_adapter.cc

@@ -15,6 +15,7 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/base/mod_ops.h"
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
 #include "webrtc/modules/congestion_controller/delay_based_bwe.h"
@@ -30,17 +31,6 @@ const int64_t kBaseTimestampScaleFactor =
     rtcp::TransportFeedback::kDeltaScaleFactor * (1 << 8);
 const int64_t kBaseTimestampRangeSizeUs = kBaseTimestampScaleFactor * (1 << 24);
 
-class PacketFeedbackComparator {
- public:
-  inline bool operator()(const PacketFeedback& lhs, const PacketFeedback& rhs) {
-    if (lhs.arrival_time_ms != rhs.arrival_time_ms)
-      return lhs.arrival_time_ms < rhs.arrival_time_ms;
-    if (lhs.send_time_ms != rhs.send_time_ms)
-      return lhs.send_time_ms < rhs.send_time_ms;
-    return lhs.sequence_number < rhs.sequence_number;
-  }
-};
-
 TransportFeedbackAdapter::TransportFeedbackAdapter(
     RtcEventLog* event_log,
     Clock* clock,
@@ -128,26 +118,48 @@ std::vector<PacketFeedback> TransportFeedbackAdapter::GetPacketFeedbackVector(
 
   auto received_packets = feedback.GetReceivedPackets();
   std::vector<PacketFeedback> packet_feedback_vector;
-  packet_feedback_vector.reserve(received_packets.size());
   if (received_packets.empty()) {
     LOG(LS_INFO) << "Empty transport feedback packet received.";
     return packet_feedback_vector;
   }
+  const uint16_t last_sequence_number =
+      received_packets.back().sequence_number();
+  const size_t packet_count =
+      1 + ForwardDiff(feedback.GetBaseSequence(), last_sequence_number);
+  packet_feedback_vector.reserve(packet_count);
+  // feedback.GetStatusVector().size() is a less efficient way to reach what
+  // should be the same value.
+  RTC_DCHECK_EQ(packet_count, feedback.GetStatusVector().size());
+
   {
     rtc::CritScope cs(&lock_);
     size_t failed_lookups = 0;
     int64_t offset_us = 0;
     int64_t timestamp_ms = 0;
-    for (const auto& packet : feedback.GetReceivedPackets()) {
+    uint16_t seq_num = feedback.GetBaseSequence();
+    for (const auto& packet : received_packets) {
+      // Insert into the vector those unreceived packets which precede this
+      // iteration's received packet.
+      for (; seq_num != packet.sequence_number(); ++seq_num) {
+        PacketFeedback packet_feedback(PacketFeedback::kNotReceived, seq_num);
+        // Note: Element not removed from history because it might be reported
+        // as received by another feedback.
+        if (!send_time_history_.GetFeedback(&packet_feedback, false))
+          ++failed_lookups;
+        packet_feedback_vector.push_back(packet_feedback);
+      }
+
+      // Handle this iteration's received packet.
       offset_us += packet.delta_us();
       timestamp_ms = current_offset_ms_ + (offset_us / 1000);
       PacketFeedback packet_feedback(timestamp_ms, packet.sequence_number());
       if (!send_time_history_.GetFeedback(&packet_feedback, true))
         ++failed_lookups;
       packet_feedback_vector.push_back(packet_feedback);
+
+      ++seq_num;
     }
-    std::sort(packet_feedback_vector.begin(), packet_feedback_vector.end(),
-              PacketFeedbackComparator());
+
     if (failed_lookups > 0) {
       LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups
                       << " packet" << (failed_lookups > 1 ? "s" : "")

webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc

@@ -90,8 +90,11 @@ class TransportFeedbackAdapterTest : public ::testing::Test {
     int64_t arrival_time_delta =
         truth[0].arrival_time_ms - input[0].arrival_time_ms;
     for (size_t i = 0; i < len; ++i) {
-      EXPECT_EQ(truth[i].arrival_time_ms,
-                input[i].arrival_time_ms + arrival_time_delta);
+      RTC_CHECK(truth[i].arrival_time_ms != PacketFeedback::kNotReceived);
+      if (input[i].arrival_time_ms != PacketFeedback::kNotReceived) {
+        EXPECT_EQ(truth[i].arrival_time_ms,
+                  input[i].arrival_time_ms + arrival_time_delta);
+      }
       EXPECT_EQ(truth[i].send_time_ms, input[i].send_time_ms);
       EXPECT_EQ(truth[i].sequence_number, input[i].sequence_number);
       EXPECT_EQ(truth[i].payload_size, input[i].payload_size);
@@ -140,6 +143,41 @@ TEST_F(TransportFeedbackAdapterTest, AdaptsFeedbackAndPopulatesSendTimes) {
   ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector());
 }
 
+TEST_F(TransportFeedbackAdapterTest, FeedbackVectorReportsUnreceived) {
+  std::vector<PacketFeedback> sent_packets = {
+      PacketFeedback(100, 220, 0, 1500, kPacingInfo0),
+      PacketFeedback(110, 210, 1, 1500, kPacingInfo0),
+      PacketFeedback(120, 220, 2, 1500, kPacingInfo0),
+      PacketFeedback(130, 230, 3, 1500, kPacingInfo0),
+      PacketFeedback(140, 240, 4, 1500, kPacingInfo0),
+      PacketFeedback(150, 250, 5, 1500, kPacingInfo0),
+      PacketFeedback(160, 260, 6, 1500, kPacingInfo0)
+  };
+
+  for (const PacketFeedback& packet : sent_packets)
+    OnSentPacket(packet);
+
+  // Note: Important to include the last packet, as only unreceived packets in
+  // between received packets can be inferred.
+  std::vector<PacketFeedback> received_packets = {
+    sent_packets[0], sent_packets[2], sent_packets[6]
+  };
+
+  rtcp::TransportFeedback feedback;
+  feedback.SetBase(received_packets[0].sequence_number,
+                   received_packets[0].arrival_time_ms * 1000);
+
+  for (const PacketFeedback& packet : received_packets) {
+    EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number,
+                                           packet.arrival_time_ms * 1000));
+  }
+
+  feedback.Build();
+
+  adapter_->OnTransportFeedback(feedback);
+  ComparePacketVectors(sent_packets, adapter_->GetTransportFeedbackVector());
+}
+
 TEST_F(TransportFeedbackAdapterTest, LongFeedbackDelays) {
   const int64_t kFeedbackTimeoutMs = 60001;
   const int kMaxConsecutiveFailedLookups = 5;
@@ -305,15 +343,11 @@ TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) {
   }
 }
 
-TEST_F(TransportFeedbackAdapterTest, HandlesReordering) {
+TEST_F(TransportFeedbackAdapterTest, HandlesArrivalReordering) {
   std::vector<PacketFeedback> packets;
   packets.push_back(PacketFeedback(120, 200, 0, 1500, kPacingInfo0));
   packets.push_back(PacketFeedback(110, 210, 1, 1500, kPacingInfo0));
   packets.push_back(PacketFeedback(100, 220, 2, 1500, kPacingInfo0));
-  std::vector<PacketFeedback> expected_packets;
-  expected_packets.push_back(packets[2]);
-  expected_packets.push_back(packets[1]);
-  expected_packets.push_back(packets[0]);
 
   for (const PacketFeedback& packet : packets)
     OnSentPacket(packet);
@@ -329,9 +363,11 @@ TEST_F(TransportFeedbackAdapterTest, HandlesReordering) {
 
   feedback.Build();
 
+  // Adapter keeps the packets ordered by sequence number (which is itself
+  // assigned by the order of transmission). Reordering by some other criteria,
+  // eg. arrival time, is up to the observers.
   adapter_->OnTransportFeedback(feedback);
-  ComparePacketVectors(expected_packets,
-                       adapter_->GetTransportFeedbackVector());
+  ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector());
 }
 
 TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) {
@@ -394,17 +430,7 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) {
 
   EXPECT_TRUE(feedback.get() != nullptr);
   adapter_->OnTransportFeedback(*feedback.get());
-  {
-    // Expected to be ordered on arrival time when the feedback message has been
-    // parsed.
-    std::vector<PacketFeedback> expected_packets;
-    expected_packets.push_back(sent_packets[0]);
-    expected_packets.push_back(sent_packets[3]);
-    expected_packets.push_back(sent_packets[1]);
-    expected_packets.push_back(sent_packets[2]);
-    ComparePacketVectors(expected_packets,
-                         adapter_->GetTransportFeedbackVector());
-  }
+  ComparePacketVectors(sent_packets, adapter_->GetTransportFeedbackVector());
 
   // Create a new feedback message and add the trailing item.
   feedback.reset(new rtcp::TransportFeedback());

webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h

@@ -279,6 +279,7 @@ struct PacketFeedback {
         pacing_info(pacing_info) {}
 
   static constexpr int kNotAProbe = -1;
+  static constexpr int64_t kNotReceived = -1;
 
   // NOTE! The variable |creation_time_ms| is not used when testing equality.
   //       This is due to |creation_time_ms| only being used by SendTimeHistory
@@ -295,7 +296,8 @@ struct PacketFeedback {
   // Time corresponding to when this object was created.
   int64_t creation_time_ms;
   // Time corresponding to when the packet was received. Timestamped with the
-  // receiver's clock.
+  // receiver's clock. For unreceived packet, the sentinel value kNotReceived
+  // is used.
   int64_t arrival_time_ms;
   // Time corresponding to when the packet was sent, timestamped with the
   // sender's clock.

webrtc/tools/event_log_visualizer/analyzer.cc

@@ -43,6 +43,26 @@ namespace plotting {
 
 namespace {
 
+class PacketFeedbackComparator {
+ public:
+  inline bool operator()(const webrtc::PacketFeedback& lhs,
+                         const webrtc::PacketFeedback& rhs) {
+    if (lhs.arrival_time_ms != rhs.arrival_time_ms)
+      return lhs.arrival_time_ms < rhs.arrival_time_ms;
+    if (lhs.send_time_ms != rhs.send_time_ms)
+      return lhs.send_time_ms < rhs.send_time_ms;
+    return lhs.sequence_number < rhs.sequence_number;
+  }
+};
+
+void SortPacketFeedbackVector(std::vector<PacketFeedback>* vec) {
+  auto pred = [](const PacketFeedback& packet_feedback) {
+    return packet_feedback.arrival_time_ms == PacketFeedback::kNotReceived;
+  };
+  vec->erase(std::remove_if(vec->begin(), vec->end(), pred), vec->end());
+  std::sort(vec->begin(), vec->end(), PacketFeedbackComparator());
+}
+
 std::string SsrcToString(uint32_t ssrc) {
   std::stringstream ss;
   ss << "SSRC " << ssrc;
@@ -1057,6 +1077,7 @@ void EventLogAnalyzer::CreateBweSimulationGraph(Plot* plot) {
             rtcp.packet.get()));
         std::vector<PacketFeedback> feedback =
             observer->GetTransportFeedbackVector();
+        SortPacketFeedbackVector(&feedback);
         rtc::Optional<uint32_t> bitrate_bps;
         if (!feedback.empty()) {
           for (const PacketFeedback& packet : feedback)
@@ -1192,6 +1213,7 @@ void EventLogAnalyzer::CreateNetworkDelayFeedbackGraph(Plot* plot) {
             *static_cast<rtcp::TransportFeedback*>(rtcp.packet.get()));
         std::vector<PacketFeedback> feedback =
             feedback_adapter.GetTransportFeedbackVector();
+        SortPacketFeedbackVector(&feedback);
         for (const PacketFeedback& packet : feedback) {
           int64_t y = packet.arrival_time_ms - packet.send_time_ms;
           float x =