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

This reverts commit 4c0a381137c04fd80830af8a041e25e3428dd33f.

Reason for revert: Breaks downstream test

Original change's description:
> 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.
> 
> Bug: none
> Change-Id: I482241269463595062548870750d33f31238c6b1
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/182082
> Commit-Queue: Per Kjellander <perkj@webrtc.org>
> Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
> Reviewed-by: Taylor <deadbeef@webrtc.org>
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#32007}

TBR=deadbeef@webrtc.org,mbonadei@webrtc.org,kwiberg@webrtc.org,perkj@webrtc.org

Change-Id: I46d5ba89fe723caccd065b0ac41d77ed45373838
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: none
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/182802
Reviewed-by: Björn Terelius <terelius@webrtc.org>
Commit-Queue: Björn Terelius <terelius@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32008}

api/BUILD.gn

@@ -185,7 +185,6 @@ 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,7 +105,6 @@
 #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"
@@ -1364,7 +1363,6 @@ 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,11 +93,6 @@ 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

@@ -1,42 +0,0 @@
-/*
- *  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,10 +426,7 @@ rtc_library("rtc_data") {
   }
 
   if (rtc_enable_sctp && rtc_build_usrsctp) {
-    deps += [
-      "../api/transport:sctp_transport_factory_interface",
-      "//third_party/usrsctp",
-    ]
+    deps += [ "//third_party/usrsctp" ]
   }
 }
 

media/sctp/sctp_transport.h

@@ -21,7 +21,6 @@
 #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"
@@ -284,7 +283,7 @@ class SctpTransport : public SctpTransportInternal,
   RTC_DISALLOW_COPY_AND_ASSIGN(SctpTransport);
 };
 
-class SctpTransportFactory : public webrtc::SctpTransportFactoryInterface {
+class SctpTransportFactory : public SctpTransportInternalFactory {
  public:
   explicit SctpTransportFactory(rtc::Thread* network_thread)
       : network_thread_(network_thread) {}

media/sctp/sctp_transport_internal.h

@@ -142,6 +142,18 @@ 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,6 +446,7 @@ 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",
@@ -607,7 +608,6 @@ 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.
-    SctpTransportFactoryInterface* sctp_factory = nullptr;
+    cricket::SctpTransportInternalFactory* sctp_factory = nullptr;
   };
 
   // The ICE related events are signaled on the |signaling_thread|.

pc/peer_connection.cc

@@ -1073,6 +1073,7 @@ 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.
@@ -1261,6 +1262,8 @@ 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
@@ -1270,7 +1273,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 = factory_->sctp_transport_factory();
+      config.sctp_factory = sctp_factory_.get();
     }
   }
 

pc/peer_connection.h

@@ -1274,6 +1274,9 @@ 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,20 +58,46 @@ 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 = rtc::Thread::Current();
-  deps.worker_thread = rtc::Thread::Current();
-  deps.signaling_thread = rtc::Thread::Current();
+  deps.network_thread = network_thread;
+  deps.worker_thread = worker_thread;
+  deps.signaling_thread = signaling_thread;
   deps.task_queue_factory = CreateDefaultTaskQueueFactory();
-  deps.media_engine = std::make_unique<cricket::FakeMediaEngine>();
-  deps.call_factory = CreateCallFactory();
-  deps.sctp_factory = std::make_unique<FakeSctpTransportFactory>();
+  deps.media_engine = std::move(media_engine);
+  deps.call_factory = std::move(call_factory);
   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;
@@ -129,12 +155,10 @@ class PeerConnectionDataChannelBaseTest : public ::testing::Test {
   WrapperPtr CreatePeerConnection(
       const RTCConfiguration& config,
       const PeerConnectionFactoryInterface::Options factory_options) {
-    auto factory_deps = CreatePeerConnectionFactoryDependencies();
-    FakeSctpTransportFactory* fake_sctp_transport_factory =
-        static_cast<FakeSctpTransportFactory*>(factory_deps.sctp_factory.get());
-    rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory =
-        CreateModularPeerConnectionFactory(std::move(factory_deps));
+    rtc::scoped_refptr<PeerConnectionFactoryForDataChannelTest> pc_factory(
+        new PeerConnectionFactoryForDataChannelTest());
     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_;
@@ -147,7 +171,9 @@ class PeerConnectionDataChannelBaseTest : public ::testing::Test {
     observer->SetPeerConnectionInterface(pc.get());
     auto wrapper = std::make_unique<PeerConnectionWrapperForDataChannelTest>(
         pc_factory, pc, std::move(observer));
-    wrapper->set_sctp_transport_factory(fake_sctp_transport_factory);
+    RTC_DCHECK(pc_factory->last_fake_sctp_transport_factory_);
+    wrapper->set_sctp_transport_factory(
+        pc_factory->last_fake_sctp_transport_factory_);
     return wrapper;
   }
 

pc/peer_connection_factory.cc

@@ -67,18 +67,6 @@ CreateModularPeerConnectionFactory(
                                             pc_factory);
 }
 
-std::unique_ptr<SctpTransportFactoryInterface> MaybeCreateSctpFactory(
-    PeerConnectionFactoryDependencies* dependencies) {
-  if (dependencies->sctp_factory) {
-    return std::move(dependencies->sctp_factory);
-  }
-#ifdef HAVE_SCTP
-  return std::make_unique<cricket::SctpTransportFactory>(
-      dependencies->network_thread);
-#endif
-  return nullptr;
-}
-
 PeerConnectionFactory::PeerConnectionFactory(
     PeerConnectionFactoryDependencies dependencies)
     : wraps_current_thread_(false),
@@ -96,7 +84,6 @@ PeerConnectionFactory::PeerConnectionFactory(
       injected_network_controller_factory_(
           std::move(dependencies.network_controller_factory)),
       neteq_factory_(std::move(dependencies.neteq_factory)),
-      sctp_factory_(MaybeCreateSctpFactory(&dependencies)),
       trials_(dependencies.trials ? std::move(dependencies.trials)
                                   : std::make_unique<FieldTrialBasedConfig>()) {
   if (!network_thread_) {
@@ -339,6 +326,15 @@ 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,9 +71,8 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface {
   bool StartAecDump(FILE* file, int64_t max_size_bytes) override;
   void StopAecDump() override;
 
-  SctpTransportFactoryInterface* sctp_transport_factory() {
-    return sctp_factory_.get();
-  }
+  virtual std::unique_ptr<cricket::SctpTransportInternalFactory>
+  CreateSctpTransportInternalFactory();
 
   virtual cricket::ChannelManager* channel_manager();
 
@@ -126,7 +125,6 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface {
   std::unique_ptr<NetworkControllerFactoryInterface>
       injected_network_controller_factory_;
   std::unique_ptr<NetEqFactory> neteq_factory_;
-  const 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,21 +41,30 @@ using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 using ::testing::Values;
 
-PeerConnectionFactoryDependencies CreatePeerConnectionFactoryDependencies() {
-  PeerConnectionFactoryDependencies dependencies;
-  dependencies.worker_thread = rtc::Thread::Current();
-  dependencies.network_thread = rtc::Thread::Current();
-  dependencies.signaling_thread = rtc::Thread::Current();
-  dependencies.task_queue_factory = CreateDefaultTaskQueueFactory();
-  cricket::MediaEngineDependencies media_deps;
-  media_deps.task_queue_factory = dependencies.task_queue_factory.get();
-  media_deps.adm = FakeAudioCaptureModule::Create();
-  SetMediaEngineDefaults(&media_deps);
-  dependencies.media_engine = cricket::CreateMediaEngine(std::move(media_deps));
-  dependencies.call_factory = CreateCallFactory();
-  dependencies.sctp_factory = std::make_unique<FakeSctpTransportFactory>();
-  return dependencies;
-}
+class PeerConnectionFactoryForJsepTest : public PeerConnectionFactory {
+ public:
+  PeerConnectionFactoryForJsepTest()
+      : PeerConnectionFactory([] {
+          PeerConnectionFactoryDependencies dependencies;
+          dependencies.worker_thread = rtc::Thread::Current();
+          dependencies.network_thread = rtc::Thread::Current();
+          dependencies.signaling_thread = rtc::Thread::Current();
+          dependencies.task_queue_factory = CreateDefaultTaskQueueFactory();
+          cricket::MediaEngineDependencies media_deps;
+          media_deps.task_queue_factory = dependencies.task_queue_factory.get();
+          media_deps.adm = FakeAudioCaptureModule::Create();
+          SetMediaEngineDefaults(&media_deps);
+          dependencies.media_engine =
+              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:
@@ -75,9 +84,9 @@ class PeerConnectionJsepTest : public ::testing::Test {
   }
 
   WrapperPtr CreatePeerConnection(const RTCConfiguration& config) {
-    rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory =
-        CreateModularPeerConnectionFactory(
-            CreatePeerConnectionFactoryDependencies());
+    rtc::scoped_refptr<PeerConnectionFactory> pc_factory(
+        new rtc::RefCountedObject<PeerConnectionFactoryForJsepTest>());
+    RTC_CHECK(pc_factory->Initialize());
     auto observer = std::make_unique<MockPeerConnectionObserver>();
     auto pc = pc_factory->CreatePeerConnection(config, nullptr, nullptr,
                                                observer.get());

pc/test/fake_sctp_transport.h

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

test/DEPS

@@ -6,7 +6,6 @@ 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

@@ -1,15 +0,0 @@
-# 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" ]
-}