Add the last_sent_packet_id to the candidate pair change signal
so that the call knows which packet ids were sent on the previous candidate pair.
Note that packet_id is actually 16bits, so we can use -1 for values that are not set.
Also moved the tests for candidate pair changes to TestSelectConnectionBeforeNomination.
BUG=
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/1842093002 .
Cr-Commit-Position: refs/heads/master@{#12184}
diff --git a/webrtc/base/networkroute.h b/webrtc/base/networkroute.h
index dcf9015..95c1c08 100644
--- a/webrtc/base/networkroute.h
+++ b/webrtc/base/networkroute.h
@@ -21,16 +21,24 @@
bool connected;
uint16_t local_network_id;
uint16_t remote_network_id;
+ int last_sent_packet_id; // Last packet id sent on the PREVIOUS route.
NetworkRoute()
- : connected(false), local_network_id(0), remote_network_id(0) {}
+ : connected(false),
+ local_network_id(0),
+ remote_network_id(0),
+ last_sent_packet_id(-1) {}
// The route is connected if the local and remote network ids are provided.
- NetworkRoute(uint16_t local_net_id, uint16_t remote_net_id)
+ NetworkRoute(uint16_t local_net_id,
+ uint16_t remote_net_id,
+ int last_packet_id)
: connected(true),
local_network_id(local_net_id),
- remote_network_id(remote_net_id) {}
+ remote_network_id(remote_net_id),
+ last_sent_packet_id(last_packet_id) {}
+ // |last_sent_packet_id| does not affect the NetworkRoute comparison.
bool operator==(const NetworkRoute& nr) const {
return connected == nr.connected &&
local_network_id == nr.local_network_id &&
diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc
index 2006391..667b2c8 100644
--- a/webrtc/p2p/base/dtlstransportchannel.cc
+++ b/webrtc/p2p/base/dtlstransportchannel.cc
@@ -638,9 +638,11 @@
void DtlsTransportChannelWrapper::OnSelectedCandidatePairChanged(
TransportChannel* channel,
- CandidatePairInterface* selected_candidate_pair) {
+ CandidatePairInterface* selected_candidate_pair,
+ int last_sent_packet_id) {
ASSERT(channel == channel_);
- SignalSelectedCandidatePairChanged(this, selected_candidate_pair);
+ SignalSelectedCandidatePairChanged(this, selected_candidate_pair,
+ last_sent_packet_id);
}
void DtlsTransportChannelWrapper::OnConnectionRemoved(
diff --git a/webrtc/p2p/base/dtlstransportchannel.h b/webrtc/p2p/base/dtlstransportchannel.h
index 0c6b303..96ea29f 100644
--- a/webrtc/p2p/base/dtlstransportchannel.h
+++ b/webrtc/p2p/base/dtlstransportchannel.h
@@ -218,7 +218,8 @@
void OnRouteChange(TransportChannel* channel, const Candidate& candidate);
void OnSelectedCandidatePairChanged(
TransportChannel* channel,
- CandidatePairInterface* selected_candidate_pair);
+ CandidatePairInterface* selected_candidate_pair,
+ int last_sent_packet_id);
void OnConnectionRemoved(TransportChannelImpl* channel);
void Reconnect();
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index fbac247..6653241 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -983,6 +983,7 @@
return -1;
}
+ last_sent_packet_id_ = options.packet_id;
int sent = best_connection_->Send(data, len, options);
if (sent <= 0) {
ASSERT(sent < 0);
@@ -1176,7 +1177,8 @@
}
// TODO(honghaiz): rename best_connection_ with selected_connection_ or
// selected_candidate pair_.
- SignalSelectedCandidatePairChanged(this, best_connection_);
+ SignalSelectedCandidatePairChanged(this, best_connection_,
+ last_sent_packet_id_);
}
// Warning: UpdateState should eventually be called whenever a connection
diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h
index 23e4d7f..5be9d04 100644
--- a/webrtc/p2p/base/p2ptransportchannel.h
+++ b/webrtc/p2p/base/p2ptransportchannel.h
@@ -326,6 +326,7 @@
int weak_ping_interval_ = WEAK_PING_INTERVAL;
TransportChannelState state_ = TransportChannelState::STATE_INIT;
IceConfig config_;
+ int last_sent_packet_id_ = -1; // -1 indicates no packet was sent before.
RTC_DISALLOW_COPY_AND_ASSIGN(P2PTransportChannel);
};
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index e2ec44e..87e92a5 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -317,8 +317,6 @@
this, &P2PTransportChannelTestBase::OnReadPacket);
channel->SignalRoleConflict.connect(
this, &P2PTransportChannelTestBase::OnRoleConflict);
- channel->SignalSelectedCandidatePairChanged.connect(
- this, &P2PTransportChannelTestBase::OnSelectedCandidatePairChanged);
channel->SetIceCredentials(local_ice_ufrag, local_ice_pwd);
if (clear_remote_candidates_ufrag_pwd_) {
// This only needs to be set if we're clearing them from the
@@ -566,7 +564,7 @@
void TestSendRecv(int channels) {
for (int i = 0; i < 10; ++i) {
- const char* data = "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
+ const char* data = "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
int len = static_cast<int>(strlen(data));
// local_channel1 <==> remote_channel1
EXPECT_EQ_WAIT(len, SendData(ep1_ch1(), data, len), 1000);
@@ -744,12 +742,6 @@
channel->SetIceRole(new_role);
}
- void OnSelectedCandidatePairChanged(
- cricket::TransportChannel* channel,
- cricket::CandidatePairInterface* candidate_pair) {
- ++num_selected_candidate_pair_changes_;
- }
-
int SendData(cricket::TransportChannel* channel,
const char* data, size_t len) {
rtc::PacketOptions options;
@@ -803,13 +795,6 @@
force_relay_ = relay;
}
- int num_selected_candidate_pair_changes() {
- return num_selected_candidate_pair_changes_;
- }
- void set_num_selected_candidate_pair_changes(int num) {
- num_selected_candidate_pair_changes_ = num;
- }
-
private:
rtc::Thread* main_;
rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
@@ -824,7 +809,6 @@
rtc::SocksProxyServer socks_server2_;
Endpoint ep1_;
Endpoint ep2_;
- int num_selected_candidate_pair_changes_ = 0;
bool clear_remote_candidates_ufrag_pwd_;
bool force_relay_;
};
@@ -1595,9 +1579,8 @@
Test(kLocalUdpToLocalUdp);
}
-// Test that we can quickly switch links if an interface goes down. This also
-// tests that when the link switches, |SignalSelectedCandidatePairChanged| will
-// be fired. The controlled side has two interfaces and one will die.
+// 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) {
AddAddress(0, kPublicAddrs[0]);
// Adding alternate address will make sure |kPublicAddrs| has the higher
@@ -1625,7 +1608,6 @@
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
- set_num_selected_candidate_pair_changes(0);
// 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]);
@@ -1646,15 +1628,12 @@
RemoteCandidate(ep1_ch1())->address().EqualIPs(kAlternateAddrs[1]));
EXPECT_TRUE(
LocalCandidate(ep2_ch1())->address().EqualIPs(kAlternateAddrs[1]));
- // It should have changed twice, one on each side.
- EXPECT_EQ(2, num_selected_candidate_pair_changes());
DestroyChannels();
}
-// Test that we can quickly switch links if an interface goes down. This also
-// tests that when the link switches, |SignalSelectedCandidatePairChanged| will
-// be fired. The controlling side has two interfaces and one will die.
+// 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) {
// Adding alternate address will make sure |kPublicAddrs| has the higher
// priority than others. This is due to FakeNetwork::AddInterface method.
@@ -1680,7 +1659,6 @@
cricket::IceConfig config = CreateIceConfig(1000, false);
ep1_ch1()->SetIceConfig(config);
ep2_ch1()->SetIceConfig(config);
- set_num_selected_candidate_pair_changes(0);
// Blackhole any traffic to or from the public addrs.
LOG(LS_INFO) << "Failing over...";
@@ -1702,7 +1680,6 @@
EXPECT_TRUE(RemoteCandidate(ep1_ch1())->address().EqualIPs(kPublicAddrs[1]));
EXPECT_TRUE(
RemoteCandidate(ep2_ch1())->address().EqualIPs(kAlternateAddrs[0]));
- EXPECT_EQ(2, num_selected_candidate_pair_changes());
DestroyChannels();
}
@@ -1920,6 +1897,8 @@
ch->SetIceRole(cricket::ICEROLE_CONTROLLING);
ch->SetIceCredentials(kIceUfrag[0], kIcePwd[0]);
ch->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
+ ch->SignalSelectedCandidatePairChanged.connect(
+ this, &P2PTransportChannelPingTest::OnSelectedCandidatePairChanged);
ch->SignalReadyToSend.connect(this,
&P2PTransportChannelPingTest::OnReadyToSend);
}
@@ -1971,10 +1950,31 @@
return conn;
}
+ int SendData(cricket::TransportChannel& channel,
+ const char* data,
+ size_t len,
+ int packet_id) {
+ rtc::PacketOptions options;
+ options.packet_id = packet_id;
+ return channel.SendPacket(data, len, options, 0);
+ }
+
+ 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;
+ }
+
void OnReadyToSend(cricket::TransportChannel* channel) {
channel_ready_to_send_ = true;
}
+ cricket::CandidatePairInterface* last_selected_candidate_pair() {
+ return last_selected_candidate_pair_;
+ }
+ int last_sent_packet_id() { return last_sent_packet_id_; }
bool channel_ready_to_send() { return channel_ready_to_send_; }
void reset_channel_ready_to_send() { channel_ready_to_send_ = false; }
@@ -1982,6 +1982,8 @@
rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
rtc::scoped_ptr<rtc::VirtualSocketServer> vss_;
rtc::SocketServerScope ss_scope_;
+ cricket::CandidatePairInterface* last_selected_candidate_pair_ = nullptr;
+ int last_sent_packet_id_ = -1;
bool channel_ready_to_send_ = false;
};
@@ -2172,8 +2174,9 @@
// 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". Plus, the channel will fire SignalReadyToSend if the new
-// best connection is writable.
+// "best connection". Plus, SignalSelectedCandidatePair will be fired if the
+// best connection changes and SignalReadyToSend will be fired if the new best
+// connection is writable.
TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) {
cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
cricket::P2PTransportChannel ch("receiving state change", 1, &pa);
@@ -2185,20 +2188,29 @@
cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn1 != nullptr);
EXPECT_EQ(conn1, ch.best_connection());
- // Channel is not ready because it is not writable.
+ EXPECT_EQ(conn1, last_selected_candidate_pair());
+ EXPECT_EQ(-1, last_sent_packet_id());
+ // Channel is not ready to send because it is not writable.
EXPECT_FALSE(channel_ready_to_send());
+ int last_packet_id = 0;
+ const char* data = "ABCDEFGH";
+ int len = static_cast<int>(strlen(data));
+ SendData(ch, data, len, ++last_packet_id);
// When a higher priority candidate comes in, the new connection is chosen
// as the best connection.
ch.AddRemoteCandidate(CreateHostCandidate("2.2.2.2", 2, 10));
cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
ASSERT_TRUE(conn2 != nullptr);
EXPECT_EQ(conn2, ch.best_connection());
+ EXPECT_EQ(conn2, last_selected_candidate_pair());
+ EXPECT_EQ(last_packet_id, last_sent_packet_id());
EXPECT_FALSE(channel_ready_to_send());
// 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.
+ SendData(ch, data, len, ++last_packet_id);
ch.AddRemoteCandidate(CreateHostCandidate("3.3.3.3", 3, 1));
cricket::Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3);
ASSERT_TRUE(conn3 != nullptr);
@@ -2210,11 +2222,14 @@
conn3->set_nominated(true);
conn3->SignalNominated(conn3);
EXPECT_EQ(conn3, ch.best_connection());
+ EXPECT_EQ(conn3, last_selected_candidate_pair());
+ EXPECT_EQ(last_packet_id, last_sent_packet_id());
EXPECT_TRUE(channel_ready_to_send());
// 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.
+ SendData(ch, data, len, ++last_packet_id);
ch.AddRemoteCandidate(CreateHostCandidate("4.4.4.4", 4, 100));
cricket::Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4);
ASSERT_TRUE(conn4 != nullptr);
@@ -2229,6 +2244,8 @@
// The best connection switches after conn4 becomes writable.
conn4->ReceivedPingResponse();
EXPECT_EQ(conn4, ch.best_connection());
+ EXPECT_EQ(conn4, last_selected_candidate_pair());
+ EXPECT_EQ(last_packet_id, last_sent_packet_id());
// SignalReadyToSend is fired again because conn4 is writable.
EXPECT_TRUE(channel_ready_to_send());
}
diff --git a/webrtc/p2p/base/transportchannel.h b/webrtc/p2p/base/transportchannel.h
index 0dba02f..d21d607 100644
--- a/webrtc/p2p/base/transportchannel.h
+++ b/webrtc/p2p/base/transportchannel.h
@@ -155,8 +155,9 @@
// Signalled when the current selected candidate pair has changed.
// The first parameter is the transport channel that signals the event.
- // The second parameter is the new selected candidate pair.
- sigslot::signal2<TransportChannel*, CandidatePairInterface*>
+ // The second parameter is the new selected candidate pair. The third
+ // parameter is the last packet id sent on the previous candidate pair.
+ sigslot::signal3<TransportChannel*, CandidatePairInterface*, int>
SignalSelectedCandidatePairChanged;
// Invoked when the channel is being destroyed.
diff --git a/webrtc/p2p/quic/quictransportchannel.cc b/webrtc/p2p/quic/quictransportchannel.cc
index 7e68612..cc0576d 100644
--- a/webrtc/p2p/quic/quictransportchannel.cc
+++ b/webrtc/p2p/quic/quictransportchannel.cc
@@ -391,9 +391,11 @@
void QuicTransportChannel::OnSelectedCandidatePairChanged(
TransportChannel* channel,
- CandidatePairInterface* selected_candidate_pair) {
+ CandidatePairInterface* selected_candidate_pair,
+ int last_sent_packet_id) {
ASSERT(channel == channel_);
- SignalSelectedCandidatePairChanged(this, selected_candidate_pair);
+ SignalSelectedCandidatePairChanged(this, selected_candidate_pair,
+ last_sent_packet_id);
}
void QuicTransportChannel::OnConnectionRemoved(TransportChannelImpl* channel) {
diff --git a/webrtc/p2p/quic/quictransportchannel.h b/webrtc/p2p/quic/quictransportchannel.h
index a3c0106..ab02c77 100644
--- a/webrtc/p2p/quic/quictransportchannel.h
+++ b/webrtc/p2p/quic/quictransportchannel.h
@@ -232,7 +232,8 @@
void OnRouteChange(TransportChannel* channel, const Candidate& candidate);
void OnSelectedCandidatePairChanged(
TransportChannel* channel,
- CandidatePairInterface* selected_candidate_pair);
+ CandidatePairInterface* selected_candidate_pair,
+ int last_sent_packet_id);
void OnConnectionRemoved(TransportChannelImpl* channel);
// Callbacks for |quic_|.
diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc
index 25ba2c0..b76d7bd 100644
--- a/webrtc/pc/channel.cc
+++ b/webrtc/pc/channel.cc
@@ -509,13 +509,15 @@
void BaseChannel::OnSelectedCandidatePairChanged(
TransportChannel* channel,
- CandidatePairInterface* selected_candidate_pair) {
+ CandidatePairInterface* selected_candidate_pair,
+ int last_sent_packet_id) {
ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_);
NetworkRoute network_route;
if (selected_candidate_pair) {
network_route =
NetworkRoute(selected_candidate_pair->local_candidate().network_id(),
- selected_candidate_pair->remote_candidate().network_id());
+ selected_candidate_pair->remote_candidate().network_id(),
+ last_sent_packet_id);
}
media_channel()->OnNetworkRouteChanged(channel->transport_name(),
network_route);
diff --git a/webrtc/pc/channel.h b/webrtc/pc/channel.h
index e22196c..e05b6e5 100644
--- a/webrtc/pc/channel.h
+++ b/webrtc/pc/channel.h
@@ -209,7 +209,8 @@
void OnSelectedCandidatePairChanged(
TransportChannel* channel,
- CandidatePairInterface* selected_candidate_pair);
+ CandidatePairInterface* selected_candidate_pair,
+ int last_sent_packet_id);
bool PacketIsRtcp(const TransportChannel* channel, const char* data,
size_t len);
diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc
index 9d20def..a423842 100644
--- a/webrtc/pc/channel_unittest.cc
+++ b/webrtc/pc/channel_unittest.cc
@@ -970,7 +970,7 @@
media_channel1_->set_num_network_route_changes(0);
// The transport channel becomes disconnected.
transport_channel1->SignalSelectedCandidatePairChanged(transport_channel1,
- nullptr);
+ nullptr, -1);
EXPECT_EQ(1, media_channel1_->num_network_route_changes());
EXPECT_FALSE(media_channel1->last_network_route().connected);
@@ -980,14 +980,18 @@
rtc::SocketAddress remote_address("192.168.1.2", 2000 /* port number */);
uint16_t local_net_id = 1;
uint16_t remote_net_id = 2;
+ int last_packet_id = 100;
rtc::scoped_ptr<cricket::CandidatePairInterface> candidate_pair(
transport_controller1_.CreateFakeCandidatePair(
local_address, local_net_id, remote_address, remote_net_id));
transport_channel1->SignalSelectedCandidatePairChanged(
- transport_channel1, candidate_pair.get());
+ transport_channel1, candidate_pair.get(), last_packet_id);
EXPECT_EQ(1, media_channel1_->num_network_route_changes());
- cricket::NetworkRoute expected_network_route(local_net_id, remote_net_id);
+ cricket::NetworkRoute expected_network_route(local_net_id, remote_net_id,
+ last_packet_id);
EXPECT_EQ(expected_network_route, media_channel1->last_network_route());
+ EXPECT_EQ(last_packet_id,
+ media_channel1->last_network_route().last_sent_packet_id);
}
// Test setting up a call.
