From 9289edae6f5a9fc79e3fb183e98098b873ac374e Mon Sep 17 00:00:00 2001 From: Alex Loiko Date: Fri, 23 Nov 2018 16:18:59 +0000 Subject: [PATCH] Revert "Replace the IceConnectionState implementation." This reverts commit 1e87b4f32b73526f9caaae2a7bccfbd0cd84dcb9. Reason for revert: Breaks internal project Original change's description: > Replace the IceConnectionState implementation. > > PeerConnection::ice_connection_state() used to return a value based on both DTLS and ICE transports. > Now that we have PeerConnection::peer_connection_state() to fill that role we can change the implementation of ice_connection_state over to match the spec. > > Bug: webrtc:6145 > Change-Id: Ia4f348f728f24faf4b976c63dea2187bb1f01ef0 > Reviewed-on: https://webrtc-review.googlesource.com/c/108780 > Reviewed-by: Karl Wiberg > Reviewed-by: Harald Alvestrand > Commit-Queue: Jonas Olsson > Cr-Commit-Position: refs/heads/master@{#25773} TBR=kwiberg@webrtc.org,hbos@webrtc.org,hta@webrtc.org,jonasolsson@webrtc.org Change-Id: Icc4368d120a4167286fa6ba2e884a3650b453eff No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:6145 Reviewed-on: https://webrtc-review.googlesource.com/c/111925 Reviewed-by: Alex Loiko Commit-Queue: Alex Loiko Cr-Commit-Position: refs/heads/master@{#25775} --- api/peerconnectionproxy.h | 1 - p2p/base/icetransportinternal.h | 8 ++ p2p/base/p2ptransportchannel.cc | 3 +- p2p/base/regatheringcontroller.cc | 2 +- pc/jseptransportcontroller.cc | 127 +++++++++--------- pc/jseptransportcontroller.h | 14 +- pc/jseptransportcontroller_unittest.cc | 73 ++++++---- pc/peerconnection.cc | 75 +++++++++-- pc/peerconnection.h | 6 +- pc/peerconnection_integrationtest.cc | 55 +++----- .../src/org/webrtc/PeerConnectionTest.java | 18 +-- 11 files changed, 226 insertions(+), 156 deletions(-) diff --git a/api/peerconnectionproxy.h b/api/peerconnectionproxy.h index 7105a78570..6b66bccc4e 100644 --- a/api/peerconnectionproxy.h +++ b/api/peerconnectionproxy.h @@ -125,7 +125,6 @@ PROXY_METHOD1(void, std::unique_ptr); PROXY_METHOD0(SignalingState, signaling_state) PROXY_METHOD0(IceConnectionState, ice_connection_state) -PROXY_METHOD0(PeerConnectionState, peer_connection_state) PROXY_METHOD0(IceGatheringState, ice_gathering_state) PROXY_METHOD2(bool, StartRtcEventLog, rtc::PlatformFile, int64_t) PROXY_METHOD2(bool, diff --git a/p2p/base/icetransportinternal.h b/p2p/base/icetransportinternal.h index e64a3b4c8a..099cea70e8 100644 --- a/p2p/base/icetransportinternal.h +++ b/p2p/base/icetransportinternal.h @@ -26,6 +26,14 @@ namespace cricket { typedef std::vector Candidates; +enum IceConnectionState { + kIceConnectionConnecting = 0, + kIceConnectionFailed, + kIceConnectionConnected, // Writable, but still checking one or more + // connections + kIceConnectionCompleted, +}; + // TODO(deadbeef): Unify with PeerConnectionInterface::IceConnectionState // once /talk/ and /webrtc/ are combined, and also switch to ENUM_NAME naming // style. diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 8f4da3dc42..f61291c5dd 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -2481,7 +2481,8 @@ void P2PTransportChannel::set_writable(bool writable) { if (writable_ == writable) { return; } - RTC_LOG(LS_VERBOSE) << ToString() << ": Changed writable_ to " << writable; + RTC_LOG(LS_VERBOSE) << ToString() + << ": Changed writable_ to " << writable; writable_ = writable; if (writable_) { SignalReadyToSend(this); diff --git a/p2p/base/regatheringcontroller.cc b/p2p/base/regatheringcontroller.cc index e9d9265069..6d4c4fd978 100644 --- a/p2p/base/regatheringcontroller.cc +++ b/p2p/base/regatheringcontroller.cc @@ -37,7 +37,7 @@ BasicRegatheringController::BasicRegatheringController( rand_(rtc::SystemTimeNanos()) { RTC_DCHECK(ice_transport_); RTC_DCHECK(thread_); - ice_transport_->SignalIceTransportStateChanged.connect( + ice_transport_->SignalStateChanged.connect( this, &BasicRegatheringController::OnIceTransportStateChanged); ice_transport->SignalWritableState.connect( this, &BasicRegatheringController::OnIceTransportWritableState); diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 66758e342d..78ecaf31de 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -446,8 +446,6 @@ JsepTransportController::CreateDtlsTransport(const std::string& transport_name, this, &JsepTransportController::OnTransportRoleConflict_n); dtls->ice_transport()->SignalStateChanged.connect( this, &JsepTransportController::OnTransportStateChanged_n); - dtls->ice_transport()->SignalIceTransportStateChanged.connect( - this, &JsepTransportController::OnTransportStateChanged_n); return dtls; } @@ -1265,12 +1263,20 @@ void JsepTransportController::UpdateAggregateStates_n() { RTC_DCHECK(network_thread_->IsCurrent()); auto dtls_transports = GetDtlsTransports(); + cricket::IceConnectionState new_connection_state = + cricket::kIceConnectionConnecting; PeerConnectionInterface::IceConnectionState new_ice_connection_state = PeerConnectionInterface::IceConnectionState::kIceConnectionNew; PeerConnectionInterface::PeerConnectionState new_combined_state = PeerConnectionInterface::PeerConnectionState::kNew; cricket::IceGatheringState new_gathering_state = cricket::kIceGatheringNew; - bool any_ice_connected = false; + bool any_failed = false; + + // TODO(http://bugs.webrtc.org/9719) If(when) media_transport disables + // dtls_transports entirely, the below line will have to be changed to account + // for the fact that dtls transports might be absent. + bool all_connected = !dtls_transports.empty(); + bool all_completed = !dtls_transports.empty(); bool any_gathering = false; bool all_done_gathering = !dtls_transports.empty(); @@ -1278,8 +1284,16 @@ void JsepTransportController::UpdateAggregateStates_n() { std::map dtls_state_counts; for (const auto& dtls : dtls_transports) { - any_ice_connected |= dtls->ice_transport()->writable(); - + any_failed = any_failed || dtls->ice_transport()->GetState() == + cricket::IceTransportState::STATE_FAILED; + all_connected = all_connected && dtls->writable(); + all_completed = + all_completed && dtls->writable() && + dtls->ice_transport()->GetState() == + cricket::IceTransportState::STATE_COMPLETED && + dtls->ice_transport()->GetIceRole() == cricket::ICEROLE_CONTROLLING && + dtls->ice_transport()->gathering_state() == + cricket::kIceGatheringComplete; any_gathering = any_gathering || dtls->ice_transport()->gathering_state() != cricket::kIceGatheringNew; all_done_gathering = @@ -1290,6 +1304,45 @@ void JsepTransportController::UpdateAggregateStates_n() { ice_state_counts[dtls->ice_transport()->GetIceTransportState()]++; } + for (auto it = jsep_transports_by_name_.begin(); + it != jsep_transports_by_name_.end(); ++it) { + auto jsep_transport = it->second.get(); + if (!jsep_transport->media_transport()) { + continue; + } + + // There is no 'kIceConnectionDisconnected', so we only need to handle + // connected and completed. + // We treat kClosed as failed, because if it happens before shutting down + // media transports it means that there was a failure. + // MediaTransportInterface allows to flip back and forth between kWritable + // and kPending, but there does not exist an implementation that does that, + // and the contract of jsep transport controller doesn't quite expect that. + // When this happens, we would go from connected to connecting state, but + // this may change in future. + any_failed |= jsep_transport->media_transport_state() == + webrtc::MediaTransportState::kClosed; + all_completed &= jsep_transport->media_transport_state() == + webrtc::MediaTransportState::kWritable; + all_connected &= jsep_transport->media_transport_state() == + webrtc::MediaTransportState::kWritable; + } + + if (any_failed) { + new_connection_state = cricket::kIceConnectionFailed; + } else if (all_completed) { + new_connection_state = cricket::kIceConnectionCompleted; + } else if (all_connected) { + new_connection_state = cricket::kIceConnectionConnected; + } + if (ice_connection_state_ != new_connection_state) { + ice_connection_state_ = new_connection_state; + invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, + [this, new_connection_state] { + SignalIceConnectionState(new_connection_state); + }); + } + // Compute the current RTCIceConnectionState as described in // https://www.w3.org/TR/webrtc/#dom-rtciceconnectionstate. // The PeerConnection is responsible for handling the "closed" state. @@ -1306,24 +1359,9 @@ void JsepTransportController::UpdateAggregateStates_n() { if (total_ice_failed > 0) { // Any of the RTCIceTransports are in the "failed" state. new_ice_connection_state = PeerConnectionInterface::kIceConnectionFailed; - } else if (total_ice_disconnected > 0 || - (!any_ice_connected && - (ice_connection_state_ == - PeerConnectionInterface::kIceConnectionCompleted || - ice_connection_state_ == - PeerConnectionInterface::kIceConnectionDisconnected))) { + } else if (total_ice_disconnected > 0) { // Any of the RTCIceTransports are in the "disconnected" state and none of // them are in the "failed" state. - // - // As a hack we also mark the connection as disconnected if it used to be - // completed but our connections are no longer writable. - if (total_ice_disconnected == 0) { - // If the IceConnectionState is disconnected the DtlsConnectionState has - // to be failed or disconnected. Setting total_ice_disconnected ensures - // that is the case, even if we got here by following the - // !any_ice_connected branch. - total_ice_disconnected = 1; - } new_ice_connection_state = PeerConnectionInterface::kIceConnectionDisconnected; } else if (total_ice_checking > 0) { @@ -1353,23 +1391,11 @@ void JsepTransportController::UpdateAggregateStates_n() { RTC_NOTREACHED(); } - if (ice_connection_state_ != new_ice_connection_state) { - if (ice_connection_state_ == - PeerConnectionInterface::kIceConnectionChecking && - new_ice_connection_state == - PeerConnectionInterface::kIceConnectionCompleted) { - // Make sure we always signal Connected. It's not clear from the spec if - // this is required, but there's little harm and it's what we used to do. - invoker_.AsyncInvoke(RTC_FROM_HERE, signaling_thread_, [this] { - SignalIceConnectionState( - PeerConnectionInterface::kIceConnectionConnected); - }); - } - - ice_connection_state_ = new_ice_connection_state; + if (standardized_ice_connection_state_ != new_ice_connection_state) { + standardized_ice_connection_state_ = new_ice_connection_state; invoker_.AsyncInvoke( RTC_FROM_HERE, signaling_thread_, [this, new_ice_connection_state] { - SignalIceConnectionState(new_ice_connection_state); + SignalStandardizedIceConnectionState(new_ice_connection_state); }); } @@ -1391,35 +1417,6 @@ void JsepTransportController::UpdateAggregateStates_n() { total_ice_new + dtls_state_counts[cricket::DTLS_TRANSPORT_NEW]; int total_transports = total_ice * 2; - // We'll factor any media transports into the combined connection state if - // they exist. Media transports aren't a concept that exist in the spec, but - // since the combined state is supposed to answer "can we send data over this - // peerconnection" it's arguably following the letter if not the spirit of the - // spec. - for (auto it = jsep_transports_by_name_.begin(); - it != jsep_transports_by_name_.end(); ++it) { - auto jsep_transport = it->second.get(); - if (!jsep_transport->media_transport()) { - continue; - } - - if (jsep_transport->media_transport_state() == - webrtc::MediaTransportState::kPending) { - total_transports++; - total_dtls_connecting++; - } else if (jsep_transport->media_transport_state() == - webrtc::MediaTransportState::kWritable) { - total_transports++; - total_connected++; - } else if (jsep_transport->media_transport_state() == - webrtc::MediaTransportState::kClosed) { - // We treat kClosed as failed, because if it happens before shutting down - // media transports it means that there was a failure. - total_transports++; - total_failed++; - } - } - if (total_failed > 0) { // Any of the RTCIceTransports or RTCDtlsTransports are in a "failed" state. new_combined_state = PeerConnectionInterface::PeerConnectionState::kFailed; diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h index b703012073..3ed7f5f433 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -181,11 +181,12 @@ class JsepTransportController : public sigslot::has_slots<> { // Else if all completed => completed, // Else if all connected => connected, // Else => connecting - sigslot::signal1 - SignalIceConnectionState; + sigslot::signal1 SignalIceConnectionState; sigslot::signal1 SignalConnectionState; + sigslot::signal1 + SignalStandardizedIceConnectionState; // If all transports done gathering => complete, // Else if any are gathering => gathering, @@ -340,8 +341,13 @@ class JsepTransportController : public sigslot::has_slots<> { std::map mid_to_transport_; // Aggregate states for Transports. - PeerConnectionInterface::IceConnectionState ice_connection_state_ = - PeerConnectionInterface::kIceConnectionNew; + // standardized_ice_connection_state_ is intended to replace + // ice_connection_state, see bugs.webrtc.org/9308 + cricket::IceConnectionState ice_connection_state_ = + cricket::kIceConnectionConnecting; + PeerConnectionInterface::IceConnectionState + standardized_ice_connection_state_ = + PeerConnectionInterface::kIceConnectionNew; PeerConnectionInterface::PeerConnectionState combined_connection_state_ = PeerConnectionInterface::PeerConnectionState::kNew; cricket::IceGatheringState ice_gathering_state_ = cricket::kIceGatheringNew; diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc index 66532bed1f..129d22a4fc 100644 --- a/pc/jseptransportcontroller_unittest.cc +++ b/pc/jseptransportcontroller_unittest.cc @@ -99,6 +99,8 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, void ConnectTransportControllerSignals() { transport_controller_->SignalIceConnectionState.connect( this, &JsepTransportControllerTest::OnConnectionState); + transport_controller_->SignalStandardizedIceConnectionState.connect( + this, &JsepTransportControllerTest::OnStandardizedIceConnectionState); transport_controller_->SignalConnectionState.connect( this, &JsepTransportControllerTest::OnCombinedConnectionState); transport_controller_->SignalIceGatheringState.connect( @@ -252,7 +254,7 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, } protected: - void OnConnectionState(PeerConnectionInterface::IceConnectionState state) { + void OnConnectionState(cricket::IceConnectionState state) { if (!signaling_thread_->IsCurrent()) { signaled_on_non_signaling_thread_ = true; } @@ -260,6 +262,15 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, ++connection_state_signal_count_; } + void OnStandardizedIceConnectionState( + PeerConnectionInterface::IceConnectionState state) { + if (!signaling_thread_->IsCurrent()) { + signaled_on_non_signaling_thread_ = true; + } + ice_connection_state_ = state; + ++ice_connection_state_signal_count_; + } + void OnCombinedConnectionState( PeerConnectionInterface::PeerConnectionState state) { if (!signaling_thread_->IsCurrent()) { @@ -299,7 +310,9 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, } // Information received from signals from transport controller. - PeerConnectionInterface::IceConnectionState connection_state_ = + cricket::IceConnectionState connection_state_ = + cricket::kIceConnectionConnecting; + PeerConnectionInterface::IceConnectionState ice_connection_state_ = PeerConnectionInterface::kIceConnectionNew; PeerConnectionInterface::PeerConnectionState combined_connection_state_ = PeerConnectionInterface::PeerConnectionState::kNew; @@ -309,6 +322,7 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, std::map candidates_; // Counts of each signal emitted. int connection_state_signal_count_ = 0; + int ice_connection_state_signal_count_ = 0; int combined_connection_state_signal_count_ = 0; int receiving_signal_count_ = 0; int gathering_state_signal_count_ = 0; @@ -713,9 +727,11 @@ TEST_F(JsepTransportControllerTest, SignalConnectionStateFailed) { fake_ice->SetConnectionCount(1); // The connection stats will be failed if there is no active connection. fake_ice->SetConnectionCount(0); - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed, + ice_connection_state_, kTimeout); + EXPECT_EQ(1, ice_connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, combined_connection_state_, kTimeout); EXPECT_EQ(1, combined_connection_state_signal_count_); @@ -745,9 +761,11 @@ TEST_F(JsepTransportControllerTest, fake_video_dtls->fake_ice_transport()->SetConnectionCount(0); fake_video_dtls->fake_ice_transport()->SetCandidatesGatheringComplete(); - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed, + ice_connection_state_, kTimeout); + EXPECT_EQ(1, ice_connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, combined_connection_state_, kTimeout); EXPECT_EQ(1, combined_connection_state_signal_count_); @@ -758,9 +776,11 @@ TEST_F(JsepTransportControllerTest, // the transport state to be STATE_CONNECTING. fake_video_dtls->fake_ice_transport()->SetConnectionCount(2); fake_video_dtls->SetWritable(true); - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionConnected, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); EXPECT_EQ(2, connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionConnected, + ice_connection_state_, kTimeout); + EXPECT_EQ(2, ice_connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, combined_connection_state_, kTimeout); EXPECT_EQ(2, combined_connection_state_signal_count_); @@ -793,23 +813,22 @@ TEST_F(JsepTransportControllerTest, fake_video_dtls->SetDtlsState(cricket::DTLS_TRANSPORT_CONNECTED); // Still not connected, because we are waiting for media transport. - EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting, - combined_connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, + kTimeout); FakeMediaTransport* media_transport = static_cast( transport_controller_->GetMediaTransport(kAudioMid1)); media_transport->SetState(webrtc::MediaTransportState::kWritable); - EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting, - combined_connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, + kTimeout); // Still waiting for the second media transport. media_transport = static_cast( transport_controller_->GetMediaTransport(kVideoMid1)); media_transport->SetState(webrtc::MediaTransportState::kWritable); - EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, - combined_connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); } TEST_F(JsepTransportControllerTest, @@ -848,12 +867,10 @@ TEST_F(JsepTransportControllerTest, media_transport->SetState(webrtc::MediaTransportState::kWritable); - EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, - combined_connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); media_transport->SetState(webrtc::MediaTransportState::kClosed); - EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, - combined_connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); } TEST_F(JsepTransportControllerTest, SignalConnectionStateComplete) { @@ -879,9 +896,11 @@ TEST_F(JsepTransportControllerTest, SignalConnectionStateComplete) { fake_video_dtls->fake_ice_transport()->SetConnectionCount(0); fake_video_dtls->fake_ice_transport()->SetCandidatesGatheringComplete(); - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); EXPECT_EQ(1, connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed, + ice_connection_state_, kTimeout); + EXPECT_EQ(1, ice_connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, combined_connection_state_, kTimeout); EXPECT_EQ(1, combined_connection_state_signal_count_); @@ -892,9 +911,11 @@ TEST_F(JsepTransportControllerTest, SignalConnectionStateComplete) { // the transport state to be STATE_COMPLETED. fake_video_dtls->fake_ice_transport()->SetConnectionCount(1); fake_video_dtls->SetWritable(true); - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); EXPECT_EQ(2, connection_state_signal_count_); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted, + ice_connection_state_, kTimeout); + EXPECT_EQ(2, ice_connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, combined_connection_state_, kTimeout); EXPECT_EQ(2, combined_connection_state_signal_count_); @@ -983,8 +1004,9 @@ TEST_F(JsepTransportControllerTest, fake_video_dtls = static_cast( transport_controller_->GetDtlsTransport(kVideoMid1)); EXPECT_EQ(fake_audio_dtls, fake_video_dtls); - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); + EXPECT_EQ(PeerConnectionInterface::kIceConnectionCompleted, + ice_connection_state_); EXPECT_EQ(PeerConnectionInterface::PeerConnectionState::kConnected, combined_connection_state_); EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout); @@ -1016,8 +1038,7 @@ TEST_F(JsepTransportControllerTest, IceSignalingOccursOnSignalingThread) { CreateLocalDescriptionAndCompleteConnectionOnNetworkThread(); // connecting --> connected --> completed - EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted, - connection_state_, kTimeout); + EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); EXPECT_EQ(2, connection_state_signal_count_); // new --> gathering --> complete diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 6a2e370381..e60474ca1d 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -998,7 +998,9 @@ bool PeerConnection::Initialize( signaling_thread(), network_thread(), port_allocator_.get(), async_resolver_factory_.get(), config)); transport_controller_->SignalIceConnectionState.connect( - this, &PeerConnection::SetIceConnectionState); + this, &PeerConnection::OnTransportControllerConnectionState); + transport_controller_->SignalStandardizedIceConnectionState.connect( + this, &PeerConnection::SetStandardizedIceConnectionState); transport_controller_->SignalConnectionState.connect( this, &PeerConnection::SetConnectionState); transport_controller_->SignalIceGatheringState.connect( @@ -1760,6 +1762,11 @@ PeerConnection::ice_connection_state() { return ice_connection_state_; } +PeerConnectionInterface::IceConnectionState +PeerConnection::standardized_ice_connection_state() { + return standardized_ice_connection_state_; +} + PeerConnectionInterface::PeerConnectionState PeerConnection::peer_connection_state() { return connection_state_; @@ -3625,17 +3632,22 @@ void PeerConnection::SetIceConnectionState(IceConnectionState new_state) { RTC_DCHECK(ice_connection_state_ != PeerConnectionInterface::kIceConnectionClosed); - if (new_state == PeerConnectionInterface::kIceConnectionConnected) { - NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED); - } else if (new_state == PeerConnectionInterface::kIceConnectionCompleted) { - NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED); - ReportTransportStats(); - } - ice_connection_state_ = new_state; Observer()->OnIceConnectionChange(ice_connection_state_); } +void PeerConnection::SetStandardizedIceConnectionState( + PeerConnectionInterface::IceConnectionState new_state) { + RTC_DCHECK(signaling_thread()->IsCurrent()); + if (standardized_ice_connection_state_ == new_state) + return; + if (IsClosed()) + return; + standardized_ice_connection_state_ = new_state; + // TODO(jonasolsson): Pass this value on to OnIceConnectionChange instead of + // the old one once disconnects are handled properly. +} + void PeerConnection::SetConnectionState( PeerConnectionInterface::PeerConnectionState new_state) { RTC_DCHECK(signaling_thread()->IsCurrent()); @@ -3694,6 +3706,8 @@ void PeerConnection::ChangeSignalingState( if (signaling_state == kClosed) { ice_connection_state_ = kIceConnectionClosed; Observer()->OnIceConnectionChange(ice_connection_state_); + standardized_ice_connection_state_ = + PeerConnectionInterface::IceConnectionState::kIceConnectionClosed; connection_state_ = PeerConnectionInterface::PeerConnectionState::kClosed; Observer()->OnConnectionChange(connection_state_); if (ice_gathering_state_ != kIceGatheringComplete) { @@ -5493,6 +5507,51 @@ void PeerConnection::OnDtlsSrtpSetupFailure(cricket::BaseChannel*, bool rtcp) { rtcp ? kDtlsSrtpSetupFailureRtcp : kDtlsSrtpSetupFailureRtp); } +void PeerConnection::OnTransportControllerConnectionState( + cricket::IceConnectionState state) { + switch (state) { + case cricket::kIceConnectionConnecting: + // If the current state is Connected or Completed, then there were + // writable channels but now there are not, so the next state must + // be Disconnected. + // kIceConnectionConnecting is currently used as the default, + // un-connected state by the TransportController, so its only use is + // detecting disconnections. + if (ice_connection_state_ == + PeerConnectionInterface::kIceConnectionConnected || + ice_connection_state_ == + PeerConnectionInterface::kIceConnectionCompleted) { + SetIceConnectionState( + PeerConnectionInterface::kIceConnectionDisconnected); + } + break; + case cricket::kIceConnectionFailed: + SetIceConnectionState(PeerConnectionInterface::kIceConnectionFailed); + break; + case cricket::kIceConnectionConnected: + RTC_LOG(LS_INFO) << "Changing to ICE connected state because " + "all transports are writable."; + SetIceConnectionState(PeerConnectionInterface::kIceConnectionConnected); + NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED); + break; + case cricket::kIceConnectionCompleted: + RTC_LOG(LS_INFO) << "Changing to ICE completed state because " + "all transports are complete."; + if (ice_connection_state_ != + PeerConnectionInterface::kIceConnectionConnected) { + // If jumping directly from "checking" to "connected", + // signal "connected" first. + SetIceConnectionState(PeerConnectionInterface::kIceConnectionConnected); + } + SetIceConnectionState(PeerConnectionInterface::kIceConnectionCompleted); + NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED); + ReportTransportStats(); + break; + default: + RTC_NOTREACHED(); + } +} + void PeerConnection::OnTransportControllerCandidatesGathered( const std::string& transport_name, const cricket::Candidates& candidates) { diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 7945aca130..7e97afab7c 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -147,6 +147,7 @@ class PeerConnection : public PeerConnectionInternal, SignalingState signaling_state() override; IceConnectionState ice_connection_state() override; + IceConnectionState standardized_ice_connection_state(); PeerConnectionState peer_connection_state() override; IceGatheringState ice_gathering_state() override; @@ -876,8 +877,7 @@ class PeerConnection : public PeerConnectionInternal, bool SrtpRequired() const; // JsepTransportController signal handlers. - void OnTransportControllerConnectionState( - PeerConnectionInterface::IceConnectionState state); + void OnTransportControllerConnectionState(cricket::IceConnectionState state); void OnTransportControllerGatheringState(cricket::IceGatheringState state); void OnTransportControllerCandidatesGathered( const std::string& transport_name, @@ -983,6 +983,8 @@ class PeerConnection : public PeerConnectionInternal, SignalingState signaling_state_ = kStable; IceConnectionState ice_connection_state_ = kIceConnectionNew; + PeerConnectionInterface::IceConnectionState + standardized_ice_connection_state_ = kIceConnectionNew; PeerConnectionInterface::PeerConnectionState connection_state_ = PeerConnectionState::kNew; diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index 7cd03a4311..a7f7aad0f5 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -553,10 +553,6 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, return pc()->ice_connection_state(); } - webrtc::PeerConnectionInterface::PeerConnectionState peer_connection_state() { - return pc()->peer_connection_state(); - } - webrtc::PeerConnectionInterface::IceGatheringState ice_gathering_state() { return pc()->ice_gathering_state(); } @@ -1205,11 +1201,17 @@ class PeerConnectionIntegrationBaseTest : public testing::Test { } bool DtlsConnected() { - return callee()->peer_connection_state() == - webrtc::PeerConnectionInterface::PeerConnectionState:: - kConnected && - caller()->peer_connection_state() == - webrtc::PeerConnectionInterface::PeerConnectionState::kConnected; + // TODO(deadbeef): kIceConnectionConnected currently means both ICE and DTLS + // are connected. This is an important distinction. Once we have separate + // ICE and DTLS state, this check needs to use the DTLS state. + return (callee()->ice_connection_state() == + webrtc::PeerConnectionInterface::kIceConnectionConnected || + callee()->ice_connection_state() == + webrtc::PeerConnectionInterface::kIceConnectionCompleted) && + (caller()->ice_connection_state() == + webrtc::PeerConnectionInterface::kIceConnectionConnected || + caller()->ice_connection_state() == + webrtc::PeerConnectionInterface::kIceConnectionCompleted); } // When |event_log_factory| is null, the default implementation of the event @@ -1599,10 +1601,7 @@ class PeerConnectionIntegrationBaseTest : public testing::Test { EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(expected_cipher_suite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_LE(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", - expected_cipher_suite)); - EXPECT_GE(2, webrtc::metrics::NumEvents( + EXPECT_EQ(1, webrtc::metrics::NumEvents( "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", expected_cipher_suite)); } @@ -2825,10 +2824,7 @@ TEST_P(PeerConnectionIntegrationTest, Dtls10CipherStatsAndUmaMetrics) { EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_LE(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", - kDefaultSrtpCryptoSuite)); - EXPECT_GE(2, webrtc::metrics::NumEvents( + EXPECT_EQ(1, webrtc::metrics::NumEvents( "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", kDefaultSrtpCryptoSuite)); } @@ -2850,10 +2846,7 @@ TEST_P(PeerConnectionIntegrationTest, Dtls12CipherStatsAndUmaMetrics) { EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_LE(1, webrtc::metrics::NumEvents( - "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", - kDefaultSrtpCryptoSuite)); - EXPECT_GE(2, webrtc::metrics::NumEvents( + EXPECT_EQ(1, webrtc::metrics::NumEvents( "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", kDefaultSrtpCryptoSuite)); } @@ -3555,13 +3548,8 @@ TEST_P(PeerConnectionIntegrationTest, IceStatesReachCompletion) { // fixed, this test should be updated. EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, caller()->ice_connection_state(), kDefaultTimeout); - EXPECT_TRUE_WAIT( - callee()->ice_connection_state() == - webrtc::PeerConnectionInterface::kIceConnectionConnected || - callee()->ice_connection_state() == - webrtc::PeerConnectionInterface::kIceConnectionCompleted, - kDefaultTimeout) - << callee()->ice_connection_state(); + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, + callee()->ice_connection_state(), kDefaultTimeout); } // Replaces the first candidate with a static address and configures a @@ -3876,13 +3864,8 @@ TEST_P(PeerConnectionIntegrationTest, MediaContinuesFlowingAfterIceRestart) { ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, caller()->ice_connection_state(), kMaxWaitForFramesMs); - EXPECT_TRUE_WAIT( - callee()->ice_connection_state() == - webrtc::PeerConnectionInterface::kIceConnectionConnected || - callee()->ice_connection_state() == - webrtc::PeerConnectionInterface::kIceConnectionCompleted, - kDefaultTimeout) - << callee()->ice_connection_state(); + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, + callee()->ice_connection_state(), kMaxWaitForFramesMs); // To verify that the ICE restart actually occurs, get // ufrag/password/candidates before and after restart. @@ -3913,7 +3896,7 @@ TEST_P(PeerConnectionIntegrationTest, MediaContinuesFlowingAfterIceRestart) { ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, caller()->ice_connection_state(), kMaxWaitForFramesMs); - EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, callee()->ice_connection_state(), kMaxWaitForFramesMs); // Grab the ufrags/candidates again. diff --git a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java index e0ee1768ac..2063199e72 100644 --- a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java +++ b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java @@ -820,7 +820,6 @@ public class PeerConnectionTest { sdpLatch = new SdpObserverLatch(); answeringExpectations.expectSignalingChange(SignalingState.STABLE); answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); - answeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); answeringPC.setLocalDescription(sdpLatch, answerSdp); assertTrue(sdpLatch.await()); assertNull(sdpLatch.getSdp()); @@ -828,7 +827,6 @@ public class PeerConnectionTest { sdpLatch = new SdpObserverLatch(); offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); - offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); offeringPC.setLocalDescription(sdpLatch, offerSdp); assertTrue(sdpLatch.await()); assertNull(sdpLatch.getSdp()); @@ -836,6 +834,7 @@ public class PeerConnectionTest { offeringExpectations.expectSignalingChange(SignalingState.STABLE); offeringExpectations.expectAddStream("answeredMediaStream"); + offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); offeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED); offeringExpectations.expectConnectionChange(PeerConnectionState.NEW); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); @@ -845,9 +844,8 @@ public class PeerConnectionTest { // // offeringExpectations.expectIceConnectionChange( // IceConnectionState.COMPLETED); + answeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); answeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED); - answeringExpectations.expectConnectionChange(PeerConnectionState.NEW); - answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED); offeringPC.setRemoteDescription(sdpLatch, answerSdp); @@ -1058,7 +1056,6 @@ public class PeerConnectionTest { sdpLatch = new SdpObserverLatch(); answeringExpectations.expectSignalingChange(SignalingState.STABLE); answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); - answeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); answeringPC.setLocalDescription(sdpLatch, answerSdp); assertTrue(sdpLatch.await()); assertNull(sdpLatch.getSdp()); @@ -1066,22 +1063,21 @@ public class PeerConnectionTest { sdpLatch = new SdpObserverLatch(); offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); - offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); offeringPC.setLocalDescription(sdpLatch, offerSdp); assertTrue(sdpLatch.await()); assertNull(sdpLatch.getSdp()); sdpLatch = new SdpObserverLatch(); offeringExpectations.expectSignalingChange(SignalingState.STABLE); + offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); offeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED); offeringExpectations.expectConnectionChange(PeerConnectionState.NEW); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED); // TODO(bemasc): uncomment once delivery of ICECompleted is reliable // (https://code.google.com/p/webrtc/issues/detail?id=3021). + answeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); answeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED); - answeringExpectations.expectConnectionChange(PeerConnectionState.NEW); - answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED); offeringPC.setRemoteDescription(sdpLatch, answerSdp); @@ -1216,7 +1212,6 @@ public class PeerConnectionTest { offeringExpectations.expectIceCandidates(2); offeringExpectations.expectIceGatheringChange(IceGatheringState.COMPLETE); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); - offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); offeringPC.setLocalDescription(sdpLatch, offerSdp); assertTrue(sdpLatch.await()); assertNull(sdpLatch.getSdp()); @@ -1249,7 +1244,6 @@ public class PeerConnectionTest { answeringExpectations.expectIceCandidates(2); answeringExpectations.expectIceGatheringChange(IceGatheringState.COMPLETE); answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); - answeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); answeringPC.setLocalDescription(sdpLatch, answerSdp); assertTrue(sdpLatch.await()); assertNull(sdpLatch.getSdp()); @@ -1259,6 +1253,7 @@ public class PeerConnectionTest { offeringExpectations.expectSignalingChange(SignalingState.STABLE); offeringExpectations.expectAddStream("answeredMediaStream"); + offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); offeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED); offeringExpectations.expectConnectionChange(PeerConnectionState.NEW); offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); @@ -1268,9 +1263,8 @@ public class PeerConnectionTest { // // offeringExpectations.expectIceConnectionChange( // IceConnectionState.COMPLETED); + answeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING); answeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED); - answeringExpectations.expectConnectionChange(PeerConnectionState.NEW); - answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING); answeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED); offeringPC.setRemoteDescription(sdpLatch, answerSdp);