Reland "Make cricket::SctpTransportInternalFactory injectable through PeerConnectionFactory Deps"

This is to allow testing without using the singleton sctp library.
cricket::SctpTransportInternalFactory is renamed to webrtc::SctpTransportFactoryInterface and moved to the API folder to follow the API structure.
Tests can use test/pc/sctp/fake_sctp_transport.h to inject a faked data channel implementation.

patch 1 contain the original cl.
patch 2 modifications

Bug: none
Change-Id: Ic088da3eb7d9aada79e6d601dbf2d1aa2be777f6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/182840
Reviewed-by: Taylor <deadbeef@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32024}

api/BUILD.gn

@@ -185,6 +185,7 @@ rtc_library("libjingle_peerconnection_api") {
     "transport:bitrate_settings",
     "transport:enums",
     "transport:network_control",
+    "transport:sctp_transport_factory_interface",
     "transport:webrtc_key_value_config",
     "transport/rtp:rtp_source",
     "units:data_rate",

api/peer_connection_interface.h

@@ -105,6 +105,7 @@
 #include "api/transport/bitrate_settings.h"
 #include "api/transport/enums.h"
 #include "api/transport/network_control.h"
+#include "api/transport/sctp_transport_factory_interface.h"
 #include "api/transport/webrtc_key_value_config.h"
 #include "api/turn_customizer.h"
 #include "media/base/media_config.h"
@@ -1363,6 +1364,7 @@ struct RTC_EXPORT PeerConnectionFactoryDependencies final {
   // used.
   std::unique_ptr<rtc::NetworkMonitorFactory> network_monitor_factory;
   std::unique_ptr<NetEqFactory> neteq_factory;
+  std::unique_ptr<SctpTransportFactoryInterface> sctp_factory;
   std::unique_ptr<WebRtcKeyValueConfig> trials;
 };
 

api/transport/BUILD.gn

@@ -93,6 +93,11 @@ rtc_library("goog_cc") {
   ]
 }
 
+rtc_source_set("sctp_transport_factory_interface") {
+  visibility = [ "*" ]
+  sources = [ "sctp_transport_factory_interface.h" ]
+}
+
 rtc_source_set("stun_types") {
   visibility = [ "*" ]
   sources = [

api/transport/sctp_transport_factory_interface.h

@@ -0,0 +1,42 @@
+/*
+ *  Copyright 2020 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef API_TRANSPORT_SCTP_TRANSPORT_FACTORY_INTERFACE_H_
+#define API_TRANSPORT_SCTP_TRANSPORT_FACTORY_INTERFACE_H_
+
+#include <memory>
+
+// These classes are not part of the API, and are treated as opaque pointers.
+namespace cricket {
+class SctpTransportInternal;
+}  // namespace cricket
+
+namespace rtc {
+class PacketTransportInternal;
+}  // namespace rtc
+
+namespace webrtc {
+
+// Factory class which can be used to allow fake SctpTransports to be injected
+// for testing. An application is not intended to implement this interface nor
+// 'cricket::SctpTransportInternal' because SctpTransportInternal is not
+// guaranteed to remain stable in future WebRTC versions.
+class SctpTransportFactoryInterface {
+ public:
+  virtual ~SctpTransportFactoryInterface() = default;
+
+  // Create an SCTP transport using |channel| for the underlying transport.
+  virtual std::unique_ptr<cricket::SctpTransportInternal> CreateSctpTransport(
+      rtc::PacketTransportInternal* channel) = 0;
+};
+
+}  // namespace webrtc
+
+#endif  // API_TRANSPORT_SCTP_TRANSPORT_FACTORY_INTERFACE_H_

media/BUILD.gn

@@ -426,7 +426,10 @@ rtc_library("rtc_data") {
   }
 
   if (rtc_enable_sctp && rtc_build_usrsctp) {
-    deps += [ "//third_party/usrsctp" ]
+    deps += [
+      "../api/transport:sctp_transport_factory_interface",
+      "//third_party/usrsctp",
+    ]
   }
 }
 

media/sctp/sctp_transport.h

@@ -21,6 +21,7 @@
 #include <vector>
 
 #include "absl/types/optional.h"
+#include "api/transport/sctp_transport_factory_interface.h"
 #include "rtc_base/async_invoker.h"
 #include "rtc_base/buffer.h"
 #include "rtc_base/constructor_magic.h"
@@ -283,7 +284,7 @@ class SctpTransport : public SctpTransportInternal,
   RTC_DISALLOW_COPY_AND_ASSIGN(SctpTransport);
 };
 
-class SctpTransportFactory : public SctpTransportInternalFactory {
+class SctpTransportFactory : public webrtc::SctpTransportFactoryInterface {
  public:
   explicit SctpTransportFactory(rtc::Thread* network_thread)
       : network_thread_(network_thread) {}

media/sctp/sctp_transport_internal.h

@@ -142,18 +142,6 @@ class SctpTransportInternal {
   virtual void set_debug_name_for_testing(const char* debug_name) = 0;
 };
 
-// Factory class which can be used to allow fake SctpTransports to be injected
-// for testing. Or, theoretically, SctpTransportInternal implementations that
-// use something other than usrsctp.
-class SctpTransportInternalFactory {
- public:
-  virtual ~SctpTransportInternalFactory() {}
-
-  // Create an SCTP transport using |channel| for the underlying transport.
-  virtual std::unique_ptr<SctpTransportInternal> CreateSctpTransport(
-      rtc::PacketTransportInternal* channel) = 0;
-};
-
 }  // namespace cricket
 
 #endif  // MEDIA_SCTP_SCTP_TRANSPORT_INTERNAL_H_

pc/BUILD.gn

@@ -446,7 +446,6 @@ if (rtc_include_tests) {
       "test/fake_periodic_video_source.h",
       "test/fake_periodic_video_track_source.h",
       "test/fake_rtc_certificate_generator.h",
-      "test/fake_sctp_transport.h",
       "test/fake_video_track_renderer.h",
       "test/fake_video_track_source.h",
       "test/frame_generator_capturer_video_track_source.h",
@@ -608,6 +607,7 @@ if (rtc_include_tests) {
       "../test:field_trial",
       "../test:fileutils",
       "../test:rtp_test_utils",
+      "../test/pc/sctp:fake_sctp_transport",
       "./scenario_tests:pc_scenario_tests",
       "//third_party/abseil-cpp/absl/algorithm:container",
       "//third_party/abseil-cpp/absl/memory",

pc/jsep_transport_controller.h

@@ -101,7 +101,7 @@ class JsepTransportController : public sigslot::has_slots<> {
     RtcEventLog* event_log = nullptr;
 
     // Factory for SCTP transports.
-    cricket::SctpTransportInternalFactory* sctp_factory = nullptr;
+    SctpTransportFactoryInterface* sctp_factory = nullptr;
   };
 
   // The ICE related events are signaled on the |signaling_thread|.

pc/peer_connection.cc

@@ -1073,7 +1073,6 @@ PeerConnection::~PeerConnection() {
   RTC_LOG(LS_INFO) << "Session: " << session_id() << " is destroyed.";
 
   webrtc_session_desc_factory_.reset();
-  sctp_factory_.reset();
   transport_controller_.reset();
 
   // port_allocator_ lives on the network thread and should be destroyed there.
@@ -1262,8 +1261,6 @@ bool PeerConnection::Initialize(
     }
   }
 
-  sctp_factory_ = factory_->CreateSctpTransportInternalFactory();
-
   if (configuration.enable_rtp_data_channel) {
     // Enable creation of RTP data channels if the kEnableRtpDataChannels is
     // set. It takes precendence over the disable_sctp_data_channels
@@ -1273,7 +1270,7 @@ bool PeerConnection::Initialize(
     // DTLS has to be enabled to use SCTP.
     if (!options.disable_sctp_data_channels && dtls_enabled_) {
       data_channel_controller_.set_data_channel_type(cricket::DCT_SCTP);
-      config.sctp_factory = sctp_factory_.get();
+      config.sctp_factory = factory_->sctp_transport_factory();
     }
   }
 

pc/peer_connection.h

@@ -1274,9 +1274,6 @@ class PeerConnection : public PeerConnectionInternal,
   std::unique_ptr<JsepTransportController>
       transport_controller_;  // TODO(bugs.webrtc.org/9987): Accessed on both
                               // signaling and network thread.
-  std::unique_ptr<cricket::SctpTransportInternalFactory>
-      sctp_factory_;  // TODO(bugs.webrtc.org/9987): Accessed on both
-                      // signaling and network thread.
 
   // |sctp_mid_| is the content name (MID) in SDP.
   // Note: this is used as the data channel MID by both SCTP and data channel

pc/peer_connection_data_channel_unittest.cc

@@ -45,8 +45,8 @@
 #ifdef WEBRTC_ANDROID
 #include "pc/test/android_test_initializer.h"
 #endif
-#include "pc/test/fake_sctp_transport.h"
 #include "rtc_base/virtual_socket_server.h"
+#include "test/pc/sctp/fake_sctp_transport.h"
 
 namespace webrtc {
 
@@ -58,46 +58,20 @@ using ::testing::Values;
 
 namespace {
 
-PeerConnectionFactoryDependencies CreatePeerConnectionFactoryDependencies(
+PeerConnectionFactoryDependencies CreatePeerConnectionFactoryDependencies() {
-    rtc::Thread* network_thread,
-    rtc::Thread* worker_thread,
-    rtc::Thread* signaling_thread,
-    std::unique_ptr<cricket::MediaEngineInterface> media_engine,
-    std::unique_ptr<CallFactoryInterface> call_factory) {
   PeerConnectionFactoryDependencies deps;
-  deps.network_thread = network_thread;
+  deps.network_thread = rtc::Thread::Current();
-  deps.worker_thread = worker_thread;
+  deps.worker_thread = rtc::Thread::Current();
-  deps.signaling_thread = signaling_thread;
+  deps.signaling_thread = rtc::Thread::Current();
   deps.task_queue_factory = CreateDefaultTaskQueueFactory();
-  deps.media_engine = std::move(media_engine);
+  deps.media_engine = std::make_unique<cricket::FakeMediaEngine>();
-  deps.call_factory = std::move(call_factory);
+  deps.call_factory = CreateCallFactory();
+  deps.sctp_factory = std::make_unique<FakeSctpTransportFactory>();
   return deps;
 }
 
 }  // namespace
 
-class PeerConnectionFactoryForDataChannelTest
-    : public rtc::RefCountedObject<PeerConnectionFactory> {
- public:
-  PeerConnectionFactoryForDataChannelTest()
-      : rtc::RefCountedObject<PeerConnectionFactory>(
-            CreatePeerConnectionFactoryDependencies(
-                rtc::Thread::Current(),
-                rtc::Thread::Current(),
-                rtc::Thread::Current(),
-                std::make_unique<cricket::FakeMediaEngine>(),
-                CreateCallFactory())) {}
-
-  std::unique_ptr<cricket::SctpTransportInternalFactory>
-  CreateSctpTransportInternalFactory() {
-    auto factory = std::make_unique<FakeSctpTransportFactory>();
-    last_fake_sctp_transport_factory_ = factory.get();
-    return factory;
-  }
-
-  FakeSctpTransportFactory* last_fake_sctp_transport_factory_ = nullptr;
-};
-
 class PeerConnectionWrapperForDataChannelTest : public PeerConnectionWrapper {
  public:
   using PeerConnectionWrapper::PeerConnectionWrapper;
@@ -155,10 +129,12 @@ class PeerConnectionDataChannelBaseTest : public ::testing::Test {
   WrapperPtr CreatePeerConnection(
       const RTCConfiguration& config,
       const PeerConnectionFactoryInterface::Options factory_options) {
-    rtc::scoped_refptr<PeerConnectionFactoryForDataChannelTest> pc_factory(
+    auto factory_deps = CreatePeerConnectionFactoryDependencies();
-        new PeerConnectionFactoryForDataChannelTest());
+    FakeSctpTransportFactory* fake_sctp_transport_factory =
+        static_cast<FakeSctpTransportFactory*>(factory_deps.sctp_factory.get());
+    rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory =
+        CreateModularPeerConnectionFactory(std::move(factory_deps));
     pc_factory->SetOptions(factory_options);
-    RTC_CHECK(pc_factory->Initialize());
     auto observer = std::make_unique<MockPeerConnectionObserver>();
     RTCConfiguration modified_config = config;
     modified_config.sdp_semantics = sdp_semantics_;
@@ -171,9 +147,7 @@ class PeerConnectionDataChannelBaseTest : public ::testing::Test {
     observer->SetPeerConnectionInterface(pc.get());
     auto wrapper = std::make_unique<PeerConnectionWrapperForDataChannelTest>(
         pc_factory, pc, std::move(observer));
-    RTC_DCHECK(pc_factory->last_fake_sctp_transport_factory_);
+    wrapper->set_sctp_transport_factory(fake_sctp_transport_factory);
-    wrapper->set_sctp_transport_factory(
-        pc_factory->last_fake_sctp_transport_factory_);
     return wrapper;
   }
 

pc/peer_connection_factory.cc

@@ -84,6 +84,7 @@ PeerConnectionFactory::PeerConnectionFactory(
       injected_network_controller_factory_(
           std::move(dependencies.network_controller_factory)),
       neteq_factory_(std::move(dependencies.neteq_factory)),
+      sctp_factory_(std::move(dependencies.sctp_factory)),
       trials_(dependencies.trials ? std::move(dependencies.trials)
                                   : std::make_unique<FieldTrialBasedConfig>()) {
   if (!network_thread_) {
@@ -113,6 +114,13 @@ PeerConnectionFactory::PeerConnectionFactory(
   signaling_thread_->AllowInvokesToThread(network_thread_);
   worker_thread_->AllowInvokesToThread(network_thread_);
   network_thread_->DisallowAllInvokes();
+
+#ifdef HAVE_SCTP
+  if (!sctp_factory_) {
+    sctp_factory_ =
+        std::make_unique<cricket::SctpTransportFactory>(network_thread_);
+  }
+#endif
 }
 
 PeerConnectionFactory::~PeerConnectionFactory() {
@@ -326,15 +334,6 @@ rtc::scoped_refptr<AudioTrackInterface> PeerConnectionFactory::CreateAudioTrack(
   return AudioTrackProxy::Create(signaling_thread_, track);
 }
 
-std::unique_ptr<cricket::SctpTransportInternalFactory>
-PeerConnectionFactory::CreateSctpTransportInternalFactory() {
-#ifdef HAVE_SCTP
-  return std::make_unique<cricket::SctpTransportFactory>(network_thread());
-#else
-  return nullptr;
-#endif
-}
-
 cricket::ChannelManager* PeerConnectionFactory::channel_manager() {
   return channel_manager_.get();
 }

pc/peer_connection_factory.h

@@ -71,8 +71,9 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface {
   bool StartAecDump(FILE* file, int64_t max_size_bytes) override;
   void StopAecDump() override;
 
-  virtual std::unique_ptr<cricket::SctpTransportInternalFactory>
+  SctpTransportFactoryInterface* sctp_transport_factory() {
-  CreateSctpTransportInternalFactory();
+    return sctp_factory_.get();
+  }
 
   virtual cricket::ChannelManager* channel_manager();
 
@@ -125,6 +126,7 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface {
   std::unique_ptr<NetworkControllerFactoryInterface>
       injected_network_controller_factory_;
   std::unique_ptr<NetEqFactory> neteq_factory_;
+  std::unique_ptr<SctpTransportFactoryInterface> sctp_factory_;
   const std::unique_ptr<WebRtcKeyValueConfig> trials_;
 };
 

pc/peer_connection_jsep_unittest.cc

@@ -21,10 +21,10 @@
 #include "pc/test/android_test_initializer.h"
 #endif
 #include "pc/test/fake_audio_capture_module.h"
-#include "pc/test/fake_sctp_transport.h"
 #include "rtc_base/gunit.h"
 #include "rtc_base/virtual_socket_server.h"
 #include "test/gmock.h"
+#include "test/pc/sctp/fake_sctp_transport.h"
 
 // This file contains tests that ensure the PeerConnection's implementation of
 // CreateOffer/CreateAnswer/SetLocalDescription/SetRemoteDescription conform
@@ -41,30 +41,21 @@ using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 using ::testing::Values;
 
-class PeerConnectionFactoryForJsepTest : public PeerConnectionFactory {
+PeerConnectionFactoryDependencies CreatePeerConnectionFactoryDependencies() {
- public:
+  PeerConnectionFactoryDependencies dependencies;
-  PeerConnectionFactoryForJsepTest()
+  dependencies.worker_thread = rtc::Thread::Current();
-      : PeerConnectionFactory([] {
+  dependencies.network_thread = rtc::Thread::Current();
-          PeerConnectionFactoryDependencies dependencies;
+  dependencies.signaling_thread = rtc::Thread::Current();
-          dependencies.worker_thread = rtc::Thread::Current();
+  dependencies.task_queue_factory = CreateDefaultTaskQueueFactory();
-          dependencies.network_thread = rtc::Thread::Current();
+  cricket::MediaEngineDependencies media_deps;
-          dependencies.signaling_thread = rtc::Thread::Current();
+  media_deps.task_queue_factory = dependencies.task_queue_factory.get();
-          dependencies.task_queue_factory = CreateDefaultTaskQueueFactory();
+  media_deps.adm = FakeAudioCaptureModule::Create();
-          cricket::MediaEngineDependencies media_deps;
+  SetMediaEngineDefaults(&media_deps);
-          media_deps.task_queue_factory = dependencies.task_queue_factory.get();
+  dependencies.media_engine = cricket::CreateMediaEngine(std::move(media_deps));
-          media_deps.adm = FakeAudioCaptureModule::Create();
+  dependencies.call_factory = CreateCallFactory();
-          SetMediaEngineDefaults(&media_deps);
+  dependencies.sctp_factory = std::make_unique<FakeSctpTransportFactory>();
-          dependencies.media_engine =
+  return dependencies;
-              cricket::CreateMediaEngine(std::move(media_deps));
+}
-          dependencies.call_factory = CreateCallFactory();
-          return dependencies;
-        }()) {}
-
-  std::unique_ptr<cricket::SctpTransportInternalFactory>
-  CreateSctpTransportInternalFactory() {
-    return std::make_unique<FakeSctpTransportFactory>();
-  }
-};
 
 class PeerConnectionJsepTest : public ::testing::Test {
  protected:
@@ -84,9 +75,9 @@ class PeerConnectionJsepTest : public ::testing::Test {
   }
 
   WrapperPtr CreatePeerConnection(const RTCConfiguration& config) {
-    rtc::scoped_refptr<PeerConnectionFactory> pc_factory(
+    rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory =
-        new rtc::RefCountedObject<PeerConnectionFactoryForJsepTest>());
+        CreateModularPeerConnectionFactory(
-    RTC_CHECK(pc_factory->Initialize());
+            CreatePeerConnectionFactoryDependencies());
     auto observer = std::make_unique<MockPeerConnectionObserver>();
     auto pc = pc_factory->CreatePeerConnection(config, nullptr, nullptr,
                                                observer.get());

test/DEPS

@@ -6,6 +6,7 @@ include_rules = [
   "+common_video",
   "+logging/rtc_event_log",
   "+media/base",
+  "+media/sctp",
   "+media/engine",
   "+modules/audio_coding",
   "+modules/congestion_controller",

test/pc/sctp/BUILD.gn

@@ -0,0 +1,15 @@
+# Copyright (c) 2020 The WebRTC project authors. All Rights Reserved.
+#
+# Use of this source code is governed by a BSD-style license
+# that can be found in the LICENSE file in the root of the source
+# tree. An additional intellectual property rights grant can be found
+# in the file PATENTS.  All contributing project authors may
+# be found in the AUTHORS file in the root of the source tree.
+
+import("../../../webrtc.gni")
+
+rtc_source_set("fake_sctp_transport") {
+  visibility = [ "*" ]
+  sources = [ "fake_sctp_transport.h" ]
+  deps = [ "../../../media:rtc_data" ]
+}

test/pc/sctp/fake_sctp_transport.h

@@ -8,8 +8,8 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#ifndef PC_TEST_FAKE_SCTP_TRANSPORT_H_
+#ifndef TEST_PC_SCTP_FAKE_SCTP_TRANSPORT_H_
-#define PC_TEST_FAKE_SCTP_TRANSPORT_H_
+#define TEST_PC_SCTP_FAKE_SCTP_TRANSPORT_H_
 
 #include <memory>
 
@@ -49,7 +49,7 @@ class FakeSctpTransport : public cricket::SctpTransportInternal {
   int max_message_size_;
 };
 
-class FakeSctpTransportFactory : public cricket::SctpTransportInternalFactory {
+class FakeSctpTransportFactory : public webrtc::SctpTransportFactoryInterface {
  public:
   std::unique_ptr<cricket::SctpTransportInternal> CreateSctpTransport(
       rtc::PacketTransportInternal*) override {
@@ -66,4 +66,4 @@ class FakeSctpTransportFactory : public cricket::SctpTransportInternalFactory {
   FakeSctpTransport* last_fake_sctp_transport_ = nullptr;
 };
 
-#endif  // PC_TEST_FAKE_SCTP_TRANSPORT_H_
+#endif  // TEST_PC_SCTP_FAKE_SCTP_TRANSPORT_H_