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}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index db0723d..50a9c5a 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -185,6 +185,7 @@
"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",
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index 3ada740..6cf6dbb 100644
--- a/api/peer_connection_interface.h
+++ b/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 @@
// 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;
};
diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn
index a4ada07..d2da445 100644
--- a/api/transport/BUILD.gn
+++ b/api/transport/BUILD.gn
@@ -93,6 +93,11 @@
]
}
+rtc_source_set("sctp_transport_factory_interface") {
+ visibility = [ "*" ]
+ sources = [ "sctp_transport_factory_interface.h" ]
+}
+
rtc_source_set("stun_types") {
visibility = [ "*" ]
sources = [
diff --git a/api/transport/sctp_transport_factory_interface.h b/api/transport/sctp_transport_factory_interface.h
new file mode 100644
index 0000000..912be3a
--- /dev/null
+++ b/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_
diff --git a/media/BUILD.gn b/media/BUILD.gn
index 238b1d2..284cd45 100644
--- a/media/BUILD.gn
+++ b/media/BUILD.gn
@@ -426,7 +426,10 @@
}
if (rtc_enable_sctp && rtc_build_usrsctp) {
- deps += [ "//third_party/usrsctp" ]
+ deps += [
+ "../api/transport:sctp_transport_factory_interface",
+ "//third_party/usrsctp",
+ ]
}
}
diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h
index 38029ff..54542af 100644
--- a/media/sctp/sctp_transport.h
+++ b/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 @@
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) {}
diff --git a/media/sctp/sctp_transport_internal.h b/media/sctp/sctp_transport_internal.h
index b0e0e0f..dc8ac45 100644
--- a/media/sctp/sctp_transport_internal.h
+++ b/media/sctp/sctp_transport_internal.h
@@ -142,18 +142,6 @@
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_
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 9d04b4c..712449f 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -446,7 +446,6 @@
"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 @@
"../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",
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index d95b475..190f740 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -101,7 +101,7 @@
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|.
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 8407d75..a39d3c6c 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -1073,7 +1073,6 @@
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 @@
}
}
- 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 @@
// 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();
}
}
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 4aeac1c..74c4ebe 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -1274,9 +1274,6 @@
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
diff --git a/pc/peer_connection_data_channel_unittest.cc b/pc/peer_connection_data_channel_unittest.cc
index 0a674f4..6c51f01 100644
--- a/pc/peer_connection_data_channel_unittest.cc
+++ b/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 @@
namespace {
-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 CreatePeerConnectionFactoryDependencies() {
PeerConnectionFactoryDependencies deps;
- deps.network_thread = network_thread;
- deps.worker_thread = worker_thread;
- deps.signaling_thread = signaling_thread;
+ deps.network_thread = rtc::Thread::Current();
+ deps.worker_thread = rtc::Thread::Current();
+ deps.signaling_thread = rtc::Thread::Current();
deps.task_queue_factory = CreateDefaultTaskQueueFactory();
- deps.media_engine = std::move(media_engine);
- deps.call_factory = std::move(call_factory);
+ deps.media_engine = std::make_unique<cricket::FakeMediaEngine>();
+ 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 @@
WrapperPtr CreatePeerConnection(
const RTCConfiguration& config,
const PeerConnectionFactoryInterface::Options factory_options) {
- rtc::scoped_refptr<PeerConnectionFactoryForDataChannelTest> pc_factory(
- new PeerConnectionFactoryForDataChannelTest());
+ 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));
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 @@
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(
- pc_factory->last_fake_sctp_transport_factory_);
+ wrapper->set_sctp_transport_factory(fake_sctp_transport_factory);
return wrapper;
}
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index d79e438..3565c52 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -84,6 +84,7 @@
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 @@
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 @@
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();
}
diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h
index 3932562..8e7a2e2 100644
--- a/pc/peer_connection_factory.h
+++ b/pc/peer_connection_factory.h
@@ -71,8 +71,9 @@
bool StartAecDump(FILE* file, int64_t max_size_bytes) override;
void StopAecDump() override;
- virtual std::unique_ptr<cricket::SctpTransportInternalFactory>
- CreateSctpTransportInternalFactory();
+ SctpTransportFactoryInterface* sctp_transport_factory() {
+ return sctp_factory_.get();
+ }
virtual cricket::ChannelManager* channel_manager();
@@ -125,6 +126,7 @@
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_;
};
diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc
index b7c0759..4d9884d 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/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::UnorderedElementsAre;
using ::testing::Values;
-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>();
- }
-};
+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 PeerConnectionJsepTest : public ::testing::Test {
protected:
@@ -84,9 +75,9 @@
}
WrapperPtr CreatePeerConnection(const RTCConfiguration& config) {
- rtc::scoped_refptr<PeerConnectionFactory> pc_factory(
- new rtc::RefCountedObject<PeerConnectionFactoryForJsepTest>());
- RTC_CHECK(pc_factory->Initialize());
+ rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory =
+ CreateModularPeerConnectionFactory(
+ CreatePeerConnectionFactoryDependencies());
auto observer = std::make_unique<MockPeerConnectionObserver>();
auto pc = pc_factory->CreatePeerConnection(config, nullptr, nullptr,
observer.get());
diff --git a/test/DEPS b/test/DEPS
index 170c408..2cbb1d2 100644
--- a/test/DEPS
+++ b/test/DEPS
@@ -6,6 +6,7 @@
"+common_video",
"+logging/rtc_event_log",
"+media/base",
+ "+media/sctp",
"+media/engine",
"+modules/audio_coding",
"+modules/congestion_controller",
diff --git a/test/pc/sctp/BUILD.gn b/test/pc/sctp/BUILD.gn
new file mode 100644
index 0000000..93ae1bf
--- /dev/null
+++ b/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" ]
+}
diff --git a/pc/test/fake_sctp_transport.h b/test/pc/sctp/fake_sctp_transport.h
similarity index 91%
rename from pc/test/fake_sctp_transport.h
rename to test/pc/sctp/fake_sctp_transport.h
index 50e59f1..5fdb3bb 100644
--- a/pc/test/fake_sctp_transport.h
+++ b/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_
-#define PC_TEST_FAKE_SCTP_TRANSPORT_H_
+#ifndef TEST_PC_SCTP_FAKE_SCTP_TRANSPORT_H_
+#define TEST_PC_SCTP_FAKE_SCTP_TRANSPORT_H_
#include <memory>
@@ -49,7 +49,7 @@
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 @@
FakeSctpTransport* last_fake_sctp_transport_ = nullptr;
};
-#endif // PC_TEST_FAKE_SCTP_TRANSPORT_H_
+#endif // TEST_PC_SCTP_FAKE_SCTP_TRANSPORT_H_
