Ignore duplicate sent packets in TransportFeedbackAdapter

Bug: webrtc:10564
Change-Id: I617b58ef8cf5858d7a81aaa39884c5cc1ac2af6e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/133564
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27689}

modules/congestion_controller/rtp/BUILD.gn

@@ -64,6 +64,7 @@ rtc_static_library("transport_feedback") {
     "../../../rtc_base:rtc_base_approved",
     "../../../rtc_base/network:sent_packet",
     "../../../system_wrappers",
+    "../../../system_wrappers:field_trial",
     "../../rtp_rtcp:rtp_rtcp_format",
   ]
 }

modules/congestion_controller/rtp/send_time_history.cc

@@ -55,12 +55,12 @@ void SendTimeHistory::AddUntracked(size_t packet_size, int64_t send_time_ms) {
       std::max(last_untracked_send_time_ms_, send_time_ms);
 }
 
-bool SendTimeHistory::OnSentPacket(uint16_t sequence_number,
-                                   int64_t send_time_ms) {
+SendTimeHistory::Status SendTimeHistory::OnSentPacket(uint16_t sequence_number,
+                                                      int64_t send_time_ms) {
   int64_t unwrapped_seq_num = seq_num_unwrapper_.Unwrap(sequence_number);
   auto it = history_.find(unwrapped_seq_num);
   if (it == history_.end())
-    return false;
+    return Status::kNotAdded;
   bool packet_retransmit = it->second.send_time_ms >= 0;
   it->second.send_time_ms = send_time_ms;
   last_send_time_ms_ = std::max(last_send_time_ms_, send_time_ms);
@@ -74,7 +74,7 @@ bool SendTimeHistory::OnSentPacket(uint16_t sequence_number,
     it->second.unacknowledged_data += pending_untracked_size_;
     pending_untracked_size_ = 0;
   }
-  return true;
+  return packet_retransmit ? Status::kDuplicate : Status::kOk;
 }
 
 absl::optional<PacketFeedback> SendTimeHistory::GetPacket(

modules/congestion_controller/rtp/send_time_history.h

@@ -23,6 +23,8 @@ struct PacketFeedback;
 
 class SendTimeHistory {
  public:
+  enum class Status { kNotAdded, kOk, kDuplicate };
+
   explicit SendTimeHistory(int64_t packet_age_limit_ms);
   ~SendTimeHistory();
 
@@ -32,8 +34,9 @@ class SendTimeHistory {
   void AddUntracked(size_t packet_size, int64_t send_time_ms);
 
   // Updates packet info identified by |sequence_number| with |send_time_ms|.
-  // Return false if not found.
-  bool OnSentPacket(uint16_t sequence_number, int64_t send_time_ms);
+  // Returns a PacketSendState indicating if the packet was not found, sent,
+  // or if it was previously already marked as sent.
+  Status OnSentPacket(uint16_t sequence_number, int64_t send_time_ms);
 
   // Retrieves packet info identified by |sequence_number|.
   absl::optional<PacketFeedback> GetPacket(uint16_t sequence_number) const;

modules/congestion_controller/rtp/transport_feedback_adapter.cc

@@ -19,6 +19,7 @@
 #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
+#include "system_wrappers/include/field_trial.h"
 
 namespace webrtc {
 namespace {
@@ -47,7 +48,9 @@ const int64_t kBaseTimestampScaleFactor =
 const int64_t kBaseTimestampRangeSizeUs = kBaseTimestampScaleFactor * (1 << 24);
 
 TransportFeedbackAdapter::TransportFeedbackAdapter()
-    : send_time_history_(kSendTimeHistoryWindowMs),
+    : allow_duplicates_(field_trial::IsEnabled(
+          "WebRTC-TransportFeedbackAdapter-AllowDuplicates")),
+      send_time_history_(kSendTimeHistoryWindowMs),
       current_offset_ms_(kNoTimestamp),
       last_timestamp_us_(kNoTimestamp),
       local_net_id_(0),
@@ -100,10 +103,14 @@ absl::optional<SentPacket> TransportFeedbackAdapter::ProcessSentPacket(
   rtc::CritScope cs(&lock_);
   // TODO(srte): Only use one way to indicate that packet feedback is used.
   if (sent_packet.info.included_in_feedback || sent_packet.packet_id != -1) {
-    send_time_history_.OnSentPacket(sent_packet.packet_id,
-                                    sent_packet.send_time_ms);
-    absl::optional<PacketFeedback> packet =
-        send_time_history_.GetPacket(sent_packet.packet_id);
+    SendTimeHistory::Status send_status = send_time_history_.OnSentPacket(
+        sent_packet.packet_id, sent_packet.send_time_ms);
+    absl::optional<PacketFeedback> packet;
+    if (allow_duplicates_ ||
+        send_status != SendTimeHistory::Status::kDuplicate) {
+      packet = send_time_history_.GetPacket(sent_packet.packet_id);
+    }
+
     if (packet) {
       SentPacket msg;
       msg.size = DataSize::bytes(packet->payload_size);

modules/congestion_controller/rtp/transport_feedback_adapter.h

@@ -63,6 +63,8 @@ class TransportFeedbackAdapter {
       const rtcp::TransportFeedback& feedback,
       Timestamp feedback_time);
 
+  const bool allow_duplicates_;
+
   rtc::CriticalSection lock_;
   SendTimeHistory send_time_history_ RTC_GUARDED_BY(&lock_);
   int64_t current_offset_ms_;

modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc

@@ -19,6 +19,7 @@
 #include "rtc_base/checks.h"
 #include "rtc_base/numerics/safe_conversions.h"
 #include "system_wrappers/include/clock.h"
+#include "test/field_trial.h"
 #include "test/gmock.h"
 #include "test/gtest.h"
 
@@ -393,6 +394,53 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) {
                                  adapter_->GetTransportFeedbackVector());
   }
 }
+
+TEST_F(TransportFeedbackAdapterTest, IgnoreDuplicatePacketSentCalls) {
+  const PacketFeedback packet(100, 200, 0, 1500, kPacingInfo0);
+
+  // Add a packet and then mark it as sent.
+  adapter_->AddPacket(kSsrc, packet.sequence_number, packet.payload_size,
+                      packet.pacing_info,
+                      Timestamp::ms(clock_.TimeInMilliseconds()));
+  absl::optional<SentPacket> sent_packet =
+      adapter_->ProcessSentPacket(rtc::SentPacket(
+          packet.sequence_number, packet.send_time_ms, rtc::PacketInfo()));
+  EXPECT_TRUE(sent_packet.has_value());
+
+  // Call ProcessSentPacket() again with the same sequence number. This packet
+  // has already been marked as sent and the call should be ignored.
+  absl::optional<SentPacket> duplicate_packet =
+      adapter_->ProcessSentPacket(rtc::SentPacket(
+          packet.sequence_number, packet.send_time_ms, rtc::PacketInfo()));
+  EXPECT_FALSE(duplicate_packet.has_value());
+}
+
+TEST_F(TransportFeedbackAdapterTest, AllowDuplicatePacketSentCallsWithTrial) {
+  // Allow duplicates if this field trial kill-switch is enabled.
+  webrtc::test::ScopedFieldTrials field_trial(
+      "WebRTC-TransportFeedbackAdapter-AllowDuplicates/Enabled/");
+  // Re-run setup so the flags goes into effect.
+  SetUp();
+
+  const PacketFeedback packet(100, 200, 0, 1500, kPacingInfo0);
+
+  // Add a packet and then mark it as sent.
+  adapter_->AddPacket(kSsrc, packet.sequence_number, packet.payload_size,
+                      packet.pacing_info,
+                      Timestamp::ms(clock_.TimeInMilliseconds()));
+  absl::optional<SentPacket> sent_packet =
+      adapter_->ProcessSentPacket(rtc::SentPacket(
+          packet.sequence_number, packet.send_time_ms, rtc::PacketInfo()));
+  EXPECT_TRUE(sent_packet.has_value());
+
+  // Call ProcessSentPacket() again with the same sequence number. This packet
+  // should still be allowed due to the field trial/
+  absl::optional<SentPacket> duplicate_packet =
+      adapter_->ProcessSentPacket(rtc::SentPacket(
+          packet.sequence_number, packet.send_time_ms, rtc::PacketInfo()));
+  EXPECT_TRUE(duplicate_packet.has_value());
+}
+
 }  // namespace test
 }  // namespace webrtc_cc
 }  // namespace webrtc