From 00c7ecf625c252115efa7c34ba8b6ce457dab220 Mon Sep 17 00:00:00 2001 From: Alex Drake Date: Tue, 6 Aug 2019 10:54:47 -0700 Subject: [PATCH] Surface CandidatePairChange event In order to be able to detect and measure context around candidate pair changes. Bug: webrtc:10419 Change-Id: Iab0d7e7c80d925d1aa44617fc35975fdc6bbc6b9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147340 Commit-Queue: Alex Drake Reviewed-by: Steve Anton Reviewed-by: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#28779} --- api/peer_connection_interface.h | 4 ++++ p2p/base/ice_transport_internal.h | 3 +++ p2p/base/p2p_transport_channel.cc | 21 ++++++++++++---- p2p/base/p2p_transport_channel.h | 2 +- p2p/base/p2p_transport_channel_unittest.cc | 28 ++++++++++++++++++++++ p2p/base/port.h | 7 ++++++ pc/jsep_transport_controller.cc | 8 +++++++ pc/jsep_transport_controller.h | 5 ++++ pc/peer_connection.cc | 15 ++++++++++++ pc/peer_connection.h | 7 ++++++ pc/peer_connection_integrationtest.cc | 13 ++++++++++ 11 files changed, 107 insertions(+), 6 deletions(-) diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index b699609622..7c354066c2 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -1184,6 +1184,10 @@ class PeerConnectionObserver { // Called when the ICE connection receiving status changes. virtual void OnIceConnectionReceivingChange(bool receiving) {} + // Called when the selected candidate pair for the ICE connection changes. + virtual void OnIceSelectedCandidatePairChanged( + const cricket::CandidatePairChangeEvent& event) {} + // This is called when a receiver and its track are created. // TODO(zhihuang): Make this pure virtual when all subclasses implement it. // Note: This is called with both Plan B and Unified Plan semantics. Unified diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index 65cfd36a30..630848f6e6 100644 --- a/p2p/base/ice_transport_internal.h +++ b/p2p/base/ice_transport_internal.h @@ -285,6 +285,9 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal { // SignalNetworkRouteChanged. sigslot::signal2 SignalRouteChange; + sigslot::signal1 + SignalCandidatePairChanged; + // Invoked when there is conflict in the ICE role between local and remote // agents. sigslot::signal1 SignalRoleConflict; diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 86772e016e..a3f90a53fe 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -284,7 +284,7 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection( if (ShouldSwitchSelectedConnection(new_connection, &missed_receiving_unchanged_threshold)) { RTC_LOG(LS_INFO) << "Switching selected connection due to: " << reason; - SwitchSelectedConnection(new_connection); + SwitchSelectedConnection(new_connection, reason); return true; } if (missed_receiving_unchanged_threshold && @@ -1917,7 +1917,8 @@ void P2PTransportChannel::PruneConnections() { } // Change the selected connection, and let listeners know. -void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { +void P2PTransportChannel::SwitchSelectedConnection(Connection* conn, + const std::string& reason) { RTC_DCHECK_RUN_ON(network_thread_); // Note: if conn is NULL, the previous |selected_connection_| has been // destroyed, so don't use it. @@ -1963,6 +1964,16 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { RTC_LOG(LS_INFO) << ToString() << ": No selected connection"; } + // Create event for candidate pair change. + CandidatePairChangeEvent pair_change; + pair_change.reason = reason; + if (selected_connection_) { + pair_change.local_candidate = selected_connection_->local_candidate(); + pair_change.remote_candidate = selected_connection_->remote_candidate(); + pair_change.last_data_received_ms = + selected_connection_->last_data_received(); + } + SignalCandidatePairChanged(pair_change); SignalNetworkRouteChanged(network_route_); } @@ -2377,7 +2388,6 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) { if (strongly_connected && latest_generation) { MaybeStopPortAllocatorSessions(); } - // We have to unroll the stack before doing this because we may be changing // the state of connections while sorting. RequestSortAndStateUpdate("candidate pair state changed"); @@ -2409,8 +2419,9 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) { // there was no selected connection. if (selected_connection_ == connection) { RTC_LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one."; - SwitchSelectedConnection(nullptr); - RequestSortAndStateUpdate("selected candidate pair destroyed"); + const std::string reason = "selected candidate pair destroyed"; + SwitchSelectedConnection(nullptr, reason); + RequestSortAndStateUpdate(reason); } else { // If a non-selected connection was destroyed, we don't need to re-sort but // we do need to update state, because we could be switching to "failed" or diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index 0bcbe10958..0546b36e3d 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -267,7 +267,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { bool PresumedWritable(const cricket::Connection* conn) const; void SortConnectionsAndUpdateState(const std::string& reason_to_sort); - void SwitchSelectedConnection(Connection* conn); + void SwitchSelectedConnection(Connection* conn, const std::string& reason); void UpdateState(); void HandleAllTimedOut(); void MaybeStopPortAllocatorSessions(); diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 59c66c4b07..a8e83bac60 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -3147,6 +3147,8 @@ class P2PTransportChannelPingTest : public ::testing::Test, &P2PTransportChannelPingTest::OnReadyToSend); ch->SignalStateChanged.connect( this, &P2PTransportChannelPingTest::OnChannelStateChanged); + ch->SignalCandidatePairChanged.connect( + this, &P2PTransportChannelPingTest::OnCandidatePairChanged); } Connection* WaitForConnectionTo( @@ -3280,6 +3282,9 @@ class P2PTransportChannelPingTest : public ::testing::Test, void OnChannelStateChanged(IceTransportInternal* channel) { channel_state_ = channel->GetState(); } + void OnCandidatePairChanged(const CandidatePairChangeEvent& event) { + last_candidate_change_event_ = event; + } int last_sent_packet_id() { return last_sent_packet_id_; } bool channel_ready_to_send() { return channel_ready_to_send_; } @@ -3303,12 +3308,27 @@ class P2PTransportChannelPingTest : public ::testing::Test, } } + bool ConnectionMatchesChangeEvent(Connection* conn, std::string reason) { + if (!conn) { + return !last_candidate_change_event_.has_value(); + } else { + return last_candidate_change_event_->local_candidate.IsEquivalent( + conn->local_candidate()) && + last_candidate_change_event_->remote_candidate.IsEquivalent( + conn->remote_candidate()) && + last_candidate_change_event_->last_data_received_ms == + conn->last_data_received() && + last_candidate_change_event_->reason == reason; + } + } + private: std::unique_ptr vss_; rtc::AutoSocketServerThread thread_; int selected_candidate_pair_switches_ = 0; int last_sent_packet_id_ = -1; bool channel_ready_to_send_ = false; + absl::optional last_candidate_change_event_; IceTransportState channel_state_ = IceTransportState::STATE_INIT; absl::optional last_network_route_; }; @@ -3712,6 +3732,8 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { conn1->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout); EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + EXPECT_TRUE(ConnectionMatchesChangeEvent( + conn1, "remote candidate generation maybe changed")); EXPECT_EQ(len, SendData(&ch, data, len, ++last_packet_id)); // When a higher priority candidate comes in, the new connection is chosen @@ -3722,6 +3744,8 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { conn2->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_WAIT(conn2, ch.selected_connection(), kDefaultTimeout); EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_TRUE( + ConnectionMatchesChangeEvent(conn2, "candidate pair state changed")); EXPECT_TRUE(channel_ready_to_send()); EXPECT_EQ(last_packet_id, last_sent_packet_id()); @@ -3740,6 +3764,8 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { NominateConnection(conn3); EXPECT_EQ(conn3, ch.selected_connection()); EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn3)); + EXPECT_TRUE( + ConnectionMatchesChangeEvent(conn3, "nomination on the controlled side")); EXPECT_EQ(last_packet_id, last_sent_packet_id()); EXPECT_TRUE(channel_ready_to_send()); @@ -3761,6 +3787,8 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { conn4->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_WAIT(conn4, ch.selected_connection(), kDefaultTimeout); EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn4)); + EXPECT_TRUE( + ConnectionMatchesChangeEvent(conn4, "candidate pair state changed")); EXPECT_EQ(last_packet_id, last_sent_packet_id()); // SignalReadyToSend is fired again because conn4 is writable. EXPECT_TRUE(channel_ready_to_send()); diff --git a/p2p/base/port.h b/p2p/base/port.h index 8e6281f689..4251cd4e04 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -148,6 +148,13 @@ struct IceCandidateErrorEvent { std::string error_text; }; +struct CandidatePairChangeEvent { + Candidate local_candidate; + Candidate remote_candidate; + int64_t last_data_received_ms; + std::string reason; +}; + typedef std::set ServerAddresses; // Represents a local communication mechanism that can be used to create diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 948b9fcfab..6db58dea6b 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -535,6 +535,8 @@ JsepTransportController::CreateDtlsTransport( this, &JsepTransportController::OnTransportStateChanged_n); dtls->ice_transport()->SignalIceTransportStateChanged.connect( this, &JsepTransportController::OnTransportStateChanged_n); + dtls->ice_transport()->SignalCandidatePairChanged.connect( + this, &JsepTransportController::OnTransportCandidatePairChanged_n); return dtls; } @@ -1401,6 +1403,12 @@ void JsepTransportController::OnTransportCandidatesRemoved_n( RTC_FROM_HERE, signaling_thread_, [this, candidates] { SignalIceCandidatesRemoved(candidates); }); } +void JsepTransportController::OnTransportCandidatePairChanged_n( + const cricket::CandidatePairChangeEvent& event) { + invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, [this, event] { + SignalIceCandidatePairChanged(event); + }); +} void JsepTransportController::OnTransportRoleConflict_n( cricket::IceTransportInternal* transport) { diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 70795b0de6..2919c711ad 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -248,6 +248,9 @@ class JsepTransportController : public sigslot::has_slots<> { sigslot::signal1&> SignalIceCandidatesRemoved; + sigslot::signal1 + SignalIceCandidatePairChanged; + sigslot::signal1 SignalDtlsHandshakeError; sigslot::signal<> SignalMediaTransportStateChanged; @@ -394,6 +397,8 @@ class JsepTransportController : public sigslot::has_slots<> { void OnTransportRoleConflict_n(cricket::IceTransportInternal* transport); void OnTransportStateChanged_n(cricket::IceTransportInternal* transport); void OnMediaTransportStateChanged_n(); + void OnTransportCandidatePairChanged_n( + const cricket::CandidatePairChangeEvent& event); void UpdateAggregateStates_n(); diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 3c03e392e6..b1ec403c0f 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1139,6 +1139,8 @@ bool PeerConnection::Initialize( this, &PeerConnection::OnTransportControllerCandidatesRemoved); transport_controller_->SignalDtlsHandshakeError.connect( this, &PeerConnection::OnTransportControllerDtlsHandshakeError); + transport_controller_->SignalIceCandidatePairChanged.connect( + this, &PeerConnection::OnTransportControllerCandidateChanged); sctp_factory_ = factory_->CreateSctpTransportInternalFactory(); @@ -4273,6 +4275,14 @@ void PeerConnection::OnIceCandidatesRemoved( Observer()->OnIceCandidatesRemoved(candidates); } +void PeerConnection::OnSelectedCandidatePairChanged( + const cricket::CandidatePairChangeEvent& event) { + if (IsClosed()) { + return; + } + Observer()->OnIceSelectedCandidatePairChanged(event); +} + void PeerConnection::ChangeSignalingState( PeerConnectionInterface::SignalingState signaling_state) { if (signaling_state_ == signaling_state) { @@ -6246,6 +6256,11 @@ void PeerConnection::OnTransportControllerCandidatesRemoved( OnIceCandidatesRemoved(candidates); } +void PeerConnection::OnTransportControllerCandidateChanged( + const cricket::CandidatePairChangeEvent& event) { + OnSelectedCandidatePairChanged(event); +} + void PeerConnection::OnTransportControllerDtlsHandshakeError( rtc::SSLHandshakeError error) { RTC_HISTOGRAM_ENUMERATION( diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 4e84b977d4..bca03ef3fd 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -458,6 +458,10 @@ class PeerConnection : public PeerConnectionInternal, void OnIceCandidatesRemoved(const std::vector& candidates) RTC_RUN_ON(signaling_thread()); + void OnSelectedCandidatePairChanged( + const cricket::CandidatePairChangeEvent& event) + RTC_RUN_ON(signaling_thread()); + // Update the state, signaling if necessary. void ChangeSignalingState(SignalingState signaling_state) RTC_RUN_ON(signaling_thread()); @@ -1041,6 +1045,9 @@ class PeerConnection : public PeerConnectionInternal, void OnTransportControllerCandidatesRemoved( const std::vector& candidates) RTC_RUN_ON(signaling_thread()); + void OnTransportControllerCandidateChanged( + const cricket::CandidatePairChangeEvent& event) + RTC_RUN_ON(signaling_thread()); void OnTransportControllerDtlsHandshakeError(rtc::SSLHandshakeError error); const char* SessionErrorToString(SessionError error) const; diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index f36ba1e52f..78263b7ca4 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -298,6 +298,10 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, ice_gathering_state_history() const { return ice_gathering_state_history_; } + std::vector + ice_candidate_pair_change_history() const { + return ice_candidate_pair_change_history_; + } void AddAudioVideoTracks() { AddAudioTrack(); @@ -931,6 +935,11 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, EXPECT_EQ(pc()->ice_gathering_state(), new_state); ice_gathering_state_history_.push_back(new_state); } + + void OnIceSelectedCandidatePairChanged( + const cricket::CandidatePairChangeEvent& event) { + ice_candidate_pair_change_history_.push_back(event); + } void OnIceCandidate(const webrtc::IceCandidateInterface* candidate) override { RTC_LOG(LS_INFO) << debug_name_ << ": OnIceCandidate"; @@ -1025,6 +1034,8 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, peer_connection_state_history_; std::vector ice_gathering_state_history_; + std::vector + ice_candidate_pair_change_history_; webrtc::FakeRtcEventLogFactory* event_log_factory_; @@ -4208,6 +4219,7 @@ TEST_P(PeerConnectionIntegrationTest, MediaContinuesFlowingAfterIceRestart) { std::string callee_ufrag_pre_restart = desc->transport_infos()[0].description.ice_ufrag; + EXPECT_EQ(caller()->ice_candidate_pair_change_history().size(), 1u); // Have the caller initiate an ICE restart. caller()->SetOfferAnswerOptions(IceRestartOfferAnswerOptions()); caller()->CreateAndSetAndSignalOffer(); @@ -4239,6 +4251,7 @@ TEST_P(PeerConnectionIntegrationTest, MediaContinuesFlowingAfterIceRestart) { ASSERT_NE(callee_candidate_pre_restart, callee_candidate_post_restart); ASSERT_NE(caller_ufrag_pre_restart, caller_ufrag_post_restart); ASSERT_NE(callee_ufrag_pre_restart, callee_ufrag_post_restart); + EXPECT_GT(caller()->ice_candidate_pair_change_history().size(), 1u); // Ensure that additional frames are received after the ICE restart. MediaExpectations media_expectations;