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