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}

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.";

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<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

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);

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 @@ 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<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, 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);