diff --git a/modules/congestion_controller/rtp/BUILD.gn b/modules/congestion_controller/rtp/BUILD.gn index 357e1a6b45..bb3fe1aaa7 100644 --- a/modules/congestion_controller/rtp/BUILD.gn +++ b/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", ] } diff --git a/modules/congestion_controller/rtp/send_time_history.cc b/modules/congestion_controller/rtp/send_time_history.cc index 76450b2872..111389be50 100644 --- a/modules/congestion_controller/rtp/send_time_history.cc +++ b/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 SendTimeHistory::GetPacket( diff --git a/modules/congestion_controller/rtp/send_time_history.h b/modules/congestion_controller/rtp/send_time_history.h index 553ba15211..65ef3185d5 100644 --- a/modules/congestion_controller/rtp/send_time_history.h +++ b/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 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 4249e64841..b1699b4cb6 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 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 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 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 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 7df0baa607..3fb4fddcee 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.h +++ b/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_; diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index fe982e241f..c9ec37b089 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 @@ 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 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 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 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 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