diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 4ebd3a41c4..e9055f688f 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -80,6 +80,8 @@ const int STABLE_WRITABLE_CONNECTION_PING_INTERVAL = 2500; // ms static const int MIN_CHECK_RECEIVING_INTERVAL = 50; // ms +static const int RECEIVING_SWITCHING_DELAY = 1000; // ms + // We periodically check if any existing networks do not have any connection // and regather on those networks. static const int DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL = 5 * 60 * 1000; @@ -112,7 +114,8 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, false /* prioritize_most_likely_candidate_pairs */, STABLE_WRITABLE_CONNECTION_PING_INTERVAL, true /* presume_writable_when_fully_relayed */, - DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL) { + DEFAULT_REGATHER_ON_FAILED_NETWORKS_INTERVAL, + RECEIVING_SWITCHING_DELAY) { uint32_t weak_ping_interval = ::strtoul( webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(), nullptr, 10); @@ -184,7 +187,8 @@ void P2PTransportChannel::AddConnection(Connection* connection) { // TODO(honghaiz): Stop the aggressive nomination on the controlling side and // implement the ice-renomination option. bool P2PTransportChannel::ShouldSwitchSelectedConnection( - Connection* new_connection) const { + Connection* new_connection, + bool* missed_receiving_unchanged_threshold) const { if (!new_connection || selected_connection_ == new_connection) { return false; } @@ -193,7 +197,11 @@ bool P2PTransportChannel::ShouldSwitchSelectedConnection( return true; } - int cmp = CompareConnections(selected_connection_, new_connection); + rtc::Optional receiving_unchanged_threshold( + rtc::TimeMillis() - config_.receiving_switching_delay.value_or(0)); + int cmp = CompareConnections(selected_connection_, new_connection, + receiving_unchanged_threshold, + missed_receiving_unchanged_threshold); if (cmp != 0) { return cmp < 0; } @@ -203,6 +211,28 @@ bool P2PTransportChannel::ShouldSwitchSelectedConnection( return new_connection->rtt() <= selected_connection_->rtt() - kMinImprovement; } +bool P2PTransportChannel::MaybeSwitchSelectedConnection( + Connection* new_connection, + const std::string& reason) { + bool missed_receiving_unchanged_threshold = false; + if (ShouldSwitchSelectedConnection(new_connection, + &missed_receiving_unchanged_threshold)) { + LOG(LS_INFO) << "Switching selected connection due to " << reason; + SwitchSelectedConnection(new_connection); + return true; + } + if (missed_receiving_unchanged_threshold && + config_.receiving_switching_delay) { + // If we do not switch to the connection because it missed the receiving + // threshold, the new connection is in a better receiving state than the + // currently selected connection. So we need to re-check whether it needs + // to be switched at a later time. + thread()->PostDelayed(RTC_FROM_HERE, *config_.receiving_switching_delay, + this, MSG_SORT_AND_UPDATE_STATE); + } + return false; +} + void P2PTransportChannel::SetIceRole(IceRole ice_role) { ASSERT(worker_thread_ == rtc::Thread::Current()); if (ice_role_ != ice_role) { @@ -369,6 +399,11 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { LOG(LS_INFO) << "Set regather_on_failed_networks_interval to " << *config_.regather_on_failed_networks_interval; } + if (config.receiving_switching_delay) { + config_.receiving_switching_delay = config.receiving_switching_delay; + LOG(LS_INFO) << "Set receiving_switching_delay to" + << *config_.receiving_switching_delay; + } } const IceConfig& P2PTransportChannel::config() const { @@ -627,20 +662,16 @@ void P2PTransportChannel::OnNominated(Connection* conn) { return; } - if (!ShouldSwitchSelectedConnection(conn)) { + if (MaybeSwitchSelectedConnection(conn, + "nomination on the controlled side")) { + // Now that we have selected a connection, it is time to prune other + // connections and update the read/write state of the channel. + RequestSortAndStateUpdate(); + } else { LOG(LS_INFO) << "Not switching the selected connection on controlled side yet: " << conn->ToString(); - return; } - - LOG(LS_INFO) - << "Switching selected connection on controlled side due to nomination: " - << conn->ToString(); - SwitchSelectedConnection(conn); - // Now that we have selected a connection, it is time to prune other - // connections and update the read/write state of the channel. - RequestSortAndStateUpdate(); } void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) { @@ -987,8 +1018,11 @@ void P2PTransportChannel::MaybeStartPinging() { // Compare two connections based on their writing, receiving, and connected // states. -int P2PTransportChannel::CompareConnectionStates(const Connection* a, - const Connection* b) const { +int P2PTransportChannel::CompareConnectionStates( + const Connection* a, + const Connection* b, + rtc::Optional receiving_unchanged_threshold, + bool* missed_receiving_unchanged_threshold) const { // First, prefer a connection that's writable or presumed writable over // one that's not writable. bool a_writable = a->writable() || PresumedWritable(a); @@ -1015,7 +1049,12 @@ int P2PTransportChannel::CompareConnectionStates(const Connection* a, return a_is_better; } if (!a->receiving() && b->receiving()) { - return b_is_better; + if (!receiving_unchanged_threshold || + (a->receiving_unchanged_since() <= *receiving_unchanged_threshold && + b->receiving_unchanged_since() <= *receiving_unchanged_threshold)) { + return b_is_better; + } + *missed_receiving_unchanged_threshold = true; } // WARNING: Some complexity here about TCP reconnecting. @@ -1082,15 +1121,19 @@ int P2PTransportChannel::CompareConnectionCandidates( (b->remote_candidate().generation() + b->port()->generation()); } -int P2PTransportChannel::CompareConnections(const Connection* a, - const Connection* b) const { +int P2PTransportChannel::CompareConnections( + const Connection* a, + const Connection* b, + rtc::Optional receiving_unchanged_threshold, + bool* missed_receiving_unchanged_threshold) const { RTC_CHECK(a != nullptr); RTC_CHECK(b != nullptr); // We prefer to switch to a writable and receiving connection over a // non-writable or non-receiving connection, even if the latter has // been nominated by the controlling side. - int state_cmp = CompareConnectionStates(a, b); + int state_cmp = CompareConnectionStates(a, b, receiving_unchanged_threshold, + missed_receiving_unchanged_threshold); if (state_cmp != 0) { return state_cmp; } @@ -1143,11 +1186,11 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() { // need to consider switching to. std::stable_sort(connections_.begin(), connections_.end(), [this](const Connection* a, const Connection* b) { - int cmp = CompareConnections(a, b); + int cmp = CompareConnections( + a, b, rtc::Optional(), nullptr); if (cmp != 0) { return cmp > 0; } - // Otherwise, sort based on latency estimate. return a->rtt() < b->rtt(); }); @@ -1164,11 +1207,7 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() { // If necessary, switch to the new choice. Note that |top_connection| doesn't // have to be writable to become the selected connection although it will // have higher priority if it is writable. - if (ShouldSwitchSelectedConnection(top_connection)) { - LOG(LS_INFO) << "Switching selected connection after sorting: " - << top_connection->ToString(); - SwitchSelectedConnection(top_connection); - } + MaybeSwitchSelectedConnection(top_connection, "sorting"); // The controlled side can prune only if the selected connection has been // nominated because otherwise it may prune the connection that will be @@ -1622,7 +1661,7 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) { // we can just set selected to nullptr and re-choose a best assuming that // there was no selected connection. if (selected_connection_ == connection) { - LOG(LS_INFO) << "selected connection destroyed. Will choose a new one."; + LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one."; SwitchSelectedConnection(nullptr); RequestSortAndStateUpdate(); } else { @@ -1726,11 +1765,8 @@ void P2PTransportChannel::OnReadPacket(Connection* connection, // May need to switch the sending connection based on the receiving media path // if this is the controlled side. - if (ice_role_ == ICEROLE_CONTROLLED && - ShouldSwitchSelectedConnection(connection)) { - LOG(LS_INFO) << "Switching selected connection on controlled side due to " - << "data received: " << connection->ToString(); - SwitchSelectedConnection(connection); + if (ice_role_ == ICEROLE_CONTROLLED) { + MaybeSwitchSelectedConnection(connection, "data received"); } } diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 14cda683f5..692b09843b 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -208,7 +208,7 @@ class P2PTransportChannel : public TransportChannelImpl, } private: - rtc::Thread* thread() { return worker_thread_; } + rtc::Thread* thread() const { return worker_thread_; } bool IsGettingPorts() { return allocator_session()->IsGettingPorts(); } // A transport channel is weak if the current best connection is either @@ -222,10 +222,18 @@ class P2PTransportChannel : public TransportChannelImpl, // that's pingable. void MaybeStartPinging(); - // The methods below return a positive value if a is preferable to b, - // a negative value if b is preferable, and 0 if they're equally preferable. - int CompareConnectionStates(const cricket::Connection* a, - const cricket::Connection* b) const; + // The methods below return a positive value if |a| is preferable to |b|, + // a negative value if |b| is preferable, and 0 if they're equally preferable. + // If |receiving_unchanged_threshold| is set, then when |b| is receiving and + // |a| is not, returns a negative value only if |b| has been in receiving + // state and |a| has been in not receiving state since + // |receiving_unchanged_threshold| and sets + // |missed_receiving_unchanged_threshold| to true otherwise. + int CompareConnectionStates( + const cricket::Connection* a, + const cricket::Connection* b, + rtc::Optional receiving_unchanged_threshold, + bool* missed_receiving_unchanged_threshold) const; int CompareConnectionCandidates(const cricket::Connection* a, const cricket::Connection* b) const; // Compares two connections based on the connection states @@ -234,12 +242,12 @@ class P2PTransportChannel : public TransportChannelImpl, // and ShouldSwitchSelectedConnection(). // Returns a positive value if |a| is better than |b|. int CompareConnections(const cricket::Connection* a, - const cricket::Connection* b) const; + const cricket::Connection* b, + rtc::Optional receiving_unchanged_threshold, + bool* missed_receiving_unchanged_threshold) const; bool PresumedWritable(const cricket::Connection* conn) const; - bool ShouldSwitchSelectedConnection(const cricket::Connection* selected, - const cricket::Connection* conn) const; void SortConnectionsAndUpdateState(); void SwitchSelectedConnection(Connection* conn); void UpdateState(); @@ -306,8 +314,16 @@ class P2PTransportChannel : public TransportChannelImpl, void OnCheckAndPing(); void OnRegatherOnFailedNetworks(); - // Returns true if the new_connection should be selected for transmission. - bool ShouldSwitchSelectedConnection(Connection* new_connection) const; + // Returns true if we should switch to the new connection. + // sets |missed_receiving_unchanged_threshold| to true if either + // the selected connection or the new connection missed its + // receiving-unchanged-threshold. + bool ShouldSwitchSelectedConnection( + Connection* new_connection, + bool* missed_receiving_unchanged_threshold) const; + // Returns true if the new_connection is selected for transmission. + bool MaybeSwitchSelectedConnection(Connection* new_connection, + const std::string& reason); void PruneConnections(); bool IsBackupConnection(const Connection* conn) const; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index cf54fc4a27..6673f8f54d 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -350,6 +350,8 @@ class P2PTransportChannelTestBase : public testing::Test, this, &P2PTransportChannelTestBase::OnReadPacket); channel->SignalRoleConflict.connect( this, &P2PTransportChannelTestBase::OnRoleConflict); + channel->SignalSelectedCandidatePairChanged.connect( + this, &P2PTransportChannelTestBase::OnSelectedCandidatePairChanged); channel->SetIceCredentials(local_ice_ufrag, local_ice_pwd); if (remote_ice_credential_source_ == FROM_SETICECREDENTIALS) { channel->SetRemoteIceCredentials(remote_ice_ufrag, remote_ice_pwd); @@ -688,6 +690,19 @@ class P2PTransportChannelTestBase : public testing::Test, new CandidatesData(ch, c)); } } + void OnSelectedCandidatePairChanged( + TransportChannel* transport_channel, + CandidatePairInterface* selected_candidate_pair, + int last_sent_packet_id, + bool ready_to_send) { + ++selected_candidate_pair_switches_; + } + + int reset_selected_candidate_pair_switches() { + int switches = selected_candidate_pair_switches_; + selected_candidate_pair_switches_ = 0; + return switches; + } void PauseCandidates(int endpoint) { GetEndpoint(endpoint)->save_candidates_ = true; @@ -850,6 +865,7 @@ class P2PTransportChannelTestBase : public testing::Test, Endpoint ep2_; RemoteIceCredentialSource remote_ice_credential_source_ = FROM_CANDIDATE; bool force_relay_; + int selected_candidate_pair_switches_ = 0; }; // The tests have only a few outcomes, which we predefine. @@ -1867,6 +1883,7 @@ TEST_F(P2PTransportChannelMultihomedTest, DISABLED_TestBasic) { // Test that we can quickly switch links if an interface goes down. // The controlled side has two interfaces and one will die. TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { + rtc::ScopedFakeClock clock; AddAddress(0, kPublicAddrs[0]); // Adding alternate address will make sure |kPublicAddrs| has the higher // priority than others. This is due to FakeNetwork::AddInterface method. @@ -1880,9 +1897,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { // Create channels and let them go writable, as usual. CreateChannels(1); - EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() && - ep2_ch1()->receiving() && ep2_ch1()->writable(), - 1000, 1000); + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && + ep2_ch1()->receiving() && + ep2_ch1()->writable(), + 3000, clock); EXPECT_TRUE(ep1_ch1()->selected_connection() && ep2_ch1()->selected_connection() && LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) && @@ -1896,19 +1914,19 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { // Blackhole any traffic to or from the public addrs. LOG(LS_INFO) << "Failing over..."; fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[1]); - // The selected connections will switch, so keep references to them. + // The selected connections may switch, so keep references to them. const Connection* selected_connection1 = ep1_ch1()->selected_connection(); const Connection* selected_connection2 = ep2_ch1()->selected_connection(); // We should detect loss of receiving within 1 second or so. - EXPECT_TRUE_WAIT( + EXPECT_TRUE_SIMULATED_WAIT( !selected_connection1->receiving() && !selected_connection2->receiving(), - 3000); + 3000, clock); - // We should switch over to use the alternate addr immediately on both sides + // We should switch over to use the alternate addr on both sides // when we are not receiving. - EXPECT_TRUE_WAIT(ep1_ch1()->selected_connection()->receiving() && - ep2_ch1()->selected_connection()->receiving(), - 1000); + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection()->receiving() && + ep2_ch1()->selected_connection()->receiving(), + 3000, clock); EXPECT_TRUE(LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0])); EXPECT_TRUE( RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1])); @@ -1921,6 +1939,7 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControlledSide) { // Test that we can quickly switch links if an interface goes down. // The controlling side has two interfaces and one will die. TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { + rtc::ScopedFakeClock clock; // Adding alternate address will make sure |kPublicAddrs| has the higher // priority than others. This is due to FakeNetwork::AddInterface method. AddAddress(0, kAlternateAddrs[0]); @@ -1933,9 +1952,10 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { // Create channels and let them go writable, as usual. CreateChannels(1); - EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() && - ep2_ch1()->receiving() && ep2_ch1()->writable(), - 1000, 1000); + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && + ep2_ch1()->receiving() && + ep2_ch1()->writable(), + 3000, clock); EXPECT_TRUE(ep1_ch1()->selected_connection() && ep2_ch1()->selected_connection() && LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) && @@ -1953,15 +1973,15 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { const Connection* selected_connection1 = ep1_ch1()->selected_connection(); const Connection* selected_connection2 = ep2_ch1()->selected_connection(); // We should detect loss of receiving within 1 second or so. - EXPECT_TRUE_WAIT( + EXPECT_TRUE_SIMULATED_WAIT( !selected_connection1->receiving() && !selected_connection2->receiving(), - 3000); + 3000, clock); - // We should switch over to use the alternate addr immediately on both sides + // We should switch over to use the alternate addr on both sides // when we are not receiving. - EXPECT_TRUE_WAIT(ep1_ch1()->selected_connection()->receiving() && - ep2_ch1()->selected_connection()->receiving(), - 1000); + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection()->receiving() && + ep2_ch1()->selected_connection()->receiving(), + 3000, clock); EXPECT_TRUE( LocalCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[0])); EXPECT_TRUE(RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1])); @@ -1971,6 +1991,126 @@ TEST_F(P2PTransportChannelMultihomedTest, TestFailoverControllingSide) { DestroyChannels(); } +// Test that if an interface fails temporarily and then recovers quickly, +// the selected connection will not switch. +// The case that it will switch over to the backup connection if the selected +// connection does not recover after enough time is covered in +// TestFailoverControlledSide and TestFailoverControllingSide. +TEST_F(P2PTransportChannelMultihomedTest, + TestConnectionSwitchDampeningControlledSide) { + rtc::ScopedFakeClock clock; + AddAddress(0, kPublicAddrs[0]); + // Adding alternate address will make sure |kPublicAddrs| has the higher + // priority than others. This is due to FakeNetwork::AddInterface method. + AddAddress(1, kAlternateAddrs[1]); + AddAddress(1, kPublicAddrs[1]); + + // Use only local ports for simplicity. + SetAllocatorFlags(0, kOnlyLocalPorts); + SetAllocatorFlags(1, kOnlyLocalPorts); + + // Create channels and let them go writable, as usual. + CreateChannels(1); + + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && + ep2_ch1()->receiving() && + ep2_ch1()->writable(), + 3000, clock); + EXPECT_TRUE(ep1_ch1()->selected_connection() && + ep2_ch1()->selected_connection() && + LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) && + RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1])); + + // Make the receiving timeout shorter for testing. + IceConfig config = CreateIceConfig(1000, GATHER_ONCE); + ep1_ch1()->SetIceConfig(config); + ep2_ch1()->SetIceConfig(config); + reset_selected_candidate_pair_switches(); + + // Blackhole any traffic to or from the public addrs. + LOG(LS_INFO) << "Failing over..."; + fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[1]); + + // The selected connections may switch, so keep references to them. + const Connection* selected_connection1 = ep1_ch1()->selected_connection(); + const Connection* selected_connection2 = ep2_ch1()->selected_connection(); + // We should detect loss of receiving within 1 second or so. + EXPECT_TRUE_SIMULATED_WAIT( + !selected_connection1->receiving() && !selected_connection2->receiving(), + 3000, clock); + // After a short while, the link recovers itself. + SIMULATED_WAIT(false, 10, clock); + fw()->ClearRules(); + + // We should remain on the public address on both sides and no connection + // switches should have happened. + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection()->receiving() && + ep2_ch1()->selected_connection()->receiving(), + 3000, clock); + EXPECT_TRUE(RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1])); + EXPECT_TRUE(LocalCandidate(ep2_ch1())->address().EqualIPs(kPublicAddrs[1])); + EXPECT_EQ(0, reset_selected_candidate_pair_switches()); + + DestroyChannels(); +} + +// Test that if an interface fails temporarily and then recovers quickly, +// the selected connection will not switch. +TEST_F(P2PTransportChannelMultihomedTest, + TestConnectionSwitchDampeningControllingSide) { + rtc::ScopedFakeClock clock; + // Adding alternate address will make sure |kPublicAddrs| has the higher + // priority than others. This is due to FakeNetwork::AddInterface method. + AddAddress(0, kAlternateAddrs[0]); + AddAddress(0, kPublicAddrs[0]); + AddAddress(1, kPublicAddrs[1]); + + // Use only local ports for simplicity. + SetAllocatorFlags(0, kOnlyLocalPorts); + SetAllocatorFlags(1, kOnlyLocalPorts); + + // Create channels and let them go writable, as usual. + CreateChannels(1); + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->receiving() && ep1_ch1()->writable() && + ep2_ch1()->receiving() && + ep2_ch1()->writable(), + 3000, clock); + EXPECT_TRUE(ep1_ch1()->selected_connection() && + ep2_ch1()->selected_connection() && + LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0]) && + RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1])); + + // Make the receiving timeout shorter for testing. + IceConfig config = CreateIceConfig(1000, GATHER_ONCE); + ep1_ch1()->SetIceConfig(config); + ep2_ch1()->SetIceConfig(config); + reset_selected_candidate_pair_switches(); + + // Blackhole any traffic to or from the public addrs. + LOG(LS_INFO) << "Failing over..."; + fw()->AddRule(false, rtc::FP_ANY, rtc::FD_ANY, kPublicAddrs[0]); + // The selected connections may switch, so keep references to them. + const Connection* selected_connection1 = ep1_ch1()->selected_connection(); + const Connection* selected_connection2 = ep2_ch1()->selected_connection(); + // We should detect loss of receiving within 1 second or so. + EXPECT_TRUE_SIMULATED_WAIT( + !selected_connection1->receiving() && !selected_connection2->receiving(), + 3000, clock); + // The link recovers after a short while. + SIMULATED_WAIT(false, 10, clock); + fw()->ClearRules(); + + // We should not switch to the alternate addr on both sides because of the + // dampening. + EXPECT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection()->receiving() && + ep2_ch1()->selected_connection()->receiving(), + 3000, clock); + EXPECT_TRUE(LocalCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[0])); + EXPECT_TRUE(RemoteCandidate(ep2_ch1())->address().EqualIPs(kPublicAddrs[0])); + EXPECT_EQ(0, reset_selected_candidate_pair_switches()); + DestroyChannels(); +} + // Tests that a Wifi-Wifi connection has the highest precedence. TEST_F(P2PTransportChannelMultihomedTest, TestPreferWifiToWifiConnection) { // The interface names are chosen so that |cellular| would have higher @@ -2448,7 +2588,7 @@ class P2PTransportChannelPingTest : public testing::Test, bool channel_ready_to_send() { return channel_ready_to_send_; } void reset_channel_ready_to_send() { channel_ready_to_send_ = false; } TransportChannelState channel_state() { return channel_state_; } - int get_and_reset_selected_candidate_pair_switches() { + int reset_selected_candidate_pair_switches() { int switches = selected_candidate_pair_switches_; selected_candidate_pair_switches_ = 0; return switches; @@ -3055,7 +3195,7 @@ TEST_F(P2PTransportChannelPingTest, ASSERT_TRUE(conn2 != nullptr); // Initially, connections are selected based on priority. - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 receives data; it becomes selected. @@ -3063,17 +3203,17 @@ TEST_F(P2PTransportChannelPingTest, // conn2 is larger. SIMULATED_WAIT(false, 1, clock); conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn2, last_selected_candidate_pair()); // conn1 also receives data; it becomes selected due to priority again. conn1->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn1, last_selected_candidate_pair()); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 10, clock); - EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(0, reset_selected_candidate_pair_switches()); } TEST_F(P2PTransportChannelPingTest, @@ -3097,28 +3237,28 @@ TEST_F(P2PTransportChannelPingTest, // Advance the clock to have a non-zero last-data-receiving time. SIMULATED_WAIT(false, 1, clock); conn1->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 is nominated; it becomes the selected connection. NominateConnection(conn2); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn2, last_selected_candidate_pair()); NominateConnection(conn1); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 received data more recently; it is selected now because it // received data more recently. SIMULATED_WAIT(false, 1, clock); conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); EXPECT_EQ(conn2, last_selected_candidate_pair()); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 10, clock); - EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(0, reset_selected_candidate_pair_switches()); } TEST_F(P2PTransportChannelPingTest, @@ -3139,26 +3279,26 @@ TEST_F(P2PTransportChannelPingTest, ASSERT_TRUE(conn2 != nullptr); NominateConnection(conn1); - EXPECT_EQ(1, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(1, reset_selected_candidate_pair_switches()); // conn2 becomes writable; it is selected even though it is not nominated. conn2->ReceivedPingResponse(LOW_RTT); - EXPECT_EQ_SIMULATED_WAIT(1, get_and_reset_selected_candidate_pair_switches(), + EXPECT_EQ_SIMULATED_WAIT(1, reset_selected_candidate_pair_switches(), kDefaultTimeout, clock); EXPECT_EQ_SIMULATED_WAIT(conn2, last_selected_candidate_pair(), kDefaultTimeout, clock); // If conn1 is also writable, it will become selected. conn1->ReceivedPingResponse(LOW_RTT); - EXPECT_EQ_SIMULATED_WAIT(1, get_and_reset_selected_candidate_pair_switches(), + EXPECT_EQ_SIMULATED_WAIT(1, reset_selected_candidate_pair_switches(), kDefaultTimeout, clock); EXPECT_EQ_SIMULATED_WAIT(conn1, last_selected_candidate_pair(), kDefaultTimeout, clock); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 10, clock); - EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); + EXPECT_EQ(0, reset_selected_candidate_pair_switches()); } // Test that if a new remote candidate has the same address and port with @@ -3289,7 +3429,9 @@ TEST_F(P2PTransportChannelPingTest, TestConnectionPrunedAgain) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); - ch.SetIceConfig(CreateIceConfig(1000, GATHER_ONCE)); + IceConfig config = CreateIceConfig(1000, GATHER_ONCE); + config.receiving_switching_delay = rtc::Optional(800); + ch.SetIceConfig(config); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 100)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index 903f13d7a6..b6173ebf45 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -898,12 +898,15 @@ void Connection::set_write_state(WriteState value) { } } -void Connection::set_receiving(bool value) { - if (value != receiving_) { - LOG_J(LS_VERBOSE, this) << "set_receiving to " << value; - receiving_ = value; - SignalStateChange(this); +void Connection::UpdateReceiving(int64_t now) { + bool receiving = now <= last_received() + receiving_timeout_; + if (receiving_ == receiving) { + return; } + LOG_J(LS_VERBOSE, this) << "set_receiving to " << receiving; + receiving_ = receiving; + receiving_unchanged_since_ = now; + SignalStateChange(this); } void Connection::set_state(State state) { @@ -948,8 +951,8 @@ void Connection::OnReadPacket( if (!port_->GetStunMessage(data, size, addr, &msg, &remote_ufrag)) { // The packet did not parse as a valid STUN message // This is a data packet, pass it along. - set_receiving(true); last_data_received_ = rtc::TimeMillis(); + UpdateReceiving(last_data_received_); recv_rate_tracker_.AddSamples(size); SignalReadPacket(this, data, size, packet_time); @@ -1165,10 +1168,8 @@ void Connection::UpdateState(int64_t now) { set_write_state(STATE_WRITE_TIMEOUT); } - // Check the receiving state. - int64_t last_recv_time = last_received(); - bool receiving = now <= last_recv_time + receiving_timeout_; - set_receiving(receiving); + // Update the receiving state. + UpdateReceiving(now); if (dead(now)) { Destroy(); } @@ -1186,8 +1187,8 @@ void Connection::Ping(int64_t now) { } void Connection::ReceivedPing() { - set_receiving(true); last_ping_received_ = rtc::TimeMillis(); + UpdateReceiving(last_ping_received_); } void Connection::ReceivedPingResponse(int rtt) { @@ -1196,11 +1197,11 @@ void Connection::ReceivedPingResponse(int rtt) { // So if we're not already, become writable. We may be bringing a pruned // connection back to life, but if we don't really want it, we can always // prune it again. - set_receiving(true); + last_ping_response_received_ = rtc::TimeMillis(); + UpdateReceiving(last_ping_response_received_); set_write_state(STATE_WRITABLE); set_state(STATE_SUCCEEDED); pings_since_last_response_.clear(); - last_ping_response_received_ = rtc::TimeMillis(); rtt_samples_++; rtt_ = (RTT_RATIO * rtt_ + rtt) / (RTT_RATIO + 1); } diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 2c3b61b718..37926c9556 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -590,6 +590,10 @@ class Connection : public CandidatePairInterface, // Returns the last received time of any data, stun request, or stun // response in milliseconds int64_t last_received() const; + // Returns the last time when the connection changed its receiving state. + int64_t receiving_unchanged_since() const { + return receiving_unchanged_since_; + } bool stable(int64_t now) const; @@ -618,7 +622,7 @@ class Connection : public CandidatePairInterface, // Changes the state and signals if necessary. void set_write_state(WriteState value); - void set_receiving(bool value); + void UpdateReceiving(int64_t now); void set_state(State state); void set_connected(bool value); @@ -648,6 +652,7 @@ class Connection : public CandidatePairInterface, // side int64_t last_data_received_; int64_t last_ping_response_received_; + int64_t receiving_unchanged_since_ = 0; std::vector pings_since_last_response_; rtc::RateTracker recv_rate_tracker_; diff --git a/webrtc/p2p/base/transport.h b/webrtc/p2p/base/transport.h index cf8223851c..e4753d217d 100644 --- a/webrtc/p2p/base/transport.h +++ b/webrtc/p2p/base/transport.h @@ -194,6 +194,11 @@ struct IceConfig { // active network having no connection on it. rtc::Optional regather_on_failed_networks_interval; + // The time period in which we will not switch the selected connection + // when a new connection becomes receiving but the selected connection is not + // in case that the selected connection may become receiving soon. + rtc::Optional receiving_switching_delay; + IceConfig() {} IceConfig(int receiving_timeout_ms, int backup_connection_ping_interval, @@ -201,7 +206,8 @@ struct IceConfig { bool prioritize_most_likely_candidate_pairs, int stable_writable_connection_ping_interval_ms, bool presume_writable_when_fully_relayed, - int regather_on_failed_networks_interval_ms) + int regather_on_failed_networks_interval_ms, + int receiving_switching_delay_ms) : receiving_timeout(receiving_timeout_ms), backup_connection_ping_interval(backup_connection_ping_interval), continual_gathering_policy(gathering_policy), @@ -212,7 +218,8 @@ struct IceConfig { presume_writable_when_fully_relayed( presume_writable_when_fully_relayed), regather_on_failed_networks_interval( - regather_on_failed_networks_interval_ms) {} + regather_on_failed_networks_interval_ms), + receiving_switching_delay(receiving_switching_delay_ms) {} }; bool BadTransportDescription(const std::string& desc, std::string* err_desc);