Refactor CongestionControllerFeedback logic

CongestrionControllerGenerator tracks received packets per SSRC.
Lost packets are included in rtcp:CongestionControlFeedback::Packets()

This is done in order to be able to track lost packets between
feedback packets.

Bug: webrtc:42225697
Change-Id: Ib47d9b55c3d150cb98a44a4f3997cfcfe6c5fbb5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/366002
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43274}

modules/remote_bitrate_estimator/BUILD.gn

@@ -95,6 +95,8 @@ rtc_library("congestion_control_feedback_generator") {
   sources = [
     "congestion_control_feedback_generator.cc",
     "congestion_control_feedback_generator.h",
+    "congestion_control_feedback_tracker.cc",
+    "congestion_control_feedback_tracker.h",
   ]
   deps = [
     ":rtp_transport_feedback_generator",
@@ -105,12 +107,15 @@ rtc_library("congestion_control_feedback_generator") {
     "../../api/units:data_size",
     "../../api/units:time_delta",
     "../../api/units:timestamp",
+    "../../rtc_base:checks",
     "../../rtc_base:logging",
     "../../rtc_base:rtc_numerics",
     "../../rtc_base/experiments:field_trial_parser",
     "../../rtc_base/network:ecn_marking",
     "../rtp_rtcp:ntp_time_util",
     "../rtp_rtcp:rtp_rtcp_format",
+    "//third_party/abseil-cpp/absl/algorithm:container",
+    "//third_party/abseil-cpp/absl/types:span",
   ]
 }
 
@@ -149,6 +154,7 @@ if (rtc_include_tests) {
     sources = [
       "aimd_rate_control_unittest.cc",
       "congestion_control_feedback_generator_unittest.cc",
+      "congestion_control_feedback_tracker_unittest.cc",
       "inter_arrival_unittest.cc",
       "overuse_detector_unittest.cc",
       "packet_arrival_map_test.cc",
@@ -170,6 +176,7 @@ if (rtc_include_tests) {
       "../../api/units:data_size",
       "../../api/units:time_delta",
       "../../api/units:timestamp",
+      "../../rtc_base:buffer",
       "../../rtc_base:checks",
       "../../rtc_base:logging",
       "../../rtc_base:random",

modules/remote_bitrate_estimator/congestion_control_feedback_generator.cc

@@ -12,18 +12,22 @@
 #include <algorithm>
 #include <cstdint>
 #include <memory>
+#include <optional>
 #include <utility>
 #include <vector>
 
 #include "api/environment/environment.h"
 #include "api/field_trials_view.h"
+#include "api/sequence_checker.h"
+#include "api/units/data_rate.h"
 #include "api/units/data_size.h"
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
 #include "modules/rtp_rtcp/source/ntp_time_util.h"
+#include "modules/rtp_rtcp/source/rtcp_packet.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
-#include "rtc_base/logging.h"
+#include "modules/rtp_rtcp/source/rtp_packet_received.h"
-#include "rtc_base/network/ecn_marking.h"
+#include "rtc_base/experiments/field_trial_parser.h"
 
 namespace webrtc {
 
@@ -47,26 +51,24 @@ void CongestionControlFeedbackGenerator::OnReceivedPacket(
   RTC_DCHECK_RUN_ON(&sequence_checker_);
 
   marker_bit_seen_ |= packet.Marker();
-  packets_.push_back({.ssrc = packet.Ssrc(),
+  if (!first_arrival_time_since_feedback_) {
-                      .unwrapped_sequence_number =
+    first_arrival_time_since_feedback_ = packet.arrival_time();
-                          sequence_number_unwrappers_[packet.Ssrc()].Unwrap(
+  }
-                              packet.SequenceNumber()),
+  feedback_trackers_[packet.Ssrc()].ReceivedPacket(packet);
-                      .arrival_time = packet.arrival_time(),
-                      .ecn = packet.ecn()});
   if (NextFeedbackTime() < packet.arrival_time()) {
     SendFeedback(env_.clock().CurrentTime());
   }
 }
 
 Timestamp CongestionControlFeedbackGenerator::NextFeedbackTime() const {
-  if (packets_.empty()) {
+  if (!first_arrival_time_since_feedback_) {
     return std::max(env_.clock().CurrentTime() + min_time_between_feedback_,
                     next_possible_feedback_send_time_);
   }
 
   if (!marker_bit_seen_) {
     return std::max(next_possible_feedback_send_time_,
-                    packets_.front().arrival_time +
+                    *first_arrival_time_since_feedback_ +
                         max_time_to_wait_for_packet_with_marker_.Get());
   }
   return next_possible_feedback_send_time_;
@@ -94,46 +96,14 @@ void CongestionControlFeedbackGenerator::SetTransportOverhead(
 }
 
 void CongestionControlFeedbackGenerator::SendFeedback(Timestamp now) {
-  absl::c_sort(packets_, [](const PacketInfo& a, const PacketInfo& b) {
-    return std::tie(a.ssrc, a.unwrapped_sequence_number, a.arrival_time) <
-           std::tie(b.ssrc, b.unwrapped_sequence_number, b.arrival_time);
-  });
   uint32_t compact_ntp =
       CompactNtp(env_.clock().ConvertTimestampToNtpTime(now));
   std::vector<rtcp::CongestionControlFeedback::PacketInfo> rtcp_packet_info;
-  rtcp_packet_info.reserve(packets_.size());
+  for (auto& [unused, tracker] : feedback_trackers_) {
-
+    tracker.AddPacketsToFeedback(now, rtcp_packet_info);
-  std::optional<uint32_t> previous_ssrc;
-  std::optional<int64_t> previous_seq_no;
-  for (const PacketInfo packet : packets_) {
-    if (previous_ssrc == packet.ssrc &&
-        previous_seq_no == packet.unwrapped_sequence_number) {
-      // According to RFC 8888:
-      // If duplicate copies of a particular RTP packet are received, then the
-      // arrival time of the first copy to arrive MUST be reported. If any of
-      // the copies of the duplicated packet are ECN-CE marked, then an ECN-CE
-      // mark MUST be reported for that packet; otherwise, the ECN mark of the
-      // first copy to arrive is reported.
-      if (packet.ecn == rtc::EcnMarking::kCe) {
-        rtcp_packet_info.back().ecn = packet.ecn;
   }
-      RTC_LOG(LS_WARNING) << "Received duplicate packet ssrc:" << packet.ssrc
-                          << " seq:"
-                          << static_cast<uint16_t>(
-                                 packet.unwrapped_sequence_number);
-    } else {
-      previous_ssrc = packet.ssrc;
-      previous_seq_no = packet.unwrapped_sequence_number;
-      rtcp_packet_info.push_back(
-          {.ssrc = packet.ssrc,
-           .sequence_number =
-               static_cast<uint16_t>(packet.unwrapped_sequence_number),
-           .arrival_time_offset = now - packet.arrival_time,
-           .ecn = packet.ecn});
-    }
-  }
-  packets_.clear();
   marker_bit_seen_ = false;
+  first_arrival_time_since_feedback_ = std::nullopt;
 
   auto feedback = std::make_unique<rtcp::CongestionControlFeedback>(
       std::move(rtcp_packet_info), compact_ntp);

modules/remote_bitrate_estimator/congestion_control_feedback_generator.h

@@ -10,9 +10,9 @@
 #ifndef MODULES_REMOTE_BITRATE_ESTIMATOR_CONGESTION_CONTROL_FEEDBACK_GENERATOR_H_
 #define MODULES_REMOTE_BITRATE_ESTIMATOR_CONGESTION_CONTROL_FEEDBACK_GENERATOR_H_
 
+#include <cstdint>
 #include <map>
-#include <memory>
+#include <optional>
-#include <vector>
 
 #include "api/environment/environment.h"
 #include "api/sequence_checker.h"
@@ -20,11 +20,10 @@
 #include "api/units/data_size.h"
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
+#include "modules/remote_bitrate_estimator/congestion_control_feedback_tracker.h"
 #include "modules/remote_bitrate_estimator/rtp_transport_feedback_generator.h"
-#include "modules/rtp_rtcp/source/rtcp_packet.h"
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "rtc_base/experiments/field_trial_parser.h"
-#include "rtc_base/numerics/sequence_number_unwrapper.h"
 
 namespace webrtc {
 
@@ -58,13 +57,6 @@ class CongestionControlFeedbackGenerator
   void SetTransportOverhead(DataSize overhead_per_packet) override;
 
  private:
-  struct PacketInfo {
-    uint32_t ssrc;
-    int64_t unwrapped_sequence_number = 0;
-    Timestamp arrival_time;
-    rtc::EcnMarking ecn = rtc::EcnMarking::kNotEct;
-  };
-
   Timestamp NextFeedbackTime() const RTC_RUN_ON(sequence_checker_);
 
   void SendFeedback(Timestamp now) RTC_RUN_ON(sequence_checker_);
@@ -84,11 +76,12 @@ class CongestionControlFeedbackGenerator
   DataSize packet_overhead_ = DataSize::Zero();
   DataSize send_rate_debt_ = DataSize::Zero();
 
-  std::map</*ssrc=*/uint32_t, SeqNumUnwrapper<uint16_t>>
+  std::map</*ssrc=*/uint32_t, CongestionControlFeedbackTracker>
-      sequence_number_unwrappers_;
+      feedback_trackers_;
 
-  std::vector<PacketInfo> packets_;
+  // std::vector<PacketInfo> packets_;
   Timestamp last_feedback_sent_time_ = Timestamp::Zero();
+  std::optional<Timestamp> first_arrival_time_since_feedback_;
   bool marker_bit_seen_ = false;
   Timestamp next_possible_feedback_send_time_ = Timestamp::Zero();
 };

modules/remote_bitrate_estimator/congestion_control_feedback_generator_unittest.cc

@@ -11,6 +11,7 @@
 #include "modules/remote_bitrate_estimator/congestion_control_feedback_generator.h"
 
 #include <algorithm>
+#include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <vector>
@@ -20,9 +21,11 @@
 #include "api/units/data_size.h"
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
+#include "modules/rtp_rtcp/source/rtcp_packet.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/common_header.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
-#include "rtc_base/logging.h"
+#include "rtc_base/buffer.h"
 #include "rtc_base/network/ecn_marking.h"
 #include "system_wrappers/include/clock.h"
 #include "test/explicit_key_value_config.h"
@@ -37,35 +40,6 @@ using ::testing::MockFunction;
 using ::testing::SizeIs;
 using ::testing::WithoutArgs;
 
-bool PacketInfoIsExpected(const CongestionControlFeedback::PacketInfo& a,
-                          const RtpPacketReceived b,
-                          Timestamp feedback_send_time) {
-  bool equal =
-      a.ssrc == b.Ssrc() && a.sequence_number == b.SequenceNumber() &&
-      (feedback_send_time - a.arrival_time_offset) == b.arrival_time() &&
-      a.ecn == b.ecn();
-  RTC_LOG_IF(LS_INFO, !equal)
-      << " Not equal got ssrc: " << a.ssrc << ", seq: " << a.sequence_number
-      << " arrival_time_offset: " << a.arrival_time_offset.ms()
-      << " ecn: " << a.ecn << " expected ssrc:" << b.Ssrc()
-      << ", seq: " << b.SequenceNumber() << " ecn: " << b.ecn();
-  return equal;
-}
-
-MATCHER_P2(PacketInfosAreExpected, expected_vector, feedback_send_time, "") {
-  if (expected_vector.size() != arg.size()) {
-    RTC_LOG(LS_INFO) << " Wrong size, expected: " << expected_vector.size()
-                     << " got: " << arg.size();
-    return false;
-  }
-  for (size_t i = 0; i < expected_vector.size(); ++i) {
-    if (!PacketInfoIsExpected(arg[i], expected_vector[i], feedback_send_time)) {
-      return false;
-    }
-  }
-  return true;
-}
-
 RtpPacketReceived CreatePacket(Timestamp arrival_time,
                                bool marker,
                                uint32_t ssrc = 1234,
@@ -244,7 +218,7 @@ TEST(CongestionControlFeedbackGeneratorTest,
 }
 
 TEST(CongestionControlFeedbackGeneratorTest,
-     SortsReceivedPacketsBySsrcAndSeqno) {
+     CanGenerateRtcpPacketFromTwoSsrcWithMissingPacketsAndWrap) {
   MockFunction<void(std::vector<std::unique_ptr<rtcp::RtcpPacket>>)>
       rtcp_sender;
   SimulatedClock clock(123456);
@@ -254,7 +228,9 @@ TEST(CongestionControlFeedbackGeneratorTest,
 
   TimeDelta time_to_next_process = generator.Process(clock.CurrentTime());
 
-  const std::vector<RtpPacketReceived> kExpectedRtcpPackInfoOrder = {
+  // Receive packets out of order, with missing packets (between  0xFFA and 1 =
+  // 6  and FFFC and 1 = 4) => total 14 packets is expected in the feedback.
+  const std::vector<RtpPacketReceived> kReceivedPackets = {
       // Reordered packet.
       CreatePacket(clock.CurrentTime() + kSmallTimeInterval, /*marker*/ false,
                    /*ssrc=*/123,
@@ -264,7 +240,7 @@ TEST(CongestionControlFeedbackGeneratorTest,
       // Reordered packet.
       CreatePacket(clock.CurrentTime() + kSmallTimeInterval,
                    /*marker*/ false, /*ssrc=*/
-                   234,
+                   /*ssrc=*/234,
                    /*seq=*/0xFFFC),
       CreatePacket(clock.CurrentTime(), /*marker*/ false, /*ssrc=*/234,
                    /*seq=*/1),
@@ -277,14 +253,17 @@ TEST(CongestionControlFeedbackGeneratorTest,
             rtcp::CongestionControlFeedback* rtcp =
                 static_cast<rtcp::CongestionControlFeedback*>(
                     rtcp_packets[0].get());
-            Timestamp feedback_send_time = clock.CurrentTime();
+
-            EXPECT_THAT(rtcp->packets(),
+            ASSERT_THAT(rtcp->packets(), SizeIs(14));
-                        PacketInfosAreExpected(kExpectedRtcpPackInfoOrder,
+            rtc::Buffer buffer = rtcp->Build();
-                                               feedback_send_time));
+            CongestionControlFeedback parsed_fb;
+            rtcp::CommonHeader header;
+            EXPECT_TRUE(header.Parse(buffer.data(), buffer.size()));
+            EXPECT_TRUE(parsed_fb.Parse(header));
+            EXPECT_THAT(parsed_fb.packets(), SizeIs(14));
           });
 
-  std::vector<RtpPacketReceived> receive_time_sorted =
+  std::vector<RtpPacketReceived> receive_time_sorted = kReceivedPackets;
-      kExpectedRtcpPackInfoOrder;
   std::sort(receive_time_sorted.begin(), receive_time_sorted.end(),
             [](const RtpPacketReceived& a, const RtpPacketReceived& b) {
               return a.arrival_time() < b.arrival_time();

modules/remote_bitrate_estimator/congestion_control_feedback_tracker.cc

@@ -0,0 +1,111 @@
+
+/*
+ *  Copyright (c) 2024 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "modules/remote_bitrate_estimator/congestion_control_feedback_tracker.h"
+
+#include <cstdint>
+#include <tuple>
+#include <vector>
+
+#include "absl/algorithm/container.h"
+#include "api/units/time_delta.h"
+#include "api/units/timestamp.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
+#include "modules/rtp_rtcp/source/rtp_packet_received.h"
+#include "rtc_base/checks.h"
+#include "rtc_base/logging.h"
+#include "rtc_base/network/ecn_marking.h"
+
+namespace webrtc {
+
+void CongestionControlFeedbackTracker::ReceivedPacket(
+    const RtpPacketReceived& packet) {
+  int64_t unwrapped_sequence_number =
+      unwrapper_.Unwrap(packet.SequenceNumber());
+  if (last_sequence_number_in_feedback_ &&
+      unwrapped_sequence_number < *last_sequence_number_in_feedback_ + 1) {
+    RTC_LOG(LS_WARNING)
+        << "Received packet unorderered between feeedback. SSRC: "
+        << packet.Ssrc() << " Seq: " << packet.SequenceNumber()
+        << " last feedback: "
+        << static_cast<uint16_t>(*last_sequence_number_in_feedback_);
+    // TODO: bugs.webrtc.org/374550342 - According to spec, the old packets
+    // should be reported again. But at the moment, we dont store history of
+    // packet we already reported and thus, they will be reported as lost. Note
+    // that this is likely not a problem in webrtc since the packets will also
+    // be removed from the send history when they are first reported as
+    // received.
+    last_sequence_number_in_feedback_ = unwrapped_sequence_number - 1;
+  }
+  packets_.push_back({.ssrc = packet.Ssrc(),
+                      .unwrapped_sequence_number = unwrapped_sequence_number,
+                      .arrival_time = packet.arrival_time(),
+                      .ecn = packet.ecn()});
+}
+
+void CongestionControlFeedbackTracker::AddPacketsToFeedback(
+    Timestamp feedback_time,
+    std::vector<rtcp::CongestionControlFeedback::PacketInfo>& packet_feedback) {
+  if (packets_.empty()) {
+    return;
+  }
+  absl::c_sort(packets_, [](const PacketInfo& a, const PacketInfo& b) {
+    return std::tie(a.unwrapped_sequence_number, a.arrival_time) <
+           std::tie(b.unwrapped_sequence_number, b.arrival_time);
+  });
+  if (!last_sequence_number_in_feedback_) {
+    last_sequence_number_in_feedback_ =
+        packets_.front().unwrapped_sequence_number - 1;
+  }
+
+  auto packet_it = packets_.begin();
+  uint32_t ssrc = packet_it->ssrc;
+  for (int64_t sequence_number = *last_sequence_number_in_feedback_ + 1;
+       sequence_number <= packets_.back().unwrapped_sequence_number;
+       ++sequence_number) {
+    RTC_DCHECK(packet_it != packets_.end());
+    RTC_DCHECK_EQ(ssrc, packet_it->ssrc);
+
+    rtc::EcnMarking ecn = rtc::EcnMarking::kNotEct;
+    TimeDelta arrival_time_offset = TimeDelta::MinusInfinity();
+
+    if (sequence_number == packet_it->unwrapped_sequence_number) {
+      arrival_time_offset = feedback_time - packet_it->arrival_time;
+      ecn = packet_it->ecn;
+      ++packet_it;
+      while (packet_it != packets_.end() &&
+             packet_it->unwrapped_sequence_number == sequence_number) {
+        // According to RFC 8888:
+        // If duplicate copies of a particular RTP packet are received, then the
+        // arrival time of the first copy to arrive MUST be reported. If any of
+        // the copies of the duplicated packet are ECN-CE marked, then an ECN-CE
+        // mark MUST be reported for that packet; otherwise, the ECN mark of the
+        // first copy to arrive is reported.
+        if (packet_it->ecn == rtc::EcnMarking::kCe) {
+          ecn = rtc::EcnMarking::kCe;
+        }
+        RTC_LOG(LS_WARNING) << "Received duplicate packet ssrc:" << ssrc
+                            << " seq:" << static_cast<uint16_t>(sequence_number)
+                            << " ecn: " << static_cast<int>(ecn);
+        ++packet_it;
+      }
+    }  // else - the packet has not been received yet.
+    packet_feedback.push_back(
+        {.ssrc = ssrc,
+         .sequence_number = static_cast<uint16_t>(sequence_number),
+         .arrival_time_offset = arrival_time_offset,
+         .ecn = ecn});
+  }
+  last_sequence_number_in_feedback_ = packets_.back().unwrapped_sequence_number;
+  packets_.clear();
+}
+
+}  // namespace webrtc

modules/remote_bitrate_estimator/congestion_control_feedback_tracker.h

@@ -0,0 +1,58 @@
+/*
+ *  Copyright (c) 2024 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+#ifndef MODULES_REMOTE_BITRATE_ESTIMATOR_CONGESTION_CONTROL_FEEDBACK_TRACKER_H_
+#define MODULES_REMOTE_BITRATE_ESTIMATOR_CONGESTION_CONTROL_FEEDBACK_TRACKER_H_
+
+#include <cstdint>
+#include <optional>
+#include <vector>
+
+#include "api/units/timestamp.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
+#include "modules/rtp_rtcp/source/rtp_packet_received.h"
+#include "rtc_base/network/ecn_marking.h"
+#include "rtc_base/numerics/sequence_number_unwrapper.h"
+namespace webrtc {
+
+// CongestionControlFeedbackTracker is reponsible for creating and keeping track
+// of feedback sent for a specific SSRC when feedback is sent according to
+// https://datatracker.ietf.org/doc/rfc8888/
+class CongestionControlFeedbackTracker {
+ public:
+  CongestionControlFeedbackTracker() = default;
+
+  void ReceivedPacket(const RtpPacketReceived& packet);
+
+  // Adds received packets to `packet_feedback`
+  // RTP sequence numbers are continous from the last created feedback unless
+  // reordering has occured between feedback packets. If so, the sequence
+  // number range may overlap with previousely sent feedback.
+  void AddPacketsToFeedback(
+      Timestamp feedback_time,
+      std::vector<rtcp::CongestionControlFeedback::PacketInfo>&
+          packet_feedback);
+
+ private:
+  struct PacketInfo {
+    uint32_t ssrc;
+    int64_t unwrapped_sequence_number = 0;
+    Timestamp arrival_time;
+    rtc::EcnMarking ecn = rtc::EcnMarking::kNotEct;
+  };
+
+  std::optional<int64_t> last_sequence_number_in_feedback_;
+  SeqNumUnwrapper<uint16_t> unwrapper_;
+
+  std::vector<PacketInfo> packets_;
+};
+
+}  // namespace webrtc
+
+#endif  // MODULES_REMOTE_BITRATE_ESTIMATOR_CONGESTION_CONTROL_FEEDBACK_TRACKER_H_

modules/remote_bitrate_estimator/congestion_control_feedback_tracker_unittest.cc

@@ -0,0 +1,201 @@
+/*
+ *  Copyright (c) 2024 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "modules/remote_bitrate_estimator/congestion_control_feedback_tracker.h"
+
+#include <cstdint>
+#include <vector>
+
+#include "api/units/time_delta.h"
+#include "api/units/timestamp.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
+#include "modules/rtp_rtcp/source/rtp_packet_received.h"
+#include "rtc_base/network/ecn_marking.h"
+#include "test/gmock.h"
+#include "test/gtest.h"
+
+namespace webrtc {
+namespace {
+
+using ::testing::AllOf;
+using ::testing::Field;
+using ::testing::Property;
+using ::testing::SizeIs;
+
+RtpPacketReceived CreatePacket(Timestamp arrival_time,
+                               uint16_t seq = 1,
+                               rtc::EcnMarking ecn = rtc::EcnMarking::kNotEct) {
+  RtpPacketReceived packet;
+  packet.SetSsrc(1234);
+  packet.SetSequenceNumber(seq);
+  packet.set_arrival_time(arrival_time);
+  packet.set_ecn(ecn);
+  return packet;
+}
+
+TEST(CongestionControlFeedbackTrackerTest,
+     FeedbackIncludeReceivedPacketsInSequenceNumberOrder) {
+  RtpPacketReceived packet_1 =
+      CreatePacket(/*arrival_time=*/Timestamp::Millis(123), /*seq =*/2);
+  RtpPacketReceived packet_2 =
+      CreatePacket(/*arrival_time=*/Timestamp::Millis(125), /*seq=*/1);
+
+  CongestionControlFeedbackTracker tracker;
+  tracker.ReceivedPacket(packet_1);
+  tracker.ReceivedPacket(packet_2);
+
+  Timestamp feedback_time = Timestamp::Millis(567);
+  std::vector<rtcp::CongestionControlFeedback::PacketInfo> feedback_info;
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(2));
+  EXPECT_THAT(
+      feedback_info[0],
+      AllOf(
+          Field(&rtcp::CongestionControlFeedback::PacketInfo::sequence_number,
+                packet_2.SequenceNumber()),
+          Field(
+              &rtcp::CongestionControlFeedback::PacketInfo::arrival_time_offset,
+              feedback_time - packet_2.arrival_time())));
+  EXPECT_THAT(
+      feedback_info[1],
+      AllOf(
+          Field(&rtcp::CongestionControlFeedback::PacketInfo::sequence_number,
+                packet_1.SequenceNumber()),
+          Field(
+              &rtcp::CongestionControlFeedback::PacketInfo::arrival_time_offset,
+              feedback_time - packet_1.arrival_time())));
+}
+
+TEST(CongestionControlFeedbackTrackerTest,
+     ReportsFirstReceivedPacketArrivalTimeButEcnFromCePacketIfDuplicate) {
+  RtpPacketReceived packet_1 =
+      CreatePacket(/*arrival_time=*/Timestamp::Millis(123), /*seq =*/1,
+                   rtc::EcnMarking::kEct1);
+  RtpPacketReceived packet_2 = CreatePacket(
+      /*arrival_time=*/Timestamp::Millis(125), /*seq=*/1, rtc::EcnMarking::kCe);
+  RtpPacketReceived packet_3 = CreatePacket(
+      /*arrival_time=*/Timestamp::Millis(126), /*seq=*/1,
+      rtc::EcnMarking::kEct1);
+
+  CongestionControlFeedbackTracker tracker;
+  tracker.ReceivedPacket(packet_1);
+  tracker.ReceivedPacket(packet_2);
+  tracker.ReceivedPacket(packet_3);
+
+  Timestamp feedback_time = Timestamp::Millis(567);
+  std::vector<rtcp::CongestionControlFeedback::PacketInfo> feedback_info;
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(1));
+  EXPECT_THAT(
+      feedback_info[0],
+      AllOf(
+          Field(
+              &rtcp::CongestionControlFeedback::PacketInfo::arrival_time_offset,
+              feedback_time - packet_1.arrival_time()),
+          Field(&rtcp::CongestionControlFeedback::PacketInfo::ecn,
+                rtc::EcnMarking::kCe)));
+}
+
+TEST(CongestionControlFeedbackTrackerTest,
+     FeedbackGeneratesContinouseSequenceNumbers) {
+  RtpPacketReceived packet_1 =
+      CreatePacket(/*arrival_time=*/Timestamp::Millis(123), /*seq =*/1);
+  // Packet with sequence number 2 is lost or reordered.
+  RtpPacketReceived packet_2 = CreatePacket(
+      /*arrival_time=*/Timestamp::Millis(125), /*seq=*/3);
+
+  CongestionControlFeedbackTracker tracker;
+  tracker.ReceivedPacket(packet_1);
+  tracker.ReceivedPacket(packet_2);
+
+  std::vector<rtcp::CongestionControlFeedback::PacketInfo> feedback_info;
+  Timestamp feedback_time = Timestamp::Millis(567);
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(3));
+  EXPECT_THAT(feedback_info[0].sequence_number, 1);
+  EXPECT_THAT(feedback_info[0].arrival_time_offset,
+              feedback_time - packet_1.arrival_time());
+  EXPECT_THAT(feedback_info[1].sequence_number, 2);
+  EXPECT_THAT(feedback_info[1].arrival_time_offset, TimeDelta::MinusInfinity());
+  EXPECT_THAT(feedback_info[2].sequence_number, 3);
+  EXPECT_THAT(feedback_info[2].arrival_time_offset,
+              feedback_time - packet_2.arrival_time());
+}
+
+TEST(CongestionControlFeedbackTrackerTest,
+     FeedbackGeneratesContinouseSequenceNumbersBetweenFeedbackPackets) {
+  RtpPacketReceived packet_1 =
+      CreatePacket(/*arrival_time=*/Timestamp::Millis(123), /*seq =*/1);
+  RtpPacketReceived packet_2 = CreatePacket(
+      /*arrival_time=*/Timestamp::Millis(125), /*seq=*/3);
+
+  CongestionControlFeedbackTracker tracker;
+  tracker.ReceivedPacket(packet_1);
+
+  std::vector<rtcp::CongestionControlFeedback::PacketInfo> feedback_info;
+  Timestamp feedback_time = Timestamp::Millis(567);
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(1));
+  EXPECT_THAT(feedback_info[0].sequence_number, 1);
+  EXPECT_THAT(feedback_info[0].arrival_time_offset,
+              feedback_time - packet_1.arrival_time());
+
+  feedback_info.clear();
+  feedback_time = Timestamp::Millis(678);
+  tracker.ReceivedPacket(packet_2);
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(2));
+  EXPECT_THAT(feedback_info[0].sequence_number, 2);
+  EXPECT_THAT(feedback_info[0].arrival_time_offset, TimeDelta::MinusInfinity());
+  EXPECT_THAT(feedback_info[1].sequence_number, 3);
+  EXPECT_THAT(feedback_info[1].arrival_time_offset,
+              feedback_time - packet_2.arrival_time());
+}
+
+TEST(CongestionControlFeedbackTrackerTest,
+     FeedbackGeneratesRepeatedSequenceNumbersOnReorderingBetweenFeedback) {
+  RtpPacketReceived packet_1 =
+      CreatePacket(/*arrival_time=*/Timestamp::Millis(123), /*seq =*/2);
+  RtpPacketReceived packet_2 = CreatePacket(
+      /*arrival_time=*/Timestamp::Millis(125), /*seq=*/1);
+  RtpPacketReceived packet_3 = CreatePacket(
+      /*arrival_time=*/Timestamp::Millis(125), /*seq=*/3);
+
+  CongestionControlFeedbackTracker tracker;
+  tracker.ReceivedPacket(packet_1);
+
+  std::vector<rtcp::CongestionControlFeedback::PacketInfo> feedback_info;
+  Timestamp feedback_time = Timestamp::Millis(567);
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(1));
+  EXPECT_THAT(feedback_info[0].sequence_number, 2);
+  EXPECT_THAT(feedback_info[0].arrival_time_offset,
+              feedback_time - packet_1.arrival_time());
+
+  feedback_info.clear();
+  feedback_time = Timestamp::Millis(678);
+  tracker.ReceivedPacket(packet_2);
+  tracker.ReceivedPacket(packet_3);
+  tracker.AddPacketsToFeedback(feedback_time, feedback_info);
+  ASSERT_THAT(feedback_info, SizeIs(3));
+  EXPECT_THAT(feedback_info[0].sequence_number, 1);
+  EXPECT_THAT(feedback_info[0].arrival_time_offset,
+              feedback_time - packet_2.arrival_time());
+  EXPECT_THAT(feedback_info[1].sequence_number, 2);
+  // TODO: bugs.webrtc.org/374550342 -  This is against the spec. According to
+  // the specification, we should have kept the history.
+  EXPECT_THAT(feedback_info[1].arrival_time_offset, TimeDelta::MinusInfinity());
+  EXPECT_THAT(feedback_info[2].sequence_number, 3);
+  EXPECT_THAT(feedback_info[2].arrival_time_offset,
+              feedback_time - packet_3.arrival_time());
+}
+
+}  // namespace
+}  // namespace webrtc

modules/rtp_rtcp/BUILD.gn

@@ -674,6 +674,7 @@ if (rtc_include_tests) {
       "../../api:create_time_controller",
       "../../api:field_trials_registry",
       "../../api:frame_transformer_factory",
+      "../../api:function_view",
       "../../api:make_ref_counted",
       "../../api:mock_frame_encryptor",
       "../../api:mock_frame_transformer",

modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.cc

@@ -10,6 +10,7 @@
 
 #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h"
 
+#include <algorithm>
 #include <cstddef>
 #include <cstdint>
 #include <utility>
@@ -19,6 +20,8 @@
 #include "api/units/time_delta.h"
 #include "api/units/timestamp.h"
 #include "modules/rtp_rtcp/source/byte_io.h"
+#include "modules/rtp_rtcp/source/rtcp_packet/common_header.h"
+#include "rtc_base/checks.h"
 #include "rtc_base/network/ecn_marking.h"
 
 namespace webrtc {
@@ -173,12 +176,17 @@ bool CongestionControlFeedback::Create(uint8_t* buffer,
                                          packets[0].sequence_number);
     *position += 2;
     // num_reports
-    uint16_t num_reports = packets[packets.size() - 1].sequence_number -
+    uint16_t num_reports = packets.size();
-                           packets[0].sequence_number + 1;
+    RTC_DCHECK_EQ(static_cast<uint16_t>(
 
-    // Each report block MUST NOT include more than 16384 packet metric blocks
+                      packets[packets.size() - 1].sequence_number -
-    // (i.e., it MUST NOT report on more than one quarter of the sequence number
+                      packets[0].sequence_number + 1),
-    // space in a single report).
+                  packets.size())
+        << "Expected continous rtp sequence numbers.";
+
+    // Each report block MUST NOT include more than 16384 packet metric
+    // blocks (i.e., it MUST NOT report on more than one quarter of the
+    // sequence number space in a single report).
     if (num_reports > 16384) {
       RTC_DCHECK_NOTREACHED() << "Unexpected number of reports:" << num_reports;
       return;
@@ -186,18 +194,12 @@ bool CongestionControlFeedback::Create(uint8_t* buffer,
     ByteWriter<uint16_t>::WriteBigEndian(&buffer[*position], num_reports);
     *position += 2;
 
-    int packet_index = 0;
+    for (const PacketInfo& packet : packets) {
-    for (int i = 0; i < num_reports; ++i) {
+      bool received = packet.arrival_time_offset.IsFinite();
-      uint16_t sequence_number = packets[0].sequence_number + i;
-
-      bool received = sequence_number == packets[packet_index].sequence_number;
-
       uint16_t packet_info = 0;
       if (received) {
-        packet_info = 0x8000 | To2BitEcn(packets[packet_index].ecn) |
+        packet_info = 0x8000 | To2BitEcn(packet.ecn) |
-                      To13bitAto(packets[packet_index].arrival_time_offset);
+                      To13bitAto(packet.arrival_time_offset);
-
-        ++packet_index;
       }
       ByteWriter<uint16_t>::WriteBigEndian(&buffer[*position], packet_info);
       *position += 2;
@@ -303,13 +305,13 @@ bool CongestionControlFeedback::Parse(const rtcp::CommonHeader& packet) {
 
       uint16_t seq_no = base_seqno + i;
       bool received = (packet_info & 0x8000);
-      if (received) {
+      packets_.push_back(
-        packets_.push_back({.ssrc = ssrc,
+          {.ssrc = ssrc,
            .sequence_number = seq_no,
-                            .arrival_time_offset = AtoToTimeDelta(packet_info),
+           .arrival_time_offset = received ? AtoToTimeDelta(packet_info)
+                                           : TimeDelta::MinusInfinity(),
            .ecn = ToEcnMarking(packet_info)});
     }
-    }
     if (num_reports % 2) {
       // 2 bytes padding
       payload += 2;

modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h

@@ -10,6 +10,7 @@
 #ifndef MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_CONGESTION_CONTROL_FEEDBACK_H_
 #define MODULES_RTP_RTCP_SOURCE_RTCP_PACKET_CONGESTION_CONTROL_FEEDBACK_H_
 
+#include <cstddef>
 #include <cstdint>
 #include <vector>
 
@@ -29,15 +30,17 @@ class CongestionControlFeedback : public Rtpfb {
   struct PacketInfo {
     uint32_t ssrc = 0;
     uint16_t sequence_number = 0;
-    //  Time offset from report timestamp.
+    //  Time offset from report timestamp. Minus infinity if the packet has not
-    TimeDelta arrival_time_offset = TimeDelta::Zero();
+    //  been received.
+    TimeDelta arrival_time_offset = TimeDelta::MinusInfinity();
     rtc::EcnMarking ecn = rtc::EcnMarking::kNotEct;
   };
 
   static constexpr uint8_t kFeedbackMessageType = 11;
 
-  // `Packets` MUST be sorted in sequence_number order per SSRC.
+  // `Packets` MUST be sorted in sequence_number order per SSRC. There MUST not
-  // `Packets` MUST not include duplicate sequence numbers.
+  // be missing sequence numbers between `Packets`. `Packets` MUST not include
+  // duplicate sequence numbers.
   CongestionControlFeedback(std::vector<PacketInfo> packets,
                             uint32_t report_timestamp_compact_ntp);
   CongestionControlFeedback() = default;

modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback_unittest.cc

@@ -13,7 +13,10 @@
 #include <cstddef>
 #include <cstdint>
 #include <utility>
+#include <vector>
 
+#include "api/array_view.h"
+#include "api/function_view.h"
 #include "api/units/time_delta.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/common_header.h"
 #include "modules/rtp_rtcp/source/rtcp_packet/rtpfb.h"
@@ -33,16 +36,21 @@ using ::testing::IsEmpty;
 // forth to CompactNtp.
 bool PacketInfoEqual(const CongestionControlFeedback::PacketInfo& a,
                      const CongestionControlFeedback::PacketInfo& b) {
+  bool arrival_time_offset_equal =
+      (a.arrival_time_offset.IsInfinite() &&
+       b.arrival_time_offset.IsInfinite()) ||
+      (a.arrival_time_offset.IsFinite() && b.arrival_time_offset.IsFinite() &&
+       (a.arrival_time_offset - b.arrival_time_offset).Abs() <
+           TimeDelta::Seconds(1) / 1024);
+
   bool equal = a.ssrc == b.ssrc && a.sequence_number == b.sequence_number &&
-               ((a.arrival_time_offset - b.arrival_time_offset).Abs() <
+               arrival_time_offset_equal && a.ecn == b.ecn;
-                TimeDelta::Seconds(1) / 1024) &&
-               a.ecn == b.ecn;
   RTC_LOG_IF(LS_INFO, !equal)
       << " Not equal got ssrc: " << a.ssrc << ", seq: " << a.sequence_number
-      << " arrival_time_offset: " << a.arrival_time_offset.ms()
+      << " arrival_time_offset: " << a.arrival_time_offset.ms_or(-1)
       << " ecn: " << a.ecn << " expected ssrc:" << b.ssrc
       << ", seq: " << b.sequence_number
-      << " arrival_time_offset: " << b.arrival_time_offset.ms()
+      << " arrival_time_offset: " << b.arrival_time_offset.ms_or(-1)
       << " ecn: " << b.ecn;
   return equal;
 }
@@ -205,6 +213,14 @@ TEST(CongestionControlFeedbackTest, CanCreateAndParseWithMissingPackets) {
       {.ssrc = 1,
        .sequence_number = 0xFFFE,
        .arrival_time_offset = TimeDelta::Millis(1)},
+      {.ssrc = 1,
+       .sequence_number = 0xFFFF,
+       // Packet lost
+       .arrival_time_offset = TimeDelta::MinusInfinity()},
+      {.ssrc = 1,
+       .sequence_number = 0,
+       // Packet lost
+       .arrival_time_offset = TimeDelta::MinusInfinity()},
       {.ssrc = 1,
        .sequence_number = 1,
        .arrival_time_offset = TimeDelta::Millis(1)}};