From d89ce53dafa9ca7b33a721363fcadd37b9fef138 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Wed, 21 Oct 2020 08:59:22 +0000 Subject: [PATCH] Make WebRtcSessionDescriptionFactory depend on SdpOfferAnswerHandler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This factory is only used by SdpOfferAnswerHandler, so it should not need to depend on PeerConnection. Bug: webrtc:11995 Change-Id: Ib27d9d9fdf440be7db8890bf0e7520d0c67bde22 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/189780 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#32460} --- pc/peer_connection.cc | 15 +----- pc/sdp_offer_answer.h | 4 +- pc/webrtc_session_description_factory.cc | 65 +++++++++++++----------- pc/webrtc_session_description_factory.h | 17 ++++--- 4 files changed, 52 insertions(+), 49 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 9f3f6ba65b..8a72bcf2d1 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -581,21 +581,10 @@ bool PeerConnection::Initialize( audio_options_.audio_jitter_buffer_enable_rtx_handling = configuration.audio_jitter_buffer_enable_rtx_handling; - // Whether the certificate generator/certificate is null or not determines - // what PeerConnectionDescriptionFactory will do, so make sure that we give it - // the right instructions by clearing the variables if needed. - if (!dtls_enabled_) { - dependencies.cert_generator.reset(); - certificate = nullptr; - } else if (certificate) { - // Favor generated certificate over the certificate generator. - dependencies.cert_generator.reset(); - } - auto webrtc_session_desc_factory = std::make_unique( - signaling_thread(), channel_manager(), this, session_id(), - std::move(dependencies.cert_generator), certificate, + signaling_thread(), channel_manager(), &sdp_handler_, session_id(), + dtls_enabled_, std::move(dependencies.cert_generator), certificate, &ssrc_generator_); webrtc_session_desc_factory->SignalCertificateReady.connect( this, &PeerConnection::OnCertificateReady); diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index da8b3a2b3a..0880c1d94e 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -76,6 +76,7 @@ class VideoRtpReceiver; class RtcEventLog; class RtpTransmissionManager; class TransceiverList; +class WebRtcSessionDescriptionFactory; // SdpOfferAnswerHandler is a component // of the PeerConnection object as defined @@ -118,6 +119,8 @@ class SdpOfferAnswerHandler { const SessionDescriptionInterface* pending_local_description() const; const SessionDescriptionInterface* pending_remote_description() const; + JsepTransportController* transport_controller(); + void RestartIce(); // JSEP01 @@ -528,7 +531,6 @@ class SdpOfferAnswerHandler { cricket::ChannelManager* channel_manager() const; TransceiverList* transceivers(); const TransceiverList* transceivers() const; - JsepTransportController* transport_controller(); DataChannelController* data_channel_controller(); const DataChannelController* data_channel_controller() const; cricket::PortAllocator* port_allocator(); diff --git a/pc/webrtc_session_description_factory.cc b/pc/webrtc_session_description_factory.cc index aaef7fdeb6..2409805adb 100644 --- a/pc/webrtc_session_description_factory.cc +++ b/pc/webrtc_session_description_factory.cc @@ -11,7 +11,7 @@ #include "pc/webrtc_session_description_factory.h" #include - +#include #include #include #include @@ -22,6 +22,8 @@ #include "api/jsep.h" #include "api/jsep_session_description.h" #include "api/rtc_error.h" +#include "pc/jsep_transport_controller.h" +#include "pc/sdp_offer_answer.h" #include "pc/session_description.h" #include "rtc_base/checks.h" #include "rtc_base/location.h" @@ -125,8 +127,9 @@ void WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription( WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( rtc::Thread* signaling_thread, cricket::ChannelManager* channel_manager, - PeerConnectionInternal* pc, + SdpOfferAnswerHandler* sdp_handler, const std::string& session_id, + bool dtls_enabled, std::unique_ptr cert_generator, const rtc::scoped_refptr& certificate, UniqueRandomIdGenerator* ssrc_generator) @@ -139,20 +142,20 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( // to just use a random number as session id and start version from // |kInitSessionVersion|. session_version_(kInitSessionVersion), - cert_generator_(std::move(cert_generator)), - pc_(pc), + cert_generator_(dtls_enabled ? std::move(cert_generator) : nullptr), + sdp_handler_(sdp_handler), 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); + if (!dtls_enabled) { + SetSdesPolicy(cricket::SEC_REQUIRED); RTC_LOG(LS_VERBOSE) << "DTLS-SRTP disabled."; return; } + // SRTP-SDES is disabled if DTLS is on. + SetSdesPolicy(cricket::SEC_DISABLED); if (certificate) { // Use |certificate|. certificate_request_state_ = CERTIFICATE_WAITING; @@ -252,13 +255,13 @@ void WebRtcSessionDescriptionFactory::CreateAnswer( PostCreateSessionDescriptionFailed(observer, error); return; } - if (!pc_->remote_description()) { + if (!sdp_handler_->remote_description()) { error += " can't be called before SetRemoteDescription."; RTC_LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailed(observer, error); return; } - if (pc_->remote_description()->GetType() != SdpType::kOffer) { + if (sdp_handler_->remote_description()->GetType() != SdpType::kOffer) { error += " failed because remote_description is not an offer."; RTC_LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailed(observer, error); @@ -325,12 +328,12 @@ void WebRtcSessionDescriptionFactory::OnMessage(rtc::Message* msg) { void WebRtcSessionDescriptionFactory::InternalCreateOffer( CreateSessionDescriptionRequest request) { - if (pc_->local_description()) { + if (sdp_handler_->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 (pc_->NeedsIceRestart(options.mid)) { + if (sdp_handler_->transport_controller()->NeedsIceRestart(options.mid)) { options.transport_options.ice_restart = true; } } @@ -338,9 +341,10 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( std::unique_ptr desc = session_desc_factory_.CreateOffer( - request.options, pc_->local_description() - ? pc_->local_description()->description() - : nullptr); + request.options, + sdp_handler_->local_description() + ? sdp_handler_->local_description()->description() + : nullptr); if (!desc) { PostCreateSessionDescriptionFailed(request.observer, "Failed to initialize the offer."); @@ -360,11 +364,11 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( auto offer = std::make_unique( SdpType::kOffer, std::move(desc), session_id_, rtc::ToString(session_version_++)); - if (pc_->local_description()) { + if (sdp_handler_->local_description()) { for (const cricket::MediaDescriptionOptions& options : request.options.media_description_options) { if (!options.transport_options.ice_restart) { - CopyCandidatesFromSessionDescription(pc_->local_description(), + CopyCandidatesFromSessionDescription(sdp_handler_->local_description(), options.mid, offer.get()); } } @@ -374,31 +378,34 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( void WebRtcSessionDescriptionFactory::InternalCreateAnswer( CreateSessionDescriptionRequest request) { - if (pc_->remote_description()) { + if (sdp_handler_->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 = - pc_->IceRestartPending(options.mid); - // We should pass the current SSL role to the transport description + sdp_handler_->IceRestartPending(options.mid); + // We should pass the current DTLS role to the transport description // factory, if there is already an existing ongoing session. - rtc::SSLRole ssl_role; - if (pc_->GetSslRole(options.mid, &ssl_role)) { + absl::optional dtls_role = + sdp_handler_->transport_controller()->GetDtlsRole(options.mid); + if (dtls_role) { options.transport_options.prefer_passive_role = - (rtc::SSL_SERVER == ssl_role); + (rtc::SSL_SERVER == *dtls_role); } } } std::unique_ptr desc = session_desc_factory_.CreateAnswer( - pc_->remote_description() ? pc_->remote_description()->description() - : nullptr, + sdp_handler_->remote_description() + ? sdp_handler_->remote_description()->description() + : nullptr, request.options, - pc_->local_description() ? pc_->local_description()->description() - : nullptr); + sdp_handler_->local_description() + ? sdp_handler_->local_description()->description() + : nullptr); if (!desc) { PostCreateSessionDescriptionFailed(request.observer, "Failed to initialize the answer."); @@ -416,13 +423,13 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer( auto answer = std::make_unique( SdpType::kAnswer, std::move(desc), session_id_, rtc::ToString(session_version_++)); - if (pc_->local_description()) { + if (sdp_handler_->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(pc_->local_description(), + CopyCandidatesFromSessionDescription(sdp_handler_->local_description(), options.mid, answer.get()); } } diff --git a/pc/webrtc_session_description_factory.h b/pc/webrtc_session_description_factory.h index f70b847b4e..21d2a139d7 100644 --- a/pc/webrtc_session_description_factory.h +++ b/pc/webrtc_session_description_factory.h @@ -12,7 +12,6 @@ #define PC_WEBRTC_SESSION_DESCRIPTION_FACTORY_H_ #include - #include #include #include @@ -22,18 +21,25 @@ #include "api/scoped_refptr.h" #include "p2p/base/transport_description.h" #include "p2p/base/transport_description_factory.h" +#include "pc/channel_manager.h" #include "pc/media_session.h" -#include "pc/peer_connection_internal.h" +#include "pc/sdp_offer_answer.h" #include "rtc_base/constructor_magic.h" #include "rtc_base/message_handler.h" #include "rtc_base/rtc_certificate.h" #include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" +#include "rtc_base/thread_message.h" #include "rtc_base/unique_id_generator.h" namespace webrtc { +// Forward declaration is necessary because there's a circular dependency +// between this class and SdpOfferAnswerHandler. +// TODO(https://bugs.webrtc.org/12060): Break the dependency. +class SdpOfferAnswerHandler; + // DTLS certificate request callback class. class WebRtcCertificateGeneratorCallback : public rtc::RTCCertificateGeneratorCallback, @@ -80,8 +86,9 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, WebRtcSessionDescriptionFactory( rtc::Thread* signaling_thread, cricket::ChannelManager* channel_manager, - PeerConnectionInternal* pc, + SdpOfferAnswerHandler* sdp_handler, const std::string& session_id, + bool dtls_enabled, std::unique_ptr cert_generator, const rtc::scoped_refptr& certificate, rtc::UniqueRandomIdGenerator* ssrc_generator); @@ -151,9 +158,7 @@ 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 peer connection once bug 2264 is - // fixed. - PeerConnectionInternal* const pc_; + SdpOfferAnswerHandler* sdp_handler_; const std::string session_id_; CertificateRequestState certificate_request_state_;