From 15543544b9dbfd0f67e3ad64fc8fa68de334e61a Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Fri, 6 Dec 2024 14:02:12 +0000 Subject: [PATCH] Test that caller adapts to link capacity using CCFB Fix todo to ensure TransportSequence numbers are generated if CCFB according to RFC 8888 is used. Transport sequence numbers are used in BWE algorithms regardless of feedback format. Bug: webrtc:42225697 Change-Id: I6eab95c0241d590f6e7a90d19c82d13ab8692f2b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/370341 Reviewed-by: Harald Alvestrand Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/main@{#43515} --- call/call.cc | 2 + modules/pacing/packet_router.cc | 21 ++++++-- modules/pacing/packet_router.h | 3 ++ modules/pacing/packet_router_unittest.cc | 57 ++++++++++++++++++++++ test/peer_scenario/tests/l4s_test.cc | 62 ++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 3 deletions(-) diff --git a/call/call.cc b/call/call.cc index f9e485ab59..6816212164 100644 --- a/call/call.cc +++ b/call/call.cc @@ -1195,6 +1195,8 @@ Call::Stats Call::GetStats() const { void Call::EnableSendCongestionControlFeedbackAccordingToRfc8888() { receive_side_cc_.EnableSendCongestionControlFeedbackAccordingToRfc8888(); + transport_send_->packet_router() + ->EnableCongestionControlFeedbackAccordingToRfc8888(); } int Call::FeedbackAccordingToRfc8888Count() { diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index f85a9af9bc..a862530617 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -87,6 +87,11 @@ void PacketRouter::RegisterNotifyBweCallback( notify_bwe_callback_ = std::move(callback); } +void PacketRouter::EnableCongestionControlFeedbackAccordingToRfc8888() { + RTC_DCHECK_RUN_ON(&thread_checker_); + use_cc_feedback_according_to_rfc8888_ = true; +} + void PacketRouter::AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, uint32_t ssrc) { RTC_DCHECK_RUN_ON(&thread_checker_); @@ -183,9 +188,19 @@ void PacketRouter::SendPacket(std::unique_ptr packet, RTC_LOG(LS_WARNING) << "Failed to send packet, Not sending media"; return; } - // TODO(bugs.webrtc.org/15368): Even if the TransportSequenceNumber extension - // is not negotiated, we will need the transport sequence number for BWE. - if (packet->HasExtension()) { + + // Transport sequence numbers are used if send side bandwidth estimation is + // used. Send side BWE relies on RTCP feedback either using format described + // in RFC 8888 or + // https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01. + // If RFC 8888 feedback is used, a transport + // sequence number is created for all RTP packets, but not sent in the RTP + // packet. Otherwise, the transport sequence number is only created + // if the TransportSequenceNumber header extension is negotiated for the + // specific media type. Historically, webrtc only used TransportSequenceNumber + // on video packets. + if (use_cc_feedback_according_to_rfc8888_ || + packet->HasExtension()) { packet->set_transport_sequence_number(transport_seq_++); } rtp_module->AssignSequenceNumber(*packet); diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index be0617dfdd..bc3bc70280 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -55,6 +55,7 @@ class PacketRouter : public PacingController::PacketSender { void RegisterNotifyBweCallback( absl::AnyInvocable callback); + void EnableCongestionControlFeedbackAccordingToRfc8888(); void AddSendRtpModule(RtpRtcpInterface* rtp_module, bool remb_candidate); void RemoveSendRtpModule(RtpRtcpInterface* rtp_module); @@ -116,6 +117,8 @@ class PacketRouter : public PacingController::PacketSender { RTC_GUARDED_BY(thread_checker_); uint64_t transport_seq_ RTC_GUARDED_BY(thread_checker_); + bool use_cc_feedback_according_to_rfc8888_ 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 5a254b850f..0db80f20c3 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -380,6 +380,63 @@ TEST_F(PacketRouterTest, AllocatesTransportSequenceNumbers) { packet_router.RemoveSendRtpModule(&rtp_1); } +TEST_F(PacketRouterTest, + DoesNotAllocateTransportSequenceNumberWithoutExtension) { + const uint16_t kSsrc1 = 1234; + + PacketRouter packet_router; + testing::MockFunction + notify_bwe_callback; + NiceMock rtp_1; + packet_router.RegisterNotifyBweCallback(notify_bwe_callback.AsStdFunction()); + + EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1)); + EXPECT_CALL(rtp_1, CanSendPacket).WillRepeatedly(Return(true)); + + packet_router.AddSendRtpModule(&rtp_1, false); + + auto packet = BuildRtpPacket(kSsrc1); + EXPECT_CALL(notify_bwe_callback, Call) + .WillOnce([](const RtpPacketToSend& packet, + const PacedPacketInfo& /* pacing_info */) { + EXPECT_EQ(packet.transport_sequence_number(), std::nullopt); + }); + packet_router.SendPacket(std::move(packet), PacedPacketInfo()); + + packet_router.OnBatchComplete(); + packet_router.RemoveSendRtpModule(&rtp_1); +} + +TEST_F(PacketRouterTest, + AllocateTransportSequenceNumberWithoutExtensionIfRfc8888Enabled) { + const uint16_t kSsrc1 = 1234; + + PacketRouter packet_router; + testing::MockFunction + notify_bwe_callback; + NiceMock rtp_1; + packet_router.RegisterNotifyBweCallback(notify_bwe_callback.AsStdFunction()); + + EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1)); + EXPECT_CALL(rtp_1, CanSendPacket).WillRepeatedly(Return(true)); + + packet_router.AddSendRtpModule(&rtp_1, false); + packet_router.EnableCongestionControlFeedbackAccordingToRfc8888(); + + auto packet = BuildRtpPacket(kSsrc1); + EXPECT_CALL(notify_bwe_callback, Call) + .WillOnce([](const RtpPacketToSend& packet, + const PacedPacketInfo& /* pacing_info */) { + EXPECT_EQ(packet.transport_sequence_number(), 1); + }); + packet_router.SendPacket(std::move(packet), PacedPacketInfo()); + + packet_router.OnBatchComplete(); + packet_router.RemoveSendRtpModule(&rtp_1); +} + TEST_F(PacketRouterTest, SendTransportFeedback) { NiceMock rtp_1; NiceMock rtp_2; diff --git a/test/peer_scenario/tests/l4s_test.cc b/test/peer_scenario/tests/l4s_test.cc index a91dd94410..f41c3bbea3 100644 --- a/test/peer_scenario/tests/l4s_test.cc +++ b/test/peer_scenario/tests/l4s_test.cc @@ -10,12 +10,15 @@ #include +#include "api/stats/rtcstats_objects.h" +#include "api/units/data_rate.h" #include "api/units/time_delta.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet/congestion_control_feedback.h" #include "modules/rtp_rtcp/source/rtcp_packet/rtpfb.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "modules/rtp_rtcp/source/rtp_util.h" +#include "pc/test/mock_peer_connection_observers.h" #include "test/create_frame_generator_capturer.h" #include "test/field_trial.h" #include "test/gmock.h" @@ -62,6 +65,26 @@ class RtcpFeedbackCounter { int transport_sequence_number_feedback_ = 0; }; +rtc::scoped_refptr GetStatsAndProcess( + PeerScenario& s, + PeerScenarioClient* client) { + auto stats_collector = + rtc::make_ref_counted(); + client->pc()->GetStats(stats_collector.get()); + s.ProcessMessages(TimeDelta::Millis(0)); + RTC_CHECK(stats_collector->called()); + return stats_collector->report(); +} + +DataRate GetAvailableSendBitrate( + const rtc::scoped_refptr& report) { + auto stats = report->GetStatsOfType(); + if (stats.empty()) { + return DataRate::Zero(); + } + return DataRate::BitsPerSec(*stats[0]->available_outgoing_bitrate); +} + TEST(L4STest, NegotiateAndUseCcfbIfEnabled) { test::ScopedFieldTrials trials( "WebRTC-RFC8888CongestionControlFeedback/Enabled/"); @@ -140,5 +163,44 @@ TEST(L4STest, NegotiateAndUseCcfbIfEnabled) { EXPECT_EQ(ret_node_feedback_counter.FeedbackAccordingToTransportCc(), 0); } +TEST(L4STest, CallerAdaptToLinkCapacityWithoutEcn) { + test::ScopedFieldTrials trials( + "WebRTC-RFC8888CongestionControlFeedback/Enabled/"); + PeerScenario s(*test_info_); + + PeerScenarioClient::Config config = PeerScenarioClient::Config(); + PeerScenarioClient* caller = s.CreateClient(config); + PeerScenarioClient* callee = s.CreateClient(config); + + auto caller_to_callee = s.net() + ->NodeBuilder() + .capacity(DataRate::KilobitsPerSec(600)) + .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()); + + 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); + s.ProcessMessages(TimeDelta::Seconds(3)); + DataRate available_bwe = + GetAvailableSendBitrate(GetStatsAndProcess(s, caller)); + EXPECT_GT(available_bwe.kbps(), 500); + EXPECT_LT(available_bwe.kbps(), 610); +} + } // namespace } // namespace webrtc