From 81125f0abacb5b1c2a2c4606caf8682309d6e58f Mon Sep 17 00:00:00 2001 From: Jonas Olsson Date: Tue, 9 Oct 2018 10:52:04 +0200 Subject: [PATCH] Implement (mostly) standards-compliant RTCIceTransportState. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to correctly implement RTCPeerConnectionState and RTCIceConnectionState the ice transports need to support RTCIceTransportState. This CL adds an implementation parallel to the current non-standard IceTransportState. It's not currently used anywhere. The old implementation will remain in place until we're ready to switch RTCIceConnectionState over. Bug: webrtc:9308 Change-Id: I30e2bbb5b4fafa410261bcd9d5e3b76c03435feb Reviewed-on: https://webrtc-review.googlesource.com/c/103220 Reviewed-by: Qingsi Wang Reviewed-by: Steve Anton Reviewed-by: Karl Wiberg Reviewed-by: Henrik Boström Commit-Queue: Jonas Olsson Cr-Commit-Position: refs/heads/master@{#25078} --- api/transport/BUILD.gn | 7 ++++ api/transport/enums.h | 32 +++++++++++++++++ p2p/BUILD.gn | 1 + p2p/base/fakeicetransport.h | 4 +++ p2p/base/icetransportinternal.h | 9 +++++ p2p/base/mockicetransport.h | 4 +++ p2p/base/p2ptransportchannel.cc | 46 ++++++++++++++++++++++++ p2p/base/p2ptransportchannel.h | 12 +++++++ p2p/base/p2ptransportchannel_unittest.cc | 4 +++ 9 files changed, 119 insertions(+) create mode 100644 api/transport/enums.h diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index 5466061123..9341da972a 100644 --- a/api/transport/BUILD.gn +++ b/api/transport/BUILD.gn @@ -19,6 +19,13 @@ rtc_source_set("bitrate_settings") { ] } +rtc_source_set("enums") { + visibility = [ "*" ] + sources = [ + "enums.h", + ] +} + rtc_static_library("network_control") { sources = [ "network_control.h", diff --git a/api/transport/enums.h b/api/transport/enums.h new file mode 100644 index 0000000000..b1d5770cb9 --- /dev/null +++ b/api/transport/enums.h @@ -0,0 +1,32 @@ +/* + * Copyright 2018 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef API_TRANSPORT_ENUMS_H_ +#define API_TRANSPORT_ENUMS_H_ + +namespace webrtc { + +// See https://w3c.github.io/webrtc-pc/#rtcicetransportstate +// Note that kFailed is currently not a terminal state, and a transport might +// incorrectly be marked as failed while gathering candidates, see +// bugs.webrtc.org/8833 +enum class IceTransportState { + kNew, + kChecking, + kConnected, + kCompleted, + kFailed, + kDisconnected, + kClosed, +}; + +} // namespace webrtc + +#endif // API_TRANSPORT_ENUMS_H_ diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 2d351bb0fa..5d4e706b9b 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -86,6 +86,7 @@ rtc_static_library("rtc_p2p") { deps = [ "../api:libjingle_peerconnection_api", "../api:ortc_api", + "../api/transport:enums", "../logging:ice_log", "../rtc_base:checks", "../rtc_base:rtc_base", diff --git a/p2p/base/fakeicetransport.h b/p2p/base/fakeicetransport.h index 48157451ea..4ba0a2d2d2 100644 --- a/p2p/base/fakeicetransport.h +++ b/p2p/base/fakeicetransport.h @@ -114,6 +114,10 @@ class FakeIceTransport : public IceTransportInternal { return IceTransportState::STATE_CONNECTING; } + webrtc::IceTransportState GetIceTransportState() const override { + return webrtc::IceTransportState::kConnected; + } + void SetIceRole(IceRole role) override { role_ = role; } IceRole GetIceRole() const override { return role_; } void SetIceTiebreaker(uint64_t tiebreaker) override { diff --git a/p2p/base/icetransportinternal.h b/p2p/base/icetransportinternal.h index 7483c3babc..31e7078c94 100644 --- a/p2p/base/icetransportinternal.h +++ b/p2p/base/icetransportinternal.h @@ -15,6 +15,7 @@ #include #include "api/candidate.h" +#include "api/transport/enums.h" #include "p2p/base/candidatepairinterface.h" #include "p2p/base/packettransportinternal.h" #include "p2p/base/port.h" @@ -188,7 +189,10 @@ class IceTransportInternal : public rtc::PacketTransportInternal { IceTransportInternal(); ~IceTransportInternal() override; + // TODO(bugs.webrtc.org/9308): Remove GetState once all uses have been + // migrated to GetIceTransportState. virtual IceTransportState GetState() const = 0; + virtual webrtc::IceTransportState GetIceTransportState() const = 0; virtual int component() const = 0; @@ -258,8 +262,13 @@ class IceTransportInternal : public rtc::PacketTransportInternal { sigslot::signal1 SignalRoleConflict; // Emitted whenever the transport state changed. + // TODO(bugs.webrtc.org/9308): Remove once all uses have migrated to the new + // IceTransportState. sigslot::signal1 SignalStateChanged; + // Emitted whenever the new standards-compliant transport state changed. + sigslot::signal1 SignalIceTransportStateChanged; + // Invoked when the transport is being destroyed. sigslot::signal1 SignalDestroyed; }; diff --git a/p2p/base/mockicetransport.h b/p2p/base/mockicetransport.h index d1863c5293..b18ce3d5ad 100644 --- a/p2p/base/mockicetransport.h +++ b/p2p/base/mockicetransport.h @@ -47,6 +47,10 @@ class MockIceTransport : public IceTransportInternal { IceTransportState GetState() const override { return IceTransportState::STATE_INIT; } + webrtc::IceTransportState GetIceTransportState() const override { + return webrtc::IceTransportState::kNew; + } + const std::string& transport_name() const override { return transport_name_; } int component() const override { return 0; } void SetIceRole(IceRole role) override {} diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index b0abe24a05..b5de0e6b5b 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -323,6 +323,10 @@ IceTransportState P2PTransportChannel::GetState() const { return state_; } +webrtc::IceTransportState P2PTransportChannel::GetIceTransportState() const { + return standardized_state_; +} + const std::string& P2PTransportChannel::transport_name() const { return transport_name_; } @@ -387,6 +391,41 @@ IceTransportState P2PTransportChannel::ComputeState() const { return IceTransportState::STATE_COMPLETED; } +// Compute the current RTCIceTransportState as described in +// https://www.w3.org/TR/webrtc/#dom-rtcicetransportstate +// TODO(bugs.webrtc.org/9308): Return IceTransportState::kDisconnected when it +// makes sense. +// TODO(bugs.webrtc.org/9218): Avoid prematurely signalling kFailed once we have +// implemented end-of-candidates signalling. +webrtc::IceTransportState P2PTransportChannel::ComputeIceTransportState() + const { + bool has_connection = false; + for (Connection* connection : connections_) { + if (connection->active()) { + has_connection = true; + break; + } + } + + switch (gathering_state_) { + case kIceGatheringComplete: + if (has_connection) + return webrtc::IceTransportState::kCompleted; + else + return webrtc::IceTransportState::kFailed; + case kIceGatheringNew: + return webrtc::IceTransportState::kNew; + case kIceGatheringGathering: + if (has_connection) + return webrtc::IceTransportState::kConnected; + else + return webrtc::IceTransportState::kChecking; + default: + RTC_NOTREACHED(); + return webrtc::IceTransportState::kFailed; + } +} + void P2PTransportChannel::SetIceParameters(const IceParameters& ice_params) { RTC_DCHECK(network_thread_ == rtc::Thread::Current()); RTC_LOG(LS_INFO) << "Set ICE ufrag: " << ice_params.ufrag @@ -1827,6 +1866,9 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { // example, we call this at the end of SortConnectionsAndUpdateState. void P2PTransportChannel::UpdateState() { IceTransportState state = ComputeState(); + webrtc::IceTransportState current_standardized_state = + ComputeIceTransportState(); + if (state_ != state) { RTC_LOG(LS_INFO) << ToString() << ": Transport channel state changed from " @@ -1868,6 +1910,10 @@ void P2PTransportChannel::UpdateState() { SignalStateChanged(this); } + if (standardized_state_ != current_standardized_state) { + standardized_state_ = current_standardized_state; + SignalIceTransportStateChanged(this); + } // If our selected connection is "presumed writable" (TURN-TURN with no // CreatePermission required), act like we're already writable to the upper // layers, so they can start media quicker. diff --git a/p2p/base/p2ptransportchannel.h b/p2p/base/p2ptransportchannel.h index fbed344cc3..84d28795dd 100644 --- a/p2p/base/p2ptransportchannel.h +++ b/p2p/base/p2ptransportchannel.h @@ -91,6 +91,8 @@ class P2PTransportChannel : public IceTransportInternal { // From TransportChannelImpl: IceTransportState GetState() const override; + webrtc::IceTransportState GetIceTransportState() const override; + const std::string& transport_name() const override; int component() const override; bool writable() const override; @@ -243,7 +245,13 @@ class P2PTransportChannel : public IceTransportInternal { void UpdateState(); void HandleAllTimedOut(); void MaybeStopPortAllocatorSessions(); + + // ComputeIceTransportState computes the RTCIceTransportState as described in + // https://w3c.github.io/webrtc-pc/#dom-rtcicetransportstate. ComputeState + // computes the value we currently export as RTCIceTransportState. + // TODO(bugs.webrtc.org/9308): Remove ComputeState once it's no longer used. IceTransportState ComputeState() const; + webrtc::IceTransportState ComputeIceTransportState() const; Connection* GetBestConnectionOnNetwork(rtc::Network* network) const; bool CreateConnections(const Candidate& remote_candidate, @@ -407,7 +415,11 @@ class P2PTransportChannel : public IceTransportInternal { std::unique_ptr regathering_controller_; int64_t last_ping_sent_ms_ = 0; int weak_ping_interval_ = WEAK_PING_INTERVAL; + // TODO(jonasolsson): Remove state_ and rename standardized_state_ once state_ + // is no longer used to compute the ICE connection state. IceTransportState state_ = IceTransportState::STATE_INIT; + webrtc::IceTransportState standardized_state_ = + webrtc::IceTransportState::kNew; IceConfig config_; int last_sent_packet_id_ = -1; // -1 indicates no packet was sent before. bool started_pinging_ = false; diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc index 76b918ca1e..9b0bcb2453 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -4137,6 +4137,7 @@ TEST_F(P2PTransportChannelPingTest, TestGetState) { clock.AdvanceTime(webrtc::TimeDelta::seconds(1)); FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("test channel", 1, &pa); + EXPECT_EQ(webrtc::IceTransportState::kNew, ch.GetIceTransportState()); PrepareChannel(&ch); ch.MaybeStartGathering(); EXPECT_EQ(IceTransportState::STATE_INIT, ch.GetState()); @@ -4144,6 +4145,8 @@ TEST_F(P2PTransportChannelPingTest, TestGetState) { ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 1)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1, &clock); Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2, &clock); + // Gathering complete with candidates. + EXPECT_EQ(webrtc::IceTransportState::kCompleted, ch.GetIceTransportState()); ASSERT_TRUE(conn1 != nullptr); ASSERT_TRUE(conn2 != nullptr); // Now there are two connections, so the transport channel is connecting. @@ -4156,6 +4159,7 @@ TEST_F(P2PTransportChannelPingTest, TestGetState) { // Need to wait until the channel state is updated. EXPECT_EQ_SIMULATED_WAIT(IceTransportState::STATE_FAILED, ch.GetState(), kShortTimeout, clock); + EXPECT_EQ(webrtc::IceTransportState::kFailed, ch.GetIceTransportState()); } // Test that when a low-priority connection is pruned, it is not deleted