From 45a985c71d8c0a999d28b20303baf0b5ddf5bb8c Mon Sep 17 00:00:00 2001 From: Joachim Reiersen Date: Thu, 24 Aug 2023 10:00:01 -0700 Subject: [PATCH] Check use_rtx() in PeerConnectionFactory::GetRtpSenderCapabilities Following https://webrtc-review.googlesource.com/c/src/+/262666, use_rtx() was checked in PeerConnectionFactory::GetRtpReceiverCapabilities but was missed in GetRtpSenderCapabilities. Therefore clients that hardcode use_rtx = false end up in an inconsistent state where RTX is not fully disabled. Bug: None Change-Id: Ice5f29a77c59e9081f9dd72c13c819024a34a7dd Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/316243 Reviewed-by: Harald Alvestrand Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#40625} --- pc/connection_context.cc | 3 +- pc/connection_context.h | 9 ++- pc/peer_connection_factory.cc | 2 +- pc/peer_connection_factory_unittest.cc | 82 ++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 3 deletions(-) diff --git a/pc/connection_context.cc b/pc/connection_context.cc index ec6f21cc13..661550e2d4 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -107,7 +107,8 @@ ConnectionContext::ConnectionContext( sctp_factory_( MaybeCreateSctpFactory(std::move(dependencies->sctp_factory), network_thread(), - *trials_.get())) { + *trials_.get())), + use_rtx_(true) { RTC_DCHECK_RUN_ON(signaling_thread_); RTC_DCHECK(!(default_network_manager_ && network_monitor_factory_)) << "You can't set both network_manager and network_monitor_factory."; diff --git a/pc/connection_context.h b/pc/connection_context.h index 0fe20c7890..38a6f8e514 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -100,7 +100,10 @@ class ConnectionContext final // use RTX, but so far, no code has been found that sets it to false. // Kept in the API in order to ease introduction if we want to resurrect // the functionality. - bool use_rtx() { return true; } + bool use_rtx() { return use_rtx_; } + + // For use by tests. + void set_use_rtx(bool use_rtx) { use_rtx_ = use_rtx; } protected: explicit ConnectionContext(PeerConnectionFactoryDependencies* dependencies); @@ -139,6 +142,10 @@ class ConnectionContext final std::unique_ptr default_socket_factory_ RTC_GUARDED_BY(signaling_thread_); std::unique_ptr const sctp_factory_; + + // Controls whether to announce support for the the rfc4588 payload format + // for retransmitted video packets. + bool use_rtx_; }; } // namespace webrtc diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index 67874b88d6..54fabce6ac 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -134,7 +134,7 @@ RtpCapabilities PeerConnectionFactory::GetRtpSenderCapabilities( } case cricket::MEDIA_TYPE_VIDEO: { cricket::VideoCodecs cricket_codecs; - cricket_codecs = media_engine()->video().send_codecs(); + cricket_codecs = media_engine()->video().send_codecs(context_->use_rtx()); auto extensions = GetDefaultEnabledRtpHeaderExtensions(media_engine()->video()); return ToRtpCapabilities(cricket_codecs, extensions); diff --git a/pc/peer_connection_factory_unittest.cc b/pc/peer_connection_factory_unittest.cc index 9c0ed4bb83..11e232c01f 100644 --- a/pc/peer_connection_factory_unittest.cc +++ b/pc/peer_connection_factory_unittest.cc @@ -10,6 +10,7 @@ #include "pc/peer_connection_factory.h" +#include #include #include #include @@ -21,6 +22,7 @@ #include "api/data_channel_interface.h" #include "api/jsep.h" #include "api/media_stream_interface.h" +#include "api/task_queue/default_task_queue_factory.h" #include "api/test/mock_packet_socket_factory.h" #include "api/video_codecs/video_decoder_factory_template.h" #include "api/video_codecs/video_decoder_factory_template_dav1d_adapter.h" @@ -33,6 +35,7 @@ #include "api/video_codecs/video_encoder_factory_template_libvpx_vp9_adapter.h" #include "api/video_codecs/video_encoder_factory_template_open_h264_adapter.h" #include "media/base/fake_frame_source.h" +#include "media/engine/webrtc_media_engine.h" #include "modules/audio_device/include/audio_device.h" #include "modules/audio_processing/include/audio_processing.h" #include "p2p/base/fake_port_allocator.h" @@ -260,6 +263,46 @@ class PeerConnectionFactoryTest : public ::testing::Test { cricket::FakePortAllocator* raw_port_allocator_; }; +// Since there is no public PeerConnectionFactory API to control RTX usage, need +// to reconstruct factory with our own ConnectionContext. +rtc::scoped_refptr +CreatePeerConnectionFactoryWithRtxDisabled() { + webrtc::PeerConnectionFactoryDependencies pcf_dependencies; + pcf_dependencies.signaling_thread = rtc::Thread::Current(); + pcf_dependencies.worker_thread = rtc::Thread::Current(); + pcf_dependencies.network_thread = rtc::Thread::Current(); + pcf_dependencies.task_queue_factory = CreateDefaultTaskQueueFactory(); + pcf_dependencies.call_factory = CreateCallFactory(); + pcf_dependencies.trials = std::make_unique(); + + cricket::MediaEngineDependencies media_dependencies; + media_dependencies.task_queue_factory = + pcf_dependencies.task_queue_factory.get(); + media_dependencies.adm = rtc::scoped_refptr( + FakeAudioCaptureModule::Create()); + media_dependencies.audio_encoder_factory = + webrtc::CreateBuiltinAudioEncoderFactory(); + media_dependencies.audio_decoder_factory = + webrtc::CreateBuiltinAudioDecoderFactory(); + media_dependencies.video_encoder_factory = + std::make_unique>(); + media_dependencies.video_decoder_factory = + std::make_unique>(), + media_dependencies.trials = pcf_dependencies.trials.get(); + pcf_dependencies.media_engine = + cricket::CreateMediaEngine(std::move(media_dependencies)); + + rtc::scoped_refptr context = + ConnectionContext::Create(&pcf_dependencies); + context->set_use_rtx(false); + return rtc::make_ref_counted(context, + &pcf_dependencies); +} + // Verify creation of PeerConnection using internal ADM, video factory and // internal libjingle threads. // TODO(henrika): disabling this test since relying on real audio can result in @@ -321,6 +364,25 @@ TEST_F(PeerConnectionFactoryTest, CheckRtpSenderVideoCapabilities) { } } +TEST_F(PeerConnectionFactoryTest, CheckRtpSenderRtxEnabledCapabilities) { + webrtc::RtpCapabilities video_capabilities = + factory_->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_VIDEO); + const auto it = std::find_if( + video_capabilities.codecs.begin(), video_capabilities.codecs.end(), + [](const auto& c) { return c.name == cricket::kRtxCodecName; }); + EXPECT_TRUE(it != video_capabilities.codecs.end()); +} + +TEST(PeerConnectionFactoryTestInternal, CheckRtpSenderRtxDisabledCapabilities) { + auto factory = CreatePeerConnectionFactoryWithRtxDisabled(); + webrtc::RtpCapabilities video_capabilities = + factory->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_VIDEO); + const auto it = std::find_if( + video_capabilities.codecs.begin(), video_capabilities.codecs.end(), + [](const auto& c) { return c.name == cricket::kRtxCodecName; }); + EXPECT_TRUE(it == video_capabilities.codecs.end()); +} + TEST_F(PeerConnectionFactoryTest, CheckRtpSenderDataCapabilities) { webrtc::RtpCapabilities data_capabilities = factory_->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_DATA); @@ -354,6 +416,26 @@ TEST_F(PeerConnectionFactoryTest, CheckRtpReceiverVideoCapabilities) { } } +TEST_F(PeerConnectionFactoryTest, CheckRtpReceiverRtxEnabledCapabilities) { + webrtc::RtpCapabilities video_capabilities = + factory_->GetRtpReceiverCapabilities(cricket::MEDIA_TYPE_VIDEO); + const auto it = std::find_if( + video_capabilities.codecs.begin(), video_capabilities.codecs.end(), + [](const auto& c) { return c.name == cricket::kRtxCodecName; }); + EXPECT_TRUE(it != video_capabilities.codecs.end()); +} + +TEST(PeerConnectionFactoryTestInternal, + CheckRtpReceiverRtxDisabledCapabilities) { + auto factory = CreatePeerConnectionFactoryWithRtxDisabled(); + webrtc::RtpCapabilities video_capabilities = + factory->GetRtpReceiverCapabilities(cricket::MEDIA_TYPE_VIDEO); + const auto it = std::find_if( + video_capabilities.codecs.begin(), video_capabilities.codecs.end(), + [](const auto& c) { return c.name == cricket::kRtxCodecName; }); + EXPECT_TRUE(it == video_capabilities.codecs.end()); +} + TEST_F(PeerConnectionFactoryTest, CheckRtpReceiverDataCapabilities) { webrtc::RtpCapabilities data_capabilities = factory_->GetRtpReceiverCapabilities(cricket::MEDIA_TYPE_DATA);