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 <alexdrake@google.com>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28779}
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index b699609..7c35406 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -1184,6 +1184,10 @@
// 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 65cfd36..630848f 100644
--- a/p2p/base/ice_transport_internal.h
+++ b/p2p/base/ice_transport_internal.h
@@ -285,6 +285,9 @@
// SignalNetworkRouteChanged.
sigslot::signal2<IceTransportInternal*, const Candidate&> SignalRouteChange;
+ sigslot::signal1<const cricket::CandidatePairChangeEvent&>
+ SignalCandidatePairChanged;
+
// Invoked when there is conflict in the ICE role between local and remote
// agents.
sigslot::signal1<IceTransportInternal*> SignalRoleConflict;
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 86772e0..a3f90a5 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -284,7 +284,7 @@
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 @@
}
// 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 @@
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 @@
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 @@
// 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 0bcbe10..0546b36 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -267,7 +267,7 @@
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 59c66c4..a8e83ba 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -3147,6 +3147,8 @@
&P2PTransportChannelPingTest::OnReadyToSend);
ch->SignalStateChanged.connect(
this, &P2PTransportChannelPingTest::OnChannelStateChanged);
+ ch->SignalCandidatePairChanged.connect(
+ this, &P2PTransportChannelPingTest::OnCandidatePairChanged);
}
Connection* WaitForConnectionTo(
@@ -3280,6 +3282,9 @@
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 @@
}
}
+ 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<rtc::VirtualSocketServer> vss_;
rtc::AutoSocketServerThread thread_;
int selected_candidate_pair_switches_ = 0;
int last_sent_packet_id_ = -1;
bool channel_ready_to_send_ = false;
+ absl::optional<CandidatePairChangeEvent> last_candidate_change_event_;
IceTransportState channel_state_ = IceTransportState::STATE_INIT;
absl::optional<rtc::NetworkRoute> last_network_route_;
};
@@ -3712,6 +3732,8 @@
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 @@
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 @@
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 @@
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 8e6281f..4251cd4 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -148,6 +148,13 @@
std::string error_text;
};
+struct CandidatePairChangeEvent {
+ Candidate local_candidate;
+ Candidate remote_candidate;
+ int64_t last_data_received_ms;
+ std::string reason;
+};
+
typedef std::set<rtc::SocketAddress> 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 948b9fc..6db58de 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -535,6 +535,8 @@
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 @@
RTC_FROM_HERE, signaling_thread_,
[this, candidates] { SignalIceCandidatesRemoved(candidates); });
}
+void JsepTransportController::OnTransportCandidatePairChanged_n(
+ const cricket::CandidatePairChangeEvent& event) {
+ invoker_.AsyncInvoke<void>(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 70795b0..2919c71 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -248,6 +248,9 @@
sigslot::signal1<const std::vector<cricket::Candidate>&>
SignalIceCandidatesRemoved;
+ sigslot::signal1<const cricket::CandidatePairChangeEvent&>
+ SignalIceCandidatePairChanged;
+
sigslot::signal1<rtc::SSLHandshakeError> SignalDtlsHandshakeError;
sigslot::signal<> SignalMediaTransportStateChanged;
@@ -394,6 +397,8 @@
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 3c03e39..b1ec403 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -1139,6 +1139,8 @@
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 @@
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 @@
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 4e84b97..bca03ef 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -458,6 +458,10 @@
void OnIceCandidatesRemoved(const std::vector<cricket::Candidate>& 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 @@
void OnTransportControllerCandidatesRemoved(
const std::vector<cricket::Candidate>& 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 f36ba1e..78263b7 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -298,6 +298,10 @@
ice_gathering_state_history() const {
return ice_gathering_state_history_;
}
+ std::vector<cricket::CandidatePairChangeEvent>
+ ice_candidate_pair_change_history() const {
+ return ice_candidate_pair_change_history_;
+ }
void AddAudioVideoTracks() {
AddAudioTrack();
@@ -931,6 +935,11 @@
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 @@
peer_connection_state_history_;
std::vector<PeerConnectionInterface::IceGatheringState>
ice_gathering_state_history_;
+ std::vector<cricket::CandidatePairChangeEvent>
+ ice_candidate_pair_change_history_;
webrtc::FakeRtcEventLogFactory* event_log_factory_;
@@ -4208,6 +4219,7 @@
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 @@
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;