From f01bd6c266a5a8daf47da021ad30745f79281f37 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Fri, 23 Oct 2020 13:30:46 +0000 Subject: [PATCH] Break circular dependency on WebRtcSessionDescriptionFactory After this change, SdpOfferAnswerHandler implements a read-only interface called SdpStateProvider, which allows enough access for WebRtcSessionDescriptionFactory to learn what it needs to know. Bug: webrtc:12060 Change-Id: Ic888b5027b2df5fee407d32b89da66ff044c40de Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/190145 Commit-Queue: Harald Alvestrand Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#32486} --- pc/BUILD.gn | 9 ++++ pc/sdp_offer_answer.cc | 15 +++++++ pc/sdp_offer_answer.h | 41 ++++++++++-------- pc/sdp_state_provider.h | 54 ++++++++++++++++++++++++ pc/webrtc_session_description_factory.cc | 45 ++++++++++---------- pc/webrtc_session_description_factory.h | 11 ++--- 6 files changed, 126 insertions(+), 49 deletions(-) create mode 100644 pc/sdp_state_provider.h diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 7c677d2740..10c5c7b7ab 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -219,6 +219,7 @@ rtc_library("peerconnection") { ":rtp_sender", ":rtp_transceiver", ":rtp_transmission_manager", + ":sdp_state_provider", ":stats_collector_interface", ":transceiver_list", ":usage_pattern", @@ -561,6 +562,14 @@ rtc_source_set("jitter_buffer_delay_interface") { ] } +rtc_source_set("sdp_state_provider") { + sources = [ "sdp_state_provider.h" ] + deps = [ + ":rtc_pc_base", + "../api:libjingle_peerconnection_api", + ] +} + rtc_source_set("jitter_buffer_delay_proxy") { sources = [ "jitter_buffer_delay_proxy.h" ] deps = [ diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index d9337f4586..51a04283fe 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include "absl/algorithm/container.h" #include "absl/strings/string_view.h" @@ -1023,6 +1024,10 @@ const TransceiverList* SdpOfferAnswerHandler::transceivers() const { JsepTransportController* SdpOfferAnswerHandler::transport_controller() { return pc_->transport_controller(); } +const JsepTransportController* SdpOfferAnswerHandler::transport_controller() + const { + return pc_->transport_controller(); +} DataChannelController* SdpOfferAnswerHandler::data_channel_controller() { return pc_->data_channel_controller(); } @@ -2768,6 +2773,16 @@ bool SdpOfferAnswerHandler::IceRestartPending( pending_ice_restarts_.end(); } +bool SdpOfferAnswerHandler::NeedsIceRestart( + const std::string& content_name) const { + return transport_controller()->NeedsIceRestart(content_name); +} + +absl::optional SdpOfferAnswerHandler::GetDtlsRole( + const std::string& mid) const { + return transport_controller()->GetDtlsRole(mid); +} + void SdpOfferAnswerHandler::UpdateNegotiationNeeded() { RTC_DCHECK_RUN_ON(signaling_thread()); if (!IsUnifiedPlan()) { diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 873c24b620..e468414b01 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -37,6 +37,7 @@ #include "api/set_remote_description_observer_interface.h" #include "api/transport/data_channel_transport_interface.h" #include "api/turn_customizer.h" +#include "api/video/video_bitrate_allocator_factory.h" #include "media/base/media_channel.h" #include "media/base/stream_params.h" #include "p2p/base/port_allocator.h" @@ -56,6 +57,7 @@ #include "pc/rtp_transceiver.h" #include "pc/rtp_transmission_manager.h" #include "pc/sctp_transport.h" +#include "pc/sdp_state_provider.h" #include "pc/session_description.h" #include "pc/stats_collector.h" #include "pc/stream_collection.h" @@ -65,7 +67,10 @@ #include "rtc_base/experiments/field_trial_parser.h" #include "rtc_base/operations_chain.h" #include "rtc_base/race_checker.h" +#include "rtc_base/rtc_certificate.h" +#include "rtc_base/ssl_stream_adapter.h" #include "rtc_base/synchronization/sequence_checker.h" +#include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" #include "rtc_base/unique_id_generator.h" @@ -73,14 +78,6 @@ namespace webrtc { -class MediaStreamObserver; -class PeerConnection; -class VideoRtpReceiver; -class RtcEventLog; -class RtpTransmissionManager; -class TransceiverList; -class WebRtcSessionDescriptionFactory; - // SdpOfferAnswerHandler is a component // of the PeerConnection object as defined // by the PeerConnectionInterface API surface. @@ -88,7 +85,8 @@ class WebRtcSessionDescriptionFactory; // - Parsing and interpreting SDP. // - Generating offers and answers based on the current state. // This class lives on the signaling thread. -class SdpOfferAnswerHandler : public sigslot::has_slots<> { +class SdpOfferAnswerHandler : public SdpStateProvider, + public sigslot::has_slots<> { public: explicit SdpOfferAnswerHandler(PeerConnection* pc); ~SdpOfferAnswerHandler(); @@ -114,16 +112,22 @@ class SdpOfferAnswerHandler : public sigslot::has_slots<> { // Called as part of destroying the owning PeerConnection. void PrepareForShutdown(); - PeerConnectionInterface::SignalingState signaling_state() const; + // Implementation of SdpStateProvider + PeerConnectionInterface::SignalingState signaling_state() const override; - const SessionDescriptionInterface* local_description() const; - const SessionDescriptionInterface* remote_description() const; - const SessionDescriptionInterface* current_local_description() const; - const SessionDescriptionInterface* current_remote_description() const; - const SessionDescriptionInterface* pending_local_description() const; - const SessionDescriptionInterface* pending_remote_description() const; + const SessionDescriptionInterface* local_description() const override; + const SessionDescriptionInterface* remote_description() const override; + const SessionDescriptionInterface* current_local_description() const override; + const SessionDescriptionInterface* current_remote_description() + const override; + const SessionDescriptionInterface* pending_local_description() const override; + const SessionDescriptionInterface* pending_remote_description() + const override; - JsepTransportController* transport_controller(); + bool NeedsIceRestart(const std::string& content_name) const override; + bool IceRestartPending(const std::string& content_name) const override; + absl::optional GetDtlsRole( + const std::string& mid) const override; void RestartIce(); @@ -168,7 +172,6 @@ class SdpOfferAnswerHandler : public sigslot::has_slots<> { absl::optional is_caller(); bool HasNewIceCredentials(); - bool IceRestartPending(const std::string& content_name) const; void UpdateNegotiationNeeded(); void SetHavePendingRtpDataChannel() { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -560,6 +563,8 @@ class SdpOfferAnswerHandler : public sigslot::has_slots<> { const cricket::PortAllocator* port_allocator() const; RtpTransmissionManager* rtp_manager(); const RtpTransmissionManager* rtp_manager() const; + JsepTransportController* transport_controller(); + const JsepTransportController* transport_controller() const; // =================================================================== const cricket::AudioOptions& audio_options() { return audio_options_; } const cricket::VideoOptions& video_options() { return video_options_; } diff --git a/pc/sdp_state_provider.h b/pc/sdp_state_provider.h new file mode 100644 index 0000000000..23ffc91bd9 --- /dev/null +++ b/pc/sdp_state_provider.h @@ -0,0 +1,54 @@ +/* + * Copyright 2020 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef PC_SDP_STATE_PROVIDER_H_ +#define PC_SDP_STATE_PROVIDER_H_ + +#include + +#include "api/jsep.h" +#include "api/peer_connection_interface.h" + +namespace webrtc { + +// This interface provides access to the state of an SDP offer/answer +// negotiation. +// +// All the functions are const, so using this interface serves as +// assurance that the user is not modifying the state. +class SdpStateProvider { + public: + virtual ~SdpStateProvider() {} + + virtual PeerConnectionInterface::SignalingState signaling_state() const = 0; + + virtual const SessionDescriptionInterface* local_description() const = 0; + virtual const SessionDescriptionInterface* remote_description() const = 0; + virtual const SessionDescriptionInterface* current_local_description() + const = 0; + virtual const SessionDescriptionInterface* current_remote_description() + const = 0; + virtual const SessionDescriptionInterface* pending_local_description() + const = 0; + virtual const SessionDescriptionInterface* pending_remote_description() + const = 0; + + // Whether an ICE restart has been asked for. Used in CreateOffer. + virtual bool NeedsIceRestart(const std::string& content_name) const = 0; + // Whether an ICE restart was indicated in the remote offer. + // Used in CreateAnswer. + virtual bool IceRestartPending(const std::string& content_name) const = 0; + virtual absl::optional GetDtlsRole( + const std::string& mid) const = 0; +}; + +} // namespace webrtc + +#endif // PC_SDP_STATE_PROVIDER_H_ diff --git a/pc/webrtc_session_description_factory.cc b/pc/webrtc_session_description_factory.cc index 2409805adb..6ab43e57bc 100644 --- a/pc/webrtc_session_description_factory.cc +++ b/pc/webrtc_session_description_factory.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -22,8 +23,7 @@ #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/sdp_state_provider.h" #include "pc/session_description.h" #include "rtc_base/checks.h" #include "rtc_base/location.h" @@ -127,7 +127,7 @@ void WebRtcSessionDescriptionFactory::CopyCandidatesFromSessionDescription( WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( rtc::Thread* signaling_thread, cricket::ChannelManager* channel_manager, - SdpOfferAnswerHandler* sdp_handler, + const SdpStateProvider* sdp_info, const std::string& session_id, bool dtls_enabled, std::unique_ptr cert_generator, @@ -143,7 +143,7 @@ WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory( // |kInitSessionVersion|. session_version_(kInitSessionVersion), cert_generator_(dtls_enabled ? std::move(cert_generator) : nullptr), - sdp_handler_(sdp_handler), + sdp_info_(sdp_info), session_id_(session_id), certificate_request_state_(CERTIFICATE_NOT_NEEDED) { RTC_DCHECK(signaling_thread_); @@ -255,13 +255,13 @@ void WebRtcSessionDescriptionFactory::CreateAnswer( PostCreateSessionDescriptionFailed(observer, error); return; } - if (!sdp_handler_->remote_description()) { + if (!sdp_info_->remote_description()) { error += " can't be called before SetRemoteDescription."; RTC_LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailed(observer, error); return; } - if (sdp_handler_->remote_description()->GetType() != SdpType::kOffer) { + if (sdp_info_->remote_description()->GetType() != SdpType::kOffer) { error += " failed because remote_description is not an offer."; RTC_LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailed(observer, error); @@ -328,12 +328,12 @@ void WebRtcSessionDescriptionFactory::OnMessage(rtc::Message* msg) { void WebRtcSessionDescriptionFactory::InternalCreateOffer( CreateSessionDescriptionRequest request) { - if (sdp_handler_->local_description()) { + if (sdp_info_->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 (sdp_handler_->transport_controller()->NeedsIceRestart(options.mid)) { + if (sdp_info_->NeedsIceRestart(options.mid)) { options.transport_options.ice_restart = true; } } @@ -341,10 +341,9 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( std::unique_ptr desc = session_desc_factory_.CreateOffer( - request.options, - sdp_handler_->local_description() - ? sdp_handler_->local_description()->description() - : nullptr); + request.options, sdp_info_->local_description() + ? sdp_info_->local_description()->description() + : nullptr); if (!desc) { PostCreateSessionDescriptionFailed(request.observer, "Failed to initialize the offer."); @@ -364,11 +363,11 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( auto offer = std::make_unique( SdpType::kOffer, std::move(desc), session_id_, rtc::ToString(session_version_++)); - if (sdp_handler_->local_description()) { + if (sdp_info_->local_description()) { for (const cricket::MediaDescriptionOptions& options : request.options.media_description_options) { if (!options.transport_options.ice_restart) { - CopyCandidatesFromSessionDescription(sdp_handler_->local_description(), + CopyCandidatesFromSessionDescription(sdp_info_->local_description(), options.mid, offer.get()); } } @@ -378,18 +377,18 @@ void WebRtcSessionDescriptionFactory::InternalCreateOffer( void WebRtcSessionDescriptionFactory::InternalCreateAnswer( CreateSessionDescriptionRequest request) { - if (sdp_handler_->remote_description()) { + if (sdp_info_->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 = - sdp_handler_->IceRestartPending(options.mid); + sdp_info_->IceRestartPending(options.mid); // We should pass the current DTLS role to the transport description // factory, if there is already an existing ongoing session. absl::optional dtls_role = - sdp_handler_->transport_controller()->GetDtlsRole(options.mid); + sdp_info_->GetDtlsRole(options.mid); if (dtls_role) { options.transport_options.prefer_passive_role = (rtc::SSL_SERVER == *dtls_role); @@ -399,12 +398,12 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer( std::unique_ptr desc = session_desc_factory_.CreateAnswer( - sdp_handler_->remote_description() - ? sdp_handler_->remote_description()->description() + sdp_info_->remote_description() + ? sdp_info_->remote_description()->description() : nullptr, request.options, - sdp_handler_->local_description() - ? sdp_handler_->local_description()->description() + sdp_info_->local_description() + ? sdp_info_->local_description()->description() : nullptr); if (!desc) { PostCreateSessionDescriptionFailed(request.observer, @@ -423,13 +422,13 @@ void WebRtcSessionDescriptionFactory::InternalCreateAnswer( auto answer = std::make_unique( SdpType::kAnswer, std::move(desc), session_id_, rtc::ToString(session_version_++)); - if (sdp_handler_->local_description()) { + if (sdp_info_->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(sdp_handler_->local_description(), + CopyCandidatesFromSessionDescription(sdp_info_->local_description(), options.mid, answer.get()); } } diff --git a/pc/webrtc_session_description_factory.h b/pc/webrtc_session_description_factory.h index 21d2a139d7..3d65271c81 100644 --- a/pc/webrtc_session_description_factory.h +++ b/pc/webrtc_session_description_factory.h @@ -23,7 +23,7 @@ #include "p2p/base/transport_description_factory.h" #include "pc/channel_manager.h" #include "pc/media_session.h" -#include "pc/sdp_offer_answer.h" +#include "pc/sdp_state_provider.h" #include "rtc_base/constructor_magic.h" #include "rtc_base/message_handler.h" #include "rtc_base/rtc_certificate.h" @@ -35,11 +35,6 @@ 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, @@ -86,7 +81,7 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, WebRtcSessionDescriptionFactory( rtc::Thread* signaling_thread, cricket::ChannelManager* channel_manager, - SdpOfferAnswerHandler* sdp_handler, + const SdpStateProvider* sdp_info, const std::string& session_id, bool dtls_enabled, std::unique_ptr cert_generator, @@ -158,7 +153,7 @@ class WebRtcSessionDescriptionFactory : public rtc::MessageHandler, cricket::MediaSessionDescriptionFactory session_desc_factory_; uint64_t session_version_; const std::unique_ptr cert_generator_; - SdpOfferAnswerHandler* sdp_handler_; + const SdpStateProvider* sdp_info_; const std::string session_id_; CertificateRequestState certificate_request_state_;