From 5b661302097ba03e1055357556c35f6e5d37b550 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Fri, 28 Jan 2022 13:08:34 +0000 Subject: [PATCH] Refactor PeerConnectionInternal to break SdpOfferAnswer dependency This CL changes the SdpOfferAnswerHandler class to depend on a new class PerConnectionInternalMethods, which is implemented by PeerConnection. This means that SdpOfferAnswerHandler no longer depends on PeerConnectionInterface. This opens the way for refactoring PeerConnection so that PeerConnectionInternalMethods is a member object (encapsulation not inheritance), which will make it possible to break some of the dependency cycles that make the "peerconnection" target in the BUILD file so huge. Bug: webrtc:11995 Change-Id: Ib8413a31c0148b8d8602764b7367dfd3068da58a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/249785 Reviewed-by: Niels Moller Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#35828} --- pc/peer_connection.cc | 2 +- pc/peer_connection.h | 72 +++++++++-------- pc/peer_connection_internal.h | 115 +++++++++++++++++++++++++--- pc/sdp_offer_answer.cc | 8 +- pc/sdp_offer_answer.h | 6 +- pc/test/fake_peer_connection_base.h | 69 +++++++++++++++++ 6 files changed, 224 insertions(+), 48 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 3e1a09bb50..dd7a616b8c 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2090,7 +2090,7 @@ bool PeerConnection::ReconfigurePortAllocator_n( stun_candidate_keepalive_interval); } -cricket::ChannelManager* PeerConnection::channel_manager() const { +cricket::ChannelManager* PeerConnection::channel_manager() { return context_->channel_manager(); } diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 20d946dfdc..cb321f07eb 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -186,6 +186,9 @@ class PeerConnection : public PeerConnectionInternal, SignalingState signaling_state() override; IceConnectionState ice_connection_state() override; + IceConnectionState ice_connection_state_internal() override { + return ice_connection_state(); + } IceConnectionState standardized_ice_connection_state() override; PeerConnectionState peer_connection_state() override; IceGatheringState ice_gathering_state() override; @@ -265,6 +268,10 @@ class PeerConnection : public PeerConnectionInternal, } // PeerConnectionInternal implementation. + rtc::Thread* signaling_thread_internal() const final { + return context_->signaling_thread(); + } + rtc::Thread* network_thread() const final { return context_->network_thread(); } @@ -313,71 +320,74 @@ class PeerConnection : public PeerConnectionInternal, // Functions needed by DataChannelController void NoteDataAddedEvent() { NoteUsageEvent(UsageEvent::DATA_ADDED); } // Returns the observer. Will crash on CHECK if the observer is removed. - PeerConnectionObserver* Observer() const; - bool IsClosed() const { + PeerConnectionObserver* Observer() const override; + bool IsClosed() const override { RTC_DCHECK_RUN_ON(signaling_thread()); return !sdp_handler_ || sdp_handler_->signaling_state() == PeerConnectionInterface::kClosed; } // Get current SSL role used by SCTP's underlying transport. - bool GetSctpSslRole(rtc::SSLRole* role); + bool GetSctpSslRole(rtc::SSLRole* role) override; // Handler for the "channel closed" signal void OnSctpDataChannelClosed(DataChannelInterface* channel); bool ShouldFireNegotiationNeededEvent(uint32_t event_id) override; // Functions needed by SdpOfferAnswerHandler - StatsCollector* stats() { + StatsCollector* stats() override { RTC_DCHECK_RUN_ON(signaling_thread()); return stats_.get(); } - DataChannelController* data_channel_controller() { + DataChannelController* data_channel_controller() override { RTC_DCHECK_RUN_ON(signaling_thread()); return &data_channel_controller_; } - bool dtls_enabled() const { + bool dtls_enabled() const override { RTC_DCHECK_RUN_ON(signaling_thread()); return dtls_enabled_; } - const PeerConnectionInterface::RTCConfiguration* configuration() const { + const PeerConnectionInterface::RTCConfiguration* configuration() + const override { RTC_DCHECK_RUN_ON(signaling_thread()); return &configuration_; } - PeerConnectionMessageHandler* message_handler() { + PeerConnectionMessageHandler* message_handler() override { RTC_DCHECK_RUN_ON(signaling_thread()); return &message_handler_; } - RtpTransmissionManager* rtp_manager() { return rtp_manager_.get(); } - const RtpTransmissionManager* rtp_manager() const { + RtpTransmissionManager* rtp_manager() override { return rtp_manager_.get(); } + const RtpTransmissionManager* rtp_manager() const override { return rtp_manager_.get(); } - cricket::ChannelManager* channel_manager() const; + cricket::ChannelManager* channel_manager() override; - JsepTransportController* transport_controller() { + JsepTransportController* transport_controller() override { return transport_controller_.get(); } - cricket::PortAllocator* port_allocator() { return port_allocator_.get(); } - Call* call_ptr() { return call_ptr_; } + cricket::PortAllocator* port_allocator() override { + return port_allocator_.get(); + } + Call* call_ptr() override { return call_ptr_; } ConnectionContext* context() { return context_.get(); } - const PeerConnectionFactoryInterface::Options* options() const { + const PeerConnectionFactoryInterface::Options* options() const override { return &options_; } - void SetIceConnectionState(IceConnectionState new_state); - void NoteUsageEvent(UsageEvent event); + void SetIceConnectionState(IceConnectionState new_state) override; + void NoteUsageEvent(UsageEvent event) override; // Asynchronously adds a remote candidate on the network thread. void AddRemoteCandidate(const std::string& mid, - const cricket::Candidate& candidate); + const cricket::Candidate& candidate) override; // Report the UMA metric SdpFormatReceived for the given remote description. void ReportSdpFormatReceived( - const SessionDescriptionInterface& remote_description); + const SessionDescriptionInterface& remote_description) override; // Report the UMA metric BundleUsage for the given remote description. void ReportSdpBundleUsage( - const SessionDescriptionInterface& remote_description); + const SessionDescriptionInterface& remote_description) override; // Returns true if the PeerConnection is configured to use Unified Plan // semantics for creating offers/answers and setting local/remote @@ -385,34 +395,34 @@ class PeerConnection : public PeerConnectionInternal, // to the user. If this is false, Plan B semantics are assumed. // TODO(bugs.webrtc.org/8530): Flip the default to be Unified Plan once // sufficient time has passed. - bool IsUnifiedPlan() const { + bool IsUnifiedPlan() const override { RTC_DCHECK_RUN_ON(signaling_thread()); return is_unified_plan_; } bool ValidateBundleSettings( const cricket::SessionDescription* desc, const std::map& - bundle_groups_by_mid); + bundle_groups_by_mid) override; // Returns the MID for the data section associated with the // SCTP data channel, if it has been set. If no data // channels are configured this will return nullopt. - absl::optional GetDataMid() const; + absl::optional GetDataMid() const override; - void SetSctpDataMid(const std::string& mid); + void SetSctpDataMid(const std::string& mid) override; - void ResetSctpDataMid(); + void ResetSctpDataMid() override; // Asynchronously calls SctpTransport::Start() on the network thread for // `sctp_mid()` if set. Called as part of setting the local description. void StartSctpTransport(int local_port, int remote_port, - int max_message_size); + int max_message_size) override; // Returns the CryptoOptions for this PeerConnection. This will always // return the RTCConfiguration.crypto_options if set and will only default // back to the PeerConnectionFactory settings if nothing was set. - CryptoOptions GetCryptoOptions(); + CryptoOptions GetCryptoOptions() override; // Internal implementation for AddTransceiver family of methods. If // `fire_callback` is set, fires OnRenegotiationNeeded callback if successful. @@ -420,18 +430,18 @@ class PeerConnection : public PeerConnectionInternal, cricket::MediaType media_type, rtc::scoped_refptr track, const RtpTransceiverInit& init, - bool fire_callback = true); + bool fire_callback = true) override; // Returns rtp transport, result can not be nullptr. RtpTransportInternal* GetRtpTransport(const std::string& mid); // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by // this session. - bool SrtpRequired() const; + bool SrtpRequired() const override; - bool SetupDataChannelTransport_n(const std::string& mid) + bool SetupDataChannelTransport_n(const std::string& mid) override RTC_RUN_ON(network_thread()); - void TeardownDataChannelTransport_n() RTC_RUN_ON(network_thread()); + void TeardownDataChannelTransport_n() override RTC_RUN_ON(network_thread()); cricket::ChannelInterface* GetChannel(const std::string& mid) RTC_RUN_ON(network_thread()); diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index 6f97612914..dc8204521b 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -19,20 +19,124 @@ #include "api/peer_connection_interface.h" #include "call/call.h" +#include "pc/data_channel_controller.h" +#include "pc/jsep_transport_controller.h" +#include "pc/peer_connection_message_handler.h" #include "pc/rtp_transceiver.h" +#include "pc/rtp_transmission_manager.h" #include "pc/sctp_data_channel.h" namespace webrtc { -// Internal interface for extra PeerConnection methods. -class PeerConnectionInternal : public PeerConnectionInterface { +class StatsCollector; + +// This interface defines the functions that are needed for +// SdpOfferAnswerHandler to access PeerConnection internal state. +class PeerConnectionSdpMethods { public: + virtual ~PeerConnectionSdpMethods() = default; + + // NOTE - signaling_thread() is a member of PeerConnectionInterface, + // so we have to use a different name for this function as long as + // PeerConnection is a subclass of PeerConnectionSdpMethods. + virtual rtc::Thread* signaling_thread_internal() const = 0; virtual rtc::Thread* network_thread() const = 0; virtual rtc::Thread* worker_thread() const = 0; // The SDP session ID as defined by RFC 3264. virtual std::string session_id() const = 0; + // Returns true if the ICE restart flag above was set, and no ICE restart has + // occurred yet for this transport (by applying a local description with + // changed ufrag/password). If the transport has been deleted as a result of + // bundling, returns false. + virtual bool NeedsIceRestart(const std::string& content_name) const = 0; + + virtual absl::optional sctp_mid() const = 0; + + // Functions below this comment are known to only be accessed + // from SdpOfferAnswerHandler. + // Return a pointer to the active configuration. + virtual const PeerConnectionInterface::RTCConfiguration* configuration() + const = 0; + + // Report the UMA metric SdpFormatReceived for the given remote description. + virtual void ReportSdpFormatReceived( + const SessionDescriptionInterface& remote_description) = 0; + + // Report the UMA metric BundleUsage for the given remote description. + virtual void ReportSdpBundleUsage( + const SessionDescriptionInterface& remote_description) = 0; + + virtual PeerConnectionMessageHandler* message_handler() = 0; + virtual RtpTransmissionManager* rtp_manager() = 0; + virtual const RtpTransmissionManager* rtp_manager() const = 0; + virtual bool dtls_enabled() const = 0; + virtual const PeerConnectionFactoryInterface::Options* options() const = 0; + + // Returns the CryptoOptions for this PeerConnection. This will always + // return the RTCConfiguration.crypto_options if set and will only default + // back to the PeerConnectionFactory settings if nothing was set. + virtual CryptoOptions GetCryptoOptions() = 0; + virtual cricket::ChannelManager* channel_manager() = 0; + virtual JsepTransportController* transport_controller() = 0; + virtual DataChannelController* data_channel_controller() = 0; + virtual cricket::PortAllocator* port_allocator() = 0; + virtual StatsCollector* stats() = 0; + // Returns the observer. Will crash on CHECK if the observer is removed. + virtual PeerConnectionObserver* Observer() const = 0; + virtual bool GetSctpSslRole(rtc::SSLRole* role) = 0; + virtual PeerConnectionInterface::IceConnectionState + ice_connection_state_internal() = 0; + virtual void SetIceConnectionState( + PeerConnectionInterface::IceConnectionState new_state) = 0; + virtual void NoteUsageEvent(UsageEvent event) = 0; + virtual bool IsClosed() const = 0; + // Returns true if the PeerConnection is configured to use Unified Plan + // semantics for creating offers/answers and setting local/remote + // descriptions. If this is true the RtpTransceiver API will also be available + // to the user. If this is false, Plan B semantics are assumed. + // TODO(bugs.webrtc.org/8530): Flip the default to be Unified Plan once + // sufficient time has passed. + virtual bool IsUnifiedPlan() const = 0; + virtual bool ValidateBundleSettings( + const cricket::SessionDescription* desc, + const std::map& + bundle_groups_by_mid) = 0; + + virtual absl::optional GetDataMid() const = 0; + // Internal implementation for AddTransceiver family of methods. If + // `fire_callback` is set, fires OnRenegotiationNeeded callback if successful. + virtual RTCErrorOr> + AddTransceiver(cricket::MediaType media_type, + rtc::scoped_refptr track, + const RtpTransceiverInit& init, + bool fire_callback = true) = 0; + // Asynchronously calls SctpTransport::Start() on the network thread for + // `sctp_mid()` if set. Called as part of setting the local description. + virtual void StartSctpTransport(int local_port, + int remote_port, + int max_message_size) = 0; + + // Asynchronously adds a remote candidate on the network thread. + virtual void AddRemoteCandidate(const std::string& mid, + const cricket::Candidate& candidate) = 0; + + virtual Call* call_ptr() = 0; + // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by + // this session. + virtual bool SrtpRequired() const = 0; + virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0; + virtual void TeardownDataChannelTransport_n() = 0; + virtual void SetSctpDataMid(const std::string& mid) = 0; + virtual void ResetSctpDataMid() = 0; +}; + +// Functions defined in this class are called by other objects, +// but not by SdpOfferAnswerHandler. +class PeerConnectionInternal : public PeerConnectionInterface, + public PeerConnectionSdpMethods { + public: // Returns true if we were the initial offerer. virtual bool initial_offerer() const = 0; @@ -50,7 +154,6 @@ class PeerConnectionInternal : public PeerConnectionInterface { } virtual absl::optional sctp_transport_name() const = 0; - virtual absl::optional sctp_mid() const = 0; virtual cricket::CandidateStatsList GetPooledCandidateStats() const = 0; @@ -71,12 +174,6 @@ class PeerConnectionInternal : public PeerConnectionInterface { // Returns true if there was an ICE restart initiated by the remote offer. virtual bool IceRestartPending(const std::string& content_name) const = 0; - // Returns true if the ICE restart flag above was set, and no ICE restart has - // occurred yet for this transport (by applying a local description with - // changed ufrag/password). If the transport has been deleted as a result of - // bundling, returns false. - virtual bool NeedsIceRestart(const std::string& content_name) const = 0; - // Get SSL role for an arbitrary m= section (handles bundling correctly). virtual bool GetSslRole(const std::string& content_name, rtc::SSLRole* role) = 0; diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 553704c02b..39416ce58d 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1161,7 +1161,7 @@ class SdpOfferAnswerHandler::LocalIceCredentialsToReplace { std::set> ice_credentials_; }; -SdpOfferAnswerHandler::SdpOfferAnswerHandler(PeerConnection* pc) +SdpOfferAnswerHandler::SdpOfferAnswerHandler(PeerConnectionSdpMethods* pc) : pc_(pc), local_streams_(StreamCollection::Create()), remote_streams_(StreamCollection::Create()), @@ -1181,7 +1181,7 @@ SdpOfferAnswerHandler::~SdpOfferAnswerHandler() {} // Static std::unique_ptr SdpOfferAnswerHandler::Create( - PeerConnection* pc, + PeerConnectionSdpMethods* pc, const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies& dependencies) { auto handler = absl::WrapUnique(new SdpOfferAnswerHandler(pc)); @@ -1308,7 +1308,7 @@ void SdpOfferAnswerHandler::RestartIce() { } rtc::Thread* SdpOfferAnswerHandler::signaling_thread() const { - return pc_->signaling_thread(); + return pc_->signaling_thread_internal(); } void SdpOfferAnswerHandler::CreateOffer( @@ -1857,7 +1857,7 @@ void SdpOfferAnswerHandler::ApplyRemoteDescription( // actually means "gathering candidates", so cannot be be used here. if (remote_description()->GetType() != SdpType::kOffer && remote_description()->number_of_mediasections() > 0u && - pc_->ice_connection_state() == + pc_->ice_connection_state_internal() == PeerConnectionInterface::kIceConnectionNew) { pc_->SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking); } diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 5baeaae548..019e4371c8 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -94,7 +94,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider, // Creates an SdpOfferAnswerHandler. Modifies dependencies. static std::unique_ptr Create( - PeerConnection* pc, + PeerConnectionSdpMethods* pc, const PeerConnectionInterface::RTCConfiguration& configuration, PeerConnectionDependencies& dependencies); @@ -205,7 +205,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider, class LocalIceCredentialsToReplace; // Only called by the Create() function. - explicit SdpOfferAnswerHandler(PeerConnection* pc); + explicit SdpOfferAnswerHandler(PeerConnectionSdpMethods* pc); // Called from the `Create()` function. Can only be called // once. Modifies dependencies. void Initialize( @@ -591,7 +591,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider, const cricket::AudioOptions& audio_options() { return audio_options_; } const cricket::VideoOptions& video_options() { return video_options_; } - PeerConnection* const pc_; + PeerConnectionSdpMethods* const pc_; std::unique_ptr webrtc_session_desc_factory_ RTC_GUARDED_BY(signaling_thread()); diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index d392862192..71ce7bca20 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -291,6 +291,75 @@ class FakePeerConnectionBase : public PeerConnectionInternal { rtc::SSLRole* role) override { return false; } + const PeerConnectionInterface::RTCConfiguration* configuration() + const override { + return nullptr; + } + + rtc::Thread* signaling_thread_internal() const override { return nullptr; } + void ReportSdpFormatReceived( + const SessionDescriptionInterface& remote_description) override {} + + void ReportSdpBundleUsage( + const SessionDescriptionInterface& remote_description) override {} + + PeerConnectionMessageHandler* message_handler() override { return nullptr; } + RtpTransmissionManager* rtp_manager() override { return nullptr; } + const RtpTransmissionManager* rtp_manager() const override { return nullptr; } + bool dtls_enabled() const override { return false; } + const PeerConnectionFactoryInterface::Options* options() const override { + return nullptr; + } + + CryptoOptions GetCryptoOptions() override { return CryptoOptions(); } + cricket::ChannelManager* channel_manager() override { return nullptr; } + JsepTransportController* transport_controller() override { return nullptr; } + DataChannelController* data_channel_controller() override { return nullptr; } + cricket::PortAllocator* port_allocator() override { return nullptr; } + StatsCollector* stats() override { return nullptr; } + PeerConnectionObserver* Observer() const override { return nullptr; } + bool GetSctpSslRole(rtc::SSLRole* role) override { return false; } + PeerConnectionInterface::IceConnectionState ice_connection_state_internal() + override { + return PeerConnectionInterface::IceConnectionState::kIceConnectionNew; + } + void SetIceConnectionState( + PeerConnectionInterface::IceConnectionState new_state) override {} + void NoteUsageEvent(UsageEvent event) override {} + bool IsClosed() const override { return false; } + bool IsUnifiedPlan() const override { return true; } + bool ValidateBundleSettings( + const cricket::SessionDescription* desc, + const std::map& + bundle_groups_by_mid) override { + return false; + } + + absl::optional GetDataMid() const override { + return absl::nullopt; + } + RTCErrorOr> AddTransceiver( + cricket::MediaType media_type, + rtc::scoped_refptr track, + const RtpTransceiverInit& init, + bool fire_callback = true) override { + return RTCError(RTCErrorType::INTERNAL_ERROR, ""); + } + void StartSctpTransport(int local_port, + int remote_port, + int max_message_size) override {} + + void AddRemoteCandidate(const std::string& mid, + const cricket::Candidate& candidate) override {} + + Call* call_ptr() override { return nullptr; } + bool SrtpRequired() const override { return false; } + bool SetupDataChannelTransport_n(const std::string& mid) override { + return false; + } + void TeardownDataChannelTransport_n() override {} + void SetSctpDataMid(const std::string& mid) override {} + void ResetSctpDataMid() override {} protected: sigslot::signal1 SignalSctpDataChannelCreated_;