diff --git a/api/peerconnectionproxy.h b/api/peerconnectionproxy.h index 6b66bccc4e..7105a78570 100644 --- a/api/peerconnectionproxy.h +++ b/api/peerconnectionproxy.h @@ -125,6 +125,7 @@ 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 099cea70e8..e64a3b4c8a 100644 --- a/p2p/base/icetransportinternal.h +++ b/p2p/base/icetransportinternal.h @@ -26,14 +26,6 @@ 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 f61291c5dd..8f4da3dc42 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -2481,8 +2481,7 @@ 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 6d4c4fd978..e9d9265069 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_->SignalStateChanged.connect( + ice_transport_->SignalIceTransportStateChanged.connect( this, &BasicRegatheringController::OnIceTransportStateChanged); ice_transport->SignalWritableState.connect( this, &BasicRegatheringController::OnIceTransportWritableState); diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 78ecaf31de..66758e342d 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -446,6 +446,8 @@ 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; } @@ -1263,20 +1265,12 @@ 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_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_ice_connected = false; bool any_gathering = false; bool all_done_gathering = !dtls_transports.empty(); @@ -1284,16 +1278,8 @@ void JsepTransportController::UpdateAggregateStates_n() { std::map dtls_state_counts; for (const auto& dtls : dtls_transports) { - 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_ice_connected |= dtls->ice_transport()->writable(); + any_gathering = any_gathering || dtls->ice_transport()->gathering_state() != cricket::kIceGatheringNew; all_done_gathering = @@ -1304,45 +1290,6 @@ 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. @@ -1359,9 +1306,24 @@ 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) { + } else if (total_ice_disconnected > 0 || + (!any_ice_connected && + (ice_connection_state_ == + PeerConnectionInterface::kIceConnectionCompleted || + ice_connection_state_ == + PeerConnectionInterface::kIceConnectionDisconnected))) { // 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) { @@ -1391,11 +1353,23 @@ void JsepTransportController::UpdateAggregateStates_n() { RTC_NOTREACHED(); } - if (standardized_ice_connection_state_ != new_ice_connection_state) { - standardized_ice_connection_state_ = new_ice_connection_state; + 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; invoker_.AsyncInvoke( RTC_FROM_HERE, signaling_thread_, [this, new_ice_connection_state] { - SignalStandardizedIceConnectionState(new_ice_connection_state); + SignalIceConnectionState(new_ice_connection_state); }); } @@ -1417,6 +1391,35 @@ 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 3ed7f5f433..b703012073 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -181,12 +181,11 @@ 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, @@ -341,13 +340,8 @@ class JsepTransportController : public sigslot::has_slots<> { std::map mid_to_transport_; // Aggregate states for Transports. - // 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::IceConnectionState 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 129d22a4fc..66532bed1f 100644 --- a/pc/jseptransportcontroller_unittest.cc +++ b/pc/jseptransportcontroller_unittest.cc @@ -99,8 +99,6 @@ 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( @@ -254,7 +252,7 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, } protected: - void OnConnectionState(cricket::IceConnectionState state) { + void OnConnectionState(PeerConnectionInterface::IceConnectionState state) { if (!signaling_thread_->IsCurrent()) { signaled_on_non_signaling_thread_ = true; } @@ -262,15 +260,6 @@ 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()) { @@ -310,9 +299,7 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, } // Information received from signals from transport controller. - cricket::IceConnectionState connection_state_ = - cricket::kIceConnectionConnecting; - PeerConnectionInterface::IceConnectionState ice_connection_state_ = + PeerConnectionInterface::IceConnectionState connection_state_ = PeerConnectionInterface::kIceConnectionNew; PeerConnectionInterface::PeerConnectionState combined_connection_state_ = PeerConnectionInterface::PeerConnectionState::kNew; @@ -322,7 +309,6 @@ 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; @@ -727,11 +713,9 @@ 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(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_); + connection_state_, kTimeout); + EXPECT_EQ(1, connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, combined_connection_state_, kTimeout); EXPECT_EQ(1, combined_connection_state_signal_count_); @@ -761,11 +745,9 @@ TEST_F(JsepTransportControllerTest, fake_video_dtls->fake_ice_transport()->SetConnectionCount(0); fake_video_dtls->fake_ice_transport()->SetCandidatesGatheringComplete(); - 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_); + connection_state_, kTimeout); + EXPECT_EQ(1, connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, combined_connection_state_, kTimeout); EXPECT_EQ(1, combined_connection_state_signal_count_); @@ -776,11 +758,9 @@ 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(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_); + connection_state_, kTimeout); + EXPECT_EQ(2, connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, combined_connection_state_, kTimeout); EXPECT_EQ(2, combined_connection_state_signal_count_); @@ -813,22 +793,23 @@ TEST_F(JsepTransportControllerTest, fake_video_dtls->SetDtlsState(cricket::DTLS_TRANSPORT_CONNECTED); // Still not connected, because we are waiting for media transport. - EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, - kTimeout); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting, + combined_connection_state_, kTimeout); FakeMediaTransport* media_transport = static_cast( transport_controller_->GetMediaTransport(kAudioMid1)); media_transport->SetState(webrtc::MediaTransportState::kWritable); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_, - kTimeout); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting, + combined_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(cricket::kIceConnectionConnected, connection_state_, kTimeout); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, + combined_connection_state_, kTimeout); } TEST_F(JsepTransportControllerTest, @@ -867,10 +848,12 @@ TEST_F(JsepTransportControllerTest, media_transport->SetState(webrtc::MediaTransportState::kWritable); - EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, + combined_connection_state_, kTimeout); media_transport->SetState(webrtc::MediaTransportState::kClosed); - EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout); + EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, + combined_connection_state_, kTimeout); } TEST_F(JsepTransportControllerTest, SignalConnectionStateComplete) { @@ -896,11 +879,9 @@ TEST_F(JsepTransportControllerTest, SignalConnectionStateComplete) { fake_video_dtls->fake_ice_transport()->SetConnectionCount(0); fake_video_dtls->fake_ice_transport()->SetCandidatesGatheringComplete(); - 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_); + connection_state_, kTimeout); + EXPECT_EQ(1, connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed, combined_connection_state_, kTimeout); EXPECT_EQ(1, combined_connection_state_signal_count_); @@ -911,11 +892,9 @@ 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(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_); + connection_state_, kTimeout); + EXPECT_EQ(2, connection_state_signal_count_); EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected, combined_connection_state_, kTimeout); EXPECT_EQ(2, combined_connection_state_signal_count_); @@ -1004,9 +983,8 @@ TEST_F(JsepTransportControllerTest, fake_video_dtls = static_cast( transport_controller_->GetDtlsTransport(kVideoMid1)); EXPECT_EQ(fake_audio_dtls, fake_video_dtls); - EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); - EXPECT_EQ(PeerConnectionInterface::kIceConnectionCompleted, - ice_connection_state_); + EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted, + connection_state_, kTimeout); EXPECT_EQ(PeerConnectionInterface::PeerConnectionState::kConnected, combined_connection_state_); EXPECT_EQ_WAIT(cricket::kIceGatheringComplete, gathering_state_, kTimeout); @@ -1038,7 +1016,8 @@ TEST_F(JsepTransportControllerTest, IceSignalingOccursOnSignalingThread) { CreateLocalDescriptionAndCompleteConnectionOnNetworkThread(); // connecting --> connected --> completed - EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout); + EXPECT_EQ_WAIT(PeerConnectionInterface::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 e60474ca1d..6a2e370381 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -998,9 +998,7 @@ bool PeerConnection::Initialize( signaling_thread(), network_thread(), port_allocator_.get(), async_resolver_factory_.get(), config)); transport_controller_->SignalIceConnectionState.connect( - this, &PeerConnection::OnTransportControllerConnectionState); - transport_controller_->SignalStandardizedIceConnectionState.connect( - this, &PeerConnection::SetStandardizedIceConnectionState); + this, &PeerConnection::SetIceConnectionState); transport_controller_->SignalConnectionState.connect( this, &PeerConnection::SetConnectionState); transport_controller_->SignalIceGatheringState.connect( @@ -1762,11 +1760,6 @@ 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_; @@ -3632,22 +3625,17 @@ 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()); @@ -3706,8 +3694,6 @@ 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) { @@ -5507,51 +5493,6 @@ 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 7e97afab7c..7945aca130 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -147,7 +147,6 @@ 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; @@ -877,7 +876,8 @@ class PeerConnection : public PeerConnectionInternal, bool SrtpRequired() const; // JsepTransportController signal handlers. - void OnTransportControllerConnectionState(cricket::IceConnectionState state); + void OnTransportControllerConnectionState( + PeerConnectionInterface::IceConnectionState state); void OnTransportControllerGatheringState(cricket::IceGatheringState state); void OnTransportControllerCandidatesGathered( const std::string& transport_name, @@ -983,8 +983,6 @@ 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 a7f7aad0f5..7cd03a4311 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -553,6 +553,10 @@ 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(); } @@ -1201,17 +1205,11 @@ class PeerConnectionIntegrationBaseTest : public testing::Test { } bool DtlsConnected() { - // 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); + return callee()->peer_connection_state() == + webrtc::PeerConnectionInterface::PeerConnectionState:: + kConnected && + caller()->peer_connection_state() == + webrtc::PeerConnectionInterface::PeerConnectionState::kConnected; } // When |event_log_factory| is null, the default implementation of the event @@ -1601,7 +1599,10 @@ 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_EQ(1, webrtc::metrics::NumEvents( + EXPECT_LE(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", + expected_cipher_suite)); + EXPECT_GE(2, webrtc::metrics::NumEvents( "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", expected_cipher_suite)); } @@ -2824,7 +2825,10 @@ TEST_P(PeerConnectionIntegrationTest, Dtls10CipherStatsAndUmaMetrics) { EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_EQ(1, webrtc::metrics::NumEvents( + EXPECT_LE(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", + kDefaultSrtpCryptoSuite)); + EXPECT_GE(2, webrtc::metrics::NumEvents( "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", kDefaultSrtpCryptoSuite)); } @@ -2846,7 +2850,10 @@ TEST_P(PeerConnectionIntegrationTest, Dtls12CipherStatsAndUmaMetrics) { EXPECT_EQ_WAIT(rtc::SrtpCryptoSuiteToName(kDefaultSrtpCryptoSuite), caller()->OldGetStats()->SrtpCipher(), kDefaultTimeout); // TODO(bugs.webrtc.org/9456): Fix it. - EXPECT_EQ(1, webrtc::metrics::NumEvents( + EXPECT_LE(1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", + kDefaultSrtpCryptoSuite)); + EXPECT_GE(2, webrtc::metrics::NumEvents( "WebRTC.PeerConnection.SrtpCryptoSuite.Audio", kDefaultSrtpCryptoSuite)); } @@ -3548,8 +3555,13 @@ TEST_P(PeerConnectionIntegrationTest, IceStatesReachCompletion) { // fixed, this test should be updated. EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, caller()->ice_connection_state(), kDefaultTimeout); - EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, - callee()->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(); } // Replaces the first candidate with a static address and configures a @@ -3864,8 +3876,13 @@ 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::kIceConnectionConnected, - callee()->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(); // To verify that the ICE restart actually occurs, get // ufrag/password/candidates before and after restart. @@ -3896,7 +3913,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::kIceConnectionConnected, + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionCompleted, 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 2063199e72..e0ee1768ac 100644 --- a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java +++ b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java @@ -820,6 +820,7 @@ 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()); @@ -827,6 +828,7 @@ 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()); @@ -834,7 +836,6 @@ 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); @@ -844,8 +845,9 @@ 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); @@ -1056,6 +1058,7 @@ 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()); @@ -1063,21 +1066,22 @@ 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); @@ -1212,6 +1216,7 @@ 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()); @@ -1244,6 +1249,7 @@ 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()); @@ -1253,7 +1259,6 @@ 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); @@ -1263,8 +1268,9 @@ 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);