From 776866774fd739996b60a6b683f7aa9fa3addd65 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Thu, 19 Dec 2024 07:58:06 +0000 Subject: [PATCH] Propagate desicion if RTP packet should be ECT(1) marked to socket With this CL, the decision if an RTP packet should be sent as ect(1) is made in RtpControllerSend depending on if RFC 8888 has been negotiated and if CCFB is received with ECN enabled. Since webrtc does not yet adapt to ECN feedback, packets are sent as ECT(1) until the first feedback is received. Change-Id: Iddf63849328afbe54a7c8f921f2e8db134aeff6a Bug: webrtc:42225697 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/367388 Commit-Queue: Per Kjellander Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#43609} --- api/call/transport.h | 1 + api/transport/network_types.h | 1 + call/call.cc | 3 +- call/rtp_transport_controller_send.cc | 38 +++++--- call/rtp_transport_controller_send.h | 6 ++ .../rtp_transport_controller_send_interface.h | 3 + .../test/mock_rtp_transport_controller_send.h | 4 + media/base/media_channel_impl.cc | 3 +- modules/congestion_controller/rtp/BUILD.gn | 1 + .../rtp/transport_feedback_adapter.cc | 11 ++- .../rtp/transport_feedback_adapter.h | 3 +- .../transport_feedback_adapter_unittest.cc | 80 +++++++++++++---- modules/pacing/packet_router.cc | 6 +- modules/pacing/packet_router.h | 7 +- modules/pacing/packet_router_unittest.cc | 30 ++++++- modules/rtp_rtcp/source/rtp_packet_to_send.h | 6 ++ modules/rtp_rtcp/source/rtp_sender_egress.cc | 1 + test/peer_scenario/tests/BUILD.gn | 1 + test/peer_scenario/tests/l4s_test.cc | 87 ++++++++++++++++++- 19 files changed, 252 insertions(+), 40 deletions(-) diff --git a/api/call/transport.h b/api/call/transport.h index eb2059f34d..e279a88cbb 100644 --- a/api/call/transport.h +++ b/api/call/transport.h @@ -31,6 +31,7 @@ struct PacketOptions { bool is_media = true; bool included_in_feedback = false; bool included_in_allocation = false; + bool send_as_ect1 = false; // Whether this packet can be part of a packet batch at lower levels. bool batchable = false; // Whether this packet is the last of a batch. diff --git a/api/transport/network_types.h b/api/transport/network_types.h index 432b1fc17c..d229d77bfc 100644 --- a/api/transport/network_types.h +++ b/api/transport/network_types.h @@ -179,6 +179,7 @@ struct RTC_EXPORT TransportPacketsFeedback { Timestamp feedback_time = Timestamp::PlusInfinity(); DataSize data_in_flight = DataSize::Zero(); + bool transport_supports_ecn = false; std::vector packet_feedbacks; // Arrival times for messages without send time information. diff --git a/call/call.cc b/call/call.cc index 6816212164..86d824b2b0 100644 --- a/call/call.cc +++ b/call/call.cc @@ -1195,8 +1195,7 @@ Call::Stats Call::GetStats() const { void Call::EnableSendCongestionControlFeedbackAccordingToRfc8888() { receive_side_cc_.EnableSendCongestionControlFeedbackAccordingToRfc8888(); - transport_send_->packet_router() - ->EnableCongestionControlFeedbackAccordingToRfc8888(); + transport_send_->EnableCongestionControlFeedbackAccordingToRfc8888(); } int Call::FeedbackAccordingToRfc8888Count() { diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index cd6a61f5c2..23f9b91483 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -635,6 +635,13 @@ void RtpTransportControllerSend::NotifyBweOfPacedSentPacket( packet, pacing_info, transport_overhead_bytes_per_packet_, creation_time); } +void RtpTransportControllerSend:: + EnableCongestionControlFeedbackAccordingToRfc8888() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + transport_is_ecn_capable_ = true; + packet_router_.ConfigureForRfc8888Feedback(transport_is_ecn_capable_); +} + void RtpTransportControllerSend::OnTransportFeedback( Timestamp receive_time, const rtcp::TransportFeedback& feedback) { @@ -645,11 +652,7 @@ void RtpTransportControllerSend::OnTransportFeedback( transport_feedback_adapter_.ProcessTransportFeedback(feedback, receive_time); if (feedback_msg) { - if (controller_) - PostUpdates(controller_->OnTransportPacketsFeedback(*feedback_msg)); - - // Only update outstanding data if any packet is first time acked. - UpdateCongestedState(); + HandleTransportPacketsFeedback(*feedback_msg); } } @@ -665,14 +668,29 @@ void RtpTransportControllerSend::OnCongestionControlFeedback( transport_feedback_adapter_.ProcessCongestionControlFeedback( feedback, receive_time); if (feedback_msg) { - if (controller_) - PostUpdates(controller_->OnTransportPacketsFeedback(*feedback_msg)); - - // Only update outstanding data if any packet is first time acked. - UpdateCongestedState(); + HandleTransportPacketsFeedback(*feedback_msg); } } +void RtpTransportControllerSend::HandleTransportPacketsFeedback( + const TransportPacketsFeedback& feedback) { + if (transport_is_ecn_capable_) { + // If transport does not support ECN, packets should not be sent as ECT(1). + // TODO: bugs.webrtc.org/42225697 - adapt to ECN feedback and continue to + // send packets as ECT(1) if transport is ECN capable. + transport_is_ecn_capable_ = false; + RTC_LOG(LS_INFO) << " Transport is " + << (feedback.transport_supports_ecn ? "" : " not ") + << " ECN capable. Stop sending ECT(1)."; + packet_router_.ConfigureForRfc8888Feedback(transport_is_ecn_capable_); + } + if (controller_) + PostUpdates(controller_->OnTransportPacketsFeedback(feedback)); + + // Only update outstanding data if any packet is first time acked. + UpdateCongestedState(); +} + void RtpTransportControllerSend::OnRemoteNetworkEstimate( NetworkStateEstimate estimate) { RTC_DCHECK_RUN_ON(&sequence_checker_); diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index 3d4185b07c..1261706b50 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -142,6 +142,9 @@ class RtpTransportControllerSend final return controller_.get(); } + // Called once it's known that the remote end supports RFC 8888. + void EnableCongestionControlFeedbackAccordingToRfc8888() override; + int ReceivedCongestionControlFeedbackCount() const override { RTC_DCHECK_RUN_ON(&sequence_checker_); return feedback_count_; @@ -153,6 +156,8 @@ class RtpTransportControllerSend final private: void MaybeCreateControllers() RTC_RUN_ON(sequence_checker_); + void HandleTransportPacketsFeedback(const TransportPacketsFeedback& feedback) + RTC_RUN_ON(sequence_checker_); void UpdateNetworkAvailability() RTC_RUN_ON(sequence_checker_); void UpdateInitialConstraints(TargetRateConstraints new_contraints) RTC_RUN_ON(sequence_checker_); @@ -237,6 +242,7 @@ class RtpTransportControllerSend final DataSize congestion_window_size_ RTC_GUARDED_BY(sequence_checker_); bool is_congested_ RTC_GUARDED_BY(sequence_checker_); + bool transport_is_ecn_capable_ = false; // Count of feedback messages received. int feedback_count_ RTC_GUARDED_BY(sequence_checker_) = 0; int transport_cc_feedback_count_ RTC_GUARDED_BY(sequence_checker_) = 0; diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index 88b3d88ea3..c216c511f4 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -161,6 +161,9 @@ class RtpTransportControllerSendInterface { virtual void EnsureStarted() = 0; virtual NetworkControllerInterface* GetNetworkController() = 0; + + // Called once it's known that the remote end supports RFC 8888. + virtual void EnableCongestionControlFeedbackAccordingToRfc8888() = 0; // Count of RFC8888 feedback reports received virtual int ReceivedCongestionControlFeedbackCount() const = 0; // Count of transport-cc feedback reports received diff --git a/call/test/mock_rtp_transport_controller_send.h b/call/test/mock_rtp_transport_controller_send.h index c580637635..9db06bcf7d 100644 --- a/call/test/mock_rtp_transport_controller_send.h +++ b/call/test/mock_rtp_transport_controller_send.h @@ -115,6 +115,10 @@ class MockRtpTransportControllerSend GetNetworkController, (), (override)); + MOCK_METHOD(void, + EnableCongestionControlFeedbackAccordingToRfc8888, + (), + (override)); MOCK_METHOD(int, ReceivedCongestionControlFeedbackCount, (), diff --git a/media/base/media_channel_impl.cc b/media/base/media_channel_impl.cc index e354db54a0..9a2d22f986 100644 --- a/media/base/media_channel_impl.cc +++ b/media/base/media_channel_impl.cc @@ -207,7 +207,7 @@ bool MediaChannelUtil::TransportForMediaChannels::SendRtp( included_in_allocation = options.included_in_allocation, batchable = options.batchable, last_packet_in_batch = options.last_packet_in_batch, - is_media = options.is_media, + is_media = options.is_media, ect_1 = options.send_as_ect1, packet = rtc::CopyOnWriteBuffer(packet, kMaxRtpPacketLen)]() mutable { rtc::PacketOptions rtc_options; rtc_options.packet_id = packet_id; @@ -219,6 +219,7 @@ bool MediaChannelUtil::TransportForMediaChannels::SendRtp( rtc_options.info_signaled_after_sent.included_in_allocation = included_in_allocation; rtc_options.info_signaled_after_sent.is_media = is_media; + rtc_options.ecn_1 = ect_1; rtc_options.batchable = batchable; rtc_options.last_packet_in_batch = last_packet_in_batch; DoSendPacket(&packet, false, rtc_options); diff --git a/modules/congestion_controller/rtp/BUILD.gn b/modules/congestion_controller/rtp/BUILD.gn index 9df7fc9e37..302c3dee01 100644 --- a/modules/congestion_controller/rtp/BUILD.gn +++ b/modules/congestion_controller/rtp/BUILD.gn @@ -41,6 +41,7 @@ rtc_library("transport_feedback") { deps = [ "../..:module_api_public", "../../../api:sequence_checker", + "../../../api/transport:ecn_marking", "../../../api/transport:network_control", "../../../api/units:data_size", "../../../api/units:time_delta", diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index 2219645cc0..b7391ee0a2 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -19,6 +19,7 @@ #include #include "absl/algorithm/container.h" +#include "api/transport/ecn_marking.h" #include "api/transport/network_types.h" #include "api/units/data_size.h" #include "api/units/time_delta.h" @@ -247,7 +248,7 @@ TransportFeedbackAdapter::ProcessTransportFeedback( << " packets because they were sent on a different route."; } return ToTransportFeedback(std::move(packet_result_vector), - feedback_receive_time); + feedback_receive_time, /*suports_ecn=*/false); } std::optional @@ -277,6 +278,7 @@ TransportFeedbackAdapter::ProcessCongestionControlFeedback( int ignored_packets = 0; int failed_lookups = 0; + bool supports_ecn = true; std::vector packet_result_vector; for (const rtcp::CongestionControlFeedback::PacketInfo& packet_info : feedback.packets()) { @@ -296,6 +298,7 @@ TransportFeedbackAdapter::ProcessCongestionControlFeedback( result.sent_packet = packet_feedback->sent; if (packet_info.arrival_time_offset.IsFinite()) { result.receive_time = current_offset_ - packet_info.arrival_time_offset; + supports_ecn &= packet_info.ecn != EcnMarking::kNotEct; } result.ecn = packet_info.ecn; packet_result_vector.push_back(result); @@ -318,13 +321,14 @@ TransportFeedbackAdapter::ProcessCongestionControlFeedback( return lhs.sent_packet.sequence_number < rhs.sent_packet.sequence_number; }); return ToTransportFeedback(std::move(packet_result_vector), - feedback_receive_time); + feedback_receive_time, supports_ecn); } std::optional TransportFeedbackAdapter::ToTransportFeedback( std::vector packet_results, - Timestamp feedback_receive_time) { + Timestamp feedback_receive_time, + bool supports_ecn) { TransportPacketsFeedback msg; msg.feedback_time = feedback_receive_time; if (packet_results.empty()) { @@ -332,6 +336,7 @@ TransportFeedbackAdapter::ToTransportFeedback( } msg.packet_feedbacks = std::move(packet_results); msg.data_in_flight = in_flight_.GetOutstandingData(network_route_); + msg.transport_supports_ecn = supports_ecn; return msg; } diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.h b/modules/congestion_controller/rtp/transport_feedback_adapter.h index 970c0f1623..4ef03e573f 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.h +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.h @@ -111,7 +111,8 @@ class TransportFeedbackAdapter { bool received); std::optional ToTransportFeedback( std::vector packet_results, - Timestamp feedback_receive_time); + Timestamp feedback_receive_time, + bool supports_ecn); DataSize pending_untracked_size_ = DataSize::Zero(); Timestamp last_send_time_ = Timestamp::MinusInfinity(); diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index ca7ebc3e2c..0b191f6832 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc @@ -532,27 +532,75 @@ TEST(TransportFeedbackAdapterCongestionFeedbackTest, CongestionControlFeedbackResultHasEcn) { TransportFeedbackAdapter adapter; - PacketTemplate packet = { - .transport_sequence_number = 1, - .rtp_sequence_number = 101, - .packet_size = DataSize::Bytes(200), - .send_timestamp = Timestamp::Millis(100), - .pacing_info = kPacingInfo0, - .receive_timestamp = Timestamp::Millis(200), - }; - adapter.AddPacket(CreatePacketToSend(packet), packet.pacing_info, - /*overhead=*/0u, TimeNow()); - adapter.ProcessSentPacket(rtc::SentPacket(packet.transport_sequence_number, - packet.send_timestamp.ms())); + const PacketTemplate packets[] = { + { + .transport_sequence_number = 1, + .rtp_sequence_number = 101, + .ecn = EcnMarking::kCe, + .send_timestamp = Timestamp::Millis(100), + .receive_timestamp = Timestamp::Millis(200), + }, + { + .transport_sequence_number = 2, + .rtp_sequence_number = 102, + .ecn = EcnMarking::kEct1, + .send_timestamp = Timestamp::Millis(110), + .receive_timestamp = Timestamp::Millis(210), + }}; + + for (const PacketTemplate& packet : packets) { + adapter.AddPacket(CreatePacketToSend(packet), packet.pacing_info, + /*overhead=*/0u, TimeNow()); + + adapter.ProcessSentPacket(rtc::SentPacket(packet.transport_sequence_number, + packet.send_timestamp.ms())); + } - packet.ecn = EcnMarking::kCe; rtcp::CongestionControlFeedback rtcp_feedback = - BuildRtcpCongestionControlFeedbackPacket(rtc::MakeArrayView(&packet, 1)); + BuildRtcpCongestionControlFeedbackPacket(packets); std::optional adapted_feedback = adapter.ProcessCongestionControlFeedback(rtcp_feedback, TimeNow()); - ASSERT_THAT(adapted_feedback->packet_feedbacks, SizeIs(1)); - ASSERT_THAT(adapted_feedback->packet_feedbacks[0].ecn, EcnMarking::kCe); + ASSERT_THAT(adapted_feedback->packet_feedbacks, SizeIs(2)); + EXPECT_THAT(adapted_feedback->packet_feedbacks[0].ecn, EcnMarking::kCe); + EXPECT_THAT(adapted_feedback->packet_feedbacks[1].ecn, EcnMarking::kEct1); + EXPECT_TRUE(adapted_feedback->transport_supports_ecn); +} + +TEST(TransportFeedbackAdapterCongestionFeedbackTest, + ReportTransportDoesNotSupportEcnIfFeedbackContainNotEctPacket) { + TransportFeedbackAdapter adapter; + + const PacketTemplate packets[] = { + { + .transport_sequence_number = 1, + .rtp_sequence_number = 101, + .ecn = EcnMarking::kCe, + .send_timestamp = Timestamp::Millis(100), + .receive_timestamp = Timestamp::Millis(200), + }, + { + .transport_sequence_number = 2, + .rtp_sequence_number = 102, + .ecn = EcnMarking::kNotEct, + .send_timestamp = Timestamp::Millis(110), + .receive_timestamp = Timestamp::Millis(210), + }}; + + for (const PacketTemplate& packet : packets) { + adapter.AddPacket(CreatePacketToSend(packet), packet.pacing_info, + /*overhead=*/0u, TimeNow()); + + adapter.ProcessSentPacket(rtc::SentPacket(packet.transport_sequence_number, + packet.send_timestamp.ms())); + } + + rtcp::CongestionControlFeedback rtcp_feedback = + BuildRtcpCongestionControlFeedbackPacket(packets); + std::optional adapted_feedback = + adapter.ProcessCongestionControlFeedback(rtcp_feedback, TimeNow()); + EXPECT_FALSE(adapted_feedback->transport_supports_ecn); + ASSERT_THAT(adapted_feedback->packet_feedbacks, SizeIs(2)); } } // namespace webrtc diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index a862530617..7fcef932a4 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -87,9 +87,10 @@ void PacketRouter::RegisterNotifyBweCallback( notify_bwe_callback_ = std::move(callback); } -void PacketRouter::EnableCongestionControlFeedbackAccordingToRfc8888() { +void PacketRouter::ConfigureForRfc8888Feedback(bool send_rtp_packets_as_ect1) { RTC_DCHECK_RUN_ON(&thread_checker_); use_cc_feedback_according_to_rfc8888_ = true; + send_rtp_packets_as_ect1_ = send_rtp_packets_as_ect1; } void PacketRouter::AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, @@ -203,6 +204,9 @@ void PacketRouter::SendPacket(std::unique_ptr packet, packet->HasExtension()) { packet->set_transport_sequence_number(transport_seq_++); } + if (send_rtp_packets_as_ect1_) { + packet->set_send_as_ect1(); + } rtp_module->AssignSequenceNumber(*packet); if (notify_bwe_callback_) { notify_bwe_callback_(*packet, cluster_info); diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index bc3bc70280..33b6069d05 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -55,7 +55,11 @@ class PacketRouter : public PacingController::PacketSender { void RegisterNotifyBweCallback( absl::AnyInvocable callback); - void EnableCongestionControlFeedbackAccordingToRfc8888(); + + // Ensures that PacketRouter generates transport sequence numbers for all RTP + // packets. If `send_rtp_packets_as_ect1` is true, packets will be requested + // to be sent as ect1. + void ConfigureForRfc8888Feedback(bool send_rtp_packets_as_ect1); void AddSendRtpModule(RtpRtcpInterface* rtp_module, bool remb_candidate); void RemoveSendRtpModule(RtpRtcpInterface* rtp_module); @@ -119,6 +123,7 @@ class PacketRouter : public PacingController::PacketSender { uint64_t transport_seq_ RTC_GUARDED_BY(thread_checker_); bool use_cc_feedback_according_to_rfc8888_ RTC_GUARDED_BY(thread_checker_) = false; + bool send_rtp_packets_as_ect1_ RTC_GUARDED_BY(thread_checker_) = false; absl::AnyInvocable notify_bwe_callback_ RTC_GUARDED_BY(thread_checker_) = nullptr; diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 0db80f20c3..33ba74f747 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -423,7 +423,7 @@ TEST_F(PacketRouterTest, EXPECT_CALL(rtp_1, CanSendPacket).WillRepeatedly(Return(true)); packet_router.AddSendRtpModule(&rtp_1, false); - packet_router.EnableCongestionControlFeedbackAccordingToRfc8888(); + packet_router.ConfigureForRfc8888Feedback(/*send_rtp_packets_as_ect1=*/false); auto packet = BuildRtpPacket(kSsrc1); EXPECT_CALL(notify_bwe_callback, Call) @@ -437,6 +437,34 @@ TEST_F(PacketRouterTest, packet_router.RemoveSendRtpModule(&rtp_1); } +TEST_F(PacketRouterTest, SendPacketsAsEct1IfConfigured) { + const uint16_t kSsrc1 = 1234; + PacketRouter packet_router; + NiceMock rtp_1; + ON_CALL(rtp_1, SSRC()).WillByDefault(Return(kSsrc1)); + ON_CALL(rtp_1, CanSendPacket).WillByDefault(Return(kSsrc1)); + + packet_router.AddSendRtpModule(&rtp_1, false); + packet_router.ConfigureForRfc8888Feedback(/*send_rtp_packets_as_ect1=*/true); + + testing::Sequence s; + EXPECT_CALL( + rtp_1, + SendPacket(Pointee(Property(&RtpPacketToSend::send_as_ect1, true)), _)) + .InSequence(s); + EXPECT_CALL( + rtp_1, + SendPacket(Pointee(Property(&RtpPacketToSend::send_as_ect1, false)), _)) + .InSequence(s); + + packet_router.SendPacket(BuildRtpPacket(kSsrc1), PacedPacketInfo()); + packet_router.ConfigureForRfc8888Feedback(/*send_rtp_packets_as_ect1=*/false); + packet_router.SendPacket(BuildRtpPacket(kSsrc1), PacedPacketInfo()); + + packet_router.OnBatchComplete(); + packet_router.RemoveSendRtpModule(&rtp_1); +} + TEST_F(PacketRouterTest, SendTransportFeedback) { NiceMock rtp_1; NiceMock rtp_2; diff --git a/modules/rtp_rtcp/source/rtp_packet_to_send.h b/modules/rtp_rtcp/source/rtp_packet_to_send.h index 7eaf1cd3ba..032f52b061 100644 --- a/modules/rtp_rtcp/source/rtp_packet_to_send.h +++ b/modules/rtp_rtcp/source/rtp_packet_to_send.h @@ -147,6 +147,11 @@ class RtpPacketToSend : public RtpPacket { void set_transport_sequence_number(int64_t transport_sequence_number) { transport_sequence_number_ = transport_sequence_number; } + // Transport is capable of handling explicit congestion notification and the + // RTP packet should be sent as ect(1) + // https://www.rfc-editor.org/rfc/rfc9331.html + bool send_as_ect1() const { return send_as_ect1_; } + void set_send_as_ect1() { send_as_ect1_ = true; } private: webrtc::Timestamp capture_time_ = webrtc::Timestamp::Zero(); @@ -161,6 +166,7 @@ class RtpPacketToSend : public RtpPacket { bool is_key_frame_ = false; bool fec_protect_packet_ = false; bool is_red_ = false; + bool send_as_ect1_ = false; std::optional time_in_send_queue_; }; diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc index 2d3ecf26ac..1f110d132b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -302,6 +302,7 @@ void RtpSenderEgress::CompleteSendPacket(const Packet& compound_packet, send_packet_observer_->OnSendPacket(packet_id, packet->capture_time(), packet->Ssrc()); } + options.send_as_ect1 = packet->send_as_ect1(); options.batchable = enable_send_packet_batching_ && !is_audio_; options.last_packet_in_batch = last_in_batch; const bool send_success = SendPacketToNetwork(*packet, options, pacing_info); diff --git a/test/peer_scenario/tests/BUILD.gn b/test/peer_scenario/tests/BUILD.gn index 1f6218ea11..1ad0e00ad1 100644 --- a/test/peer_scenario/tests/BUILD.gn +++ b/test/peer_scenario/tests/BUILD.gn @@ -24,6 +24,7 @@ if (rtc_include_tests) { "../../:field_trial", "../../:test_support", "../../../api:rtc_stats_api", + "../../../api/transport:ecn_marking", "../../../api/units:data_rate", "../../../api/units:time_delta", "../../../media:stream_params", diff --git a/test/peer_scenario/tests/l4s_test.cc b/test/peer_scenario/tests/l4s_test.cc index f41c3bbea3..69c4978147 100644 --- a/test/peer_scenario/tests/l4s_test.cc +++ b/test/peer_scenario/tests/l4s_test.cc @@ -47,6 +47,26 @@ class RtcpFeedbackCounter { } if (header.fmt() == rtcp::CongestionControlFeedback::kFeedbackMessageType) { ++congestion_control_feedback_; + rtcp::CongestionControlFeedback fb; + ASSERT_TRUE(fb.Parse(header)); + for (const rtcp::CongestionControlFeedback::PacketInfo& info : + fb.packets()) { + switch (info.ecn) { + case EcnMarking::kNotEct: + ++not_ect_; + break; + case EcnMarking::kEct0: + // Not used. + RTC_CHECK_NOTREACHED(); + break; + case EcnMarking::kEct1: + // ECN-Capable Transport + ++ect1_; + break; + case EcnMarking::kCe: + ++ce_; + } + } } if (header.fmt() == rtcp::TransportFeedback::kFeedbackMessageType) { ++transport_sequence_number_feedback_; @@ -59,10 +79,16 @@ class RtcpFeedbackCounter { int FeedbackAccordingToTransportCc() const { return transport_sequence_number_feedback_; } + int not_ect() const { return not_ect_; } + int ect1() const { return ect1_; } + int ce() const { return ce_; } private: int congestion_control_feedback_ = 0; int transport_sequence_number_feedback_ = 0; + int not_ect_ = 0; + int ect1_ = 0; + int ce_ = 0; }; rtc::scoped_refptr GetStatsAndProcess( @@ -102,14 +128,12 @@ TEST(L4STest, NegotiateAndUseCcfbIfEnabled) { s.net()->CreateRoute(callee->endpoint(), {ret_node}, caller->endpoint()); RtcpFeedbackCounter send_node_feedback_counter; - send_node->router()->SetFilter([&](const EmulatedIpPacket& packet) { + send_node->router()->SetWatcher([&](const EmulatedIpPacket& packet) { send_node_feedback_counter.Count(packet); - return true; }); RtcpFeedbackCounter ret_node_feedback_counter; - ret_node->router()->SetFilter([&](const EmulatedIpPacket& packet) { + ret_node->router()->SetWatcher([&](const EmulatedIpPacket& packet) { ret_node_feedback_counter.Count(packet); - return true; }); auto signaling = s.ConnectSignaling(caller, callee, {send_node}, {ret_node}); @@ -202,5 +226,60 @@ TEST(L4STest, CallerAdaptToLinkCapacityWithoutEcn) { EXPECT_LT(available_bwe.kbps(), 610); } +TEST(L4STest, SendsEct1UntilFirstFeedback) { + test::ScopedFieldTrials trials( + "WebRTC-RFC8888CongestionControlFeedback/Enabled/"); + PeerScenario s(*test_info_); + + PeerScenarioClient::Config config = PeerScenarioClient::Config(); + config.disable_encryption = true; + PeerScenarioClient* caller = s.CreateClient(config); + PeerScenarioClient* callee = s.CreateClient(config); + + // Create network path from caller to callee. + auto caller_to_callee = s.net()->NodeBuilder().Build().node; + auto callee_to_caller = s.net()->NodeBuilder().Build().node; + s.net()->CreateRoute(caller->endpoint(), {caller_to_callee}, + callee->endpoint()); + s.net()->CreateRoute(callee->endpoint(), {callee_to_caller}, + caller->endpoint()); + + RtcpFeedbackCounter feedback_counter; + std::atomic seen_ect1_feedback = false; + std::atomic seen_not_ect_feedback = false; + callee_to_caller->router()->SetWatcher([&](const EmulatedIpPacket& packet) { + feedback_counter.Count(packet); + if (feedback_counter.ect1() > 0) { + seen_ect1_feedback = true; + RTC_LOG(LS_INFO) << " ect 1" << feedback_counter.ect1(); + } + if (feedback_counter.not_ect() > 0) { + seen_not_ect_feedback = true; + RTC_LOG(LS_INFO) << " not ect" << feedback_counter.not_ect(); + } + }); + + auto signaling = s.ConnectSignaling(caller, callee, {caller_to_callee}, + {callee_to_caller}); + PeerScenarioClient::VideoSendTrackConfig video_conf; + video_conf.generator.squares_video->framerate = 15; + + caller->CreateVideo("VIDEO_1", video_conf); + signaling.StartIceSignaling(); + + std::atomic offer_exchange_done(false); + signaling.NegotiateSdp([&](const SessionDescriptionInterface& answer) { + offer_exchange_done = true; + }); + s.WaitAndProcess(&offer_exchange_done); + + // Wait for first feedback where packets have been sent with ECT(1). Then + // feedback for packets sent as not ECT since currently webrtc does not + // implement adaptation to ECN. + s.WaitAndProcess(&seen_ect1_feedback, TimeDelta::Seconds(1)); + EXPECT_FALSE(seen_not_ect_feedback); + s.WaitAndProcess(&seen_not_ect_feedback, TimeDelta::Seconds(1)); +} + } // namespace } // namespace webrtc