From ffd5dc70ebbb69e1251e5e9b36ffe69e0bae8299 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Tue, 20 Oct 2020 15:35:31 +0000 Subject: [PATCH] Delete the "initialize" method of PeerConnectionFactory Also remove the "initialized" concept from ConnectionContext. This CL also always creates the objects on the signaling thread. Makes the initialization code slightly more readable. Bug: webrtc:11967 Change-Id: I5e451a3c5225c29c30d32bb4843df8c107ec30c0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/188626 Commit-Queue: Harald Alvestrand Reviewed-by: Tommi Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#32453} --- pc/BUILD.gn | 9 +++ pc/connection_context.cc | 83 +++++++++++++----------- pc/connection_context.h | 24 +++++-- pc/peer_connection_factory.cc | 82 +++++++++++++++-------- pc/peer_connection_factory.h | 40 ++++++++++-- pc/peer_connection_histogram_unittest.cc | 1 - pc/peer_connection_interface_unittest.cc | 2 - 7 files changed, 162 insertions(+), 79 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 821802f00d..fbd0295176 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -228,6 +228,7 @@ rtc_library("peerconnection") { "../api:array_view", "../api:audio_options_api", "../api:call_api", + "../api:callfactory_api", "../api:fec_controller_api", "../api:frame_transformer_interface", "../api:ice_transport_factory", @@ -246,12 +247,15 @@ rtc_library("peerconnection") { "../api/adaptation:resource_adaptation_api", "../api/crypto:frame_decryptor_interface", "../api/crypto:options", + "../api/neteq:neteq_api", "../api/rtc_event_log", "../api/task_queue", "../api/transport:bitrate_settings", "../api/transport:datagram_transport_interface", "../api/transport:enums", "../api/transport:field_trial_based_config", + "../api/transport:network_control", + "../api/transport:sctp_transport_factory_interface", "../api/transport:webrtc_key_value_config", "../api/units:data_rate", "../api/video:builtin_video_bitrate_allocator_factory", @@ -305,14 +309,19 @@ rtc_library("connection_context") { ] deps = [ ":rtc_pc_base", + "../api:callfactory_api", "../api:libjingle_peerconnection_api", "../api:media_stream_interface", "../api:scoped_refptr", + "../api/neteq:neteq_api", "../api/transport:field_trial_based_config", + "../api/transport:sctp_transport_factory_interface", + "../api/transport:webrtc_key_value_config", "../media:rtc_data", "../media:rtc_media_base", "../p2p:rtc_p2p", "../rtc_base", + "../rtc_base:checks", ] } diff --git a/pc/connection_context.cc b/pc/connection_context.cc index ea9fb72f53..7477d8e6b7 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -10,10 +10,15 @@ #include "pc/connection_context.h" +#include +#include #include #include "api/transport/field_trial_based_config.h" #include "media/base/rtp_data_engine.h" +#include "rtc_base/helpers.h" +#include "rtc_base/ref_counted_object.h" +#include "rtc_base/time_utils.h" namespace webrtc { @@ -67,29 +72,59 @@ std::unique_ptr MaybeCreateSctpFactory( } // namespace +// Static +rtc::scoped_refptr ConnectionContext::Create( + PeerConnectionFactoryDependencies* dependencies) { + auto context = new rtc::RefCountedObject(dependencies); + if (!context->channel_manager_->Init()) { + return nullptr; + } + return context; +} + ConnectionContext::ConnectionContext( - PeerConnectionFactoryDependencies& dependencies) - : network_thread_(MaybeStartThread(dependencies.network_thread, + PeerConnectionFactoryDependencies* dependencies) + : network_thread_(MaybeStartThread(dependencies->network_thread, "pc_network_thread", true, owned_network_thread_)), - worker_thread_(MaybeStartThread(dependencies.worker_thread, + worker_thread_(MaybeStartThread(dependencies->worker_thread, "pc_worker_thread", false, owned_worker_thread_)), - signaling_thread_(MaybeWrapThread(dependencies.signaling_thread, + signaling_thread_(MaybeWrapThread(dependencies->signaling_thread, wraps_current_thread_)), - network_monitor_factory_(std::move(dependencies.network_monitor_factory)), - call_factory_(std::move(dependencies.call_factory)), - media_engine_(std::move(dependencies.media_engine)), - sctp_factory_(MaybeCreateSctpFactory(std::move(dependencies.sctp_factory), - network_thread())), - trials_(dependencies.trials ? std::move(dependencies.trials) - : std::make_unique()) { + network_monitor_factory_( + std::move(dependencies->network_monitor_factory)), + call_factory_(std::move(dependencies->call_factory)), + media_engine_(std::move(dependencies->media_engine)), + sctp_factory_( + MaybeCreateSctpFactory(std::move(dependencies->sctp_factory), + network_thread())), + trials_(dependencies->trials + ? std::move(dependencies->trials) + : std::make_unique()) { signaling_thread_->AllowInvokesToThread(worker_thread_); signaling_thread_->AllowInvokesToThread(network_thread_); worker_thread_->AllowInvokesToThread(network_thread_); network_thread_->DisallowAllInvokes(); + + RTC_DCHECK_RUN_ON(signaling_thread_); + rtc::InitRandom(rtc::Time32()); + + // If network_monitor_factory_ is non-null, it will be used to create a + // network monitor while on the network thread. + default_network_manager_ = std::make_unique( + network_monitor_factory_.get()); + + default_socket_factory_ = + std::make_unique(network_thread()); + + channel_manager_ = std::make_unique( + std::move(media_engine_), std::make_unique(), + worker_thread(), network_thread()); + + channel_manager_->SetVideoRtxEnabled(true); } ConnectionContext::~ConnectionContext() { @@ -111,32 +146,6 @@ void ConnectionContext::SetOptions( options_ = options; } -bool ConnectionContext::Initialize() { - RTC_DCHECK_RUN_ON(signaling_thread_); - rtc::InitRandom(rtc::Time32()); - - // If network_monitor_factory_ is non-null, it will be used to create a - // network monitor while on the network thread. - default_network_manager_.reset( - new rtc::BasicNetworkManager(network_monitor_factory_.get())); - if (!default_network_manager_) { - return false; - } - - default_socket_factory_.reset( - new rtc::BasicPacketSocketFactory(network_thread())); - if (!default_socket_factory_) { - return false; - } - - channel_manager_ = std::make_unique( - std::move(media_engine_), std::make_unique(), - worker_thread(), network_thread()); - - channel_manager_->SetVideoRtxEnabled(true); - return channel_manager_->Init(); -} - cricket::ChannelManager* ConnectionContext::channel_manager() const { return channel_manager_.get(); } diff --git a/pc/connection_context.h b/pc/connection_context.h index 502ba5a88c..2f95869381 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -14,14 +14,24 @@ #include #include +#include "api/call/call_factory_interface.h" #include "api/media_stream_interface.h" #include "api/peer_connection_interface.h" #include "api/scoped_refptr.h" +#include "api/transport/sctp_transport_factory_interface.h" +#include "api/transport/webrtc_key_value_config.h" +#include "media/base/media_engine.h" #include "media/sctp/sctp_transport_internal.h" #include "p2p/base/basic_packet_socket_factory.h" #include "pc/channel_manager.h" +#include "rtc_base/checks.h" +#include "rtc_base/network.h" +#include "rtc_base/network_monitor_factory.h" +#include "rtc_base/ref_count.h" #include "rtc_base/rtc_certificate_generator.h" +#include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/thread.h" +#include "rtc_base/thread_annotations.h" namespace rtc { class BasicNetworkManager; @@ -36,8 +46,16 @@ class RtcEventLog; // objects. A reference to this object is passed to each PeerConnection. The // methods on this object are assumed not to change the state in any way that // interferes with the operation of other PeerConnections. +// +// This class must be created and destroyed on the signaling thread. class ConnectionContext : public rtc::RefCountInterface { public: + // Creates a ConnectionContext. May return null if initialization fails. + // The Dependencies class allows simple management of all new dependencies + // being added to the ConnectionContext. + static rtc::scoped_refptr Create( + PeerConnectionFactoryDependencies* dependencies); + // This class is not copyable or movable. ConnectionContext(const ConnectionContext&) = delete; ConnectionContext& operator=(const ConnectionContext&) = delete; @@ -45,8 +63,6 @@ class ConnectionContext : public rtc::RefCountInterface { // Functions called from PeerConnectionFactory void SetOptions(const PeerConnectionFactoryInterface::Options& options); - bool Initialize(); - // Functions called from PeerConnection and friends SctpTransportFactoryInterface* sctp_transport_factory() const { RTC_DCHECK_RUN_ON(signaling_thread_); @@ -83,9 +99,7 @@ class ConnectionContext : public rtc::RefCountInterface { } protected: - // The Dependencies class allows simple management of all new dependencies - // being added to the ConnectionContext. - explicit ConnectionContext(PeerConnectionFactoryDependencies& dependencies); + explicit ConnectionContext(PeerConnectionFactoryDependencies* dependencies); virtual ~ConnectionContext(); diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index 9e7c17cee3..ba81c29901 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -12,23 +12,25 @@ #include #include +#include #include -#include #include "absl/strings/match.h" +#include "api/async_resolver_factory.h" +#include "api/call/call_factory_interface.h" #include "api/fec_controller.h" +#include "api/ice_transport_interface.h" #include "api/media_stream_proxy.h" #include "api/media_stream_track_proxy.h" #include "api/network_state_predictor.h" +#include "api/packet_socket_factory.h" #include "api/peer_connection_factory_proxy.h" #include "api/peer_connection_proxy.h" #include "api/rtc_event_log/rtc_event_log.h" -#include "api/transport/field_trial_based_config.h" -#include "api/turn_customizer.h" +#include "api/transport/bitrate_settings.h" #include "api/units/data_rate.h" -#include "api/video_track_source_proxy.h" -#include "media/base/rtp_data_engine.h" -#include "media/sctp/sctp_transport.h" +#include "call/audio_state.h" +#include "media/base/media_engine.h" #include "p2p/base/basic_async_resolver_factory.h" #include "p2p/base/basic_packet_socket_factory.h" #include "p2p/base/default_ice_transport_factory.h" @@ -38,12 +40,17 @@ #include "pc/media_stream.h" #include "pc/peer_connection.h" #include "pc/rtp_parameters_conversion.h" +#include "pc/session_description.h" #include "pc/video_track.h" #include "rtc_base/bind.h" #include "rtc_base/checks.h" #include "rtc_base/experiments/field_trial_parser.h" #include "rtc_base/experiments/field_trial_units.h" +#include "rtc_base/location.h" +#include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" +#include "rtc_base/ref_counted_object.h" +#include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/system/file_wrapper.h" namespace webrtc { @@ -51,42 +58,61 @@ namespace webrtc { rtc::scoped_refptr CreateModularPeerConnectionFactory( PeerConnectionFactoryDependencies dependencies) { - rtc::scoped_refptr pc_factory( - new rtc::RefCountedObject( - std::move(dependencies))); - // Call Initialize synchronously but make sure it is executed on - // |signaling_thread|. - MethodCall call( - pc_factory.get(), &PeerConnectionFactory::Initialize); - bool result = call.Marshal(RTC_FROM_HERE, pc_factory->signaling_thread()); + // The PeerConnectionFactory must be created on the signaling thread. + if (dependencies.signaling_thread && + !dependencies.signaling_thread->IsCurrent()) { + return dependencies.signaling_thread + ->Invoke>( + RTC_FROM_HERE, [&dependencies] { + return CreateModularPeerConnectionFactory( + std::move(dependencies)); + }); + } - if (!result) { + auto pc_factory = PeerConnectionFactory::Create(std::move(dependencies)); + if (!pc_factory) { return nullptr; } + // Verify that the invocation and the initialization ended up agreeing on the + // thread. + RTC_DCHECK_RUN_ON(pc_factory->signaling_thread()); return PeerConnectionFactoryProxy::Create(pc_factory->signaling_thread(), pc_factory); } +// Static +rtc::scoped_refptr PeerConnectionFactory::Create( + PeerConnectionFactoryDependencies dependencies) { + auto context = ConnectionContext::Create(&dependencies); + if (!context) { + return nullptr; + } + return new rtc::RefCountedObject(context, + &dependencies); +} + +PeerConnectionFactory::PeerConnectionFactory( + rtc::scoped_refptr context, + PeerConnectionFactoryDependencies* dependencies) + : context_(context), + task_queue_factory_(std::move(dependencies->task_queue_factory)), + event_log_factory_(std::move(dependencies->event_log_factory)), + fec_controller_factory_(std::move(dependencies->fec_controller_factory)), + network_state_predictor_factory_( + std::move(dependencies->network_state_predictor_factory)), + injected_network_controller_factory_( + std::move(dependencies->network_controller_factory)), + neteq_factory_(std::move(dependencies->neteq_factory)) {} + PeerConnectionFactory::PeerConnectionFactory( PeerConnectionFactoryDependencies dependencies) - : context_(new rtc::RefCountedObject(dependencies)), - task_queue_factory_(std::move(dependencies.task_queue_factory)), - event_log_factory_(std::move(dependencies.event_log_factory)), - fec_controller_factory_(std::move(dependencies.fec_controller_factory)), - network_state_predictor_factory_( - std::move(dependencies.network_state_predictor_factory)), - injected_network_controller_factory_( - std::move(dependencies.network_controller_factory)), - neteq_factory_(std::move(dependencies.neteq_factory)) {} + : PeerConnectionFactory(ConnectionContext::Create(&dependencies), + &dependencies) {} PeerConnectionFactory::~PeerConnectionFactory() { RTC_DCHECK_RUN_ON(signaling_thread()); } -bool PeerConnectionFactory::Initialize() { - return context_->Initialize(); -} - void PeerConnectionFactory::SetOptions(const Options& options) { context_->SetOptions(options); } diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h index 430eca5452..499ea0b855 100644 --- a/pc/peer_connection_factory.h +++ b/pc/peer_connection_factory.h @@ -12,13 +12,30 @@ #ifndef PC_PEER_CONNECTION_FACTORY_H_ #define PC_PEER_CONNECTION_FACTORY_H_ +#include +#include #include #include +#include "absl/strings/string_view.h" +#include "api/audio_options.h" +#include "api/fec_controller.h" #include "api/media_stream_interface.h" +#include "api/media_types.h" +#include "api/neteq/neteq_factory.h" +#include "api/network_state_predictor.h" #include "api/peer_connection_interface.h" +#include "api/rtc_event_log/rtc_event_log.h" +#include "api/rtc_event_log/rtc_event_log_factory_interface.h" +#include "api/rtp_parameters.h" #include "api/scoped_refptr.h" +#include "api/task_queue/task_queue_factory.h" +#include "api/transport/network_control.h" +#include "api/transport/sctp_transport_factory_interface.h" +#include "api/transport/webrtc_key_value_config.h" +#include "call/call.h" #include "media/sctp/sctp_transport_internal.h" +#include "p2p/base/port_allocator.h" #include "pc/channel_manager.h" #include "pc/connection_context.h" #include "rtc_base/rtc_certificate_generator.h" @@ -35,6 +52,14 @@ class RtcEventLog; class PeerConnectionFactory : public PeerConnectionFactoryInterface { public: + // Creates a PeerConnectionFactory. It returns nullptr on initialization + // error. + // + // The Dependencies structure allows simple management of all new + // dependencies being added to the PeerConnectionFactory. + static rtc::scoped_refptr Create( + PeerConnectionFactoryDependencies dependencies); + void SetOptions(const Options& options) override; rtc::scoped_refptr CreatePeerConnection( @@ -47,8 +72,6 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies) override; - bool Initialize(); - RtpCapabilities GetRtpSenderCapabilities( cricket::MediaType kind) const override; @@ -83,16 +106,18 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { // created in CreatePeerConnectionFactory(). return context_->signaling_thread(); } - rtc::Thread* worker_thread() const { return context_->worker_thread(); } - rtc::Thread* network_thread() const { return context_->network_thread(); } const Options& options() const { return context_->options(); } const WebRtcKeyValueConfig& trials() const { return context_->trials(); } protected: - // This structure allows simple management of all new dependencies being added - // to the PeerConnectionFactory. + // Constructor used by the static Create() method. Modifies the dependencies. + PeerConnectionFactory(rtc::scoped_refptr context, + PeerConnectionFactoryDependencies* dependencies); + + // Constructor for use in testing. Ignores the possibility of initialization + // failure. The dependencies are passed in by std::move(). explicit PeerConnectionFactory( PeerConnectionFactoryDependencies dependencies); @@ -103,6 +128,9 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { virtual ~PeerConnectionFactory(); private: + rtc::Thread* worker_thread() const { return context_->worker_thread(); } + rtc::Thread* network_thread() const { return context_->network_thread(); } + bool IsTrialEnabled(absl::string_view key) const; const cricket::ChannelManager* channel_manager() const { return context_->channel_manager(); diff --git a/pc/peer_connection_histogram_unittest.cc b/pc/peer_connection_histogram_unittest.cc index 8fcb87a68e..a9c5c9b322 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc @@ -339,7 +339,6 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { rtc::scoped_refptr pc_factory( new PeerConnectionFactoryForUsageHistogramTest()); pc_factory->SetOptions(factory_options); - RTC_CHECK(pc_factory->Initialize()); if (immediate_report) { pc_factory->ReturnHistogramVeryQuickly(); } diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index a0e3845fd6..abedf48688 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -699,7 +699,6 @@ class PeerConnectionInterfaceBaseTest : public ::testing::Test { ASSERT_TRUE(pc_factory_); pc_factory_for_test_ = PeerConnectionFactoryForTest::CreatePeerConnectionFactoryForTest(); - pc_factory_for_test_->Initialize(); } void CreatePeerConnection() { @@ -3923,7 +3922,6 @@ class PeerConnectionMediaConfigTest : public ::testing::Test { protected: void SetUp() override { pcf_ = PeerConnectionFactoryForTest::CreatePeerConnectionFactoryForTest(); - pcf_->Initialize(); } const cricket::MediaConfig TestCreatePeerConnection( const RTCConfiguration& config) {