From 4da4a87d9792b57a7dac91cdbb9abe2cd3b8b984 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Wed, 4 Nov 2020 10:34:21 +0000 Subject: [PATCH] Move "options" from ConnectionContext to PeerConnectionFactory and pass it as an argument to PeerConnection::Create This makes it obvious that 1) options only affect peerconnections if they are set on the factory before creating the PeerConnection, and 2) options are unchangeable after PeerConnection creation. Bug: webrtc:11967 Change-Id: I052eaa3975ac97dccbedde610110f32bf1a17c98 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/191487 Commit-Queue: Harald Alvestrand Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#32549} --- pc/connection_context.cc | 6 ------ pc/connection_context.h | 9 --------- pc/peer_connection.cc | 38 ++++++++++++++++++----------------- pc/peer_connection.h | 17 ++++++++++------ pc/peer_connection_factory.cc | 11 +++++----- pc/peer_connection_factory.h | 7 ++++++- pc/sdp_offer_answer.cc | 2 +- 7 files changed, 44 insertions(+), 46 deletions(-) diff --git a/pc/connection_context.cc b/pc/connection_context.cc index 7477d8e6b7..727fbd6542 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -140,12 +140,6 @@ ConnectionContext::~ConnectionContext() { rtc::ThreadManager::Instance()->UnwrapCurrentThread(); } -void ConnectionContext::SetOptions( - const PeerConnectionFactoryInterface::Options& options) { - RTC_DCHECK_RUN_ON(signaling_thread_); - options_ = options; -} - cricket::ChannelManager* ConnectionContext::channel_manager() const { return channel_manager_.get(); } diff --git a/pc/connection_context.h b/pc/connection_context.h index 2f95869381..02d08a191e 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -60,9 +60,6 @@ class ConnectionContext : public rtc::RefCountInterface { ConnectionContext(const ConnectionContext&) = delete; ConnectionContext& operator=(const ConnectionContext&) = delete; - // Functions called from PeerConnectionFactory - void SetOptions(const PeerConnectionFactoryInterface::Options& options); - // Functions called from PeerConnection and friends SctpTransportFactoryInterface* sctp_transport_factory() const { RTC_DCHECK_RUN_ON(signaling_thread_); @@ -78,10 +75,6 @@ class ConnectionContext : public rtc::RefCountInterface { rtc::Thread* network_thread() { return network_thread_; } const rtc::Thread* network_thread() const { return network_thread_; } - const PeerConnectionFactoryInterface::Options& options() const { - return options_; - } - const WebRtcKeyValueConfig& trials() const { return *trials_.get(); } // Accessors only used from the PeerConnectionFactory class @@ -117,8 +110,6 @@ class ConnectionContext : public rtc::RefCountInterface { rtc::Thread* const network_thread_; rtc::Thread* const worker_thread_; rtc::Thread* const signaling_thread_; - PeerConnectionFactoryInterface::Options options_ - RTC_GUARDED_BY(signaling_thread_); // channel_manager is accessed both on signaling thread and worker thread. std::unique_ptr channel_manager_; std::unique_ptr const network_monitor_factory_ diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 4b2a9d3ad6..165968c11b 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -389,6 +389,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator!=( rtc::scoped_refptr PeerConnection::Create( rtc::scoped_refptr context, + const PeerConnectionFactoryInterface::Options& options, std::unique_ptr event_log, std::unique_ptr call, const PeerConnectionInterface::RTCConfiguration& configuration, @@ -418,21 +419,24 @@ rtc::scoped_refptr PeerConnection::Create( configuration.sdp_semantics == SdpSemantics::kUnifiedPlan; // The PeerConnection constructor consumes some, but not all, dependencies. rtc::scoped_refptr pc( - new rtc::RefCountedObject(context, is_unified_plan, - std::move(event_log), - std::move(call), dependencies)); + new rtc::RefCountedObject( + context, options, is_unified_plan, std::move(event_log), + std::move(call), dependencies)); if (!pc->Initialize(configuration, std::move(dependencies))) { return nullptr; } return pc; } -PeerConnection::PeerConnection(rtc::scoped_refptr context, - bool is_unified_plan, - std::unique_ptr event_log, - std::unique_ptr call, - PeerConnectionDependencies& dependencies) +PeerConnection::PeerConnection( + rtc::scoped_refptr context, + const PeerConnectionFactoryInterface::Options& options, + bool is_unified_plan, + std::unique_ptr event_log, + std::unique_ptr call, + PeerConnectionDependencies& dependencies) : context_(context), + options_(options), observer_(dependencies.observer), is_unified_plan_(is_unified_plan), event_log_(std::move(event_log)), @@ -541,8 +545,6 @@ bool PeerConnection::Initialize( RTC_HISTOGRAM_ENUMERATION("WebRTC.PeerConnection.IPMetrics", address_family, kPeerConnectionAddressFamilyCounter_Max); - const PeerConnectionFactoryInterface::Options& options = context_->options(); - // RFC 3264: The numeric value of the session id and version in the // o line MUST be representable with a "64 bit signed integer". // Due to this constraint session id |session_id_| is max limited to @@ -551,15 +553,15 @@ bool PeerConnection::Initialize( JsepTransportController::Config config; config.redetermine_role_on_ice_restart = configuration.redetermine_role_on_ice_restart; - config.ssl_max_version = context_->options().ssl_max_version; - config.disable_encryption = options.disable_encryption; + config.ssl_max_version = options_.ssl_max_version; + config.disable_encryption = options_.disable_encryption; config.bundle_policy = configuration.bundle_policy; config.rtcp_mux_policy = configuration.rtcp_mux_policy; - // TODO(bugs.webrtc.org/9891) - Remove options.crypto_options then remove this - // stub. + // TODO(bugs.webrtc.org/9891) - Remove options_.crypto_options then remove + // this stub. config.crypto_options = configuration.crypto_options.has_value() ? *configuration.crypto_options - : options.crypto_options; + : options_.crypto_options; config.transport_observer = this; config.rtcp_handler = InitializeRtcpCallback(); config.event_log = event_log_ptr_; @@ -568,7 +570,7 @@ bool PeerConnection::Initialize( #endif config.active_reset_srtp_params = configuration.active_reset_srtp_params; - if (options.disable_encryption) { + if (options_.disable_encryption) { dtls_enabled_ = false; } else { // Enable DTLS by default if we have an identity store or a certificate. @@ -587,7 +589,7 @@ bool PeerConnection::Initialize( data_channel_controller_.set_data_channel_type(cricket::DCT_RTP); } else { // DTLS has to be enabled to use SCTP. - if (!options.disable_sctp_data_channels && dtls_enabled_) { + if (!options_.disable_sctp_data_channels && dtls_enabled_) { data_channel_controller_.set_data_channel_type(cricket::DCT_SCTP); config.sctp_factory = context_->sctp_transport_factory(); } @@ -2528,7 +2530,7 @@ CryptoOptions PeerConnection::GetCryptoOptions() { // after it has been removed. return configuration_.crypto_options.has_value() ? *configuration_.crypto_options - : context_->options().crypto_options; + : options_.crypto_options; } void PeerConnection::ClearStatsCache() { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 82c65b5ce9..52a302b2de 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -126,6 +126,7 @@ class PeerConnection : public PeerConnectionInternal, // either use them or release them, whether it succeeds or fails. static rtc::scoped_refptr Create( rtc::scoped_refptr context, + const PeerConnectionFactoryInterface::Options& options, std::unique_ptr event_log, std::unique_ptr call, const PeerConnectionInterface::RTCConfiguration& configuration, @@ -371,7 +372,9 @@ class PeerConnection : public PeerConnectionInternal, Call* call_ptr() { return call_ptr_; } ConnectionContext* context() { return context_.get(); } - + const PeerConnectionFactoryInterface::Options* options() const { + return &options_; + } cricket::DataChannelType data_channel_type() const; void SetIceConnectionState(IceConnectionState new_state); void NoteUsageEvent(UsageEvent event); @@ -446,11 +449,12 @@ class PeerConnection : public PeerConnectionInternal, protected: // Available for rtc::scoped_refptr creation - explicit PeerConnection(rtc::scoped_refptr context, - bool is_unified_plan, - std::unique_ptr event_log, - std::unique_ptr call, - PeerConnectionDependencies& dependencies); + PeerConnection(rtc::scoped_refptr context, + const PeerConnectionFactoryInterface::Options& options, + bool is_unified_plan, + std::unique_ptr event_log, + std::unique_ptr call, + PeerConnectionDependencies& dependencies); ~PeerConnection() override; @@ -595,6 +599,7 @@ class PeerConnection : public PeerConnectionInternal, InitializeRtcpCallback(); const rtc::scoped_refptr context_; + const PeerConnectionFactoryInterface::Options options_; PeerConnectionObserver* observer_ RTC_GUARDED_BY(signaling_thread()) = nullptr; diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index fec600a01c..b8d9084679 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -114,7 +114,8 @@ PeerConnectionFactory::~PeerConnectionFactory() { } void PeerConnectionFactory::SetOptions(const Options& options) { - context_->SetOptions(options); + RTC_DCHECK_RUN_ON(signaling_thread()); + options_ = options; } RtpCapabilities PeerConnectionFactory::GetRtpSenderCapabilities( @@ -205,7 +206,7 @@ rtc::scoped_refptr PeerConnectionFactory::CreatePeerConnection( const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies) { - RTC_DCHECK(signaling_thread()->IsCurrent()); + RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(!(dependencies.allocator && dependencies.packet_socket_factory)) << "You can't set both allocator and packet_socket_factory; " "the former is going away (see bugs.webrtc.org/7447"; @@ -249,9 +250,9 @@ PeerConnectionFactory::CreatePeerConnection( RTC_FROM_HERE, rtc::Bind(&PeerConnectionFactory::CreateCall_w, this, event_log.get())); - rtc::scoped_refptr pc = - PeerConnection::Create(context_, std::move(event_log), std::move(call), - configuration, std::move(dependencies)); + rtc::scoped_refptr pc = PeerConnection::Create( + context_, options_, std::move(event_log), std::move(call), configuration, + std::move(dependencies)); if (!pc) { return nullptr; } diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h index dcf0eaf603..427207f9cc 100644 --- a/pc/peer_connection_factory.h +++ b/pc/peer_connection_factory.h @@ -107,7 +107,10 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { return context_->signaling_thread(); } - const Options& options() const { return context_->options(); } + const Options& options() const { + RTC_DCHECK_RUN_ON(signaling_thread()); + return options_; + } const WebRtcKeyValueConfig& trials() const { return context_->trials(); } @@ -136,6 +139,8 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { std::unique_ptr CreateCall_w(RtcEventLog* event_log); rtc::scoped_refptr context_; + PeerConnectionFactoryInterface::Options options_ + RTC_GUARDED_BY(signaling_thread()); std::unique_ptr task_queue_factory_; std::unique_ptr event_log_factory_; std::unique_ptr fec_controller_factory_; diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index b625848277..7902a35b05 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -998,7 +998,7 @@ void SdpOfferAnswerHandler::Initialize( webrtc_session_desc_factory_->SignalCertificateReady.connect( this, &SdpOfferAnswerHandler::OnCertificateReady); - if (pc_->context()->options().disable_encryption) { + if (pc_->options()->disable_encryption) { webrtc_session_desc_factory_->SetSdesPolicy(cricket::SEC_DISABLED); }