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 <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40625}
diff --git a/pc/connection_context.cc b/pc/connection_context.cc
index ec6f21c..661550e 100644
--- a/pc/connection_context.cc
+++ b/pc/connection_context.cc
@@ -107,7 +107,8 @@
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 0fe20c7..38a6f8e 100644
--- a/pc/connection_context.h
+++ b/pc/connection_context.h
@@ -100,7 +100,10 @@
// 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 @@
std::unique_ptr<rtc::PacketSocketFactory> default_socket_factory_
RTC_GUARDED_BY(signaling_thread_);
std::unique_ptr<SctpTransportFactoryInterface> 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 67874b8..54fabce 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -134,7 +134,7 @@
}
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 9c0ed4b..11e232c 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 <algorithm>
#include <memory>
#include <utility>
#include <vector>
@@ -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 @@
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<PeerConnectionFactoryInterface>
+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<webrtc::FieldTrialBasedConfig>();
+
+ cricket::MediaEngineDependencies media_dependencies;
+ media_dependencies.task_queue_factory =
+ pcf_dependencies.task_queue_factory.get();
+ media_dependencies.adm = rtc::scoped_refptr<webrtc::AudioDeviceModule>(
+ FakeAudioCaptureModule::Create());
+ media_dependencies.audio_encoder_factory =
+ webrtc::CreateBuiltinAudioEncoderFactory();
+ media_dependencies.audio_decoder_factory =
+ webrtc::CreateBuiltinAudioDecoderFactory();
+ media_dependencies.video_encoder_factory =
+ std::make_unique<VideoEncoderFactoryTemplate<
+ LibvpxVp8EncoderTemplateAdapter, LibvpxVp9EncoderTemplateAdapter,
+ OpenH264EncoderTemplateAdapter, LibaomAv1EncoderTemplateAdapter>>();
+ media_dependencies.video_decoder_factory =
+ std::make_unique<VideoDecoderFactoryTemplate<
+ LibvpxVp8DecoderTemplateAdapter, LibvpxVp9DecoderTemplateAdapter,
+ OpenH264DecoderTemplateAdapter, Dav1dDecoderTemplateAdapter>>(),
+ media_dependencies.trials = pcf_dependencies.trials.get();
+ pcf_dependencies.media_engine =
+ cricket::CreateMediaEngine(std::move(media_dependencies));
+
+ rtc::scoped_refptr<webrtc::ConnectionContext> context =
+ ConnectionContext::Create(&pcf_dependencies);
+ context->set_use_rtx(false);
+ return rtc::make_ref_counted<PeerConnectionFactory>(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, 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, 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);
