diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 7364aa0ced..4cb3d65815 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -515,6 +515,16 @@ class PeerConnectionInterface : public rtc::RefCountInterface { rtc::Optional ice_check_interval_weak_connectivity; rtc::Optional 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 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 ice_unwritable_min_checks; + // The interval in milliseconds at which STUN candidates will resend STUN // binding requests to keep NAT bindings open. rtc::Optional stun_candidate_keepalive_interval; diff --git a/p2p/base/icetransportinternal.h b/p2p/base/icetransportinternal.h index a111f44eac..f5cafd09c7 100644 --- a/p2p/base/icetransportinternal.h +++ b/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 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 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 ice_unwritable_min_checks; // The interval in milliseconds at which STUN candidates will resend STUN // binding requests to keep NAT bindings open. rtc::Optional stun_keepalive_interval; diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 5384801674..1398cb1813 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/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(); diff --git a/p2p/base/port.cc b/p2p/base/port.cc index b142865b75..dfc3f12949 100644 --- a/p2p/base/port.cc +++ b/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_, - CONNECTION_WRITE_CONNECT_FAILURES, - rtt, + TooManyFailures(pings_since_last_response_, unwritable_min_checks(), rtt, now) && - TooLongWithoutResponse(pings_since_last_response_, - CONNECTION_WRITE_CONNECT_TIMEOUT, + TooLongWithoutResponse(pings_since_last_response_, unwritable_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 diff --git a/p2p/base/port.h b/p2p/base/port.h index e45308def5..d9f7bf9e8f 100644 --- a/p2p/base/port.h +++ b/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& 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& 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 unwritable_timeout_; + rtc::Optional unwritable_min_checks_; + bool reported_; IceCandidatePairState state_; // Time duration to switch from receiving to not receiving. diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index 8f5867f1c7..bbf2fef69a 100644 --- a/p2p/base/port_unittest.cc +++ b/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()); } diff --git a/p2p/base/tcpport.cc b/p2p/base/tcpport.cc index 4c1a8e4235..b780a1e527 100644 --- a/p2p/base/tcpport.cc +++ b/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) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index fadb47bcca..7f1ddef78a 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -666,6 +666,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( rtc::Optional ice_check_interval_strong_connectivity; rtc::Optional ice_check_interval_weak_connectivity; rtc::Optional ice_check_min_interval; + rtc::Optional ice_unwritable_timeout; + rtc::Optional ice_unwritable_min_checks; rtc::Optional stun_candidate_keepalive_interval; rtc::Optional 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; diff --git a/sdk/android/api/org/webrtc/PeerConnection.java b/sdk/android/api/org/webrtc/PeerConnection.java index 65aa734983..cf0eed50b9 100644 --- a/sdk/android/api/org/webrtc/PeerConnection.java +++ b/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; diff --git a/sdk/android/src/jni/pc/peerconnection.cc b/sdk/android/src/jni/pc/peerconnection.cc index c18af7f17f..16e40d17df 100644 --- a/sdk/android/src/jni/pc/peerconnection.cc +++ b/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 j_ice_unwritable_timeout = + Java_RTCConfiguration_getIceUnwritableTimeout(jni, j_rtc_config); + rtc_config->ice_unwritable_timeout = + JavaToNativeOptionalInt(jni, j_ice_unwritable_timeout); + ScopedJavaLocalRef 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 j_stun_candidate_keepalive_interval = Java_RTCConfiguration_getStunCandidateKeepaliveInterval(jni, j_rtc_config);