From 8b0b21161abdcdc2f2528aadf25f1f8f5c99e8b2 Mon Sep 17 00:00:00 2001 From: "henrike@webrtc.org" Date: Mon, 8 Sep 2014 22:46:28 +0000 Subject: [PATCH] Revert 7093: "Implementing ICE Transports type handling in libjingle transport." TBR=mallinath@webrtc.org BUG=N/A Review URL: https://webrtc-codereview.appspot.com/28419004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7112 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/peerconnection.cc | 2 +- talk/app/webrtc/webrtcsession.cc | 26 +------- talk/app/webrtc/webrtcsession.h | 2 +- talk/app/webrtc/webrtcsession_unittest.cc | 61 ------------------ talk/p2p/base/portallocator.h | 44 ++++--------- talk/p2p/client/basicportallocator.cc | 56 ++++------------- talk/p2p/client/basicportallocator.h | 3 +- talk/p2p/client/portallocator_unittest.cc | 75 +---------------------- 8 files changed, 30 insertions(+), 239 deletions(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 3fba86241..201269ad5 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -672,7 +672,7 @@ bool PeerConnection::UpdateIce(const RTCConfiguration& config) { } } } - return session_->SetIceTransports(config.type); + return session_->UpdateIce(config.type); } bool PeerConnection::AddIceCandidate( diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 13e212822..ae81f2492 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -387,22 +387,6 @@ static void SetOptionFromOptionalConstraint( } } -uint32 ConvertIceTransportTypeToCandidateFilter( - PeerConnectionInterface::IceTransportsType type) { - switch (type) { - case PeerConnectionInterface::kNone: - return cricket::CF_NONE; - case PeerConnectionInterface::kRelay: - return cricket::CF_RELAY; - case PeerConnectionInterface::kNoHost: - return (cricket::CF_ALL & ~cricket::CF_HOST); - case PeerConnectionInterface::kAll: - return cricket::CF_ALL; - default: ASSERT(false); - } - return cricket::CF_NONE; -} - // Help class used to remember if a a remote peer has requested ice restart by // by sending a description with new ice ufrag and password. class IceRestartAnswerLatch { @@ -656,8 +640,7 @@ bool WebRtcSession::Initialize( if (options.disable_encryption) { webrtc_session_desc_factory_->SetSdesPolicy(cricket::SEC_DISABLED); } - port_allocator()->set_candidate_filter( - ConvertIceTransportTypeToCandidateFilter(ice_transport)); + return true; } @@ -763,7 +746,6 @@ bool WebRtcSession::SetLocalDescription(SessionDescriptionInterface* desc, if (!UpdateSessionState(action, cricket::CS_LOCAL, err_desc)) { return false; } - // Kick starting the ice candidates allocation. StartCandidatesAllocation(); @@ -925,10 +907,8 @@ bool WebRtcSession::ProcessIceMessage(const IceCandidateInterface* candidate) { return UseCandidate(candidate); } -bool WebRtcSession::SetIceTransports( - PeerConnectionInterface::IceTransportsType type) { - return port_allocator()->set_candidate_filter( - ConvertIceTransportTypeToCandidateFilter(type)); +bool WebRtcSession::UpdateIce(PeerConnectionInterface::IceTransportsType type) { + return false; } bool WebRtcSession::GetLocalTrackIdBySsrc(uint32 ssrc, std::string* track_id) { diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h index 86ae43599..d1e5645b3 100644 --- a/talk/app/webrtc/webrtcsession.h +++ b/talk/app/webrtc/webrtcsession.h @@ -160,7 +160,7 @@ class WebRtcSession : public cricket::BaseSession, std::string* err_desc); bool ProcessIceMessage(const IceCandidateInterface* ice_candidate); - bool SetIceTransports(PeerConnectionInterface::IceTransportsType type); + bool UpdateIce(PeerConnectionInterface::IceTransportsType type); const SessionDescriptionInterface* local_description() const { return local_desc_.get(); diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index 2cf422b88..2f731a966 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -108,8 +108,6 @@ static const char kClientAddrHost2[] = "22.22.22.22"; static const char kStunAddrHost[] = "99.99.99.1"; static const SocketAddress kTurnUdpIntAddr("99.99.99.4", 3478); static const SocketAddress kTurnUdpExtAddr("99.99.99.6", 0); -static const char kTurnUsername[] = "test"; -static const char kTurnPassword[] = "test"; static const char kSessionVersion[] = "1"; @@ -1085,18 +1083,6 @@ class WebRtcSessionTest : public testing::Test { } } - void ConfigureAllocatorWithTurn() { - cricket::RelayServerConfig relay_server(cricket::RELAY_TURN); - cricket::RelayCredentials credentials(kTurnUsername, kTurnPassword); - relay_server.credentials = credentials; - relay_server.ports.push_back(cricket::ProtocolAddress( - kTurnUdpIntAddr, cricket::PROTO_UDP, false)); - allocator_->AddRelay(relay_server); - allocator_->set_step_delay(cricket::kMinimumStepDelay); - allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | - cricket::PORTALLOCATOR_ENABLE_BUNDLE); - } - cricket::FakeMediaEngine* media_engine_; cricket::FakeDataEngine* data_engine_; cricket::FakeDeviceManager* device_manager_; @@ -1176,53 +1162,6 @@ TEST_F(WebRtcSessionTest, TestStunError) { EXPECT_EQ(6u, observer_.mline_1_candidates_.size()); } -// Test session delivers no candidates gathered when constraint set to "none". -TEST_F(WebRtcSessionTest, TestIceTransportsNone) { - AddInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort)); - SetIceTransportType(PeerConnectionInterface::kNone); - Init(NULL); - mediastream_signaling_.SendAudioVideoStream1(); - InitiateCall(); - EXPECT_TRUE_WAIT(observer_.oncandidatesready_, kIceCandidatesTimeout); - EXPECT_EQ(0u, observer_.mline_0_candidates_.size()); - EXPECT_EQ(0u, observer_.mline_1_candidates_.size()); -} - -// Test session delivers only relay candidates gathered when constaint set to -// "relay". -TEST_F(WebRtcSessionTest, TestIceTransportsRelay) { - AddInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort)); - ConfigureAllocatorWithTurn(); - SetIceTransportType(PeerConnectionInterface::kRelay); - Init(NULL); - mediastream_signaling_.SendAudioVideoStream1(); - InitiateCall(); - EXPECT_TRUE_WAIT(observer_.oncandidatesready_, kIceCandidatesTimeout); - EXPECT_EQ(2u, observer_.mline_0_candidates_.size()); - EXPECT_EQ(2u, observer_.mline_1_candidates_.size()); - for (size_t i = 0; i < observer_.mline_0_candidates_.size(); ++i) { - EXPECT_EQ(cricket::RELAY_PORT_TYPE, - observer_.mline_0_candidates_[i].type()); - } - for (size_t i = 0; i < observer_.mline_1_candidates_.size(); ++i) { - EXPECT_EQ(cricket::RELAY_PORT_TYPE, - observer_.mline_1_candidates_[i].type()); - } -} - -// Test session delivers all candidates gathered when constaint set to "all". -TEST_F(WebRtcSessionTest, TestIceTransportsAll) { - AddInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort)); - SetIceTransportType(PeerConnectionInterface::kAll); - Init(NULL); - mediastream_signaling_.SendAudioVideoStream1(); - InitiateCall(); - EXPECT_TRUE_WAIT(observer_.oncandidatesready_, kIceCandidatesTimeout); - // Host + STUN. By default allocator is disabled to gather relay candidates. - EXPECT_EQ(4u, observer_.mline_0_candidates_.size()); - EXPECT_EQ(4u, observer_.mline_1_candidates_.size()); -} - TEST_F(WebRtcSessionTest, SetSdpFailedOnInvalidSdp) { Init(NULL); SessionDescriptionInterface* offer = NULL; diff --git a/talk/p2p/base/portallocator.h b/talk/p2p/base/portallocator.h index a3ff031b5..84e5feac9 100644 --- a/talk/p2p/base/portallocator.h +++ b/talk/p2p/base/portallocator.h @@ -44,19 +44,17 @@ namespace cricket { // Clients can override this class to control port allocation, including // what kinds of ports are allocated. -enum { - PORTALLOCATOR_DISABLE_UDP = 0x01, - PORTALLOCATOR_DISABLE_STUN = 0x02, - PORTALLOCATOR_DISABLE_RELAY = 0x04, - PORTALLOCATOR_DISABLE_TCP = 0x08, - PORTALLOCATOR_ENABLE_SHAKER = 0x10, - PORTALLOCATOR_ENABLE_BUNDLE = 0x20, - PORTALLOCATOR_ENABLE_IPV6 = 0x40, - PORTALLOCATOR_ENABLE_SHARED_UFRAG = 0x80, - PORTALLOCATOR_ENABLE_SHARED_SOCKET = 0x100, - PORTALLOCATOR_ENABLE_STUN_RETRANSMIT_ATTRIBUTE = 0x200, - PORTALLOCATOR_ENABLE_TURN_SHARED_SOCKET = 0x400, -}; +const uint32 PORTALLOCATOR_DISABLE_UDP = 0x01; +const uint32 PORTALLOCATOR_DISABLE_STUN = 0x02; +const uint32 PORTALLOCATOR_DISABLE_RELAY = 0x04; +const uint32 PORTALLOCATOR_DISABLE_TCP = 0x08; +const uint32 PORTALLOCATOR_ENABLE_SHAKER = 0x10; +const uint32 PORTALLOCATOR_ENABLE_BUNDLE = 0x20; +const uint32 PORTALLOCATOR_ENABLE_IPV6 = 0x40; +const uint32 PORTALLOCATOR_ENABLE_SHARED_UFRAG = 0x80; +const uint32 PORTALLOCATOR_ENABLE_SHARED_SOCKET = 0x100; +const uint32 PORTALLOCATOR_ENABLE_STUN_RETRANSMIT_ATTRIBUTE = 0x200; +const uint32 PORTALLOCATOR_ENABLE_TURN_SHARED_SOCKET = 0x400; const uint32 kDefaultPortAllocatorFlags = 0; @@ -65,15 +63,6 @@ const uint32 kDefaultStepDelay = 1000; // 1 sec step delay. // internal. Less than 20ms is not acceptable. We choose 50ms as our default. const uint32 kMinimumStepDelay = 50; -// CF = CANDIDATE FILTER -enum { - CF_NONE = 0x0, - CF_HOST = 0x1, - CF_REFLEXIVE = 0x2, - CF_RELAY = 0x4, - CF_ALL = 0x7, -}; - class PortAllocatorSessionMuxer; class PortAllocatorSession : public sigslot::has_slots<> { @@ -129,8 +118,7 @@ class PortAllocator : public sigslot::has_slots<> { min_port_(0), max_port_(0), step_delay_(kDefaultStepDelay), - allow_tcp_listen_(true), - candidate_filter_(CF_ALL) { + allow_tcp_listen_(true) { // This will allow us to have old behavior on non webrtc clients. } virtual ~PortAllocator(); @@ -179,13 +167,6 @@ class PortAllocator : public sigslot::has_slots<> { allow_tcp_listen_ = allow_tcp_listen; } - uint32 candidate_filter() { return candidate_filter_; } - bool set_candidate_filter(uint32 filter) { - // TODO(mallinath) - Do transition check? - candidate_filter_ = filter; - return true; - } - protected: virtual PortAllocatorSession* CreateSessionInternal( const std::string& content_name, @@ -203,7 +184,6 @@ class PortAllocator : public sigslot::has_slots<> { uint32 step_delay_; SessionMuxerMap muxers_; bool allow_tcp_listen_; - uint32 candidate_filter_; }; } // namespace cricket diff --git a/talk/p2p/client/basicportallocator.cc b/talk/p2p/client/basicportallocator.cc index 6c184b469..d39db8f15 100644 --- a/talk/p2p/client/basicportallocator.cc +++ b/talk/p2p/client/basicportallocator.cc @@ -47,15 +47,13 @@ using rtc::CreateRandomString; namespace { -enum { - MSG_CONFIG_START, - MSG_CONFIG_READY, - MSG_ALLOCATE, - MSG_ALLOCATION_PHASE, - MSG_SHAKE, - MSG_SEQUENCEOBJECTS_CREATED, - MSG_CONFIG_STOP, -}; +const uint32 MSG_CONFIG_START = 1; +const uint32 MSG_CONFIG_READY = 2; +const uint32 MSG_ALLOCATE = 3; +const uint32 MSG_ALLOCATION_PHASE = 4; +const uint32 MSG_SHAKE = 5; +const uint32 MSG_SEQUENCEOBJECTS_CREATED = 6; +const uint32 MSG_CONFIG_STOP = 7; const int PHASE_UDP = 0; const int PHASE_RELAY = 1; @@ -230,11 +228,10 @@ BasicPortAllocator::~BasicPortAllocator() { PortAllocatorSession *BasicPortAllocator::CreateSessionInternal( const std::string& content_name, int component, const std::string& ice_ufrag, const std::string& ice_pwd) { - return new BasicPortAllocatorSession( - this, content_name, component, ice_ufrag, ice_pwd); + return new BasicPortAllocatorSession(this, content_name, component, + ice_ufrag, ice_pwd); } - // BasicPortAllocatorSession BasicPortAllocatorSession::BasicPortAllocatorSession( BasicPortAllocator *allocator, @@ -533,10 +530,8 @@ void BasicPortAllocatorSession::OnCandidateReady( // Send candidates whose protocol is enabled. std::vector candidates; ProtocolType pvalue; - bool candidate_allowed_to_send = CheckCandidateFilter(c); if (StringToProto(c.protocol().c_str(), &pvalue) && - data->sequence()->ProtocolEnabled(pvalue) && - candidate_allowed_to_send) { + data->sequence()->ProtocolEnabled(pvalue)) { candidates.push_back(c); } @@ -547,9 +542,7 @@ void BasicPortAllocatorSession::OnCandidateReady( // Moving to READY state as we have atleast one candidate from the port. // Since this port has atleast one candidate we should forward this port // to listners, to allow connections from this port. - // Also we should make sure that candidate gathered from this port is allowed - // to send outside. - if (!data->ready() && candidate_allowed_to_send) { + if (!data->ready()) { data->set_ready(); SignalPortReady(this, port); } @@ -595,8 +588,6 @@ void BasicPortAllocatorSession::OnProtocolEnabled(AllocationSequence* seq, const std::vector& potentials = it->port()->Candidates(); for (size_t i = 0; i < potentials.size(); ++i) { - if (!CheckCandidateFilter(potentials[i])) - continue; ProtocolType pvalue; if (!StringToProto(potentials[i].protocol().c_str(), &pvalue)) continue; @@ -611,31 +602,6 @@ void BasicPortAllocatorSession::OnProtocolEnabled(AllocationSequence* seq, } } -bool BasicPortAllocatorSession::CheckCandidateFilter(const Candidate& c) { - uint32 filter = allocator_->candidate_filter(); - bool allowed = false; - if (filter & CF_RELAY) { - allowed |= (c.type() == RELAY_PORT_TYPE); - } - - if (filter & CF_REFLEXIVE) { - // We allow host candidates if the filter allows server-reflexive candidates - // and the candidate is a public IP. Because we don't generate - // server-reflexive candidates if they have the same IP as the host - // candidate (i.e. when the host candidate is a public IP), filtering to - // only server-reflexive candidates won't work right when the host - // candidates have public IPs. - allowed |= (c.type() == STUN_PORT_TYPE) || - (c.type() == LOCAL_PORT_TYPE && !c.address().IsPrivateIP()); - } - - if (filter & CF_HOST) { - allowed |= (c.type() == LOCAL_PORT_TYPE); - } - - return allowed; -} - void BasicPortAllocatorSession::OnPortAllocationComplete( AllocationSequence* seq) { // Send candidate allocation complete signal if all ports are done. diff --git a/talk/p2p/client/basicportallocator.h b/talk/p2p/client/basicportallocator.h index d4247722d..5f43880dc 100644 --- a/talk/p2p/client/basicportallocator.h +++ b/talk/p2p/client/basicportallocator.h @@ -160,6 +160,7 @@ class BasicPortAllocatorSession : public PortAllocatorSession, void set_ready() { ASSERT(state_ == STATE_INIT); state_ = STATE_READY; } void set_complete() { + ASSERT(state_ == STATE_READY); state_ = STATE_COMPLETE; } void set_error() { @@ -200,8 +201,6 @@ class BasicPortAllocatorSession : public PortAllocatorSession, void OnPortAllocationComplete(AllocationSequence* seq); PortData* FindPort(Port* port); - bool CheckCandidateFilter(const Candidate& c); - BasicPortAllocator* allocator_; rtc::Thread* network_thread_; rtc::scoped_ptr owned_socket_factory_; diff --git a/talk/p2p/client/portallocator_unittest.cc b/talk/p2p/client/portallocator_unittest.cc index 51142d015..57cfbe3e7 100644 --- a/talk/p2p/client/portallocator_unittest.cc +++ b/talk/p2p/client/portallocator_unittest.cc @@ -53,7 +53,6 @@ using rtc::SocketAddress; using rtc::Thread; static const SocketAddress kClientAddr("11.11.11.11", 0); -static const SocketAddress kPrivateAddr("192.168.1.11", 0); static const SocketAddress kClientIPv6Addr( "2401:fa00:4:1000:be30:5bff:fee5:c3", 0); static const SocketAddress kClientAddr2("22.22.22.22", 0); @@ -539,7 +538,7 @@ TEST_F(PortAllocatorTest, TestCandidatePriorityOfMultipleInterfaces) { // Test to verify ICE restart process. TEST_F(PortAllocatorTest, TestGetAllPortsRestarts) { AddInterface(kClientAddr); - EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); + EXPECT_TRUE(CreateSession(1)); session_->StartGettingPorts(); EXPECT_EQ_WAIT(7U, candidates_.size(), kDefaultAllocationTimeout); EXPECT_EQ(4U, ports_.size()); @@ -547,78 +546,6 @@ TEST_F(PortAllocatorTest, TestGetAllPortsRestarts) { // TODO - Extend this to verify ICE restart. } -// Test ICE candidate filter mechanism with options Relay/Host/Reflexive. -TEST_F(PortAllocatorTest, TestCandidateFilterWithRelayOnly) { - AddInterface(kClientAddr); - allocator().set_candidate_filter(cricket::CF_RELAY); - EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - // Using GTURN, we will have 4 candidates. - EXPECT_EQ(4U, candidates_.size()); - EXPECT_EQ(1U, ports_.size()); // Only Relay port will be in ready state. - for (size_t i = 0; i < candidates_.size(); ++i) { - EXPECT_EQ(cricket::RELAY_PORT_TYPE, candidates_[i].type()); - } -} - -TEST_F(PortAllocatorTest, TestCandidateFilterWithHostOnly) { - AddInterface(kClientAddr); - allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | - cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET); - allocator().set_candidate_filter(cricket::CF_HOST); - EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - EXPECT_EQ(2U, candidates_.size()); // Host UDP/TCP candidates only. - EXPECT_EQ(2U, ports_.size()); // UDP/TCP ports only. - for (size_t i = 0; i < candidates_.size(); ++i) { - EXPECT_EQ(cricket::LOCAL_PORT_TYPE, candidates_[i].type()); - } -} - -// Host is behind the NAT. -TEST_F(PortAllocatorTest, TestCandidateFilterWithReflexiveOnly) { - AddInterface(kPrivateAddr); - rtc::scoped_ptr nat_server( - CreateNatServer(kNatAddr, rtc::NAT_OPEN_CONE)); - ServerAddresses stun_servers; - stun_servers.insert(kStunAddr); - allocator_.reset(new cricket::BasicPortAllocator( - &network_manager_, &nat_socket_factory_, stun_servers)); - allocator().set_step_delay(cricket::kMinimumStepDelay); - allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | - cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET); - allocator().set_candidate_filter(cricket::CF_REFLEXIVE); - EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - // Host is behind NAT, no private address will be exposed. Hence only UDP - // port with STUN candidate will be sent outside. - EXPECT_EQ(1U, candidates_.size()); // Only STUN candidate. - EXPECT_EQ(1U, ports_.size()); // Only UDP port will be in ready state. - for (size_t i = 0; i < candidates_.size(); ++i) { - EXPECT_EQ(cricket::STUN_PORT_TYPE, candidates_[i].type()); - } -} - -// Host is not behind the NAT. -TEST_F(PortAllocatorTest, TestCandidateFilterWithReflexiveOnlyAndNoNAT) { - AddInterface(kClientAddr); - allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | - cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET); - allocator().set_candidate_filter(cricket::CF_REFLEXIVE); - EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); - session_->StartGettingPorts(); - EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - // Host has a public address, both UDP and TCP candidates will be exposed. - EXPECT_EQ(2U, candidates_.size()); // Local UDP + TCP candidate. - EXPECT_EQ(2U, ports_.size()); // UDP and TCP ports will be in ready state. - for (size_t i = 0; i < candidates_.size(); ++i) { - EXPECT_EQ(cricket::LOCAL_PORT_TYPE, candidates_[i].type()); - } -} - TEST_F(PortAllocatorTest, TestBasicMuxFeatures) { AddInterface(kClientAddr); allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_BUNDLE);