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 <kwiberg@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Jonas Olsson <jonasolsson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25773}

api/peerconnectionproxy.h

@@ -125,6 +125,7 @@ PROXY_METHOD1(void,
               std::unique_ptr<rtc::BitrateAllocationStrategy>);
 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,

p2p/base/icetransportinternal.h

@@ -26,14 +26,6 @@ namespace cricket {
 
 typedef std::vector<Candidate> 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.

p2p/base/p2ptransportchannel.cc

@@ -2481,8 +2481,7 @@ void P2PTransportChannel::set_writable(bool writable) {
   if (writable_ == writable) {
     return;
   }
-  RTC_LOG(LS_VERBOSE) << ToString()
+  RTC_LOG(LS_VERBOSE) << ToString() << ": Changed writable_ to " << writable;
-                      << ": Changed writable_ to " << writable;
   writable_ = writable;
   if (writable_) {
     SignalReadyToSend(this);

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);

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;
+  bool any_ice_connected = 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();
 
@@ -1284,16 +1278,8 @@ void JsepTransportController::UpdateAggregateStates_n() {
   std::map<cricket::DtlsTransportState, int> dtls_state_counts;
 
   for (const auto& dtls : dtls_transports) {
-    any_failed = any_failed || dtls->ice_transport()->GetState() ==
+    any_ice_connected |= dtls->ice_transport()->writable();
-                                   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 =
@@ -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<void>(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) {
+  if (ice_connection_state_ != new_ice_connection_state) {
-    standardized_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<void>(RTC_FROM_HERE, signaling_thread_, [this] {
+        SignalIceConnectionState(
+            PeerConnectionInterface::kIceConnectionConnected);
+      });
+    }
+
+    ice_connection_state_ = new_ice_connection_state;
     invoker_.AsyncInvoke<void>(
         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;

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<cricket::IceConnectionState> SignalIceConnectionState;
+  sigslot::signal1<PeerConnectionInterface::IceConnectionState>
+      SignalIceConnectionState;
 
   sigslot::signal1<PeerConnectionInterface::PeerConnectionState>
       SignalConnectionState;
-  sigslot::signal1<PeerConnectionInterface::IceConnectionState>
-      SignalStandardizedIceConnectionState;
 
   // If all transports done gathering => complete,
   // Else if any are gathering => gathering,
@@ -341,12 +340,7 @@ class JsepTransportController : public sigslot::has_slots<> {
   std::map<std::string, cricket::JsepTransport*> mid_to_transport_;
 
   // Aggregate states for Transports.
-  // standardized_ice_connection_state_ is intended to replace
+  PeerConnectionInterface::IceConnectionState ice_connection_state_ =
-  // 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;

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_ =
+  PeerConnectionInterface::IceConnectionState connection_state_ =
-      cricket::kIceConnectionConnecting;
-  PeerConnectionInterface::IceConnectionState ice_connection_state_ =
       PeerConnectionInterface::kIceConnectionNew;
   PeerConnectionInterface::PeerConnectionState combined_connection_state_ =
       PeerConnectionInterface::PeerConnectionState::kNew;
@@ -322,7 +309,6 @@ class JsepTransportControllerTest : public JsepTransportController::Observer,
   std::map<std::string, Candidates> 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);
+                 connection_state_, kTimeout);
-  EXPECT_EQ(1, ice_connection_state_signal_count_);
+  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);
+                 connection_state_, kTimeout);
-  EXPECT_EQ(1, ice_connection_state_signal_count_);
+  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);
+                 connection_state_, kTimeout);
-  EXPECT_EQ(2, ice_connection_state_signal_count_);
+  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_,
+  EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting,
-                 kTimeout);
+                 combined_connection_state_, kTimeout);
 
   FakeMediaTransport* media_transport = static_cast<FakeMediaTransport*>(
       transport_controller_->GetMediaTransport(kAudioMid1));
 
   media_transport->SetState(webrtc::MediaTransportState::kWritable);
-  EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_,
+  EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting,
-                 kTimeout);
+                 combined_connection_state_, kTimeout);
 
   // Still waiting for the second media transport.
   media_transport = static_cast<FakeMediaTransport*>(
       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);
+                 connection_state_, kTimeout);
-  EXPECT_EQ(1, ice_connection_state_signal_count_);
+  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);
+                 connection_state_, kTimeout);
-  EXPECT_EQ(2, ice_connection_state_signal_count_);
+  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<FakeDtlsTransport*>(
       transport_controller_->GetDtlsTransport(kVideoMid1));
   EXPECT_EQ(fake_audio_dtls, fake_video_dtls);
-  EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout);
+  EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
-  EXPECT_EQ(PeerConnectionInterface::kIceConnectionCompleted,
+                 connection_state_, kTimeout);
-            ice_connection_state_);
   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

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);
+      this, &PeerConnection::SetIceConnectionState);
-  transport_controller_->SignalStandardizedIceConnectionState.connect(
-      this, &PeerConnection::SetStandardizedIceConnectionState);
   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,20 +3625,15 @@ void PeerConnection::SetIceConnectionState(IceConnectionState new_state) {
   RTC_DCHECK(ice_connection_state_ !=
              PeerConnectionInterface::kIceConnectionClosed);
 
-  ice_connection_state_ = new_state;
+  if (new_state == PeerConnectionInterface::kIceConnectionConnected) {
-  Observer()->OnIceConnectionChange(ice_connection_state_);
+    NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED);
+  } else if (new_state == PeerConnectionInterface::kIceConnectionCompleted) {
+    NoteUsageEvent(UsageEvent::ICE_STATE_CONNECTED);
+    ReportTransportStats();
   }
 
-void PeerConnection::SetStandardizedIceConnectionState(
+  ice_connection_state_ = new_state;
-    PeerConnectionInterface::IceConnectionState new_state) {
+  Observer()->OnIceConnectionChange(ice_connection_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(
@@ -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) {

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;
 

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
+    return callee()->peer_connection_state() ==
-    // are connected. This is an important distinction. Once we have separate
+               webrtc::PeerConnectionInterface::PeerConnectionState::
-    // ICE and DTLS state, this check needs to use the DTLS state.
+                   kConnected &&
-    return (callee()->ice_connection_state() ==
+           caller()->peer_connection_state() ==
-                webrtc::PeerConnectionInterface::kIceConnectionConnected ||
+               webrtc::PeerConnectionInterface::PeerConnectionState::kConnected;
-            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
@@ -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,
+  EXPECT_TRUE_WAIT(
-                 callee()->ice_connection_state(), kDefaultTimeout);
+      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,
+  EXPECT_TRUE_WAIT(
-                 callee()->ice_connection_state(), kMaxWaitForFramesMs);
+      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.

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);