From 3d3b531b64fa5bf97a75155a42ac36b7608ec276 Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Thu, 27 Aug 2020 12:43:10 +0200 Subject: [PATCH] Add WebRTC-IceFieldTrial send_ping_on_selected_ice_controlling This patch adds send_ping_on_selected_ice_controlling that is the same like send_ping_on_switch_ice_controlling except that it also sends the ping on initial selection (i.e not a switch(?)). I.e it sends "extra" pings when selecting a connection. This lets the peer know that this candidate pair has been selected, which causes some peers to unblock sending on the candidate pair. TODO deprecate send_ping_on_switch_ice_controlling in favor of this, i.e I should have done it like this on first attempt :( Bug: webrtc:10273 Change-Id: I0e02cd5fd6d677720c1fa863af5d7c2334d02d4f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/182780 Commit-Queue: Jonas Oreland Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#32006} --- p2p/base/p2p_transport_channel.cc | 10 ++++--- .../p2p_transport_channel_ice_field_trials.h | 6 +++++ p2p/base/p2p_transport_channel_unittest.cc | 27 +++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 090d5a4e22..6350df549d 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -696,6 +696,9 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { // Make sure that nomination reaching ICE controlled asap. "send_ping_on_switch_ice_controlling", &field_trials_.send_ping_on_switch_ice_controlling, + // Make sure that nomination reaching ICE controlled asap. + "send_ping_on_selected_ice_controlling", + &field_trials_.send_ping_on_selected_ice_controlling, // Reply to nomination ASAP. "send_ping_on_nomination_ice_controlled", &field_trials_.send_ping_on_nomination_ice_controlled, @@ -1768,9 +1771,10 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn, RTC_LOG(LS_INFO) << ToString() << ": No selected connection"; } - if (field_trials_.send_ping_on_switch_ice_controlling && - ice_role_ == ICEROLE_CONTROLLING && old_selected_connection != nullptr && - conn != nullptr) { + if (conn != nullptr && ice_role_ == ICEROLE_CONTROLLING && + ((field_trials_.send_ping_on_switch_ice_controlling && + old_selected_connection != nullptr) || + field_trials_.send_ping_on_selected_ice_controlling)) { PingConnection(conn); MarkConnectionPinged(conn); } diff --git a/p2p/base/p2p_transport_channel_ice_field_trials.h b/p2p/base/p2p_transport_channel_ice_field_trials.h index f30366fd1f..00e1151ba3 100644 --- a/p2p/base/p2p_transport_channel_ice_field_trials.h +++ b/p2p/base/p2p_transport_channel_ice_field_trials.h @@ -44,8 +44,14 @@ struct IceFieldTrials { int rtt_estimate_halftime_ms = 500; // Sending a PING directly after a switch on ICE_CONTROLLING-side. + // TODO(jonaso) : Deprecate this in favor of + // |send_ping_on_selected_ice_controlling|. bool send_ping_on_switch_ice_controlling = false; + // Sending a PING directly after selecting a connection + // (i.e either a switch or the inital selection). + bool send_ping_on_selected_ice_controlling = false; + // Sending a PING directly after a nomination on ICE_CONTROLLED-side. bool send_ping_on_nomination_ice_controlled = false; diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index ad62920c5a..5acb75b740 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -3826,6 +3826,33 @@ TEST_F(P2PTransportChannelPingTest, TestPingOnSwitch) { EXPECT_EQ(conn2->num_pings_sent(), before + 1); } +// Test the field trial send_ping_on_switch_ice_controlling +// that sends a ping directly when selecteing a new connection +// on the ICE_CONTROLLING-side (i.e also initial selection). +TEST_F(P2PTransportChannelPingTest, TestPingOnSelected) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-IceFieldTrials/send_ping_on_selected_ice_controlling:true/"); + FakePortAllocator pa(rtc::Thread::Current(), nullptr); + P2PTransportChannel ch("receiving state change", 1, &pa); + PrepareChannel(&ch); + ch.SetIceConfig(ch.config()); + ch.SetIceRole(ICEROLE_CONTROLLING); + ch.MaybeStartGathering(); + ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); + Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_TRUE(conn1 != nullptr); + + const int before = conn1->num_pings_sent(); + + // A connection needs to be writable before it is selected for transmission. + conn1->ReceivedPingResponse(LOW_RTT, "id"); + EXPECT_EQ_WAIT(conn1, ch.selected_connection(), kDefaultTimeout); + EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + + // And the additional ping should have been sent directly. + EXPECT_EQ(conn1->num_pings_sent(), before + 1); +} + // The controlled side will select a connection as the "selected connection" // based on requests from an unknown address before the controlling side // nominates a connection, and will nominate a connection from an unknown