From 6216693363cbeeb7c587d9319643c6eb90d4162d Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 26 Oct 2020 08:30:41 +0000 Subject: [PATCH] Change PeerConnection creation to use a static "Create" method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows making more members (including IsUnifiedPlan) const in a future CL. Also revises the test for ReportUsageHistogram to use a configuration member variable rather than a hook function in PeerConnectionFactory. Bug: webrtc:12079 Change-Id: I6f1af7d6164c8a0d8466f76378a925d72d57d685 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/190280 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Reviewed-by: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#32485} --- api/peer_connection_interface.h | 3 + pc/peer_connection.cc | 188 +++++++++++++---------- pc/peer_connection.h | 51 +++--- pc/peer_connection_factory.cc | 9 +- pc/peer_connection_factory.h | 4 - pc/peer_connection_histogram_unittest.cc | 48 ++---- 6 files changed, 147 insertions(+), 156 deletions(-) diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index 6cf6dbb299..0e6cd5ce07 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -640,6 +640,9 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface { // Whether network condition based codec switching is allowed. absl::optional allow_codec_switching; + // The delay before doing a usage histogram report for long-lived + // PeerConnections. Used for testing only. + absl::optional report_usage_pattern_delay_ms; // // Don't forget to update operator== if adding something. // diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index b2437fa5ac..7cb39ba862 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -209,6 +209,61 @@ bool NeedIceRestart(bool surface_ice_candidates_on_ice_transport_type_changed, return (current_filter & modified_filter) != current_filter; } +cricket::IceConfig ParseIceConfig( + const PeerConnectionInterface::RTCConfiguration& config) { + cricket::ContinualGatheringPolicy gathering_policy; + switch (config.continual_gathering_policy) { + case PeerConnectionInterface::GATHER_ONCE: + gathering_policy = cricket::GATHER_ONCE; + break; + case PeerConnectionInterface::GATHER_CONTINUALLY: + gathering_policy = cricket::GATHER_CONTINUALLY; + break; + default: + RTC_NOTREACHED(); + gathering_policy = cricket::GATHER_ONCE; + } + + cricket::IceConfig ice_config; + ice_config.receiving_timeout = RTCConfigurationToIceConfigOptionalInt( + config.ice_connection_receiving_timeout); + ice_config.prioritize_most_likely_candidate_pairs = + config.prioritize_most_likely_ice_candidate_pairs; + ice_config.backup_connection_ping_interval = + RTCConfigurationToIceConfigOptionalInt( + config.ice_backup_candidate_pair_ping_interval); + ice_config.continual_gathering_policy = gathering_policy; + ice_config.presume_writable_when_fully_relayed = + config.presume_writable_when_fully_relayed; + ice_config.surface_ice_candidates_on_ice_transport_type_changed = + config.surface_ice_candidates_on_ice_transport_type_changed; + ice_config.ice_check_interval_strong_connectivity = + config.ice_check_interval_strong_connectivity; + ice_config.ice_check_interval_weak_connectivity = + config.ice_check_interval_weak_connectivity; + ice_config.ice_check_min_interval = config.ice_check_min_interval; + ice_config.ice_unwritable_timeout = config.ice_unwritable_timeout; + ice_config.ice_unwritable_min_checks = config.ice_unwritable_min_checks; + ice_config.ice_inactive_timeout = config.ice_inactive_timeout; + ice_config.stun_keepalive_interval = config.stun_candidate_keepalive_interval; + ice_config.network_preference = config.network_preference; + return ice_config; +} + +// Ensures the configuration doesn't have any parameters with invalid values, +// or values that conflict with other parameters. +// +// Returns RTCError::OK() if there are no issues. +RTCError ValidateConfiguration( + const PeerConnectionInterface::RTCConfiguration& config) { + return cricket::P2PTransportChannel::ValidateIceConfig( + ParseIceConfig(config)); +} + +bool HasRtcpMuxEnabled(const cricket::ContentInfo* content) { + return content->media_description()->rtcp_mux(); +} + } // namespace bool PeerConnectionInterface::RTCConfiguration::operator==( @@ -263,6 +318,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( std::string turn_logging_id; bool enable_implicit_rollback; absl::optional allow_codec_switching; + absl::optional report_usage_pattern_delay_ms; }; static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this), "Did you add something to RTCConfiguration and forget to " @@ -322,7 +378,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( offer_extmap_allow_mixed == o.offer_extmap_allow_mixed && turn_logging_id == o.turn_logging_id && enable_implicit_rollback == o.enable_implicit_rollback && - allow_codec_switching == o.allow_codec_switching; + allow_codec_switching == o.allow_codec_switching && + report_usage_pattern_delay_ms == o.report_usage_pattern_delay_ms; } bool PeerConnectionInterface::RTCConfiguration::operator!=( @@ -330,10 +387,50 @@ bool PeerConnectionInterface::RTCConfiguration::operator!=( return !(*this == o); } +rtc::scoped_refptr PeerConnection::Create( + rtc::scoped_refptr context, + std::unique_ptr event_log, + std::unique_ptr call, + const PeerConnectionInterface::RTCConfiguration& configuration, + PeerConnectionDependencies dependencies) { + RTCError config_error = cricket::P2PTransportChannel::ValidateIceConfig( + ParseIceConfig(configuration)); + if (!config_error.ok()) { + RTC_LOG(LS_ERROR) << "Invalid configuration: " << config_error.message(); + return nullptr; + } + + if (!dependencies.allocator) { + RTC_LOG(LS_ERROR) + << "PeerConnection initialized without a PortAllocator? " + "This shouldn't happen if using PeerConnectionFactory."; + return nullptr; + } + + if (!dependencies.observer) { + // TODO(deadbeef): Why do we do this? + RTC_LOG(LS_ERROR) << "PeerConnection initialized without a " + "PeerConnectionObserver"; + return nullptr; + } + + bool is_unified_plan = + configuration.sdp_semantics == SdpSemantics::kUnifiedPlan; + rtc::scoped_refptr pc( + new rtc::RefCountedObject( + context, is_unified_plan, std::move(event_log), std::move(call))); + 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) : context_(context), + is_unified_plan_(is_unified_plan), event_log_(std::move(event_log)), event_log_ptr_(event_log_.get()), call_(std::move(call)), @@ -393,26 +490,6 @@ bool PeerConnection::Initialize( RTC_DCHECK_RUN_ON(signaling_thread()); TRACE_EVENT0("webrtc", "PeerConnection::Initialize"); - RTCError config_error = ValidateConfiguration(configuration); - if (!config_error.ok()) { - RTC_LOG(LS_ERROR) << "Invalid configuration: " << config_error.message(); - return false; - } - - if (!dependencies.allocator) { - RTC_LOG(LS_ERROR) - << "PeerConnection initialized without a PortAllocator? " - "This shouldn't happen if using PeerConnectionFactory."; - return false; - } - - if (!dependencies.observer) { - // TODO(deadbeef): Why do we do this? - RTC_LOG(LS_ERROR) << "PeerConnection initialized without a " - "PeerConnectionObserver"; - return false; - } - observer_ = dependencies.observer; async_resolver_factory_ = std::move(dependencies.async_resolver_factory); port_allocator_ = std::move(dependencies.allocator); @@ -487,20 +564,12 @@ bool PeerConnection::Initialize( #endif config.active_reset_srtp_params = configuration.active_reset_srtp_params; - // Obtain a certificate from RTCConfiguration if any were provided (optional). - rtc::scoped_refptr certificate; - if (!configuration.certificates.empty()) { - // TODO(hbos,torbjorng): Decide on certificate-selection strategy instead of - // just picking the first one. The decision should be made based on the DTLS - // handshake. The DTLS negotiations need to know about all certificates. - certificate = configuration.certificates[0]; - } - if (options.disable_encryption) { dtls_enabled_ = false; } else { // Enable DTLS by default if we have an identity store or a certificate. - dtls_enabled_ = (dependencies.cert_generator || certificate); + dtls_enabled_ = + (dependencies.cert_generator || !configuration.certificates.empty()); // |configuration| can override the default |dtls_enabled_| value. if (configuration.enable_dtls_srtp) { dtls_enabled_ = *(configuration.enable_dtls_srtp); @@ -571,11 +640,11 @@ bool PeerConnection::Initialize( RtpTransceiverProxyWithInternal::Create( signaling_thread(), new RtpTransceiver(cricket::MEDIA_TYPE_VIDEO))); } - int delay_ms = - return_histogram_very_quickly_ ? 0 : REPORT_USAGE_PATTERN_DELAY_MS; - sdp_handler_.Initialize(configuration, &dependencies); + int delay_ms = configuration.report_usage_pattern_delay_ms + ? *configuration.report_usage_pattern_delay_ms + : REPORT_USAGE_PATTERN_DELAY_MS; message_handler_.RequestUsagePatternReport( [this]() { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -586,12 +655,6 @@ bool PeerConnection::Initialize( return true; } -RTCError PeerConnection::ValidateConfiguration( - const RTCConfiguration& config) const { - return cricket::P2PTransportChannel::ValidateIceConfig( - ParseIceConfig(config)); -} - rtc::scoped_refptr PeerConnection::local_streams() { RTC_DCHECK_RUN_ON(signaling_thread()); RTC_CHECK(!IsUnifiedPlan()) << "local_streams is not available with Unified " @@ -1912,47 +1975,6 @@ bool PeerConnection::GetTransportDescription( return true; } -cricket::IceConfig PeerConnection::ParseIceConfig( - const PeerConnectionInterface::RTCConfiguration& config) const { - cricket::ContinualGatheringPolicy gathering_policy; - switch (config.continual_gathering_policy) { - case PeerConnectionInterface::GATHER_ONCE: - gathering_policy = cricket::GATHER_ONCE; - break; - case PeerConnectionInterface::GATHER_CONTINUALLY: - gathering_policy = cricket::GATHER_CONTINUALLY; - break; - default: - RTC_NOTREACHED(); - gathering_policy = cricket::GATHER_ONCE; - } - - cricket::IceConfig ice_config; - ice_config.receiving_timeout = RTCConfigurationToIceConfigOptionalInt( - config.ice_connection_receiving_timeout); - ice_config.prioritize_most_likely_candidate_pairs = - config.prioritize_most_likely_ice_candidate_pairs; - ice_config.backup_connection_ping_interval = - RTCConfigurationToIceConfigOptionalInt( - config.ice_backup_candidate_pair_ping_interval); - ice_config.continual_gathering_policy = gathering_policy; - ice_config.presume_writable_when_fully_relayed = - config.presume_writable_when_fully_relayed; - ice_config.surface_ice_candidates_on_ice_transport_type_changed = - config.surface_ice_candidates_on_ice_transport_type_changed; - ice_config.ice_check_interval_strong_connectivity = - config.ice_check_interval_strong_connectivity; - ice_config.ice_check_interval_weak_connectivity = - config.ice_check_interval_weak_connectivity; - ice_config.ice_check_min_interval = config.ice_check_min_interval; - ice_config.ice_unwritable_timeout = config.ice_unwritable_timeout; - ice_config.ice_unwritable_min_checks = config.ice_unwritable_min_checks; - ice_config.ice_inactive_timeout = config.ice_inactive_timeout; - ice_config.stun_keepalive_interval = config.stun_candidate_keepalive_interval; - ice_config.network_preference = config.network_preference; - return ice_config; -} - std::vector PeerConnection::GetDataChannelStats() const { RTC_DCHECK_RUN_ON(signaling_thread()); return data_channel_controller_.GetDataChannelStats(); @@ -2250,10 +2272,6 @@ bool PeerConnection::ValidateBundleSettings(const SessionDescription* desc) { return true; } -bool PeerConnection::HasRtcpMuxEnabled(const cricket::ContentInfo* content) { - return content->media_description()->rtcp_mux(); -} - void PeerConnection::ReportSdpFormatReceived( const SessionDescriptionInterface& remote_offer) { int num_audio_mlines = 0; diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 9edc43a962..b7c9f4614e 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -118,11 +118,16 @@ class PeerConnection : public PeerConnectionInternal, public JsepTransportController::Observer, public sigslot::has_slots<> { public: - explicit PeerConnection(rtc::scoped_refptr context, - std::unique_ptr event_log, - std::unique_ptr call); - - bool Initialize( + // Creates a PeerConnection and initializes it with the given values. + // If the initialization fails, the function releases the PeerConnection + // and returns nullptr. + // + // Note that the function takes ownership of dependencies, and will + // either use them or release them, whether it succeeds or fails. + static rtc::scoped_refptr Create( + rtc::scoped_refptr context, + std::unique_ptr event_log, + std::unique_ptr call, const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies dependencies); @@ -396,7 +401,7 @@ class PeerConnection : public PeerConnectionInternal, // sufficient time has passed. bool IsUnifiedPlan() const { RTC_DCHECK_RUN_ON(signaling_thread()); - return configuration_.sdp_semantics == SdpSemantics::kUnifiedPlan; + return is_unified_plan_; } bool ValidateBundleSettings(const cricket::SessionDescription* desc); @@ -454,9 +459,19 @@ class PeerConnection : public PeerConnectionInternal, void RequestUsagePatternReportForTesting(); 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); + ~PeerConnection() override; private: + bool Initialize( + const PeerConnectionInterface::RTCConfiguration& configuration, + PeerConnectionDependencies dependencies); + rtc::scoped_refptr> FindTransceiverBySender(rtc::scoped_refptr sender) RTC_RUN_ON(signaling_thread()); @@ -527,16 +542,6 @@ class PeerConnection : public PeerConnectionInternal, // This function should only be called from the worker thread. void StopRtcEventLog_w(); - // Ensures the configuration doesn't have any parameters with invalid values, - // or values that conflict with other parameters. - // - // Returns RTCError::OK() if there are no issues. - RTCError ValidateConfiguration(const RTCConfiguration& config) const; - - cricket::IceConfig ParseIceConfig( - const PeerConnectionInterface::RTCConfiguration& config) const; - - // Returns true and the TransportInfo of the given |content_name| // from |description|. Returns false if it's not available. static bool GetTransportDescription( @@ -551,12 +556,6 @@ class PeerConnection : public PeerConnectionInternal, int* sdp_mline_index) RTC_RUN_ON(signaling_thread()); - bool HasRtcpMuxEnabled(const cricket::ContentInfo* content); - - // Verifies a=setup attribute as per RFC 5763. - bool ValidateDtlsSetupAttribute(const cricket::SessionDescription* desc, - SdpType type); - // JsepTransportController signal handlers. void OnTransportControllerConnectionState(cricket::IceConnectionState state) RTC_RUN_ON(signaling_thread()); @@ -608,16 +607,12 @@ class PeerConnection : public PeerConnectionInternal, int64_t packet_time_us)> InitializeRtcpCallback(); - // Storing the factory as a scoped reference pointer ensures that the memory - // in the PeerConnectionFactoryImpl remains available as long as the - // PeerConnection is running. It is passed to PeerConnection as a raw pointer. - // However, since the reference counting is done in the - // PeerConnectionFactoryInterface all instances created using the raw pointer - // will refer to the same reference count. const rtc::scoped_refptr context_; PeerConnectionObserver* observer_ RTC_GUARDED_BY(signaling_thread()) = nullptr; + const bool is_unified_plan_; + // The EventLog needs to outlive |call_| (and any other object that uses it). std::unique_ptr event_log_ RTC_GUARDED_BY(worker_thread()); diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index 8a998481c1..237c84d3ff 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -251,11 +251,10 @@ PeerConnectionFactory::CreatePeerConnection( RTC_FROM_HERE, rtc::Bind(&PeerConnectionFactory::CreateCall_w, this, event_log.get())); - rtc::scoped_refptr pc( - new rtc::RefCountedObject(context_, std::move(event_log), - std::move(call))); - ActionsBeforeInitializeForTesting(pc); - if (!pc->Initialize(configuration, std::move(dependencies))) { + rtc::scoped_refptr pc = + PeerConnection::Create(context_, std::move(event_log), std::move(call), + configuration, std::move(dependencies)); + if (!pc) { return nullptr; } return PeerConnectionProxy::Create(signaling_thread(), pc); diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h index 499ea0b855..dcf0eaf603 100644 --- a/pc/peer_connection_factory.h +++ b/pc/peer_connection_factory.h @@ -121,10 +121,6 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { explicit PeerConnectionFactory( PeerConnectionFactoryDependencies dependencies); - // Hook to let testing framework insert actions between - // "new RTCPeerConnection" and "pc.Initialize" - virtual void ActionsBeforeInitializeForTesting(PeerConnectionInterface*) {} - virtual ~PeerConnectionFactory(); private: diff --git a/pc/peer_connection_histogram_unittest.cc b/pc/peer_connection_histogram_unittest.cc index a9c5c9b322..8730ac4bb4 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc @@ -85,18 +85,6 @@ class PeerConnectionFactoryForUsageHistogramTest dependencies.call_factory = CreateCallFactory(); return dependencies; }()) {} - - void ActionsBeforeInitializeForTesting(PeerConnectionInterface* pc) override { - PeerConnection* internal_pc = static_cast(pc); - if (return_histogram_very_quickly_) { - internal_pc->ReturnHistogramVeryQuicklyForTesting(); - } - } - - void ReturnHistogramVeryQuickly() { return_histogram_very_quickly_ = true; } - - private: - bool return_histogram_very_quickly_ = false; }; class PeerConnectionWrapperForUsageHistogramTest; @@ -255,14 +243,13 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { } WrapperPtr CreatePeerConnection() { - return CreatePeerConnection(RTCConfiguration(), - PeerConnectionFactoryInterface::Options(), - nullptr, false); + return CreatePeerConnection( + RTCConfiguration(), PeerConnectionFactoryInterface::Options(), nullptr); } WrapperPtr CreatePeerConnection(const RTCConfiguration& config) { return CreatePeerConnection( - config, PeerConnectionFactoryInterface::Options(), nullptr, false); + config, PeerConnectionFactoryInterface::Options(), nullptr); } WrapperPtr CreatePeerConnectionWithMdns(const RTCConfiguration& config) { @@ -282,15 +269,15 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { deps.async_resolver_factory = std::move(resolver_factory); deps.allocator = std::move(port_allocator); - return CreatePeerConnection(config, - PeerConnectionFactoryInterface::Options(), - std::move(deps), false); + return CreatePeerConnection( + config, PeerConnectionFactoryInterface::Options(), std::move(deps)); } WrapperPtr CreatePeerConnectionWithImmediateReport() { - return CreatePeerConnection(RTCConfiguration(), - PeerConnectionFactoryInterface::Options(), - nullptr, true); + RTCConfiguration configuration; + configuration.report_usage_pattern_delay_ms = 0; + return CreatePeerConnection( + configuration, PeerConnectionFactoryInterface::Options(), nullptr); } WrapperPtr CreatePeerConnectionWithPrivateLocalAddresses() { @@ -300,10 +287,9 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { auto port_allocator = std::make_unique(fake_network); - return CreatePeerConnection(RTCConfiguration(), PeerConnectionFactoryInterface::Options(), - std::move(port_allocator), false); + std::move(port_allocator)); } WrapperPtr CreatePeerConnectionWithPrivateIpv6LocalAddresses() { @@ -316,32 +302,26 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { return CreatePeerConnection(RTCConfiguration(), PeerConnectionFactoryInterface::Options(), - std::move(port_allocator), false); + std::move(port_allocator)); } WrapperPtr CreatePeerConnection( const RTCConfiguration& config, const PeerConnectionFactoryInterface::Options factory_options, - std::unique_ptr allocator, - bool immediate_report) { + std::unique_ptr allocator) { PeerConnectionDependencies deps(nullptr); deps.allocator = std::move(allocator); - return CreatePeerConnection(config, factory_options, std::move(deps), - immediate_report); + return CreatePeerConnection(config, factory_options, std::move(deps)); } WrapperPtr CreatePeerConnection( const RTCConfiguration& config, const PeerConnectionFactoryInterface::Options factory_options, - PeerConnectionDependencies deps, - bool immediate_report) { + PeerConnectionDependencies deps) { rtc::scoped_refptr pc_factory( new PeerConnectionFactoryForUsageHistogramTest()); pc_factory->SetOptions(factory_options); - if (immediate_report) { - pc_factory->ReturnHistogramVeryQuickly(); - } // If no allocator is provided, one will be created using a network manager // that uses the host network. This doesn't work on all trybots.