From 5eb527cf7fb28e22d0687c0295cb9f1bb458160c Mon Sep 17 00:00:00 2001 From: Lahiru Ginnaliya Gamathige Date: Mon, 18 Jan 2021 23:32:22 -0800 Subject: [PATCH] Replace sigslot usages with callback list library. - Replace few sigslot usages in jsep_transport_controller. - There is still one sigslot usages in this file so keeping the inheritance and that is the reason for not having a binary size gain in this CL. - Remaining sigslot will be removed in a separate CL. Bug: webrtc:11943 Change-Id: Idb8fa1090b037c48eeb62f54cffd3c485cebfcda Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/190146 Reviewed-by: Andrey Logvin Reviewed-by: Mirko Bonadei Reviewed-by: Harald Alvestrand Reviewed-by: Lahiru Ginnaliya Gamathige Commit-Queue: Lahiru Ginnaliya Gamathige Cr-Commit-Position: refs/heads/master@{#33034} --- pc/jsep_transport_controller.cc | 41 ++++++----- pc/jsep_transport_controller.h | 87 ++++++++++++++++++----- pc/jsep_transport_controller_unittest.cc | 28 +++++--- pc/peer_connection.cc | 57 +++++++++++---- test/peer_scenario/scenario_connection.cc | 8 ++- 5 files changed, 157 insertions(+), 64 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 9b14dc0342..d2a00017fa 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -1154,7 +1154,8 @@ void JsepTransportController::OnTransportCandidateGathered_n( std::string transport_name = transport->transport_name(); invoker_.AsyncInvoke( RTC_FROM_HERE, signaling_thread_, [this, transport_name, candidate] { - SignalIceCandidatesGathered(transport_name, {candidate}); + signal_ice_candidates_gathered_.Send( + transport_name, std::vector{candidate}); }); } @@ -1163,20 +1164,21 @@ void JsepTransportController::OnTransportCandidateError_n( const cricket::IceCandidateErrorEvent& event) { RTC_DCHECK(network_thread_->IsCurrent()); - invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, - [this, event] { SignalIceCandidateError(event); }); + invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, [this, event] { + signal_ice_candidate_error_.Send(event); + }); } void JsepTransportController::OnTransportCandidatesRemoved_n( cricket::IceTransportInternal* transport, const cricket::Candidates& candidates) { invoker_.AsyncInvoke( RTC_FROM_HERE, signaling_thread_, - [this, candidates] { SignalIceCandidatesRemoved(candidates); }); + [this, candidates] { signal_ice_candidates_removed_.Send(candidates); }); } void JsepTransportController::OnTransportCandidatePairChanged_n( const cricket::CandidatePairChangeEvent& event) { invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, [this, event] { - SignalIceCandidatePairChanged(event); + signal_ice_candidate_pair_changed_.Send(event); }); } @@ -1259,7 +1261,7 @@ void JsepTransportController::UpdateAggregateStates_n() { invoker_.AsyncInvoke( RTC_FROM_HERE, signaling_thread_, [this, new_connection_state] { - SignalIceConnectionState.Send(new_connection_state); + signal_ice_connection_state_.Send(new_connection_state); }); } @@ -1317,15 +1319,16 @@ void JsepTransportController::UpdateAggregateStates_n() { PeerConnectionInterface::kIceConnectionCompleted) { // Ensure that we never skip over the "connected" state. invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, [this] { - SignalStandardizedIceConnectionState( + signal_standardized_ice_connection_state_.Send( PeerConnectionInterface::kIceConnectionConnected); }); } standardized_ice_connection_state_ = new_ice_connection_state; - invoker_.AsyncInvoke( - RTC_FROM_HERE, signaling_thread_, [this, new_ice_connection_state] { - SignalStandardizedIceConnectionState(new_ice_connection_state); - }); + invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, + [this, new_ice_connection_state] { + signal_standardized_ice_connection_state_.Send( + new_ice_connection_state); + }); } // Compute the current RTCPeerConnectionState as described in @@ -1376,10 +1379,10 @@ void JsepTransportController::UpdateAggregateStates_n() { if (combined_connection_state_ != new_combined_state) { combined_connection_state_ = new_combined_state; - invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, - [this, new_combined_state] { - SignalConnectionState(new_combined_state); - }); + invoker_.AsyncInvoke( + RTC_FROM_HERE, signaling_thread_, [this, new_combined_state] { + signal_connection_state_.Send(new_combined_state); + }); } // Compute the gathering state. @@ -1392,10 +1395,10 @@ void JsepTransportController::UpdateAggregateStates_n() { } if (ice_gathering_state_ != new_gathering_state) { ice_gathering_state_ = new_gathering_state; - invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, - [this, new_gathering_state] { - SignalIceGatheringState(new_gathering_state); - }); + invoker_.AsyncInvoke( + RTC_FROM_HERE, signaling_thread_, [this, new_gathering_state] { + signal_ice_gathering_state_.Send(new_gathering_state); + }); } } diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 64a599088e..f123997ae1 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -192,41 +192,90 @@ class JsepTransportController : public sigslot::has_slots<> { // and deletes unused transports, but doesn't consider anything more complex. void RollbackTransports(); - // All of these signals are fired on the signaling thread. + sigslot::signal1 SignalDtlsHandshakeError; + + // F: void(const std::string&, const std::vector&) + template + void SubscribeIceCandidateGathered(F&& callback) { + signal_ice_candidates_gathered_.AddReceiver(std::forward(callback)); + } + + // F: void(cricket::IceConnectionState) + template + void SubscribeIceConnectionState(F&& callback) { + signal_ice_connection_state_.AddReceiver(std::forward(callback)); + } + + // F: void(PeerConnectionInterface::PeerConnectionState) + template + void SubscribeConnectionState(F&& callback) { + signal_connection_state_.AddReceiver(std::forward(callback)); + } + + // F: void(PeerConnectionInterface::IceConnectionState) + template + void SubscribeStandardizedIceConnectionState(F&& callback) { + signal_standardized_ice_connection_state_.AddReceiver( + std::forward(callback)); + } + + // F: void(cricket::IceGatheringState) + template + void SubscribeIceGatheringState(F&& callback) { + signal_ice_gathering_state_.AddReceiver(std::forward(callback)); + } + + // F: void(const cricket::IceCandidateErrorEvent&) + template + void SubscribeIceCandidateError(F&& callback) { + signal_ice_candidate_error_.AddReceiver(std::forward(callback)); + } + + // F: void(const std::vector&) + template + void SubscribeIceCandidatesRemoved(F&& callback) { + signal_ice_candidates_removed_.AddReceiver(std::forward(callback)); + } + + // F: void(const cricket::CandidatePairChangeEvent&) + template + void SubscribeIceCandidatePairChanged(F&& callback) { + signal_ice_candidate_pair_changed_.AddReceiver(std::forward(callback)); + } + + private: + // All of these callbacks are fired on the signaling thread. // If any transport failed => failed, // Else if all completed => completed, // Else if all connected => connected, // Else => connecting - CallbackList SignalIceConnectionState; + CallbackList signal_ice_connection_state_; - sigslot::signal1 - SignalConnectionState; + CallbackList + signal_connection_state_; - sigslot::signal1 - SignalStandardizedIceConnectionState; + CallbackList + signal_standardized_ice_connection_state_; // If all transports done gathering => complete, // Else if any are gathering => gathering, // Else => new - sigslot::signal1 SignalIceGatheringState; + CallbackList signal_ice_gathering_state_; - // (mid, candidates) - sigslot::signal2&> - SignalIceCandidatesGathered; + // [mid, candidates] + CallbackList&> + signal_ice_candidates_gathered_; - sigslot::signal1 - SignalIceCandidateError; + CallbackList + signal_ice_candidate_error_; - sigslot::signal1&> - SignalIceCandidatesRemoved; + CallbackList&> + signal_ice_candidates_removed_; - sigslot::signal1 - SignalIceCandidatePairChanged; + CallbackList + signal_ice_candidate_pair_changed_; - sigslot::signal1 SignalDtlsHandshakeError; - - private: RTCError ApplyDescription_n(bool local, SdpType type, const cricket::SessionDescription* description) diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 40dc23e535..06ac36119a 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -89,18 +89,28 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, } void ConnectTransportControllerSignals() { - transport_controller_->SignalIceConnectionState.AddReceiver( + transport_controller_->SubscribeIceConnectionState( [this](cricket::IceConnectionState s) { JsepTransportControllerTest::OnConnectionState(s); }); - transport_controller_->SignalStandardizedIceConnectionState.connect( - this, &JsepTransportControllerTest::OnStandardizedIceConnectionState); - transport_controller_->SignalConnectionState.connect( - this, &JsepTransportControllerTest::OnCombinedConnectionState); - transport_controller_->SignalIceGatheringState.connect( - this, &JsepTransportControllerTest::OnGatheringState); - transport_controller_->SignalIceCandidatesGathered.connect( - this, &JsepTransportControllerTest::OnCandidatesGathered); + transport_controller_->SubscribeConnectionState( + [this](PeerConnectionInterface::PeerConnectionState s) { + JsepTransportControllerTest::OnCombinedConnectionState(s); + }); + transport_controller_->SubscribeStandardizedIceConnectionState( + [this](PeerConnectionInterface::IceConnectionState s) { + JsepTransportControllerTest::OnStandardizedIceConnectionState(s); + }); + transport_controller_->SubscribeIceGatheringState( + [this](cricket::IceGatheringState s) { + JsepTransportControllerTest::OnGatheringState(s); + }); + transport_controller_->SubscribeIceCandidateGathered( + [this](const std::string& transport, + const std::vector& candidates) { + JsepTransportControllerTest::OnCandidatesGathered(transport, + candidates); + }); } std::unique_ptr diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index b5af73580c..308c5fb76a 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -606,28 +606,55 @@ RTCError PeerConnection::Initialize( transport_controller_.reset(new JsepTransportController( signaling_thread(), network_thread(), port_allocator_.get(), async_resolver_factory_.get(), config)); - transport_controller_->SignalStandardizedIceConnectionState.connect( - this, &PeerConnection::SetStandardizedIceConnectionState); - transport_controller_->SignalConnectionState.connect( - this, &PeerConnection::SetConnectionState); - transport_controller_->SignalIceGatheringState.connect( - this, &PeerConnection::OnTransportControllerGatheringState); - transport_controller_->SignalIceCandidatesGathered.connect( - this, &PeerConnection::OnTransportControllerCandidatesGathered); - transport_controller_->SignalIceCandidateError.connect( - this, &PeerConnection::OnTransportControllerCandidateError); - transport_controller_->SignalIceCandidatesRemoved.connect( - this, &PeerConnection::OnTransportControllerCandidatesRemoved); + transport_controller_->SignalDtlsHandshakeError.connect( this, &PeerConnection::OnTransportControllerDtlsHandshakeError); - transport_controller_->SignalIceCandidatePairChanged.connect( - this, &PeerConnection::OnTransportControllerCandidateChanged); - transport_controller_->SignalIceConnectionState.AddReceiver( + // Following RTC_DCHECKs are added by looking at the caller thread. + // If this is incorrect there might not be test failures + // due to lack of unit tests which trigger these scenarios. + // TODO(bugs.webrtc.org/12160): Remove above comments. + transport_controller_->SubscribeIceConnectionState( [this](cricket::IceConnectionState s) { RTC_DCHECK_RUN_ON(signaling_thread()); OnTransportControllerConnectionState(s); }); + transport_controller_->SubscribeConnectionState( + [this](PeerConnectionInterface::PeerConnectionState s) { + RTC_DCHECK_RUN_ON(signaling_thread()); + SetConnectionState(s); + }); + transport_controller_->SubscribeStandardizedIceConnectionState( + [this](PeerConnectionInterface::IceConnectionState s) { + RTC_DCHECK_RUN_ON(signaling_thread()); + SetStandardizedIceConnectionState(s); + }); + transport_controller_->SubscribeIceGatheringState( + [this](cricket::IceGatheringState s) { + RTC_DCHECK_RUN_ON(signaling_thread()); + OnTransportControllerGatheringState(s); + }); + transport_controller_->SubscribeIceCandidateGathered( + [this](const std::string& transport, + const std::vector& candidates) { + RTC_DCHECK_RUN_ON(signaling_thread()); + OnTransportControllerCandidatesGathered(transport, candidates); + }); + transport_controller_->SubscribeIceCandidateError( + [this](const cricket::IceCandidateErrorEvent& event) { + RTC_DCHECK_RUN_ON(signaling_thread()); + OnTransportControllerCandidateError(event); + }); + transport_controller_->SubscribeIceCandidatesRemoved( + [this](const std::vector& c) { + RTC_DCHECK_RUN_ON(signaling_thread()); + OnTransportControllerCandidatesRemoved(c); + }); + transport_controller_->SubscribeIceCandidatePairChanged( + [this](const cricket::CandidatePairChangeEvent& event) { + RTC_DCHECK_RUN_ON(signaling_thread()); + OnTransportControllerCandidateChanged(event); + }); configuration_ = configuration; diff --git a/test/peer_scenario/scenario_connection.cc b/test/peer_scenario/scenario_connection.cc index 92082f5097..8e5b3162cb 100644 --- a/test/peer_scenario/scenario_connection.cc +++ b/test/peer_scenario/scenario_connection.cc @@ -165,8 +165,12 @@ void ScenarioIceConnectionImpl::SetRemoteSdp(SdpType type, const std::string& remote_sdp) { RTC_DCHECK_RUN_ON(signaling_thread_); remote_description_ = webrtc::CreateSessionDescription(type, remote_sdp); - jsep_controller_->SignalIceCandidatesGathered.connect( - this, &ScenarioIceConnectionImpl::OnCandidates); + jsep_controller_->SubscribeIceCandidateGathered( + [this](const std::string& transport, + const std::vector& candidate) { + ScenarioIceConnectionImpl::OnCandidates(transport, candidate); + }); + auto res = jsep_controller_->SetRemoteDescription( remote_description_->GetType(), remote_description_->description()); RTC_CHECK(res.ok()) << res.message();