From 049fbb1883344dd32081bf6ae02012aef927eaea Mon Sep 17 00:00:00 2001 From: Honghai Zhang Date: Mon, 7 Mar 2016 11:13:07 -0800 Subject: [PATCH] Renaming variables in p2ptransportchannel to be consistent. Also change the type of "time interval" to int from uint32. Fixed a few TODO therein. I think we should have the following convention: 1. All time delay/intervals should have type int although the time instant should have time uint32_t. 2. "interval" is preferred to "delay" if the delay will be repeated (like rescheduling). BUG= R=pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1762863002 . Cr-Commit-Position: refs/heads/master@{#11888} --- webrtc/api/webrtcsession.cc | 2 +- webrtc/p2p/base/faketransportcontroller.h | 2 +- webrtc/p2p/base/p2ptransportchannel.cc | 78 ++++++++++--------- webrtc/p2p/base/p2ptransportchannel.h | 16 ++-- .../p2p/base/p2ptransportchannel_unittest.cc | 24 +++--- webrtc/p2p/base/transport.h | 17 ++-- .../p2p/base/transportcontroller_unittest.cc | 4 +- 7 files changed, 73 insertions(+), 70 deletions(-) diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index 57f80c7d13..8cbcb977e2 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -1123,7 +1123,7 @@ bool WebRtcSession::SetIceTransports( cricket::IceConfig WebRtcSession::ParseIceConfig( const PeerConnectionInterface::RTCConfiguration& config) const { cricket::IceConfig ice_config; - ice_config.receiving_timeout_ms = config.ice_connection_receiving_timeout; + ice_config.receiving_timeout = config.ice_connection_receiving_timeout; ice_config.prioritize_most_likely_candidate_pairs = config.prioritize_most_likely_ice_candidate_pairs; ice_config.backup_connection_ping_interval = diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index 63e8fd3124..c5e2afea54 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -180,7 +180,7 @@ class FakeTransportChannel : public TransportChannelImpl, void SetReceiving(bool receiving) { set_receiving(receiving); } void SetIceConfig(const IceConfig& config) override { - receiving_timeout_ = config.receiving_timeout_ms; + receiving_timeout_ = config.receiving_timeout; gather_continually_ = config.gather_continually; } diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index d6f3cbc3d2..b21d584bf8 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -208,21 +208,20 @@ namespace cricket { // we don't want to degrade the quality on a modem. These numbers should work // well on a 28.8K modem, which is the slowest connection on which the voice // quality is reasonable at all. -static const uint32_t PING_PACKET_SIZE = 60 * 8; -// TODO(honghaiz): Change the word DELAY to INTERVAL whenever appropriate. -// STRONG_PING_DELAY (480ms) is applied when the best connection is both +static const int PING_PACKET_SIZE = 60 * 8; +// STRONG_PING_INTERVAL (480ms) is applied when the best connection is both // writable and receiving. -static const uint32_t STRONG_PING_DELAY = 1000 * PING_PACKET_SIZE / 1000; -// WEAK_PING_DELAY (48ms) is applied when the best connection is either not +static const int STRONG_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 1000; +// WEAK_PING_INTERVAL (48ms) is applied when the best connection is either not // writable or not receiving. -const uint32_t WEAK_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000; +const int WEAK_PING_INTERVAL = 1000 * PING_PACKET_SIZE / 10000; // If the current best connection is both writable and receiving, then we will // also try hard to make sure it is pinged at this rate (a little less than -// 2 * STRONG_PING_DELAY). -static const uint32_t MAX_CURRENT_STRONG_DELAY = 900; +// 2 * STRONG_PING_INTERVAL). +static const int MAX_CURRENT_STRONG_INTERVAL = 900; // ms -static const int MIN_CHECK_RECEIVING_DELAY = 50; // ms +static const int MIN_CHECK_RECEIVING_INTERVAL = 50; // ms P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, int component, @@ -245,17 +244,17 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, ice_role_(ICEROLE_UNKNOWN), tiebreaker_(0), gathering_state_(kIceGatheringNew), - check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5), - config_(MIN_CHECK_RECEIVING_DELAY * 50 /* receiving_timeout */, + check_receiving_interval_(MIN_CHECK_RECEIVING_INTERVAL * 5), + config_(MIN_CHECK_RECEIVING_INTERVAL * 50 /* receiving_timeout */, 0 /* backup_connection_ping_interval */, false /* gather_continually */, false /* prioritize_most_likely_candidate_pairs */, - MAX_CURRENT_STRONG_DELAY /* most_strong_delay */) { - uint32_t weak_ping_delay = ::strtoul( + MAX_CURRENT_STRONG_INTERVAL /* max_strong_interval */) { + uint32_t weak_ping_interval = ::strtoul( webrtc::field_trial::FindFullName("WebRTC-StunInterPacketDelay").c_str(), nullptr, 10); - if (weak_ping_delay) { - weak_ping_delay_ = weak_ping_delay; + if (weak_ping_interval) { + weak_ping_interval_ = static_cast(weak_ping_interval); } } @@ -291,7 +290,7 @@ void P2PTransportChannel::AddConnection(Connection* connection) { connections_.push_back(connection); unpinged_connections_.insert(connection); connection->set_remote_ice_mode(remote_ice_mode_); - connection->set_receiving_timeout(config_.receiving_timeout_ms); + connection->set_receiving_timeout(config_.receiving_timeout); connection->SignalReadPacket.connect( this, &P2PTransportChannel::OnReadPacket); connection->SignalReadyToSend.connect( @@ -413,17 +412,17 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { << config_.backup_connection_ping_interval << " milliseconds."; } - if (config.receiving_timeout_ms >= 0 && - config_.receiving_timeout_ms != config.receiving_timeout_ms) { - config_.receiving_timeout_ms = config.receiving_timeout_ms; - check_receiving_delay_ = - std::max(MIN_CHECK_RECEIVING_DELAY, config_.receiving_timeout_ms / 10); + if (config.receiving_timeout >= 0 && + config_.receiving_timeout != config.receiving_timeout) { + config_.receiving_timeout = config.receiving_timeout; + check_receiving_interval_ = + std::max(MIN_CHECK_RECEIVING_INTERVAL, config_.receiving_timeout / 10); for (Connection* connection : connections_) { - connection->set_receiving_timeout(config_.receiving_timeout_ms); + connection->set_receiving_timeout(config_.receiving_timeout); } - LOG(LS_INFO) << "Set ICE receiving timeout to " - << config_.receiving_timeout_ms << " milliseconds"; + LOG(LS_INFO) << "Set ICE receiving timeout to " << config_.receiving_timeout + << " milliseconds"; } config_.prioritize_most_likely_candidate_pairs = @@ -431,10 +430,11 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { LOG(LS_INFO) << "Set ping most likely connection to " << config_.prioritize_most_likely_candidate_pairs; - if (config.max_strong_delay >= 0 && - config_.max_strong_delay != config.max_strong_delay) { - config_.max_strong_delay = config.max_strong_delay; - LOG(LS_INFO) << "Set max strong delay to " << config_.max_strong_delay; + if (config.max_strong_interval >= 0 && + config_.max_strong_interval != config.max_strong_interval) { + config_.max_strong_interval = config.max_strong_interval; + LOG(LS_INFO) << "Set max strong interval to " + << config_.max_strong_interval; } } @@ -1245,17 +1245,17 @@ void P2PTransportChannel::OnCheckAndPing() { // which ones are pingable). UpdateConnectionStates(); // When the best connection is either not receiving or not writable, - // switch to weak ping delay. - int ping_delay = weak() ? weak_ping_delay_ : STRONG_PING_DELAY; - if (rtc::Time() >= last_ping_sent_ms_ + ping_delay) { + // switch to weak ping interval. + int ping_interval = weak() ? weak_ping_interval_ : STRONG_PING_INTERVAL; + if (rtc::Time() >= last_ping_sent_ms_ + ping_interval) { Connection* conn = FindNextPingableConnection(); if (conn) { PingConnection(conn); MarkConnectionPinged(conn); } } - int check_delay = std::min(ping_delay, check_receiving_delay_); - thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING); + int delay = std::min(ping_interval, check_receiving_interval_); + thread()->PostDelayed(delay, this, MSG_CHECK_AND_PING); } // A connection is considered a backup connection if the channel state @@ -1300,16 +1300,18 @@ bool P2PTransportChannel::IsPingable(Connection* conn, uint32_t now) { // Returns the next pingable connection to ping. This will be the oldest // pingable connection unless we have a connected, writable connection that is -// past the maximum acceptable ping delay. When reconnecting a TCP connection, -// the best connection is disconnected, although still WRITABLE while -// reconnecting. The newly created connection should be selected as the ping -// target to become writable instead. See the big comment in CompareConnections. +// past the maximum acceptable ping interval. When reconnecting a TCP +// connection, the best connection is disconnected, although still WRITABLE +// while reconnecting. The newly created connection should be selected as the +// ping target to become writable instead. See the big comment in +// CompareConnections. Connection* P2PTransportChannel::FindNextPingableConnection() { uint32_t now = rtc::Time(); Connection* conn_to_ping = nullptr; if (best_connection_ && best_connection_->connected() && best_connection_->writable() && - (best_connection_->last_ping_sent() + config_.max_strong_delay <= now)) { + (best_connection_->last_ping_sent() + config_.max_strong_interval <= + now)) { conn_to_ping = best_connection_; } else { conn_to_ping = FindConnectionToPing(now); diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index eac405d916..3ca25d4cba 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -34,7 +34,7 @@ namespace cricket { -extern const uint32_t WEAK_PING_DELAY; +extern const int WEAK_PING_INTERVAL; struct IceParameters { std::string ufrag; @@ -92,8 +92,10 @@ class P2PTransportChannel : public TransportChannelImpl, return gathering_state_; } void AddRemoteCandidate(const Candidate& candidate) override; - // Sets the receiving timeout and gather_continually. - // This also sets the check_receiving_delay proportionally. + // Sets the parameters in IceConfig. We do not set them blindly. Instead, we + // only update the parameter if it is considered set in |config|. For example, + // a negative value of receiving_timeout will be considered "not set" and we + // will not use it to update the respective parameter in |config_|. void SetIceConfig(const IceConfig& config) override; const IceConfig& config() const; @@ -166,8 +168,8 @@ class P2PTransportChannel : public TransportChannelImpl, return false; } - int receiving_timeout() const { return config_.receiving_timeout_ms; } - int check_receiving_delay() const { return check_receiving_delay_; } + int receiving_timeout() const { return config_.receiving_timeout; } + int check_receiving_interval() const { return check_receiving_interval_; } // Helper method used only in unittest. rtc::DiffServCodePoint DefaultDscpValue() const; @@ -317,9 +319,9 @@ class P2PTransportChannel : public TransportChannelImpl, uint64_t tiebreaker_; IceGatheringState gathering_state_; - int check_receiving_delay_; + int check_receiving_interval_; uint32_t last_ping_sent_ms_ = 0; - int weak_ping_delay_ = WEAK_PING_DELAY; + int weak_ping_interval_ = WEAK_PING_INTERVAL; TransportChannelState state_ = TransportChannelState::STATE_INIT; IceConfig config_; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 49446ddad7..c54f054fd8 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -102,11 +102,11 @@ enum { MSG_CANDIDATE }; -static cricket::IceConfig CreateIceConfig(int receiving_timeout_ms, +static cricket::IceConfig CreateIceConfig(int receiving_timeout, bool gather_continually, int backup_ping_interval = -1) { cricket::IceConfig config; - config.receiving_timeout_ms = receiving_timeout_ms; + config.receiving_timeout = receiving_timeout; config.gather_continually = gather_continually; config.backup_connection_ping_interval = backup_ping_interval; return config; @@ -2084,13 +2084,13 @@ TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("receiving state change", 1, &pa); PrepareChannel(&ch); - // Default receiving timeout and checking receiving delay should not be too + // Default receiving timeout and checking receiving interval should not be too // small. EXPECT_LE(1000, ch.receiving_timeout()); - EXPECT_LE(200, ch.check_receiving_delay()); + EXPECT_LE(200, ch.check_receiving_interval()); ch.SetIceConfig(CreateIceConfig(500, false)); EXPECT_EQ(500, ch.receiving_timeout()); - EXPECT_EQ(50, ch.check_receiving_delay()); + EXPECT_EQ(50, ch.check_receiving_interval()); ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1)); @@ -2492,13 +2492,13 @@ class P2PTransportChannelMostLikelyToWorkFirstTest cricket::P2PTransportChannel& StartTransportChannel( bool prioritize_most_likely_to_work, - uint32_t max_strong_delay) { + int max_strong_interval) { channel_.reset( new cricket::P2PTransportChannel("checks", 1, nullptr, allocator())); cricket::IceConfig config = channel_->config(); config.prioritize_most_likely_candidate_pairs = prioritize_most_likely_to_work; - config.max_strong_delay = max_strong_delay; + config.max_strong_interval = max_strong_interval; channel_->SetIceConfig(config); PrepareChannel(channel_.get()); channel_->Connect(); @@ -2546,9 +2546,9 @@ class P2PTransportChannelMostLikelyToWorkFirstTest // we have a best connection. TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, TestRelayRelayFirstWhenNothingPingedYet) { - const uint32_t max_strong_delay = 100; + const int max_strong_interval = 100; cricket::P2PTransportChannel& ch = - StartTransportChannel(true, max_strong_delay); + StartTransportChannel(true, max_strong_interval); EXPECT_TRUE_WAIT(ch.ports().size() == 2, 5000); EXPECT_EQ(ch.ports()[0]->Type(), cricket::LOCAL_PORT_TYPE); EXPECT_EQ(ch.ports()[1]->Type(), cricket::RELAY_PORT_TYPE); @@ -2579,10 +2579,10 @@ TEST_F(P2PTransportChannelMostLikelyToWorkFirstTest, conn3->ReceivedPing(); // Verify that conn3 will be the "best connection" since it is readable and - // writable. After |MAX_CURRENT_STRONG_DELAY|, it should be the next pingable - // connection. + // writable. After |MAX_CURRENT_STRONG_INTERVAL|, it should be the next + // pingable connection. EXPECT_TRUE_WAIT(conn3 == ch.best_connection(), 5000); - WAIT(false, max_strong_delay + 100); + WAIT(false, max_strong_interval + 100); conn3->ReceivedPingResponse(); ASSERT_TRUE(conn3->writable()); EXPECT_EQ(conn3, FindNextPingableConnectionAndPingIt(&ch)); diff --git a/webrtc/p2p/base/transport.h b/webrtc/p2p/base/transport.h index 13c8431a67..84581c8e7b 100644 --- a/webrtc/p2p/base/transport.h +++ b/webrtc/p2p/base/transport.h @@ -139,9 +139,8 @@ struct TransportStats { // Information about ICE configuration. struct IceConfig { - // The ICE connection receiving timeout value. - // TODO(honghaiz): Remove suffix _ms to be consistent. - int receiving_timeout_ms = -1; + // The ICE connection receiving timeout value in milliseconds. + int receiving_timeout = -1; // Time interval in milliseconds to ping a backup connection when the ICE // channel is strongly connected. int backup_connection_ping_interval = -1; @@ -154,21 +153,21 @@ struct IceConfig { // If the current best connection is both writable and receiving, // then we will also try hard to make sure it is pinged at this rate - // (Default value is a little less than 2 * STRONG_PING_DELAY). - int max_strong_delay = -1; + // (Default value is a little less than 2 * STRONG_PING_INTERVAL). + int max_strong_interval = -1; IceConfig() {} - IceConfig(int receiving_timeout, + IceConfig(int receiving_timeout_ms, int backup_connection_ping_interval, bool gather_continually, bool prioritize_most_likely_candidate_pairs, - int max_strong_delay) - : receiving_timeout_ms(receiving_timeout), + int max_strong_interval_ms) + : receiving_timeout(receiving_timeout_ms), backup_connection_ping_interval(backup_connection_ping_interval), gather_continually(gather_continually), prioritize_most_likely_candidate_pairs( prioritize_most_likely_candidate_pairs), - max_strong_delay(max_strong_delay) {} + max_strong_interval(max_strong_interval_ms) {} }; bool BadTransportDescription(const std::string& desc, std::string* err_desc); diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index 96d0e98718..4016273708 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -134,10 +134,10 @@ class TransportControllerTest : public testing::Test, channel2->SetConnectionCount(1); } - cricket::IceConfig CreateIceConfig(int receiving_timeout_ms, + cricket::IceConfig CreateIceConfig(int receiving_timeout, bool gather_continually) { cricket::IceConfig config; - config.receiving_timeout_ms = receiving_timeout_ms; + config.receiving_timeout = receiving_timeout; config.gather_continually = gather_continually; return config; }