Change PeerConnection creation to use a static "Create" method
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 <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32485}
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index 6cf6dbb..0e6cd5c 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -640,6 +640,9 @@
// Whether network condition based codec switching is allowed.
absl::optional<bool> allow_codec_switching;
+ // The delay before doing a usage histogram report for long-lived
+ // PeerConnections. Used for testing only.
+ absl::optional<int> 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 b2437fa..7cb39ba 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -209,6 +209,61 @@
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 @@
std::string turn_logging_id;
bool enable_implicit_rollback;
absl::optional<bool> allow_codec_switching;
+ absl::optional<int> 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 @@
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 @@
return !(*this == o);
}
+rtc::scoped_refptr<PeerConnection> PeerConnection::Create(
+ rtc::scoped_refptr<ConnectionContext> context,
+ std::unique_ptr<RtcEventLog> event_log,
+ std::unique_ptr<Call> 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<PeerConnection> pc(
+ new rtc::RefCountedObject<PeerConnection>(
+ 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<ConnectionContext> context,
+ bool is_unified_plan,
std::unique_ptr<RtcEventLog> event_log,
std::unique_ptr<Call> 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 @@
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 @@
#endif
config.active_reset_srtp_params = configuration.active_reset_srtp_params;
- // Obtain a certificate from RTCConfiguration if any were provided (optional).
- rtc::scoped_refptr<rtc::RTCCertificate> 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 @@
RtpTransceiverProxyWithInternal<RtpTransceiver>::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 @@
return true;
}
-RTCError PeerConnection::ValidateConfiguration(
- const RTCConfiguration& config) const {
- return cricket::P2PTransportChannel::ValidateIceConfig(
- ParseIceConfig(config));
-}
-
rtc::scoped_refptr<StreamCollectionInterface> PeerConnection::local_streams() {
RTC_DCHECK_RUN_ON(signaling_thread());
RTC_CHECK(!IsUnifiedPlan()) << "local_streams is not available with Unified "
@@ -1912,47 +1975,6 @@
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<DataChannelStats> PeerConnection::GetDataChannelStats() const {
RTC_DCHECK_RUN_ON(signaling_thread());
return data_channel_controller_.GetDataChannelStats();
@@ -2250,10 +2272,6 @@
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 9edc43a..b7c9f46 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -118,11 +118,16 @@
public JsepTransportController::Observer,
public sigslot::has_slots<> {
public:
- explicit PeerConnection(rtc::scoped_refptr<ConnectionContext> context,
- std::unique_ptr<RtcEventLog> event_log,
- std::unique_ptr<Call> 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<PeerConnection> Create(
+ rtc::scoped_refptr<ConnectionContext> context,
+ std::unique_ptr<RtcEventLog> event_log,
+ std::unique_ptr<Call> call,
const PeerConnectionInterface::RTCConfiguration& configuration,
PeerConnectionDependencies dependencies);
@@ -396,7 +401,7 @@
// 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 @@
void RequestUsagePatternReportForTesting();
protected:
+ // Available for rtc::scoped_refptr creation
+ explicit PeerConnection(rtc::scoped_refptr<ConnectionContext> context,
+ bool is_unified_plan,
+ std::unique_ptr<RtcEventLog> event_log,
+ std::unique_ptr<Call> call);
+
~PeerConnection() override;
private:
+ bool Initialize(
+ const PeerConnectionInterface::RTCConfiguration& configuration,
+ PeerConnectionDependencies dependencies);
+
rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>
FindTransceiverBySender(rtc::scoped_refptr<RtpSenderInterface> sender)
RTC_RUN_ON(signaling_thread());
@@ -527,16 +542,6 @@
// 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 @@
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 @@
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<ConnectionContext> 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<RtcEventLog> event_log_ RTC_GUARDED_BY(worker_thread());
diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc
index 8a99848..237c84d 100644
--- a/pc/peer_connection_factory.cc
+++ b/pc/peer_connection_factory.cc
@@ -251,11 +251,10 @@
RTC_FROM_HERE,
rtc::Bind(&PeerConnectionFactory::CreateCall_w, this, event_log.get()));
- rtc::scoped_refptr<PeerConnection> pc(
- new rtc::RefCountedObject<PeerConnection>(context_, std::move(event_log),
- std::move(call)));
- ActionsBeforeInitializeForTesting(pc);
- if (!pc->Initialize(configuration, std::move(dependencies))) {
+ rtc::scoped_refptr<PeerConnection> 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 499ea0b..dcf0eaf 100644
--- a/pc/peer_connection_factory.h
+++ b/pc/peer_connection_factory.h
@@ -121,10 +121,6 @@
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 a9c5c9b..8730ac4 100644
--- a/pc/peer_connection_histogram_unittest.cc
+++ b/pc/peer_connection_histogram_unittest.cc
@@ -85,18 +85,6 @@
dependencies.call_factory = CreateCallFactory();
return dependencies;
}()) {}
-
- void ActionsBeforeInitializeForTesting(PeerConnectionInterface* pc) override {
- PeerConnection* internal_pc = static_cast<PeerConnection*>(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 @@
}
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 @@
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 @@
auto port_allocator =
std::make_unique<cricket::BasicPortAllocator>(fake_network);
-
return CreatePeerConnection(RTCConfiguration(),
PeerConnectionFactoryInterface::Options(),
- std::move(port_allocator), false);
+ std::move(port_allocator));
}
WrapperPtr CreatePeerConnectionWithPrivateIpv6LocalAddresses() {
@@ -316,32 +302,26 @@
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<cricket::PortAllocator> allocator,
- bool immediate_report) {
+ std::unique_ptr<cricket::PortAllocator> 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<PeerConnectionFactoryForUsageHistogramTest> 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.
