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}diff --git a/modules/congestion_controller/rtp/BUILD.gn b/modules/congestion_controller/rtp/BUILD.gn
index 357e1a6..bb3fe1a 100644
--- a/modules/congestion_controller/rtp/BUILD.gn
+++ b/modules/congestion_controller/rtp/BUILD.gn
@@ -64,6 +64,7 @@
"../../../rtc_base:rtc_base_approved",
"../../../rtc_base/network:sent_packet",
"../../../system_wrappers",
+ "../../../system_wrappers:field_trial",
"../../rtp_rtcp:rtp_rtcp_format",
]
}
diff --git a/modules/congestion_controller/rtp/send_time_history.cc b/modules/congestion_controller/rtp/send_time_history.cc
index 76450b2..111389b 100644
--- a/modules/congestion_controller/rtp/send_time_history.cc
+++ b/modules/congestion_controller/rtp/send_time_history.cc
@@ -55,12 +55,12 @@
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 @@
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(
diff --git a/modules/congestion_controller/rtp/send_time_history.h b/modules/congestion_controller/rtp/send_time_history.h
index 553ba15..65ef318 100644
--- a/modules/congestion_controller/rtp/send_time_history.h
+++ b/modules/congestion_controller/rtp/send_time_history.h
@@ -23,6 +23,8 @@
class SendTimeHistory {
public:
+ enum class Status { kNotAdded, kOk, kDuplicate };
+
explicit SendTimeHistory(int64_t packet_age_limit_ms);
~SendTimeHistory();
@@ -32,8 +34,9 @@
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;
diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc
index 4249e64..b1699b4 100644
--- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc
+++ b/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 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 @@
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);
diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.h b/modules/congestion_controller/rtp/transport_feedback_adapter.h
index 7df0baa..3fb4fdd 100644
--- a/modules/congestion_controller/rtp/transport_feedback_adapter.h
+++ b/modules/congestion_controller/rtp/transport_feedback_adapter.h
@@ -63,6 +63,8 @@
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_;
diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc
index fe982e2..c9ec37b 100644
--- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc
+++ b/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 @@
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
