diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 5ef81f63e0..088c405538 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -91,10 +91,23 @@ int CompareCandidatePairsByNetworkPreference( return a_and_b_equal; } +uint32_t GetWeakPingIntervalInFieldTrial() { + uint32_t weak_ping_interval = ::strtoul( + webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(), + nullptr, 10); + if (weak_ping_interval) { + return static_cast(weak_ping_interval); + } + return cricket::WEAK_PING_INTERVAL; +} + } // unnamed namespace namespace cricket { +using webrtc::RTCErrorType; +using webrtc::RTCError; + bool IceCredentialsChanged(const std::string& old_ufrag, const std::string& old_pwd, const std::string& new_ufrag, @@ -130,12 +143,10 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, true /* presume_writable_when_fully_relayed */, REGATHER_ON_FAILED_NETWORKS_INTERVAL, RECEIVING_SWITCHING_DELAY) { - uint32_t weak_ping_interval = ::strtoul( - webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(), - nullptr, 10); - if (weak_ping_interval) { - weak_ping_interval_ = static_cast(weak_ping_interval); - } + weak_ping_interval_ = GetWeakPingIntervalInFieldTrial(); + // Validate IceConfig even for mostly built-in constant default values in case + // we change them. + RTC_DCHECK(ValidateIceConfig(config_).ok()); ice_event_log_.set_event_log(event_log); } @@ -562,12 +573,64 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { RTC_LOG(LS_INFO) << "Set STUN keepalive interval to " << config.stun_keepalive_interval_or_default(); } + + RTC_DCHECK(ValidateIceConfig(config_).ok()); } const IceConfig& P2PTransportChannel::config() const { return config_; } +RTCError P2PTransportChannel::ValidateIceConfig(const IceConfig& config) { + if (config.regather_all_networks_interval_range && + config.continual_gathering_policy == GATHER_ONCE) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "regather_all_networks_interval_range specified but " + "continual gathering policy is GATHER_ONCE"); + } + + if (config.ice_check_interval_strong_connectivity_or_default() < + config.ice_check_interval_weak_connectivity.value_or( + GetWeakPingIntervalInFieldTrial())) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Ping interval of candidate pairs is shorter when ICE is " + "strongly connected than that when ICE is weakly " + "connected"); + } + + if (config.receiving_timeout_or_default() < + std::max(config.ice_check_interval_strong_connectivity_or_default(), + config.ice_check_min_interval_or_default())) { + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "Receiving timeout is shorter than the minimal ping interval."); + } + + if (config.backup_connection_ping_interval_or_default() < + config.ice_check_interval_strong_connectivity_or_default()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Ping interval of backup candidate pairs is shorter than " + "that of general candidate pairs when ICE is strongly " + "connected"); + } + + if (config.stable_writable_connection_ping_interval_or_default() < + config.ice_check_interval_strong_connectivity_or_default()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Ping interval of stable and writable candidate pairs is " + "shorter than that of general candidate pairs when ICE is " + "strongly connected"); + } + + if (config.ice_unwritable_timeout_or_default() > CONNECTION_WRITE_TIMEOUT) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "The timeout period for the writability state to become " + "UNRELIABLE is longer than that to become TIMEOUT."); + } + + return RTCError::OK(); +} + int P2PTransportChannel::check_receiving_interval() const { return std::max(MIN_CHECK_RECEIVING_INTERVAL, config_.receiving_timeout_or_default() / 10); diff --git a/p2p/base/p2ptransportchannel.h b/p2p/base/p2ptransportchannel.h index d811b631bd..cb7bbce72b 100644 --- a/p2p/base/p2ptransportchannel.h +++ b/p2p/base/p2ptransportchannel.h @@ -28,6 +28,7 @@ #include #include "api/candidate.h" +#include "api/rtcerror.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h" #include "logging/rtc_event_log/icelogger.h" #include "p2p/base/candidatepairinterface.h" @@ -106,6 +107,7 @@ class P2PTransportChannel : public IceTransportInternal, // TODO(deadbeef): Use rtc::Optional instead of negative values. void SetIceConfig(const IceConfig& config) override; const IceConfig& config() const; + static webrtc::RTCError ValidateIceConfig(const IceConfig& config); void SetMetricsObserver(webrtc::MetricsObserverInterface* observer) override; // From TransportChannel: diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc index a650b22c19..45662ae8e7 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -4423,7 +4423,7 @@ class P2PTransportChannelMostLikelyToWorkFirstTest // we have a selected connection. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestRelayRelayFirstWhenNothingPingedYet) { - const int max_strong_interval = 100; + const int max_strong_interval = 500; P2PTransportChannel& ch = StartTransportChannel(true, max_strong_interval); EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); @@ -4480,7 +4480,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, // in the first round. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestRelayRelayFirstWhenEverythingPinged) { - P2PTransportChannel& ch = StartTransportChannel(true, 100); + P2PTransportChannel& ch = StartTransportChannel(true, 500); EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE); @@ -4511,7 +4511,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, // before we re-ping Relay/Relay connections again. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestNoStarvationOnNonRelayConnection) { - P2PTransportChannel& ch = StartTransportChannel(true, 100); + P2PTransportChannel& ch = StartTransportChannel(true, 500); EXPECT_TRUE_WAIT(ch.ports().size() == 2, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE); @@ -4550,7 +4550,7 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestTcpTurn) { config.ports.push_back(ProtocolAddress(kTurnTcpIntAddr, PROTO_TCP)); allocator()->AddTurnServer(config); - P2PTransportChannel& ch = StartTransportChannel(true, 100); + P2PTransportChannel& ch = StartTransportChannel(true, 500); EXPECT_TRUE_WAIT(ch.ports().size() == 3, kDefaultTimeout); EXPECT_EQ(ch.ports()[0]->Type(), LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), RELAY_PORT_TYPE); diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 5f6edb13cc..cfecf7f4f2 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -147,6 +147,9 @@ const int64_t kForgetPacketAfter = 30000; // 30 seconds namespace cricket { +using webrtc::RTCErrorType; +using webrtc::RTCError; + // TODO(ronghuawu): Use "local", "srflx", "prflx" and "relay". But this requires // the signaling part be updated correspondingly as well. const char LOCAL_PORT_TYPE[] = "local"; diff --git a/p2p/base/port.h b/p2p/base/port.h index 5655b968cc..434aaef204 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -19,6 +19,7 @@ #include "api/candidate.h" #include "api/optional.h" +#include "api/rtcerror.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair.h" #include "logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h" #include "logging/rtc_event_log/icelogger.h" @@ -552,9 +553,6 @@ class Connection : public CandidatePairInterface, 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; diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 84376ce58a..538f5c1f78 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1028,7 +1028,9 @@ RTCError PeerConnection::ValidateConfiguration( "ice_regather_interval_range specified but continual " "gathering policy is GATHER_ONCE"); } - return RTCError::OK(); + auto result = + cricket::P2PTransportChannel::ValidateIceConfig(ParseIceConfig(config)); + return result; } rtc::scoped_refptr diff --git a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm index 65bedde704..686bf2d37d 100644 --- a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm +++ b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm @@ -40,8 +40,8 @@ config.tcpCandidatePolicy = RTCTcpCandidatePolicyDisabled; config.candidateNetworkPolicy = RTCCandidateNetworkPolicyLowCost; const int maxPackets = 60; - const int timeout = 1; - const int interval = 2; + const int timeout = 1500; + const int interval = 2000; config.audioJitterBufferMaxPackets = maxPackets; config.audioJitterBufferFastAccelerate = YES; config.iceConnectionReceivingTimeout = timeout;