Fix RFC 8888 feedback reordering timeline reset Allow a negative delta offset (up to -500ms) during RFC 8888 congestion control feedback processing. This prevents the absolute time mapping offset from resetting completely when return-path feedback packets are reordered, which previously caused a permanent decoupling/spike in the one-way delay metrics. Bug: webrtc:515090595 Change-Id: Icbae84b023cb2808d3a07c5b717e9dc3090d2187 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/474500 Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Commit-Queue: Per Kjellander <perkj@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47770}
diff --git a/modules/congestion_controller/rtp/BUILD.gn b/modules/congestion_controller/rtp/BUILD.gn index 23e6a06..168f684 100644 --- a/modules/congestion_controller/rtp/BUILD.gn +++ b/modules/congestion_controller/rtp/BUILD.gn
@@ -77,6 +77,7 @@ "../../../rtc_base:buffer", "../../../rtc_base/network:sent_packet", "../../../system_wrappers", + "../../../test:near_matcher", "../../../test:test_support", "../../rtp_rtcp:ntp_time_util", "../../rtp_rtcp:rtp_rtcp_format",
diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index 90af19e..f455fee 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc
@@ -294,7 +294,8 @@ *last_feedback_compact_ntp_time_) : TimeDelta::Zero(); last_feedback_compact_ntp_time_ = feedback.report_timestamp_compact_ntp(); - if (feedback_delta < TimeDelta::Zero()) { + + if (feedback_delta < -TimeDelta::Millis(500)) { RTC_LOG(LS_WARNING) << "Unexpected feedback ntp time delta " << feedback_delta << "."; current_offset_ = feedback_receive_time;
diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index 239bdf3..a9ffad1 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc
@@ -34,6 +34,7 @@ #include "system_wrappers/include/clock.h" #include "test/gmock.h" #include "test/gtest.h" +#include "test/near_matcher.h" namespace webrtc { namespace { @@ -216,15 +217,18 @@ std::optional<TransportPacketsFeedback> CreateAndProcessFeedback( std::span<const PacketTemplate> packets, - TransportFeedbackAdapter& adapter) { + TransportFeedbackAdapter& adapter, + std::optional<Timestamp> feedback_receive_time = std::nullopt) { + Timestamp receive_time = feedback_receive_time.value_or(TimeNow()); if (UseRfc8888CongestionControlFeedback()) { rtcp::CongestionControlFeedback rtcp_feedback = BuildRtcpCongestionControlFeedbackPacket(packets); - return adapter.ProcessCongestionControlFeedback(rtcp_feedback, TimeNow()); + return adapter.ProcessCongestionControlFeedback(rtcp_feedback, + receive_time); } else { rtcp::TransportFeedback rtcp_feedback = BuildRtcpTransportFeedbackPacket(packets); - return adapter.ProcessTransportFeedback(rtcp_feedback, TimeNow()); + return adapter.ProcessTransportFeedback(rtcp_feedback, receive_time); } } }; @@ -526,7 +530,7 @@ ASSERT_EQ(adapted_feedback_1->packet_feedbacks.size(), adapted_feedback_2->packet_feedbacks.size()); - ASSERT_THAT(adapted_feedback_1->packet_feedbacks, testing::SizeIs(1)); + ASSERT_THAT(adapted_feedback_1->packet_feedbacks, SizeIs(1)); EXPECT_EQ((adapted_feedback_1->packet_feedbacks[0].receive_time - adapted_feedback_1->packet_feedbacks[0].sent_packet.send_time) .RoundTo(TimeDelta::Millis(1)), @@ -659,6 +663,52 @@ EXPECT_FALSE(FindFeedback(adapted_feedback4, 2).has_value()); } +TEST_P(TransportFeedbackAdapterTest, HandlesReorderedFeedbackPackets) { + TransportFeedbackAdapter adapter; + + PacketTemplate packet_1 = {.transport_sequence_number = 1, + .rtp_sequence_number = 101, + .send_timestamp = Timestamp::Millis(100), + .receive_timestamp = Timestamp::Millis(120)}; + + PacketTemplate packet_2 = {.transport_sequence_number = 2, + .rtp_sequence_number = 102, + .send_timestamp = Timestamp::Millis(110), + .receive_timestamp = Timestamp::Millis(130)}; + + adapter.AddPacket(CreatePacketToSend(packet_1), packet_1.pacing_info, + /*overhead=*/0u, TimeNow()); + adapter.ProcessSentPacket(SentPacketInfo(packet_1.transport_sequence_number, + packet_1.send_timestamp.ms())); + adapter.AddPacket(CreatePacketToSend(packet_2), packet_2.pacing_info, + /*overhead=*/0u, TimeNow()); + adapter.ProcessSentPacket(SentPacketInfo(packet_2.transport_sequence_number, + packet_2.send_timestamp.ms())); + + // Process the later feedback packet first (reordered) + std::optional<TransportPacketsFeedback> adapted_feedback2 = + CreateAndProcessFeedback( + {{packet_2}}, adapter, + /*feedback_receive_time=*/Timestamp::Millis(1000)); + ASSERT_THAT(adapted_feedback2->packet_feedbacks, SizeIs(1)); + + // Process the earlier feedback packet 10ms later due to reordering. + std::optional<TransportPacketsFeedback> adapted_feedback1 = + CreateAndProcessFeedback( + {{packet_1}}, adapter, + /*feedback_receive_time=*/Timestamp::Millis(1010)); + ASSERT_THAT(adapted_feedback1->packet_feedbacks, SizeIs(1)); + + const PacketResult& result2 = adapted_feedback2->packet_feedbacks[0]; + const PacketResult& result1 = adapted_feedback1->packet_feedbacks[0]; + + // Even though processed out-of-order, the offset between send and receive + // time should be identical within 1ms (due to compact NTP fractional tick + // precision limits). + EXPECT_THAT(result2.receive_time - result2.sent_packet.send_time, + Near(result1.receive_time - result1.sent_packet.send_time)); +} + TEST(TransportFeedbackAdapterCongestionFeedbackTest, CongestionControlFeedbackResultHasEcn) { TransportFeedbackAdapter adapter;