From d5585ca95632b94a6069b31edda53c0b45647e7d Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Mon, 23 Oct 2017 14:49:26 -0700 Subject: [PATCH] Move almost all references from WebRtcSession to PeerConnection WebRtcSession is being merged into PeerConnection, and to make the code review easier this is the first step towards achieving that. Bug: webrtc:8323 Change-Id: I33778e46f20cb14089dff4328947868e207476bd Reviewed-on: https://webrtc-review.googlesource.com/8760 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#20413} --- pc/peerconnection.cc | 6 +-- pc/peerconnection.h | 9 ++++ pc/rtcstatscollector.cc | 3 +- pc/statscollector.cc | 5 +- pc/statscollector.h | 3 +- pc/webrtcsession.cc | 33 +++++------- pc/webrtcsession.h | 13 +++-- pc/webrtcsessiondescriptionfactory.cc | 75 ++++++++------------------- pc/webrtcsessiondescriptionfactory.h | 32 ++++-------- 9 files changed, 68 insertions(+), 111 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 5007f3351c..0420b602f6 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -487,10 +487,8 @@ bool PeerConnection::Initialize( stats_collector_ = RTCStatsCollector::Create(this); // Initialize the WebRtcSession. It creates transport channels etc. - if (!session_->Initialize(factory_->options(), std::move(cert_generator), - configuration)) { - return false; - } + session_->Initialize(factory_->options(), std::move(cert_generator), + configuration, this); // Register PeerConnection as receiver of local ice candidates. // All the callbacks will be posted to the application from PeerConnection. diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 99737f90a4..9163c36631 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -209,6 +209,15 @@ class PeerConnection : public PeerConnectionInterface, virtual bool GetRemoteTrackIdBySsrc(uint32_t ssrc, std::string* track_id) { return session_->GetRemoteTrackIdBySsrc(ssrc, track_id); } + bool IceRestartPending(const std::string& content_name) const { + return session_->IceRestartPending(content_name); + } + bool NeedsIceRestart(const std::string& content_name) const { + return session_->NeedsIceRestart(content_name); + } + bool GetSslRole(const std::string& content_name, rtc::SSLRole* role) { + return session_->GetSslRole(content_name, role); + } // This is needed for stats tests to inject a MockWebRtcSession. Once // WebRtcSession has been merged in, this will no longer be needed. diff --git a/pc/rtcstatscollector.cc b/pc/rtcstatscollector.cc index 36a4d6eb06..d20f2ed2fc 100644 --- a/pc/rtcstatscollector.cc +++ b/pc/rtcstatscollector.cc @@ -22,7 +22,6 @@ #include "p2p/base/p2pconstants.h" #include "p2p/base/port.h" #include "pc/peerconnection.h" -#include "pc/webrtcsession.h" #include "rtc_base/checks.h" #include "rtc_base/stringutils.h" #include "rtc_base/timeutils.h" @@ -77,7 +76,7 @@ std::string RTCTransportStatsIDFromTransportChannel( } std::string RTCTransportStatsIDFromBaseChannel( - const ProxyTransportMap& proxy_to_transport, + const std::map& proxy_to_transport, const cricket::BaseChannel& base_channel) { auto proxy_it = proxy_to_transport.find(base_channel.content_name()); if (proxy_it == proxy_to_transport.cend()) diff --git a/pc/statscollector.cc b/pc/statscollector.cc index eedd2286bc..919ea464f3 100644 --- a/pc/statscollector.cc +++ b/pc/statscollector.cc @@ -50,8 +50,9 @@ typedef TypeForAdd FloatForAdd; typedef TypeForAdd Int64ForAdd; typedef TypeForAdd IntForAdd; -StatsReport::Id GetTransportIdFromProxy(const ProxyTransportMap& map, - const std::string& proxy) { +StatsReport::Id GetTransportIdFromProxy( + const std::map& map, + const std::string& proxy) { RTC_DCHECK(!proxy.empty()); auto found = map.find(proxy); if (found == map.end()) { diff --git a/pc/statscollector.h b/pc/statscollector.h index 568d1ee82b..5c9006646a 100644 --- a/pc/statscollector.h +++ b/pc/statscollector.h @@ -21,7 +21,6 @@ #include "api/mediastreaminterface.h" #include "api/peerconnectioninterface.h" #include "api/statstypes.h" -#include "pc/webrtcsession.h" namespace webrtc { @@ -139,7 +138,7 @@ class StatsCollector { // Raw pointer to the peer connection the statistics are gathered from. PeerConnection* const pc_; double stats_gathering_started_; - ProxyTransportMap proxy_to_transport_; + std::map proxy_to_transport_; // TODO(tommi): We appear to be holding on to raw pointers to reference // counted objects? We should be using scoped_refptr here. diff --git a/pc/webrtcsession.cc b/pc/webrtcsession.cc index bbe9964e4d..66b515c5ea 100644 --- a/pc/webrtcsession.cc +++ b/pc/webrtcsession.cc @@ -536,10 +536,11 @@ WebRtcSession::~WebRtcSession() { LOG(LS_INFO) << "Session: " << id() << " is destroyed."; } -bool WebRtcSession::Initialize( +void WebRtcSession::Initialize( const PeerConnectionFactoryInterface::Options& options, std::unique_ptr cert_generator, - const PeerConnectionInterface::RTCConfiguration& rtc_configuration) { + const PeerConnectionInterface::RTCConfiguration& rtc_configuration, + PeerConnection* pc) { bundle_policy_ = rtc_configuration.bundle_policy; rtcp_mux_policy_ = rtc_configuration.rtcp_mux_policy; transport_controller_->SetSslMaxProtocolVersion(options.ssl_max_version); @@ -589,24 +590,20 @@ bool WebRtcSession::Initialize( audio_options_.audio_jitter_buffer_fast_accelerate = rtc::Optional( rtc_configuration.audio_jitter_buffer_fast_accelerate); + // Whether the certificate generator/certificate is null or not determines + // what WebRtcSessionDescriptionFactory will do, so make sure that we give it + // the right instructions by clearing the variables if needed. if (!dtls_enabled_) { - // Construct with DTLS disabled. - webrtc_session_desc_factory_.reset(new WebRtcSessionDescriptionFactory( - signaling_thread(), channel_manager_, this, id(), - std::unique_ptr())); - } else { - // Construct with DTLS enabled. - if (!certificate) { - webrtc_session_desc_factory_.reset(new WebRtcSessionDescriptionFactory( - signaling_thread(), channel_manager_, this, id(), - std::move(cert_generator))); - } else { - // Use the already generated certificate. - webrtc_session_desc_factory_.reset(new WebRtcSessionDescriptionFactory( - signaling_thread(), channel_manager_, this, id(), certificate)); - } + cert_generator.reset(); + certificate = nullptr; + } else if (certificate) { + // Favor generated certificate over the certificate generator. + cert_generator.reset(); } + webrtc_session_desc_factory_.reset(new WebRtcSessionDescriptionFactory( + signaling_thread(), channel_manager_, pc, id(), std::move(cert_generator), + certificate)); webrtc_session_desc_factory_->SignalCertificateReady.connect( this, &WebRtcSession::OnCertificateReady); @@ -616,8 +613,6 @@ bool WebRtcSession::Initialize( webrtc_session_desc_factory_->set_enable_encrypted_rtp_header_extensions( options.crypto_options.enable_encrypted_rtp_header_extensions); - - return true; } void WebRtcSession::Close() { diff --git a/pc/webrtcsession.h b/pc/webrtcsession.h index 37d553a89c..07202eec80 100644 --- a/pc/webrtcsession.h +++ b/pc/webrtcsession.h @@ -46,6 +46,7 @@ namespace webrtc { class IceRestartAnswerLatch; class JsepIceCandidate; class MediaStreamSignaling; +class PeerConnection; class RtcEventLog; class WebRtcSessionDescriptionFactory; @@ -99,14 +100,11 @@ class IceObserver { }; // Statistics for all the transports of the session. -typedef std::map TransportStatsMap; -typedef std::map ProxyTransportMap; - // TODO(pthatcher): Think of a better name for this. We already have // a TransportStats in transport.h. Perhaps TransportsStats? struct SessionStats { - ProxyTransportMap proxy_to_transport; - TransportStatsMap transport_stats; + std::map proxy_to_transport; + std::map transport_stats; }; struct ChannelNamePair { @@ -172,10 +170,11 @@ class WebRtcSession : // The ID of this session. const std::string& id() const { return sid_; } - bool Initialize( + void Initialize( const PeerConnectionFactoryInterface::Options& options, std::unique_ptr cert_generator, - const PeerConnectionInterface::RTCConfiguration& rtc_configuration); + const PeerConnectionInterface::RTCConfiguration& rtc_configuration, + PeerConnection* pc); // Deletes the voice, video and data channel and changes the session state // to STATE_CLOSED. void Close(); diff --git a/pc/webrtcsessiondescriptionfactory.cc b/pc/webrtcsessiondescriptionfactory.cc index ea3e6aede0..bbab6462bc 100644 --- a/pc/webrtcsessiondescriptionfactory.cc +++ b/pc/webrtcsessiondescriptionfactory.cc @@ -15,7 +15,7 @@ #include "api/jsep.h" #include "api/jsepsessiondescription.h" #include "api/mediaconstraintsinterface.h" -#include "pc/webrtcsession.h" +#include "pc/peerconnection.h" #include "rtc_base/checks.h" #include "rtc_base/sslidentity.h" @@ -118,7 +118,7 @@ void WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription( WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( rtc::Thread* signaling_thread, cricket::ChannelManager* channel_manager, - WebRtcSession* session, + PeerConnection* pc, const std::string& session_id, std::unique_ptr cert_generator, const rtc::scoped_refptr& certificate) @@ -130,10 +130,11 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( // |kInitSessionVersion|. session_version_(kInitSessionVersion), cert_generator_(std::move(cert_generator)), - session_(session), + pc_(pc), session_id_(session_id), certificate_request_state_(CERTIFICATE_NOT_NEEDED) { RTC_DCHECK(signaling_thread_); + RTC_DCHECK(!(cert_generator_ && certificate)); bool dtls_enabled = cert_generator_ || certificate; // SRTP-SDES is disabled if DTLS is on. SetSdesPolicy(dtls_enabled ? cricket::SEC_DISABLED : cricket::SEC_REQUIRED); @@ -175,36 +176,6 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( } } -WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( - rtc::Thread* signaling_thread, - cricket::ChannelManager* channel_manager, - WebRtcSession* session, - const std::string& session_id, - std::unique_ptr cert_generator) - : WebRtcSessionDescriptionFactory( - signaling_thread, - channel_manager, - session, - session_id, - std::move(cert_generator), - nullptr) { -} - -WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( - rtc::Thread* signaling_thread, - cricket::ChannelManager* channel_manager, - WebRtcSession* session, - const std::string& session_id, - const rtc::scoped_refptr& certificate) - : WebRtcSessionDescriptionFactory(signaling_thread, - channel_manager, - session, - session_id, - nullptr, - certificate) { - RTC_DCHECK(certificate); -} - WebRtcSessionDescriptionFactory::~WebRtcSessionDescriptionFactory() { RTC_DCHECK(signaling_thread_->IsCurrent()); @@ -270,14 +241,13 @@ void WebRtcSessionDescriptionFactory::CreateAnswer( PostCreateSessionDescriptionFailed(observer, error); return; } - if (!session_->remote_description()) { + if (!pc_->remote_description()) { error += " can't be called before SetRemoteDescription."; LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailed(observer, error); return; } - if (session_->remote_description()->type() != - JsepSessionDescription::kOffer) { + if (pc_->remote_description()->type() != JsepSessionDescription::kOffer) { error += " failed because remote_description is not an offer."; LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailed(observer, error); @@ -344,20 +314,20 @@ void WebRtcSessionDescriptionFactory::OnMessage(rtc::Message* msg) { void WebRtcSessionDescriptionFactory::InternalCreateOffer( CreateSessionDescriptionRequest request) { - if (session_->local_description()) { + if (pc_->local_description()) { // If the needs-ice-restart flag is set as described by JSEP, we should // generate an offer with a new ufrag/password to trigger an ICE restart. for (cricket::MediaDescriptionOptions& options : request.options.media_description_options) { - if (session_->NeedsIceRestart(options.mid)) { + if (pc_->NeedsIceRestart(options.mid)) { options.transport_options.ice_restart = true; } } } cricket::SessionDescription* desc(session_desc_factory_.CreateOffer( - request.options, session_->local_description() - ? session_->local_description()->description() + request.options, pc_->local_description() + ? pc_->local_description()->description() : nullptr)); // RFC 3264 // When issuing an offer that modifies the session, @@ -378,11 +348,11 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( "Failed to initialize the offer."); return; } - if (session_->local_description()) { + if (pc_->local_description()) { for (const cricket::MediaDescriptionOptions& options : request.options.media_description_options) { if (!options.transport_options.ice_restart) { - CopyCandidatesFromSessionDescription(session_->local_description(), + CopyCandidatesFromSessionDescription(pc_->local_description(), options.mid, offer); } } @@ -392,18 +362,18 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( void WebRtcSessionDescriptionFactory::InternalCreateAnswer( CreateSessionDescriptionRequest request) { - if (session_->remote_description()) { + if (pc_->remote_description()) { for (cricket::MediaDescriptionOptions& options : request.options.media_description_options) { // According to http://tools.ietf.org/html/rfc5245#section-9.2.1.1 // an answer should also contain new ICE ufrag and password if an offer // has been received with new ufrag and password. options.transport_options.ice_restart = - session_->IceRestartPending(options.mid); + pc_->IceRestartPending(options.mid); // We should pass the current SSL role to the transport description // factory, if there is already an existing ongoing session. rtc::SSLRole ssl_role; - if (session_->GetSslRole(options.mid, &ssl_role)) { + if (pc_->GetSslRole(options.mid, &ssl_role)) { options.transport_options.prefer_passive_role = (rtc::SSL_SERVER == ssl_role); } @@ -411,12 +381,11 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer( } cricket::SessionDescription* desc(session_desc_factory_.CreateAnswer( - session_->remote_description() - ? session_->remote_description()->description() - : nullptr, - request.options, session_->local_description() - ? session_->local_description()->description() - : nullptr)); + pc_->remote_description() ? pc_->remote_description()->description() + : nullptr, + request.options, + pc_->local_description() ? pc_->local_description()->description() + : nullptr)); // RFC 3264 // If the answer is different from the offer in any way (different IP // addresses, ports, etc.), the origin line MUST be different in the answer. @@ -434,13 +403,13 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer( "Failed to initialize the answer."); return; } - if (session_->local_description()) { + if (pc_->local_description()) { // Include all local ICE candidates in the SessionDescription unless // the remote peer has requested an ICE restart. for (const cricket::MediaDescriptionOptions& options : request.options.media_description_options) { if (!options.transport_options.ice_restart) { - CopyCandidatesFromSessionDescription(session_->local_description(), + CopyCandidatesFromSessionDescription(pc_->local_description(), options.mid, answer); } } diff --git a/pc/webrtcsessiondescriptionfactory.h b/pc/webrtcsessiondescriptionfactory.h index 231f718ad4..5c3e18ed8a 100644 --- a/pc/webrtcsessiondescriptionfactory.h +++ b/pc/webrtcsessiondescriptionfactory.h @@ -29,6 +29,7 @@ class TransportDescriptionFactory; namespace webrtc { class CreateSessionDescriptionObserver; class MediaConstraintsInterface; +class PeerConnection; class SessionDescriptionInterface; class WebRtcSession; @@ -74,20 +75,16 @@ struct CreateSessionDescriptionRequest { class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, public sigslot::has_slots<> { public: - // If |certificate_generator| is not null, DTLS is enabled and a default - // certificate is generated asynchronously; otherwise DTLS is disabled. + // Can specify either a |cert_generator| or |certificate| to enable DTLS. If + // a certificate generator is given, starts generating the certificate + // asynchronously. If a certificate is given, will use that for identifying + // over DTLS. If neither is specified, DTLS is disabled. WebRtcSessionDescriptionFactory( rtc::Thread* signaling_thread, cricket::ChannelManager* channel_manager, - WebRtcSession* session, - const std::string& session_id, - std::unique_ptr cert_generator); - // Construct with DTLS enabled using the specified |certificate|. - WebRtcSessionDescriptionFactory( - rtc::Thread* signaling_thread, - cricket::ChannelManager* channel_manager, - WebRtcSession* session, + PeerConnection* pc, const std::string& session_id, + std::unique_ptr cert_generator, const rtc::scoped_refptr& certificate); virtual ~WebRtcSessionDescriptionFactory(); @@ -126,16 +123,6 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, CERTIFICATE_FAILED, }; - // If |certificate_generator| or |certificate| is not null DTLS is enabled, - // otherwise DTLS is disabled. - WebRtcSessionDescriptionFactory( - rtc::Thread* signaling_thread, - cricket::ChannelManager* channel_manager, - WebRtcSession* session, - const std::string& session_id, - std::unique_ptr cert_generator, - const rtc::scoped_refptr& certificate); - // MessageHandler implementation. virtual void OnMessage(rtc::Message* msg); @@ -161,8 +148,9 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, cricket::MediaSessionDescriptionFactory session_desc_factory_; uint64_t session_version_; const std::unique_ptr cert_generator_; - // TODO(jiayl): remove the dependency on session once bug 2264 is fixed. - WebRtcSession* const session_; + // TODO(jiayl): remove the dependency on peer connection once bug 2264 is + // fixed. + PeerConnection* const pc_; const std::string session_id_; CertificateRequestState certificate_request_state_;