Add configurable threshold for writability state update.

Add configurable parameters in RTCConfiguration with the default value
given by the constants CONNECTION_WRITE_CONNECT_TIME and
CONNECTION_WRITE_CONNECT_FAILURES in the ICE implementation. These two
parameters define the time period for which a candidate pair must wait
for ping response and the minimum number of connectivity checks that
the pair must send without response before its state becomes unreliable
from writable as defined in the current ICE implementation.

Bug: webrtc:8988
Change-Id: I484599b7d776489a87741ffea8926df766095da9
Reviewed-on: https://webrtc-review.googlesource.com/60704
Commit-Queue: Qingsi Wang <qingsi@google.com>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Sami Kalliomäki <sakal@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22411}

api/peerconnectioninterface.h

@@ -515,6 +515,16 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
     rtc::Optional<int> ice_check_interval_weak_connectivity;
     rtc::Optional<int> ice_check_min_interval;
 
+    // The min time period for which a candidate pair must wait for response to
+    // connectivity checks before it becomes unwritable. This parameter
+    // overrides the default value in the ICE implementation if set.
+    rtc::Optional<int> ice_unwritable_timeout;
+
+    // The min number of connectivity checks that a candidate pair must sent
+    // without receiving response before it becomes unwritable. This parameter
+    // overrides the default value in the ICE implementation if set.
+    rtc::Optional<int> ice_unwritable_min_checks;
+
     // The interval in milliseconds at which STUN candidates will resend STUN
     // binding requests to keep NAT bindings open.
     rtc::Optional<int> stun_candidate_keepalive_interval;

p2p/base/icetransportinternal.h

@@ -128,6 +128,17 @@ struct IceConfig {
   // candidate pairs with strong or weak connectivity, if either of the above
   // interval is shorter than the min interval.
   rtc::Optional<int> ice_check_min_interval;
+  // The min time period for which a candidate pair must wait for response to
+  // connectivity checks before it becomes unwritable. This parameter
+  // overrides the default value given by |CONNECTION_WRITE_CONNECT_TIMEOUT|
+  // in port.h if set, when determining the writability of a candidate pair.
+  rtc::Optional<int> ice_unwritable_timeout;
+
+  // The min number of connectivity checks that a candidate pair must sent
+  // without receiving response before it becomes unwritable. This parameter
+  // overrides the default value given by |CONNECTION_WRITE_CONNECT_FAILURES| in
+  // port.h if set, when determining the writability of a candidate pair.
+  rtc::Optional<int> ice_unwritable_min_checks;
   // The interval in milliseconds at which STUN candidates will resend STUN
   // binding requests to keep NAT bindings open.
   rtc::Optional<int> stun_keepalive_interval;

p2p/base/p2ptransportchannel.cc

@@ -215,6 +215,8 @@ void P2PTransportChannel::AddConnection(Connection* connection) {
   unpinged_connections_.insert(connection);
   connection->set_remote_ice_mode(remote_ice_mode_);
   connection->set_receiving_timeout(config_.receiving_timeout);
+  connection->set_unwritable_timeout(config_.ice_unwritable_timeout);
+  connection->set_unwritable_min_checks(config_.ice_unwritable_min_checks);
   connection->SignalReadPacket.connect(
       this, &P2PTransportChannel::OnReadPacket);
   connection->SignalReadyToSend.connect(
@@ -566,6 +568,24 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) {
                      << config_.ice_check_min_interval.value_or(-1);
   }
 
+  if (config_.ice_unwritable_timeout != config.ice_unwritable_timeout) {
+    config_.ice_unwritable_timeout = config.ice_unwritable_timeout;
+    for (Connection* conn : connections_) {
+      conn->set_unwritable_timeout(config_.ice_unwritable_timeout);
+    }
+    RTC_LOG(LS_INFO) << "Set unwritable timeout to "
+                     << config_.ice_unwritable_timeout.value_or(-1);
+  }
+
+  if (config_.ice_unwritable_min_checks != config.ice_unwritable_min_checks) {
+    config_.ice_unwritable_min_checks = config.ice_unwritable_min_checks;
+    for (Connection* conn : connections_) {
+      conn->set_unwritable_min_checks(config_.ice_unwritable_min_checks);
+    }
+    RTC_LOG(LS_INFO) << "Set unwritable min checks to "
+                     << config_.ice_unwritable_min_checks.value_or(-1);
+  }
+
   if (config_.network_preference != config.network_preference) {
     config_.network_preference = config.network_preference;
     RequestSortAndStateUpdate();

p2p/base/port.cc

@@ -1126,6 +1126,14 @@ void Connection::set_use_candidate_attr(bool enable) {
   use_candidate_attr_ = enable;
 }
 
+int Connection::unwritable_timeout() const {
+  return unwritable_timeout_.value_or(CONNECTION_WRITE_CONNECT_TIMEOUT);
+}
+
+int Connection::unwritable_min_checks() const {
+  return unwritable_min_checks_.value_or(CONNECTION_WRITE_CONNECT_FAILURES);
+}
+
 void Connection::OnSendStunPacket(const void* data, size_t size,
                                   StunRequest* req) {
   rtc::PacketOptions options(port_->DefaultDscpValue());
@@ -1351,14 +1359,11 @@ void Connection::UpdateState(int64_t now) {
   // allow for changes in network conditions.
 
   if ((write_state_ == STATE_WRITABLE) &&
-      TooManyFailures(pings_since_last_response_,
+      TooManyFailures(pings_since_last_response_, unwritable_min_checks(), rtt,
-                      CONNECTION_WRITE_CONNECT_FAILURES,
-                      rtt,
                       now) &&
-      TooLongWithoutResponse(pings_since_last_response_,
+      TooLongWithoutResponse(pings_since_last_response_, unwritable_timeout(),
-                             CONNECTION_WRITE_CONNECT_TIMEOUT,
                              now)) {
-    uint32_t max_pings = CONNECTION_WRITE_CONNECT_FAILURES;
+    uint32_t max_pings = unwritable_min_checks();
     LOG_J(LS_INFO, this) << "Unwritable after " << max_pings
                          << " ping failures and "
                          << now - pings_since_last_response_[0].sent_time

p2p/base/port.h

@@ -575,6 +575,18 @@ class Connection : public CandidatePairInterface,
   // Estimate of the round-trip time over this connection.
   int rtt() const { return rtt_; }
 
+  int unwritable_timeout() const;
+  void set_unwritable_timeout(const rtc::Optional<int>& value_ms) {
+    // TODO(qingsi): Validate configuration parameters in
+    // PeerConnection::ValidateConfiguration.
+    RTC_CHECK_LT(value_ms.value_or(-1), CONNECTION_WRITE_TIMEOUT);
+    unwritable_timeout_ = value_ms;
+  }
+  int unwritable_min_checks() const;
+  void set_unwritable_min_checks(const rtc::Optional<int>& value) {
+    unwritable_min_checks_ = value;
+  }
+
   // Gets the |ConnectionInfo| stats, where |best_connection| has not been
   // populated (default value false).
   ConnectionInfo stats();
@@ -817,6 +829,9 @@ class Connection : public CandidatePairInterface,
 
   PacketLossEstimator packet_loss_estimator_;
 
+  rtc::Optional<int> unwritable_timeout_;
+  rtc::Optional<int> unwritable_min_checks_;
+
   bool reported_;
   IceCandidatePairState state_;
   // Time duration to switch from receiving to not receiving.

p2p/base/port_unittest.cc

@@ -52,6 +52,7 @@ namespace {
 
 constexpr int kDefaultTimeout = 3000;
 constexpr int kShortTimeout = 1000;
+constexpr int kMaxExpectedSimulatedRtt = 200;
 const SocketAddress kLocalAddr1("192.168.1.2", 0);
 const SocketAddress kLocalAddr2("192.168.1.3", 0);
 const SocketAddress kNatAddr1("77.77.77.77", rtc::NAT_SERVER_UDP_PORT);
@@ -2559,6 +2560,12 @@ TEST_F(PortTest, TestConnectionPriority) {
 #endif
 }
 
+// Note that UpdateState takes into account the estimated RTT, and the
+// correctness of using |kMaxExpectedSimulatedRtt| as an upper bound of RTT in
+// the following tests depends on the link rate and the delay distriubtion
+// configured in VirtualSocketServer::AddPacketToNetwork. The tests below use
+// the default setup where the RTT is deterministically one, which generates an
+// estimate given by |MINIMUM_RTT| = 100.
 TEST_F(PortTest, TestWritableState) {
   rtc::ScopedFakeClock clock;
   UDPPort* port1 = CreateUdpPort(kLocalAddr1);
@@ -2604,7 +2611,8 @@ TEST_F(PortTest, TestWritableState) {
   for (uint32_t i = 1; i <= CONNECTION_WRITE_CONNECT_FAILURES; ++i) {
     ch1.Ping(i);
   }
-  int unreliable_timeout_delay = CONNECTION_WRITE_CONNECT_TIMEOUT + 500;
+  int unreliable_timeout_delay =
+      CONNECTION_WRITE_CONNECT_TIMEOUT + kMaxExpectedSimulatedRtt;
   ch1.conn()->UpdateState(unreliable_timeout_delay);
   EXPECT_EQ(Connection::STATE_WRITE_UNRELIABLE, ch1.conn()->write_state());
 
@@ -2615,14 +2623,13 @@ TEST_F(PortTest, TestWritableState) {
   // responses.
   EXPECT_EQ_SIMULATED_WAIT(Connection::STATE_WRITABLE,
                            ch1.conn()->write_state(), kDefaultTimeout, clock);
-
   // Wait long enough for a full timeout (past however long we've already
   // waited).
   for (uint32_t i = 1; i <= CONNECTION_WRITE_CONNECT_FAILURES; ++i) {
     ch1.Ping(unreliable_timeout_delay + i);
   }
   ch1.conn()->UpdateState(unreliable_timeout_delay + CONNECTION_WRITE_TIMEOUT +
-                          500u);
+                          kMaxExpectedSimulatedRtt);
   EXPECT_EQ(Connection::STATE_WRITE_TIMEOUT, ch1.conn()->write_state());
 
   // Even if the connection has timed out, the Connection shouldn't block
@@ -2633,6 +2640,64 @@ TEST_F(PortTest, TestWritableState) {
   ch2.Stop();
 }
 
+// Test writability states using the configured threshold value to replace
+// the default value given by |CONNECTION_WRITE_CONNECT_TIMEOUT| and
+// |CONNECTION_WRITE_CONNECT_FAILURES|.
+TEST_F(PortTest, TestWritableStateWithConfiguredThreshold) {
+  rtc::ScopedFakeClock clock;
+  UDPPort* port1 = CreateUdpPort(kLocalAddr1);
+  port1->SetIceRole(cricket::ICEROLE_CONTROLLING);
+  UDPPort* port2 = CreateUdpPort(kLocalAddr2);
+  port2->SetIceRole(cricket::ICEROLE_CONTROLLED);
+
+  // Set up channels.
+  TestChannel ch1(port1);
+  TestChannel ch2(port2);
+
+  // Acquire addresses.
+  ch1.Start();
+  ch2.Start();
+  ASSERT_EQ_SIMULATED_WAIT(1, ch1.complete_count(), kDefaultTimeout, clock);
+  ASSERT_EQ_SIMULATED_WAIT(1, ch2.complete_count(), kDefaultTimeout, clock);
+
+  // Send a ping from src to dst.
+  ch1.CreateConnection(GetCandidate(port2));
+  ASSERT_TRUE(ch1.conn() != NULL);
+  ch1.Ping();
+  SIMULATED_WAIT(!ch2.remote_address().IsNil(), kShortTimeout, clock);
+
+  // Accept the connection to return the binding response, transition to
+  // writable, and allow data to be sent.
+  ch2.AcceptConnection(GetCandidate(port1));
+  EXPECT_EQ_SIMULATED_WAIT(Connection::STATE_WRITABLE,
+                           ch1.conn()->write_state(), kDefaultTimeout, clock);
+
+  ch1.conn()->set_unwritable_timeout(1000);
+  ch1.conn()->set_unwritable_min_checks(3);
+  // Send two checks.
+  ch1.Ping(1);
+  ch1.Ping(2);
+  // We have not reached the timeout nor have we sent the minimum number of
+  // checks to change the state to Unreliable.
+  ch1.conn()->UpdateState(999);
+  EXPECT_EQ(Connection::STATE_WRITABLE, ch1.conn()->write_state());
+  // We have not sent the minimum number of checks without responses.
+  ch1.conn()->UpdateState(1000 + kMaxExpectedSimulatedRtt);
+  EXPECT_EQ(Connection::STATE_WRITABLE, ch1.conn()->write_state());
+  // Last ping after which the candidate pair should become Unreliable after
+  // timeout.
+  ch1.Ping(3);
+  // We have not reached the timeout.
+  ch1.conn()->UpdateState(999);
+  EXPECT_EQ(Connection::STATE_WRITABLE, ch1.conn()->write_state());
+  // We should be in the state Unreliable now.
+  ch1.conn()->UpdateState(1000 + kMaxExpectedSimulatedRtt);
+  EXPECT_EQ(Connection::STATE_WRITE_UNRELIABLE, ch1.conn()->write_state());
+
+  ch1.Stop();
+  ch2.Stop();
+}
+
 TEST_F(PortTest, TestTimeoutForNeverWritable) {
   UDPPort* port1 = CreateUdpPort(kLocalAddr1);
   port1->SetIceRole(cricket::ICEROLE_CONTROLLING);
@@ -2655,7 +2720,7 @@ TEST_F(PortTest, TestTimeoutForNeverWritable) {
   for (uint32_t i = 1; i <= CONNECTION_WRITE_CONNECT_FAILURES; ++i) {
     ch1.Ping(i);
   }
-  ch1.conn()->UpdateState(CONNECTION_WRITE_TIMEOUT + 500u);
+  ch1.conn()->UpdateState(CONNECTION_WRITE_TIMEOUT + kMaxExpectedSimulatedRtt);
   EXPECT_EQ(Connection::STATE_WRITE_TIMEOUT, ch1.conn()->write_state());
 }
 

p2p/base/tcpport.cc

@@ -323,6 +323,10 @@ void TCPPort::OnAddressReady(rtc::AsyncPacketSocket* socket,
              0, "", true);
 }
 
+// TODO(qingsi): |CONNECTION_WRITE_CONNECT_TIMEOUT| is overriden by
+// |ice_unwritable_timeout| in IceConfig when determining the writability state.
+// Replace this constant with the config parameter assuming the default value if
+// we decide it is also applicable here.
 TCPConnection::TCPConnection(TCPPort* port,
                              const Candidate& candidate,
                              rtc::AsyncPacketSocket* socket)

pc/peerconnection.cc

@@ -666,6 +666,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==(
     rtc::Optional<int> ice_check_interval_strong_connectivity;
     rtc::Optional<int> ice_check_interval_weak_connectivity;
     rtc::Optional<int> ice_check_min_interval;
+    rtc::Optional<int> ice_unwritable_timeout;
+    rtc::Optional<int> ice_unwritable_min_checks;
     rtc::Optional<int> stun_candidate_keepalive_interval;
     rtc::Optional<rtc::IntervalRange> ice_regather_interval_range;
     webrtc::TurnCustomizer* turn_customizer;
@@ -710,6 +712,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==(
          ice_check_interval_weak_connectivity ==
              o.ice_check_interval_weak_connectivity &&
          ice_check_min_interval == o.ice_check_min_interval &&
+         ice_unwritable_timeout == o.ice_unwritable_timeout &&
+         ice_unwritable_min_checks == o.ice_unwritable_min_checks &&
          stun_candidate_keepalive_interval ==
              o.stun_candidate_keepalive_interval &&
          ice_regather_interval_range == o.ice_regather_interval_range &&
@@ -2722,6 +2726,9 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
       configuration.ice_check_interval_strong_connectivity;
   modified_config.ice_check_interval_weak_connectivity =
       configuration.ice_check_interval_weak_connectivity;
+  modified_config.ice_unwritable_timeout = configuration.ice_unwritable_timeout;
+  modified_config.ice_unwritable_min_checks =
+      configuration.ice_unwritable_min_checks;
   modified_config.stun_candidate_keepalive_interval =
       configuration.stun_candidate_keepalive_interval;
   modified_config.turn_customizer = configuration.turn_customizer;

sdk/android/api/org/webrtc/PeerConnection.java

@@ -409,6 +409,12 @@ public class PeerConnection {
     public Integer iceCheckIntervalStrongConnectivityMs;
     public Integer iceCheckIntervalWeakConnectivityMs;
     public Integer iceCheckMinInterval;
+    // The time period in milliseconds for which a candidate pair must wait for response to
+    // connectivitiy checks before it becomes unwritable.
+    public Integer iceUnwritableTimeMs;
+    // The minimum number of connectivity checks that a candidate pair must sent without receiving
+    // response before it becomes unwritable.
+    public Integer iceUnwritableMinChecks;
     // The interval in milliseconds at which STUN candidates will resend STUN binding requests
     // to keep NAT bindings open.
     // The default value in the implementation is used if this field is null.
@@ -462,6 +468,8 @@ public class PeerConnection {
       iceCheckIntervalStrongConnectivityMs = null;
       iceCheckIntervalWeakConnectivityMs = null;
       iceCheckMinInterval = null;
+      iceUnwritableTimeMs = null;
+      iceUnwritableMinChecks = null;
       stunCandidateKeepaliveIntervalMs = null;
       disableIPv6OnWifi = false;
       maxIPv6Networks = 5;
@@ -568,6 +576,16 @@ public class PeerConnection {
       return iceCheckMinInterval;
     }
 
+    @CalledByNative("RTCConfiguration")
+    Integer getIceUnwritableTimeout() {
+      return iceUnwritableTimeMs;
+    }
+
+    @CalledByNative("RTCConfiguration")
+    Integer getIceUnwritableMinChecks() {
+      return iceUnwritableMinChecks;
+    }
+
     @CalledByNative("RTCConfiguration")
     Integer getStunCandidateKeepaliveInterval() {
       return stunCandidateKeepaliveIntervalMs;

sdk/android/src/jni/pc/peerconnection.cc

@@ -187,6 +187,14 @@ void JavaToNativeRTCConfiguration(
       Java_RTCConfiguration_getIceCheckMinInterval(jni, j_rtc_config);
   rtc_config->ice_check_min_interval =
       JavaToNativeOptionalInt(jni, j_ice_check_min_interval);
+  ScopedJavaLocalRef<jobject> j_ice_unwritable_timeout =
+      Java_RTCConfiguration_getIceUnwritableTimeout(jni, j_rtc_config);
+  rtc_config->ice_unwritable_timeout =
+      JavaToNativeOptionalInt(jni, j_ice_unwritable_timeout);
+  ScopedJavaLocalRef<jobject> j_ice_unwritable_min_checks =
+      Java_RTCConfiguration_getIceUnwritableMinChecks(jni, j_rtc_config);
+  rtc_config->ice_unwritable_min_checks =
+      JavaToNativeOptionalInt(jni, j_ice_unwritable_min_checks);
   ScopedJavaLocalRef<jobject> j_stun_candidate_keepalive_interval =
       Java_RTCConfiguration_getStunCandidateKeepaliveInterval(jni,
                                                               j_rtc_config);