First step of passive aggressive nomination. On the controlled side, a stun request without use-candidate attribute will be used for sending media. BUG=4900 Review URL: https://codereview.webrtc.org/1270613006 Cr-Commit-Position: refs/heads/master@{#9747}
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 732e527..094a8dc 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -229,8 +229,7 @@ this, &P2PTransportChannel::OnConnectionStateChange); connection->SignalDestroyed.connect( this, &P2PTransportChannel::OnConnectionDestroyed); - connection->SignalUseCandidate.connect( - this, &P2PTransportChannel::OnUseCandidate); + connection->SignalNominated.connect(this, &P2PTransportChannel::OnNominated); } void P2PTransportChannel::SetIceRole(IceRole ice_role) { @@ -522,7 +521,7 @@ // There shouldn't be an existing connection with this remote address. // When ports are muxed, this channel might get multiple unknown address // signals. In that case if the connection is already exists, we should - // simply ignore the signal othewise send server error. + // simply ignore the signal otherwise send server error. if (port->GetConnection(remote_candidate.address())) { if (port_muxed) { LOG(LS_INFO) << "Connection already exists for peer reflexive " @@ -553,6 +552,13 @@ AddConnection(connection); connection->ReceivedPing(); + bool received_use_candidate = + stun_msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr; + if (received_use_candidate && ice_role_ == ICEROLE_CONTROLLED) { + connection->set_nominated(true); + OnNominated(connection); + } + // Update the list of connections since we just added another. We do this // after sending the response since it could (in principle) delete the // connection in question. @@ -574,7 +580,7 @@ } } -void P2PTransportChannel::OnUseCandidate(Connection* conn) { +void P2PTransportChannel::OnNominated(Connection* conn) { ASSERT(worker_thread_ == rtc::Thread::Current()); ASSERT(ice_role_ == ICEROLE_CONTROLLED); @@ -917,16 +923,10 @@ // Any changes after this point will require a re-sort. sort_dirty_ = false; - // Get a list of the networks that we are using. - std::set<rtc::Network*> networks; - for (uint32 i = 0; i < connections_.size(); ++i) - networks.insert(connections_[i]->port()->Network()); - // Find the best alternative connection by sorting. It is important to note // that amongst equal preference, writable connections, this will choose the // one whose estimated latency is lowest. So it is the only one that we // need to consider switching to. - ConnectionCompare cmp; std::stable_sort(connections_.begin(), connections_.end(), cmp); LOG(LS_VERBOSE) << "Sorting available connections:"; @@ -934,46 +934,25 @@ LOG(LS_VERBOSE) << connections_[i]->ToString(); } - Connection* top_connection = NULL; - if (connections_.size() > 0) - top_connection = connections_[0]; - - // We don't want to pick the best connections if channel is - // CONTROLLED, as connections will be selected by the CONTROLLING - // agent. + Connection* top_connection = + (connections_.size() > 0) ? connections_[0] : nullptr; // If necessary, switch to the new choice. - if (ice_role_ == ICEROLE_CONTROLLING) { - if (ShouldSwitch(best_connection_, top_connection)) { - LOG(LS_INFO) << "Switching best connection on controlling side: " - << top_connection->ToString(); - SwitchBestConnectionTo(top_connection); - } + // Note that |top_connection| doesn't have to be writable to become the best + // connection although it will have higher priority if it is writable. + // The controlled side can switch the best connection only if the current + // |best connection_| has not been nominated by the controlling side yet. + if ((ice_role_ == ICEROLE_CONTROLLING || !best_nominated_connection()) && + ShouldSwitch(best_connection_, top_connection)) { + LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString(); + SwitchBestConnectionTo(top_connection); } - // We can prune any connection for which there is a connected, writable - // connection on the same network with better or equal priority. We leave - // those with better priority just in case they become writable later (at - // which point, we would prune out the current best connection). We leave - // connections on other networks because they may not be using the same - // resources and they may represent very distinct paths over which we can - // switch. If the |primier| connection is not connected, we may be - // reconnecting a TCP connection and temporarily do not prune connections in - // this network. See the big comment in CompareConnections. - std::set<rtc::Network*>::iterator network; - for (network = networks.begin(); network != networks.end(); ++network) { - Connection* primier = GetBestConnectionOnNetwork(*network); - if (!primier || (primier->write_state() != Connection::STATE_WRITABLE) || - !primier->connected()) - continue; - - for (uint32 i = 0; i < connections_.size(); ++i) { - if ((connections_[i] != primier) && - (connections_[i]->port()->Network() == *network) && - (CompareConnectionCandidates(primier, connections_[i]) >= 0)) { - connections_[i]->Prune(); - } - } + // Controlled side can prune only if the best connection has been nominated. + // because otherwise it may delete the connection that will be selected by + // the controlling side. + if (ice_role_ == ICEROLE_CONTROLLING || best_nominated_connection()) { + PruneConnections(); } // Check if all connections are timedout. @@ -1000,6 +979,41 @@ UpdateChannelState(); } +Connection* P2PTransportChannel::best_nominated_connection() const { + return (best_connection_ && best_connection_->nominated()) ? best_connection_ + : nullptr; +} + +void P2PTransportChannel::PruneConnections() { + // We can prune any connection for which there is a connected, writable + // connection on the same network with better or equal priority. We leave + // those with better priority just in case they become writable later (at + // which point, we would prune out the current best connection). We leave + // connections on other networks because they may not be using the same + // resources and they may represent very distinct paths over which we can + // switch. If the |primier| connection is not connected, we may be + // reconnecting a TCP connection and temporarily do not prune connections in + // this network. See the big comment in CompareConnections. + + // Get a list of the networks that we are using. + std::set<rtc::Network*> networks; + for (const Connection* conn : connections_) { + networks.insert(conn->port()->Network()); + } + for (rtc::Network* network : networks) { + Connection* primier = GetBestConnectionOnNetwork(network); + if (!(primier && primier->writable() && primier->connected())) { + continue; + } + + for (Connection* conn : connections_) { + if ((conn != primier) && (conn->port()->Network() == network) && + (CompareConnectionCandidates(primier, conn) >= 0)) { + conn->Prune(); + } + } + } +} // Track the best connection, and let listeners know void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) { @@ -1332,6 +1346,13 @@ // Let the client know of an incoming packet SignalReadPacket(this, data, len, packet_time, 0); + + // May need to switch the sending connection based on the receiving media path + // if this is the controlled side. + if (ice_role_ == ICEROLE_CONTROLLED && !best_nominated_connection() && + connection->writable() && best_connection_ != connection) { + SwitchBestConnectionTo(connection); + } } void P2PTransportChannel::OnReadyToSend(Connection* connection) {
diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 286a547..335ee7d 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h
@@ -211,7 +211,7 @@ void OnReadyToSend(Connection* connection); void OnConnectionDestroyed(Connection *connection); - void OnUseCandidate(Connection* conn); + void OnNominated(Connection* conn); virtual void OnMessage(rtc::Message *pmsg); void OnSort(); @@ -219,6 +219,9 @@ void OnCheckReceiving(); + void PruneConnections(); + Connection* best_nominated_connection() const; + P2PTransport* transport_; PortAllocator *allocator_; rtc::Thread *worker_thread_;
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index c337107..3bb3be5 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1764,3 +1764,181 @@ EXPECT_TRUE_WAIT(ch.receiving(), 1000); EXPECT_TRUE_WAIT(!ch.receiving(), 1000); } + +// The controlled side will select a connection as the "best connection" based +// on priority until the controlling side nominates a connection, at which +// point the controlled side will select that connection as the +// "best connection". +TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.SetIceRole(cricket::ICEROLE_CONTROLLED); + ch.Connect(); + ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 1)); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(conn1, ch.best_connection()); + + // When a higher priority candidate comes in, the new connection is chosen + // as the best connection. + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 10)); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(conn2, ch.best_connection()); + + // If a stun request with use-candidate attribute arrives, the receiving + // connection will be set as the best connection, even though + // its priority is lower. + ch.OnCandidate(CreateCandidate("3.3.3.3", 3, 1)); + cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3); + ASSERT_TRUE(conn3 != nullptr); + // Because it has a lower priority, the best connection is still conn2. + EXPECT_EQ(conn2, ch.best_connection()); + conn3->ReceivedPingResponse(); // Become writable. + // But if it is nominated via use_candidate, it is chosen as the best + // connection. + conn3->set_nominated(true); + conn3->SignalNominated(conn3); + EXPECT_EQ(conn3, ch.best_connection()); + + // Even if another higher priority candidate arrives, + // it will not be set as the best connection because the best connection + // is nominated by the controlling side. + ch.OnCandidate(CreateCandidate("4.4.4.4", 4, 100)); + cricket::Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4); + ASSERT_TRUE(conn4 != nullptr); + EXPECT_EQ(conn3, ch.best_connection()); + // But if it is nominated via use_candidate and writable, it will be set as + // the best connection. + conn4->set_nominated(true); + conn4->SignalNominated(conn4); + // Not switched yet because conn4 is not writable. + EXPECT_EQ(conn3, ch.best_connection()); + // The best connection switches after conn4 becomes writable. + conn4->ReceivedPingResponse(); + EXPECT_EQ(conn4, ch.best_connection()); +} + +// The controlled side will select a connection as the "best connection" based +// on requests from an unknown address before the controlling side nominates +// a connection, and will nominate a connection from an unknown address if the +// request contains the use_candidate attribute. +TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.SetIceRole(cricket::ICEROLE_CONTROLLED); + ch.Connect(); + // A minimal STUN message with prflx priority. + cricket::IceMessage request; + request.SetType(cricket::STUN_BINDING_REQUEST); + request.AddAttribute(new cricket::StunByteStringAttribute( + cricket::STUN_ATTR_USERNAME, kIceUfrag[1])); + uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24; + request.AddAttribute(new cricket::StunUInt32Attribute( + cricket::STUN_ATTR_PRIORITY, prflx_priority)); + cricket::Port* port = GetPort(&ch); + port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(conn1, ch.best_connection()); + conn1->ReceivedPingResponse(); + EXPECT_EQ(conn1, ch.best_connection()); + + // Another connection is nominated via use_candidate. + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 1)); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn2 != nullptr); + // Because it has a lower priority, the best connection is still conn1. + EXPECT_EQ(conn1, ch.best_connection()); + // When it is nominated via use_candidate and writable, it is chosen as the + // best connection. + conn2->ReceivedPingResponse(); // Become writable. + conn2->set_nominated(true); + conn2->SignalNominated(conn2); + EXPECT_EQ(conn2, ch.best_connection()); + + // Another request with unknown address, it will not be set as the best + // connection because the best connection was nominated by the controlling + // side. + port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 3), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3); + ASSERT_TRUE(conn3 != nullptr); + conn3->ReceivedPingResponse(); // Become writable. + EXPECT_EQ(conn2, ch.best_connection()); + + // However if the request contains use_candidate attribute, it will be + // selected as the best connection. + request.AddAttribute( + new cricket::StunByteStringAttribute(cricket::STUN_ATTR_USE_CANDIDATE)); + port->SignalUnknownAddress(port, rtc::SocketAddress("4.4.4.4", 4), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + cricket::Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4); + ASSERT_TRUE(conn4 != nullptr); + // conn4 is not the best connection yet because it is not writable. + EXPECT_EQ(conn2, ch.best_connection()); + conn4->ReceivedPingResponse(); // Become writable. + EXPECT_EQ(conn4, ch.best_connection()); +} + +// The controlled side will select a connection as the "best connection" +// based on media received until the controlling side nominates a connection, +// at which point the controlled side will select that connection as +// the "best connection". +TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) { + cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); + PrepareChannel(&ch); + ch.SetIceRole(cricket::ICEROLE_CONTROLLED); + ch.Connect(); + ch.OnCandidate(CreateCandidate("1.1.1.1", 1, 10)); + cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + EXPECT_EQ(conn1, ch.best_connection()); + + // If a data packet is received on conn2, the best connection should + // switch to conn2 because the controlled side must mirror the media path + // chosen by the controlling side. + ch.OnCandidate(CreateCandidate("2.2.2.2", 2, 1)); + cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); + ASSERT_TRUE(conn2 != nullptr); + conn2->ReceivedPing(); // Become readable. + // Do not switch because it is not writable. + conn2->OnReadPacket("ABC", 3, rtc::CreatePacketTime(0)); + EXPECT_EQ(conn1, ch.best_connection()); + + conn2->ReceivedPingResponse(); // Become writable. + // Switch because it is writable. + conn2->OnReadPacket("DEF", 3, rtc::CreatePacketTime(0)); + EXPECT_EQ(conn2, ch.best_connection()); + + // Now another STUN message with an unknown address and use_candidate will + // nominate the best connection. + cricket::IceMessage request; + request.SetType(cricket::STUN_BINDING_REQUEST); + request.AddAttribute(new cricket::StunByteStringAttribute( + cricket::STUN_ATTR_USERNAME, kIceUfrag[1])); + uint32 prflx_priority = cricket::ICE_TYPE_PREFERENCE_PRFLX << 24; + request.AddAttribute(new cricket::StunUInt32Attribute( + cricket::STUN_ATTR_PRIORITY, prflx_priority)); + request.AddAttribute( + new cricket::StunByteStringAttribute(cricket::STUN_ATTR_USE_CANDIDATE)); + cricket::Port* port = GetPort(&ch); + port->SignalUnknownAddress(port, rtc::SocketAddress("3.3.3.3", 3), + cricket::PROTO_UDP, &request, kIceUfrag[1], false); + cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3); + ASSERT_TRUE(conn3 != nullptr); + EXPECT_EQ(conn2, ch.best_connection()); // Not writable yet. + conn3->ReceivedPingResponse(); // Become writable. + EXPECT_EQ(conn3, ch.best_connection()); + + // Now another data packet will not switch the best connection because the + // best connection was nominated by the controlling side. + conn2->ReceivedPing(); + conn2->ReceivedPingResponse(); + conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); + EXPECT_EQ(conn3, ch.best_connection()); +}
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index 89eed8b..8048201 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc
@@ -787,6 +787,7 @@ connected_(true), pruned_(false), use_candidate_attr_(false), + nominated_(false), remote_ice_mode_(ICEMODE_FULL), requests_(port->thread()), rtt_(DEFAULT_RTT), @@ -954,8 +955,10 @@ if (port_->GetIceRole() == ICEROLE_CONTROLLED) { const StunByteStringAttribute* use_candidate_attr = msg->GetByteString(STUN_ATTR_USE_CANDIDATE); - if (use_candidate_attr) - SignalUseCandidate(this); + if (use_candidate_attr) { + set_nominated(true); + SignalNominated(this); + } } } else { // The packet had the right local username, but the remote username
diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 143b2e4..fbda9ce 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h
@@ -488,6 +488,9 @@ bool use_candidate_attr() const { return use_candidate_attr_; } void set_use_candidate_attr(bool enable); + bool nominated() const { return nominated_; } + void set_nominated(bool nominated) { nominated_ = nominated; } + void set_remote_ice_mode(IceMode mode) { remote_ice_mode_ = mode; } @@ -519,10 +522,9 @@ bool reported() const { return reported_; } void set_reported(bool reported) { reported_ = reported;} - // This flag will be set if this connection is the chosen one for media - // transmission. This connection will send STUN ping with USE-CANDIDATE - // attribute. - sigslot::signal1<Connection*> SignalUseCandidate; + // This signal will be fired if this connection is nominated by the + // controlling side. + sigslot::signal1<Connection*> SignalNominated; // Invoked when Connection receives STUN error response with 487 code. void HandleRoleConflictFromPeer(); @@ -581,10 +583,13 @@ bool connected_; bool pruned_; // By default |use_candidate_attr_| flag will be true, - // as we will be using agrressive nomination. + // as we will be using aggressive nomination. // But when peer is ice-lite, this flag "must" be initialized to false and // turn on when connection becomes "best connection". bool use_candidate_attr_; + // Whether this connection has been nominated by the controlling side via + // the use_candidate attribute. + bool nominated_; IceMode remote_ice_mode_; StunRequestManager requests_; uint32 rtt_;