diff --git a/api/BUILD.gn b/api/BUILD.gn index db0723dec8..50a9c5a242 100644 --- a/api/BUILD.gn +++ b/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", diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 3ada740f18..6cf6dbb299 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 @@ struct RTC_EXPORT PeerConnectionFactoryDependencies final { // used. std::unique_ptr network_monitor_factory; std::unique_ptr neteq_factory; + std::unique_ptr sctp_factory; std::unique_ptr trials; }; diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index a4ada07108..d2da4453bf 100644 --- a/api/transport/BUILD.gn +++ b/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 = [ diff --git a/api/transport/sctp_transport_factory_interface.h b/api/transport/sctp_transport_factory_interface.h new file mode 100644 index 0000000000..912be3a374 --- /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 + +// 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 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 238b1d2f01..284cd45714 100644 --- a/media/BUILD.gn +++ b/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", + ] } } diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h index 38029ffeb3..54542af6b3 100644 --- a/media/sctp/sctp_transport.h +++ b/media/sctp/sctp_transport.h @@ -21,6 +21,7 @@ #include #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) {} diff --git a/media/sctp/sctp_transport_internal.h b/media/sctp/sctp_transport_internal.h index b0e0e0f7e6..dc8ac4558d 100644 --- a/media/sctp/sctp_transport_internal.h +++ b/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 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 9d04b4c787..712449f1c2 100644 --- a/pc/BUILD.gn +++ b/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", diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index d95b475969..190f740c7c 100644 --- a/pc/jsep_transport_controller.h +++ b/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|. diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 8407d75f0c..a39d3c6c42 100644 --- a/pc/peer_connection.cc +++ b/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(); } } diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 4aeac1c390..74c4ebee3c 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -1274,9 +1274,6 @@ class PeerConnection : public PeerConnectionInternal, std::unique_ptr transport_controller_; // TODO(bugs.webrtc.org/9987): Accessed on both // signaling and network thread. - std::unique_ptr - 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 0a674f462b..6c51f01594 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 @@ using ::testing::Values; namespace { -PeerConnectionFactoryDependencies CreatePeerConnectionFactoryDependencies( - rtc::Thread* network_thread, - rtc::Thread* worker_thread, - rtc::Thread* signaling_thread, - std::unique_ptr media_engine, - std::unique_ptr 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(); + deps.call_factory = CreateCallFactory(); + deps.sctp_factory = std::make_unique(); return deps; } } // namespace -class PeerConnectionFactoryForDataChannelTest - : public rtc::RefCountedObject { - public: - PeerConnectionFactoryForDataChannelTest() - : rtc::RefCountedObject( - CreatePeerConnectionFactoryDependencies( - rtc::Thread::Current(), - rtc::Thread::Current(), - rtc::Thread::Current(), - std::make_unique(), - CreateCallFactory())) {} - - std::unique_ptr - CreateSctpTransportInternalFactory() { - auto factory = std::make_unique(); - 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 pc_factory( - new PeerConnectionFactoryForDataChannelTest()); + auto factory_deps = CreatePeerConnectionFactoryDependencies(); + FakeSctpTransportFactory* fake_sctp_transport_factory = + static_cast(factory_deps.sctp_factory.get()); + rtc::scoped_refptr pc_factory = + CreateModularPeerConnectionFactory(std::move(factory_deps)); pc_factory->SetOptions(factory_options); - RTC_CHECK(pc_factory->Initialize()); auto observer = std::make_unique(); 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( 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 d79e438152..3565c52390 100644 --- a/pc/peer_connection_factory.cc +++ b/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()) { 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(network_thread_); + } +#endif } PeerConnectionFactory::~PeerConnectionFactory() { @@ -326,15 +334,6 @@ rtc::scoped_refptr PeerConnectionFactory::CreateAudioTrack( return AudioTrackProxy::Create(signaling_thread_, track); } -std::unique_ptr -PeerConnectionFactory::CreateSctpTransportInternalFactory() { -#ifdef HAVE_SCTP - return std::make_unique(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 3932562d22..8e7a2e27ee 100644 --- a/pc/peer_connection_factory.h +++ b/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 - CreateSctpTransportInternalFactory(); + SctpTransportFactoryInterface* sctp_transport_factory() { + return sctp_factory_.get(); + } virtual cricket::ChannelManager* channel_manager(); @@ -125,6 +126,7 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { std::unique_ptr injected_network_controller_factory_; std::unique_ptr neteq_factory_; + std::unique_ptr sctp_factory_; const std::unique_ptr trials_; }; diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index b7c07598cf..4d9884d615 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::ElementsAre; 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 - CreateSctpTransportInternalFactory() { - return std::make_unique(); - } -}; +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(); + return dependencies; +} class PeerConnectionJsepTest : public ::testing::Test { protected: @@ -84,9 +75,9 @@ class PeerConnectionJsepTest : public ::testing::Test { } WrapperPtr CreatePeerConnection(const RTCConfiguration& config) { - rtc::scoped_refptr pc_factory( - new rtc::RefCountedObject()); - RTC_CHECK(pc_factory->Initialize()); + rtc::scoped_refptr pc_factory = + CreateModularPeerConnectionFactory( + CreatePeerConnectionFactoryDependencies()); auto observer = std::make_unique(); auto pc = pc_factory->CreatePeerConnection(config, nullptr, nullptr, observer.get()); diff --git a/test/DEPS b/test/DEPS index 170c4086d7..2cbb1d2dc3 100644 --- a/test/DEPS +++ b/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", diff --git a/test/pc/sctp/BUILD.gn b/test/pc/sctp/BUILD.gn new file mode 100644 index 0000000000..93ae1bf59c --- /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 50e59f1fc2..5fdb3bbe42 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 @@ -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 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_