Do not switch best connection on the controlled side too frequently

webrtc/p2p/base/p2ptransportchannel_unittest.cc

Index: webrtc/p2p/base/p2ptransportchannel_unittest.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index cbf5e76a7ae23a13376e6f8aa76ad10559978fbe..484f37f9ea60c44742c676d3f3d0a54634348e85 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1977,12 +1977,32 @@ class P2PTransportChannelPingTest : public testing::Test,
     return channel.SendPacket(data, len, options, 0);
   }
 
+  cricket::Connection* CreateConnectionWithCandidate(
+      cricket::P2PTransportChannel& channel,
+      const std::string& ip_addr,
+      int port,
+      int priority,
+      bool writable) {
+    channel.AddRemoteCandidate(CreateHostCandidate(ip_addr, port, priority));
+    cricket::Connection* conn = WaitForConnectionTo(&channel, ip_addr, port);
+    if (conn && writable) {
+      conn->ReceivedPingResponse();  // make it writable
+    }
+    return conn;
+  }
+
+  void NominateConnection(cricket::Connection* conn) {
+    conn->set_nominated(true);
+    conn->SignalNominated(conn);
+  }
+
   void OnSelectedCandidatePairChanged(
       cricket::TransportChannel* transport_channel,
       cricket::CandidatePairInterface* selected_candidate_pair,
       int last_sent_packet_id) {
     last_selected_candidate_pair_ = selected_candidate_pair;
     last_sent_packet_id_ = last_sent_packet_id;
+    ++num_selected_candidate_pair_switch_;

[pthatcher1, 2016/06/15 21:59:53]

switch_ => switches_

And you can probably drop "num_" from the front.

[honghaiz3, 2016/06/16 23:03:49]

Done.

   }
 
   void ReceivePingOnConnection(cricket::Connection* conn,
@@ -2018,12 +2038,19 @@ 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; }
   cricket::TransportChannelState channel_state() { return channel_state_; }
+  int num_selected_candidate_pair_switch() {
+    return num_selected_candidate_pair_switch_;
+  }
+  void reset_num_selected_candidate_pair_switch() {
+    num_selected_candidate_pair_switch_ = 0;
+  }
 
  private:
   std::unique_ptr<rtc::PhysicalSocketServer> pss_;
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
   rtc::SocketServerScope ss_scope_;
   cricket::CandidatePairInterface* last_selected_candidate_pair_ = nullptr;
+  int num_selected_candidate_pair_switch_ = 0;
   int last_sent_packet_id_ = -1;
   bool channel_ready_to_send_ = false;
   cricket::TransportChannelState channel_state_ = cricket::STATE_INIT;
@@ -2473,6 +2500,77 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) {
   EXPECT_EQ(conn3, ch.best_connection());
 }
 
+TEST_F(P2PTransportChannelPingTest, TestDonotSwitchConnectionTooFrequently) {
+  rtc::ScopedFakeClock clock;
+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("receiving state change", 1, &pa);
+  PrepareChannel(&ch);
+  ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
+  ch.Connect();
+  ch.MaybeStartGathering();
+  // The connections have decreasing priority.
+  cricket::Connection* conn1 =
+      CreateConnectionWithCandidate(ch, "1.1.1.1", 1, 10, true);
+  ASSERT_TRUE(conn1 != nullptr);
+  cricket::Connection* conn2 =
+      CreateConnectionWithCandidate(ch, "2.2.2.2", 2, 9, true);
+  ASSERT_TRUE(conn2 != nullptr);
+  cricket::Connection* conn3 =
+      CreateConnectionWithCandidate(ch, "3.3.3.3", 3, 8, true);
+  ASSERT_TRUE(conn3 != nullptr);
+  cricket::Connection* conn4 =
+      CreateConnectionWithCandidate(ch, "4.4.4.4", 4, 7, false);
+  ASSERT_TRUE(conn4 != nullptr);
+
+  // Simulate the aggressive nomination on the controlling side.

[pthatcher1, 2016/06/15 21:59:53]

the aggressive nomination => aggressive nomination

[honghaiz3, 2016/06/16 23:03:49]

Done.

+  reset_num_selected_candidate_pair_switch();
+  NominateConnection(conn2);
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn2, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();

[pthatcher1, 2016/06/15 21:59:53]

Why not have reset return the last value.  Then you can just do this:

EXPECT_EQ(1, reset_selected_candidate_pair_switches());

[honghaiz3, 2016/06/17 19:18:18]

Done.

+
+  // Selected candidate pair will switch because conn1 has higher priority.
+  NominateConnection(conn1);
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn1, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair will not switch because conn2 has lower priority.
+  NominateConnection(conn2);
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair will switch if it is nominated again.

[pthatcher1, 2016/06/15 21:59:53]

I don't think we should do this one, because an aggressively nominating
controlling side will nominate a given candidate pair multiple times, and will
still goes frequent switches on the controlled side.  Maybe not as frequent, but
still frequent.

[honghaiz3, 2016/06/16 01:13:08]

If we do not do this, how do we handle the case that some channel may not
receive data? When there are multiple candidate pairs, the chance that a
connection is nominated twice consecutively should be pretty small. And when it
converge, it should converge to the one selected on the controlling side. 

+  NominateConnection(conn2);
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn2, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair won't switch because conn3 has lower priority

[pthatcher1, 2016/06/15 21:59:53]

Can you add "until conn3 receives data"?

[honghaiz3, 2016/06/16 23:03:49]

Done.

+  NominateConnection(conn3);
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+  // Will switch if conn3 receives data.
+  conn3->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn3, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Selected candidate pair won't switch even if it is nominated and
+  // received data because it is not writable.
+  NominateConnection(conn4);
+  conn4->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0));
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+  // It will switch if it becomes writable
+  conn4->ReceivedPingResponse();
+  EXPECT_EQ(1, num_selected_candidate_pair_switch());
+  EXPECT_EQ(conn4, last_selected_candidate_pair());
+  reset_num_selected_candidate_pair_switch();
+
+  // Make sure sorting won't reselect candidate pair.
+  SIMULATED_WAIT(false, 10, clock);
+  EXPECT_EQ(0, num_selected_candidate_pair_switch());
+}
+
 // Test that if a new remote candidate has the same address and port with
 // an old one, it will be used to create a new connection.
 TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithAddressReuse) {