Ensure Scream only treat reported lost packets once Also add test to TransportFeedbackAdapterTest - to verify that lost feedback packets are not treated as lost sent RTP packets. - to verify that packets reported as implicitly and explicitly lost has the flag reported_lost_for_the_first_time set only the first time it is reported lost. Bug: webrtc:483936079 Change-Id: Id820783fe8f7c774451d50b7a8dac4a07881dc16 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/457641 Reviewed-by: Björn Terelius <terelius@webrtc.org> Commit-Queue: Per Kjellander <perkj@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47184}
diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index 1abf22f..bdb63b7 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc
@@ -606,6 +606,59 @@ EXPECT_EQ(adapted_feedback_2->data_in_flight, DataSize::Zero()); } +TEST_P(TransportFeedbackAdapterTest, + PacketsInLostFeedbackAreNotReportedAsLost) { + TransportFeedbackAdapter adapter; + + std::vector<PacketTemplate> packets = { + {.transport_sequence_number = 1, + .rtp_sequence_number = 101, + .send_timestamp = Timestamp::Millis(100), + .receive_timestamp = Timestamp::Millis(120)}, + {.transport_sequence_number = 2, + .rtp_sequence_number = 102, + .send_timestamp = Timestamp::Millis(110), + .receive_timestamp = Timestamp::Millis(130)}, + {.transport_sequence_number = 3, + .rtp_sequence_number = 103, + .send_timestamp = Timestamp::Millis(120), + .receive_timestamp = Timestamp::Millis(140)}, + {.transport_sequence_number = 4, + .rtp_sequence_number = 104, + .send_timestamp = Timestamp::Millis(130), + .receive_timestamp = Timestamp::Millis(150)}}; + + for (const PacketTemplate& packet : packets) { + adapter.AddPacket(CreatePacketToSend(packet), packet.pacing_info, + /*overhead=*/0u, TimeNow()); + adapter.ProcessSentPacket(SentPacketInfo(packet.transport_sequence_number, + packet.send_timestamp.ms())); + } + + // Feedback 1: Covers packet 1 + std::vector<PacketTemplate> feedback1_packets = {packets[0]}; + CreateAndProcessFeedback(feedback1_packets, adapter); + + // Feedback 2 (Lost network packet, never processed): would cover packet 2. + + // Feedback 3: Covers packet 3 + std::vector<PacketTemplate> feedback3_packets = {packets[2]}; + std::optional<TransportPacketsFeedback> adapted_feedback3 = + CreateAndProcessFeedback(feedback3_packets, adapter); + + // Packet 2 is not reported as lost. + EXPECT_FALSE(FindFeedback(adapted_feedback3, 2).has_value()); + + // Feedback 4: Covers packet 4 + std::vector<PacketTemplate> feedback4_packets = {packets[3]}; + std::optional<TransportPacketsFeedback> adapted_feedback4 = + CreateAndProcessFeedback(feedback4_packets, adapter); + + // Both CCFB and TWCC never infer packet 2 as lost unless explicitly skipped + // within the sequence number range of a single received feedback packet. + EXPECT_FALSE(FindFeedback(adapted_feedback4, 2).has_value()); +} + TEST(TransportFeedbackAdapterCongestionFeedbackTest, CongestionControlFeedbackResultHasEcn) { TransportFeedbackAdapter adapter; @@ -813,6 +866,61 @@ } TEST(TransportFeedbackAdapterCongestionFeedbackTest, + CongestionControlFeedbackResultReportsImplicitlyLostPacketOnce) { + TransportFeedbackAdapter adapter; + + PacketTemplate packets[] = {{.ssrc = 1, + .transport_sequence_number = 1, + .rtp_sequence_number = 101, + .send_timestamp = Timestamp::Millis(100)}, + {.ssrc = 2, + .transport_sequence_number = 2, + .rtp_sequence_number = 201, + .send_timestamp = Timestamp::Millis(110)}, + {.ssrc = 1, + .transport_sequence_number = 3, + .rtp_sequence_number = 102, + .send_timestamp = Timestamp::Millis(120)}, + {.ssrc = 2, + .transport_sequence_number = 4, + .rtp_sequence_number = 202, + .send_timestamp = Timestamp::Millis(120)}}; + + for (const PacketTemplate& packet : packets) { + adapter.AddPacket(CreatePacketToSend(packet), packet.pacing_info, + /*overhead=*/0u, TimeNow()); + + adapter.ProcessSentPacket(SentPacketInfo(packet.transport_sequence_number, + packet.send_timestamp.ms())); + } + + // Produce feedback where 2nd packet is lost. + packets[0].receive_timestamp = Timestamp::Millis(200); + packets[2].receive_timestamp = Timestamp::Millis(220); + std::vector<PacketTemplate> feedback_1 = {packets[0], packets[2]}; + std::optional<PacketResult> packet_feedback = FindFeedback( + adapter.ProcessCongestionControlFeedback( + BuildRtcpCongestionControlFeedbackPacket(feedback_1), TimeNow()), + /*transport_sequence_number=*/2); + ASSERT_TRUE(packet_feedback.has_value()); + EXPECT_FALSE(packet_feedback->IsReceived()); + + // Produce feedback where 2nd packet is reported lost and 4th packet is + // received. + packets[3].receive_timestamp = Timestamp::Millis(240); + std::vector<PacketTemplate> feedback_2 = {packets[1], packets[3]}; + rtcp::CongestionControlFeedback rtcp_feedback = + BuildRtcpCongestionControlFeedbackPacket(feedback_2); + // 2nd packet is still lost. + packet_feedback = FindFeedback( + adapter.ProcessCongestionControlFeedback(rtcp_feedback, TimeNow()), + /*transport_sequence_number=*/2); + ASSERT_TRUE(packet_feedback.has_value()); + EXPECT_FALSE(packet_feedback->IsReceived()); + EXPECT_FALSE(packet_feedback->reported_lost_for_the_first_time); +} + +TEST(TransportFeedbackAdapterCongestionFeedbackTest, CongestionControlFeedbackResultReportsRecoveredPacketOnce) { TransportFeedbackAdapter adapter;
diff --git a/modules/congestion_controller/scream/scream_v2.cc b/modules/congestion_controller/scream/scream_v2.cc index fc5d533..ea45876 100644 --- a/modules/congestion_controller/scream/scream_v2.cc +++ b/modules/congestion_controller/scream/scream_v2.cc
@@ -45,7 +45,7 @@ bool HasLostPackets(const TransportPacketsFeedback& msg) { for (const auto& packet : msg.PacketsWithFeedback()) { - if (!packet.IsReceived()) { + if (!packet.IsReceived() && packet.reported_lost_for_the_first_time) { return true; } }
diff --git a/modules/congestion_controller/scream/scream_v2_unittest.cc b/modules/congestion_controller/scream/scream_v2_unittest.cc index 84ccf60..39d8936 100644 --- a/modules/congestion_controller/scream/scream_v2_unittest.cc +++ b/modules/congestion_controller/scream/scream_v2_unittest.cc
@@ -161,6 +161,43 @@ EXPECT_GT(scream_1.ref_window(), scream_2.ref_window()); } +TEST(ScreamV2Test, ReferenceWindowDecreaseIfPacketsAreLostForTheFirstTime) { + SimulatedClock clock(Timestamp::Seconds(1'234)); + Environment env = CreateTestEnvironment({.time = &clock}); + ScreamV2 scream(env); + + TransportPacketsFeedback feedback = + CreateFeedback(clock.CurrentTime(), /*rtt=*/TimeDelta::Millis(10), + /*number_of_ect1_packets=*/20, + /*number_of_packets_in_flight=*/20); + + scream.OnTransportPacketsFeedback(feedback); + DataSize ref_window = scream.ref_window(); + clock.AdvanceTime(TimeDelta::Millis(25)); + + TransportPacketsFeedback loss_feedback = + CreateFeedback(clock.CurrentTime(), /*rtt=*/TimeDelta::Millis(10), + /*number_of_ect1_packets=*/5, + /*number_of_packets_in_flight=*/5); + loss_feedback.packet_feedbacks[3].receive_time = Timestamp::PlusInfinity(); + loss_feedback.packet_feedbacks[3].reported_lost_for_the_first_time = true; + + scream.OnTransportPacketsFeedback(loss_feedback); + EXPECT_LT(scream.ref_window(), ref_window); + ref_window = scream.ref_window(); + + clock.AdvanceTime(TimeDelta::Millis(25)); + TransportPacketsFeedback loss_feedback2 = + CreateFeedback(clock.CurrentTime(), /*rtt=*/TimeDelta::Millis(10), + /*number_of_ect1_packets=*/5, + /*number_of_packets_in_flight=*/5); + loss_feedback2.packet_feedbacks[0].receive_time = Timestamp::PlusInfinity(); + loss_feedback2.packet_feedbacks[0].reported_lost_for_the_first_time = false; + + scream.OnTransportPacketsFeedback(loss_feedback2); + EXPECT_GE(scream.ref_window(), ref_window); +} + TEST(ScreamV2Test, ReferenceWindowIncreaseToDataInflight) { SimulatedClock clock(Timestamp::Seconds(1'234)); Environment env = CreateTestEnvironment({.time = &clock});
diff --git a/modules/congestion_controller/scream/test/cc_feedback_generator.cc b/modules/congestion_controller/scream/test/cc_feedback_generator.cc index b754a20..1da5c34 100644 --- a/modules/congestion_controller/scream/test/cc_feedback_generator.cc +++ b/modules/congestion_controller/scream/test/cc_feedback_generator.cc
@@ -146,6 +146,7 @@ PacketResult packet_result; packet_result.sent_packet.send_time = packets_in_flight_.front().send_time(); + packet_result.reported_lost_for_the_first_time = true; packet_result.sent_packet.size = packets_in_flight_.front().packet_size(); packets_in_flight_.pop_front(); feedback.packet_feedbacks.push_back(packet_result);