From 8c316c1a89b8ba3c31996d7dc7943fdd68e29c20 Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Mon, 13 Nov 2017 21:13:45 +0000 Subject: [PATCH] Revert "Replaced the SignalSelectedCandidatePairChanged with a new signal." This reverts commit 71677452f9cf210aa98162c6f4bd8d339e625337. Reason for revert: Broke Chromium. Original change's description: > Replaced the SignalSelectedCandidatePairChanged with a new signal. > > |transport overhead| field is added to rtc::NetworkRoute structure. > > In PackTransportInternal: > 1. network_route() is added which returns the current network route. > 2. debug_name() is removed. > 3. transport_name() is moved from DtlsTransportInternal and > IceTransportInternal to PacketTransportInternal. > > When the selected candidate pair is changed, the P2PTransportChannel > will fire the SignalNetworkRouteChanged instead of > SignalSelectedCandidatePairChanged to upper layers. > > The Rtp/SrtpTransport takes the responsibility of calculating the > transport overhead from the BaseChannel so that the BaseChannel > doesn't need to depend on P2P layer transports. > > Bug: webrtc:7013 > Change-Id: I60d30d785666a50a95052d00bf08f829d8f57e9c > Reviewed-on: https://webrtc-review.googlesource.com/13520 > Commit-Queue: Zhi Huang > Reviewed-by: Peter Thatcher > Cr-Commit-Position: refs/heads/master@{#20661} TBR=steveanton@webrtc.org,zhihuang@webrtc.org,pthatcher@webrtc.org Change-Id: Ie0c76786855b65bb8caba7065593c961e4bf9de7 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:7013 Reviewed-on: https://webrtc-review.googlesource.com/22764 Reviewed-by: Zhi Huang Commit-Queue: Zhi Huang Cr-Commit-Position: refs/heads/master@{#20662} --- p2p/base/dtlstransport.cc | 12 --- p2p/base/dtlstransport.h | 3 - p2p/base/dtlstransport_unittest.cc | 31 ++++---- p2p/base/dtlstransportinternal.cc | 4 + p2p/base/dtlstransportinternal.h | 5 ++ p2p/base/fakedtlstransport.h | 13 ---- p2p/base/fakeicetransport.h | 10 --- p2p/base/fakepackettransport.h | 17 +--- p2p/base/icetransportinternal.cc | 4 + p2p/base/icetransportinternal.h | 18 ++++- p2p/base/jseptransport_unittest.cc | 1 + p2p/base/p2ptransportchannel.cc | 23 +----- p2p/base/p2ptransportchannel.h | 4 - p2p/base/p2ptransportchannel_unittest.cc | 99 +++++++++++------------- p2p/base/packettransportinternal.h | 16 ++-- p2p/base/port.cc | 5 ++ p2p/base/port.h | 6 +- p2p/base/udptransport.cc | 10 +-- p2p/base/udptransport.h | 5 +- pc/channel.cc | 98 ++++++++++++++++------- pc/channel.h | 8 +- pc/channel_unittest.cc | 31 +++----- pc/rtptransport.cc | 22 +----- pc/rtptransport.h | 3 - pc/rtptransport_unittest.cc | 68 ---------------- pc/rtptransportinternal.h | 8 -- pc/srtptransport.cc | 16 ---- pc/srtptransport.h | 3 +- pc/transportcontroller.cc | 2 +- rtc_base/BUILD.gn | 2 - rtc_base/nethelper.cc | 42 ---------- rtc_base/nethelper.h | 33 -------- rtc_base/networkroute.h | 11 +-- 33 files changed, 211 insertions(+), 422 deletions(-) delete mode 100644 rtc_base/nethelper.cc delete mode 100644 rtc_base/nethelper.h diff --git a/p2p/base/dtlstransport.cc b/p2p/base/dtlstransport.cc index 0520a8517b..c1b65eec73 100644 --- a/p2p/base/dtlstransport.cc +++ b/p2p/base/dtlstransport.cc @@ -124,7 +124,6 @@ DtlsTransport::DtlsTransport(IceTransportInternal* ice_transport, ssl_role_(rtc::SSL_CLIENT), ssl_max_version_(rtc::SSL_PROTOCOL_DTLS_12), crypto_options_(crypto_options) { - RTC_DCHECK(ice_transport_); ice_transport_->SignalWritableState.connect(this, &DtlsTransport::OnWritableState); ice_transport_->SignalReadPacket.connect(this, &DtlsTransport::OnReadPacket); @@ -133,8 +132,6 @@ DtlsTransport::DtlsTransport(IceTransportInternal* ice_transport, &DtlsTransport::OnReadyToSend); ice_transport_->SignalReceivingState.connect( this, &DtlsTransport::OnReceivingState); - ice_transport_->SignalNetworkRouteChanged.connect( - this, &DtlsTransport::OnNetworkRouteChanged); } DtlsTransport::~DtlsTransport() = default; @@ -431,10 +428,6 @@ int DtlsTransport::GetError() { return ice_transport_->GetError(); } -rtc::Optional DtlsTransport::network_route() const { - return ice_transport_->network_route(); -} - bool DtlsTransport::GetOption(rtc::Socket::Option opt, int* value) { return ice_transport_->GetOption(opt, value); } @@ -638,11 +631,6 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { } } -void DtlsTransport::OnNetworkRouteChanged( - rtc::Optional network_route) { - SignalNetworkRouteChanged(network_route); -} - void DtlsTransport::MaybeStartDtls() { if (dtls_ && ice_transport_->writable()) { ConfigureHandshakeTimeout(); diff --git a/p2p/base/dtlstransport.h b/p2p/base/dtlstransport.h index 4dd9bc257d..eb86deb337 100644 --- a/p2p/base/dtlstransport.h +++ b/p2p/base/dtlstransport.h @@ -156,8 +156,6 @@ class DtlsTransport : public DtlsTransportInternal { int GetError() override; - rtc::Optional network_route() const override; - int SetOption(rtc::Socket::Option opt, int value) override; std::string ToString() const { @@ -181,7 +179,6 @@ class DtlsTransport : public DtlsTransportInternal { void OnReadyToSend(rtc::PacketTransportInternal* transport); void OnReceivingState(rtc::PacketTransportInternal* transport); void OnDtlsEvent(rtc::StreamInterface* stream_, int sig, int err); - void OnNetworkRouteChanged(rtc::Optional network_route); bool SetupDtls(); void MaybeStartDtls(); bool HandleDtlsPacket(const char* data, size_t size); diff --git a/p2p/base/dtlstransport_unittest.cc b/p2p/base/dtlstransport_unittest.cc index ed010e02c4..eaa0cecd3a 100644 --- a/p2p/base/dtlstransport_unittest.cc +++ b/p2p/base/dtlstransport_unittest.cc @@ -101,11 +101,11 @@ class DtlsTestClient : public sigslot::has_slots<> { (role == cricket::ICEROLE_CONTROLLING) ? 1 : 2); dtls->SetSslMaxProtocolVersion(ssl_max_version_); dtls->SignalWritableState.connect( - this, &DtlsTestClient::OnTransportWritableState); - dtls->SignalReadPacket.connect(this, - &DtlsTestClient::OnTransportReadPacket); - dtls->SignalSentPacket.connect(this, - &DtlsTestClient::OnTransportSentPacket); + this, &DtlsTestClient::OnTransportChannelWritableState); + dtls->SignalReadPacket.connect( + this, &DtlsTestClient::OnTransportChannelReadPacket); + dtls->SignalSentPacket.connect( + this, &DtlsTestClient::OnTransportChannelSentPacket); dtls_transports_.push_back(std::unique_ptr(dtls)); fake_ice_transports_.push_back( std::unique_ptr(fake_ice_channel)); @@ -356,17 +356,18 @@ class DtlsTestClient : public sigslot::has_slots<> { return (num_matches < ((static_cast(size) - 5) / 10)); } - // Transport callbacks - void OnTransportWritableState(rtc::PacketTransportInternal* transport) { - RTC_LOG(LS_INFO) << name_ << ": Transport '" << transport->transport_name() + // Transport channel callbacks + void OnTransportChannelWritableState( + rtc::PacketTransportInternal* transport) { + RTC_LOG(LS_INFO) << name_ << ": Channel '" << transport->debug_name() << "' is writable"; } - void OnTransportReadPacket(rtc::PacketTransportInternal* transport, - const char* data, - size_t size, - const rtc::PacketTime& packet_time, - int flags) { + void OnTransportChannelReadPacket(rtc::PacketTransportInternal* transport, + const char* data, + size_t size, + const rtc::PacketTime& packet_time, + int flags) { uint32_t packet_num = 0; ASSERT_TRUE(VerifyPacket(data, size, &packet_num)); received_.insert(packet_num); @@ -376,8 +377,8 @@ class DtlsTestClient : public sigslot::has_slots<> { ASSERT_EQ(expected_flags, flags); } - void OnTransportSentPacket(rtc::PacketTransportInternal* transport, - const rtc::SentPacket& sent_packet) { + void OnTransportChannelSentPacket(rtc::PacketTransportInternal* transport, + const rtc::SentPacket& sent_packet) { sent_packet_ = sent_packet; } diff --git a/p2p/base/dtlstransportinternal.cc b/p2p/base/dtlstransportinternal.cc index a294ccbc4e..5e28cb0022 100644 --- a/p2p/base/dtlstransportinternal.cc +++ b/p2p/base/dtlstransportinternal.cc @@ -16,4 +16,8 @@ DtlsTransportInternal::DtlsTransportInternal() = default; DtlsTransportInternal::~DtlsTransportInternal() = default; +std::string DtlsTransportInternal::debug_name() const { + return transport_name() + " " + rtc::ToString(component()); +} + } // namespace cricket diff --git a/p2p/base/dtlstransportinternal.h b/p2p/base/dtlstransportinternal.h index b08868fde5..be3db6b663 100644 --- a/p2p/base/dtlstransportinternal.h +++ b/p2p/base/dtlstransportinternal.h @@ -43,6 +43,8 @@ class DtlsTransportInternal : public rtc::PacketTransportInternal { virtual DtlsTransportState dtls_state() const = 0; + virtual const std::string& transport_name() const = 0; + virtual int component() const = 0; virtual bool IsDtlsActive() const = 0; @@ -91,6 +93,9 @@ class DtlsTransportInternal : public rtc::PacketTransportInternal { // Emitted whenever the Dtls handshake failed on some transport channel. sigslot::signal1 SignalDtlsHandshakeError; + // Debugging description of this transport. + std::string debug_name() const override; + protected: DtlsTransportInternal(); diff --git a/p2p/base/fakedtlstransport.h b/p2p/base/fakedtlstransport.h index 508158f678..01a4ed8dfb 100644 --- a/p2p/base/fakedtlstransport.h +++ b/p2p/base/fakedtlstransport.h @@ -31,11 +31,8 @@ class FakeDtlsTransport : public DtlsTransportInternal { transport_name_(ice_transport->transport_name()), component_(ice_transport->component()), dtls_fingerprint_("", nullptr, 0) { - RTC_DCHECK(ice_transport_); ice_transport_->SignalReadPacket.connect( this, &FakeDtlsTransport::OnIceTransportReadPacket); - ice_transport_->SignalNetworkRouteChanged.connect( - this, &FakeDtlsTransport::OnNetworkRouteChanged); } // If this constructor is called, a new fake ICE transport will be created, @@ -48,8 +45,6 @@ class FakeDtlsTransport : public DtlsTransportInternal { ice_transport_ = owned_ice_transport_.get(); ice_transport_->SignalReadPacket.connect( this, &FakeDtlsTransport::OnIceTransportReadPacket); - ice_transport_->SignalNetworkRouteChanged.connect( - this, &FakeDtlsTransport::OnNetworkRouteChanged); } ~FakeDtlsTransport() override { @@ -206,10 +201,6 @@ class FakeDtlsTransport : public DtlsTransportInternal { } int GetError() override { return ice_transport_->GetError(); } - rtc::Optional network_route() const override { - return ice_transport_->network_route(); - } - private: void OnIceTransportReadPacket(PacketTransportInternal* ice_, const char* data, @@ -238,10 +229,6 @@ class FakeDtlsTransport : public DtlsTransportInternal { SignalWritableState(this); } - void OnNetworkRouteChanged(rtc::Optional network_route) { - SignalNetworkRouteChanged(network_route); - } - FakeIceTransport* ice_transport_; std::unique_ptr owned_ice_transport_; std::string transport_name_; diff --git a/p2p/base/fakeicetransport.h b/p2p/base/fakeicetransport.h index cc6cec99d8..7b6f78a33e 100644 --- a/p2p/base/fakeicetransport.h +++ b/p2p/base/fakeicetransport.h @@ -190,7 +190,6 @@ class FakeIceTransport : public IceTransportInternal { SignalSentPacket(this, sent_packet); return static_cast(len); } - int SetOption(rtc::Socket::Option opt, int value) override { socket_options_[opt] = value; return true; @@ -204,16 +203,8 @@ class FakeIceTransport : public IceTransportInternal { return false; } } - int GetError() override { return 0; } - rtc::Optional network_route() const override { - return network_route_; - } - void SetNetworkRoute(rtc::Optional network_route) { - network_route_ = network_route; - } - private: void set_writable(bool writable) { if (writable_ == writable) { @@ -264,7 +255,6 @@ class FakeIceTransport : public IceTransportInternal { bool receiving_ = false; bool combine_outgoing_packets_ = false; rtc::CopyOnWriteBuffer send_packet_; - rtc::Optional network_route_; std::map socket_options_; }; diff --git a/p2p/base/fakepackettransport.h b/p2p/base/fakepackettransport.h index cbff5110ed..2af14c1bda 100644 --- a/p2p/base/fakepackettransport.h +++ b/p2p/base/fakepackettransport.h @@ -23,8 +23,8 @@ namespace rtc { // Used to simulate a packet-based transport. class FakePacketTransport : public PacketTransportInternal { public: - explicit FakePacketTransport(const std::string& transport_name) - : transport_name_(transport_name) {} + explicit FakePacketTransport(const std::string& debug_name) + : debug_name_(debug_name) {} ~FakePacketTransport() override { if (dest_ && dest_->dest_ == this) { dest_->dest_ = nullptr; @@ -59,7 +59,7 @@ class FakePacketTransport : public PacketTransportInternal { } // Fake PacketTransportInternal implementation. - const std::string& transport_name() const override { return transport_name_; } + std::string debug_name() const override { return debug_name_; } bool writable() const override { return writable_; } bool receiving() const override { return receiving_; } int SendPacket(const char* data, @@ -88,13 +88,6 @@ class FakePacketTransport : public PacketTransportInternal { const CopyOnWriteBuffer* last_sent_packet() { return &last_sent_packet_; } - Optional network_route() const override { - return network_route_; - } - void SetNetworkRoute(Optional network_route) { - network_route_ = network_route; - } - private: void set_writable(bool writable) { if (writable_ == writable) { @@ -125,14 +118,12 @@ class FakePacketTransport : public PacketTransportInternal { CopyOnWriteBuffer last_sent_packet_; AsyncInvoker invoker_; - std::string transport_name_; + std::string debug_name_; FakePacketTransport* dest_ = nullptr; bool async_ = false; int async_delay_ms_ = 0; bool writable_ = false; bool receiving_ = false; - - Optional network_route_; }; } // namespace rtc diff --git a/p2p/base/icetransportinternal.cc b/p2p/base/icetransportinternal.cc index d5fa989613..a741205c21 100644 --- a/p2p/base/icetransportinternal.cc +++ b/p2p/base/icetransportinternal.cc @@ -26,4 +26,8 @@ void IceTransportInternal::SetRemoteIceCredentials(const std::string& ice_ufrag, SetRemoteIceParameters(IceParameters(ice_ufrag, ice_pwd, false)); } +std::string IceTransportInternal::debug_name() const { + return transport_name() + " " + rtc::ToString(component()); +} + } // namespace cricket diff --git a/p2p/base/icetransportinternal.h b/p2p/base/icetransportinternal.h index 0a71162eea..b7dfc42fed 100644 --- a/p2p/base/icetransportinternal.h +++ b/p2p/base/icetransportinternal.h @@ -52,6 +52,8 @@ class IceTransportInternal : public rtc::PacketTransportInternal { virtual IceTransportState GetState() const = 0; + virtual const std::string& transport_name() const = 0; + virtual int component() const = 0; virtual IceRole GetIceRole() const = 0; @@ -109,14 +111,21 @@ class IceTransportInternal : public rtc::PacketTransportInternal { sigslot::signal2 SignalCandidatesRemoved; - // Deprecated by PacketTransportInternal::SignalNetworkRouteChanged. + // Deprecated by SignalSelectedCandidatePairChanged // This signal occurs when there is a change in the way that packets are // being routed, i.e. to a different remote location. The candidate // indicates where and how we are currently sending media. - // TODO(zhihuang): Update the Chrome remoting to use the new - // SignalNetworkRouteChanged. sigslot::signal2 SignalRouteChange; + // Signalled when the current selected candidate pair has changed. + // The first parameter is the transport that signals the event. + // The second parameter is the new selected candidate pair. The third + // parameter is the last packet id sent on the previous candidate pair. + // The fourth parameter is a boolean which is true if the Transport + // is ready to send with this candidate pair. + sigslot::signal4 + SignalSelectedCandidatePairChanged; + // Invoked when there is conflict in the ICE role between local and remote // agents. sigslot::signal1 SignalRoleConflict; @@ -126,6 +135,9 @@ class IceTransportInternal : public rtc::PacketTransportInternal { // Invoked when the transport is being destroyed. sigslot::signal1 SignalDestroyed; + + // Debugging description of this transport. + std::string debug_name() const override; }; } // namespace cricket diff --git a/p2p/base/jseptransport_unittest.cc b/p2p/base/jseptransport_unittest.cc index 8d09e0f9bd..644bcd90b0 100644 --- a/p2p/base/jseptransport_unittest.cc +++ b/p2p/base/jseptransport_unittest.cc @@ -17,6 +17,7 @@ #include "rtc_base/network.h" using cricket::JsepTransport; +using cricket::TransportChannel; using cricket::FakeDtlsTransport; using cricket::FakeIceTransport; using cricket::IceRole; diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index 19abf38dcb..71af75c7d2 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -23,7 +23,6 @@ #include "rtc_base/checks.h" #include "rtc_base/crc32.h" #include "rtc_base/logging.h" -#include "rtc_base/nethelper.h" #include "rtc_base/stringencode.h" #include "rtc_base/timeutils.h" #include "system_wrappers/include/field_trial.h" @@ -1083,10 +1082,6 @@ bool P2PTransportChannel::GetStats(ConnectionInfos *infos) { return true; } -rtc::Optional P2PTransportChannel::network_route() const { - return network_route_; -} - rtc::DiffServCodePoint P2PTransportChannel::DefaultDscpValue() const { OptionMap::const_iterator it = options_.find(rtc::Socket::OPT_DSCP); if (it == options_.end()) { @@ -1472,7 +1467,6 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { // destroyed, so don't use it. Connection* old_selected_connection = selected_connection_; selected_connection_ = conn; - network_route_.reset(); if (selected_connection_) { ++nomination_; if (old_selected_connection) { @@ -1491,23 +1485,12 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { PresumedWritable(selected_connection_)) { SignalReadyToSend(this); } - - network_route_.emplace(rtc::NetworkRoute()); - network_route_->connected = ReadyToSend(selected_connection_); - network_route_->local_network_id = - selected_connection_->local_candidate().network_id(); - network_route_->remote_network_id = - selected_connection_->remote_candidate().network_id(); - network_route_->last_sent_packet_id = last_sent_packet_id_; - network_route_->packet_overhead = - GetIpOverhead( - selected_connection_->local_candidate().address().family()) + - GetProtocolOverhead(selected_connection_->local_candidate().protocol()); } else { LOG_J(LS_INFO, this) << "No selected connection"; } - - SignalNetworkRouteChanged(network_route_); + SignalSelectedCandidatePairChanged(this, selected_connection_, + last_sent_packet_id_, + ReadyToSend(selected_connection_)); } // Warning: UpdateState should eventually be called whenever a connection diff --git a/p2p/base/p2ptransportchannel.h b/p2p/base/p2ptransportchannel.h index 188f60c4f1..f9e9a53761 100644 --- a/p2p/base/p2ptransportchannel.h +++ b/p2p/base/p2ptransportchannel.h @@ -127,8 +127,6 @@ class P2PTransportChannel : public IceTransportInternal, int receiving_timeout() const { return config_.receiving_timeout; } int check_receiving_interval() const { return check_receiving_interval_; } - rtc::Optional network_route() const override; - // Helper method used only in unittest. rtc::DiffServCodePoint DefaultDscpValue() const; @@ -402,8 +400,6 @@ class P2PTransportChannel : public IceTransportInternal, webrtc::MetricsObserverInterface* metrics_observer_ = nullptr; - rtc::Optional network_route_; - RTC_DISALLOW_COPY_AND_ASSIGN(P2PTransportChannel); }; diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc index 37f68073e1..326cafa0bc 100644 --- a/p2p/base/p2ptransportchannel_unittest.cc +++ b/p2p/base/p2ptransportchannel_unittest.cc @@ -379,8 +379,8 @@ class P2PTransportChannelTestBase : public testing::Test, this, &P2PTransportChannelTestBase::OnReadPacket); channel->SignalRoleConflict.connect( this, &P2PTransportChannelTestBase::OnRoleConflict); - channel->SignalNetworkRouteChanged.connect( - this, &P2PTransportChannelTestBase::OnNetworkRouteChanged); + channel->SignalSelectedCandidatePairChanged.connect( + this, &P2PTransportChannelTestBase::OnSelectedCandidatePairChanged); channel->SetIceParameters(local_ice); if (remote_ice_parameter_source_ == FROM_SETICEPARAMETERS) { channel->SetRemoteIceParameters(remote_ice); @@ -697,12 +697,14 @@ class P2PTransportChannelTestBase : public testing::Test, new CandidatesData(ch, c)); } } - - void OnNetworkRouteChanged(rtc::Optional network_route) { - // If the |network_route| is unset, don't count. This is used in the case - // when the network on remote side is down, the signal will be fired with an - // unset network route and it shouldn't trigger a connection switch. - if (network_route) { + void OnSelectedCandidatePairChanged( + IceTransportInternal* transport_channel, + CandidatePairInterface* selected_candidate_pair, + int last_sent_packet_id, + bool ready_to_send) { + // Do not count if it switches to nullptr. This may happen if all + // connections timed out. + if (selected_candidate_pair != nullptr) { ++selected_candidate_pair_switches_; } } @@ -3053,8 +3055,8 @@ class P2PTransportChannelPingTest : public testing::Test, ch->SetIceRole(ICEROLE_CONTROLLING); ch->SetIceParameters(kIceParams[0]); ch->SetRemoteIceParameters(kIceParams[1]); - ch->SignalNetworkRouteChanged.connect( - this, &P2PTransportChannelPingTest::OnNetworkRouteChanged); + ch->SignalSelectedCandidatePairChanged.connect( + this, &P2PTransportChannelPingTest::OnSelectedCandidatePairChanged); ch->SignalReadyToSend.connect(this, &P2PTransportChannelPingTest::OnReadyToSend); ch->SignalStateChanged.connect( @@ -3140,11 +3142,13 @@ class P2PTransportChannelPingTest : public testing::Test, conn->SignalNominated(conn); } - void OnNetworkRouteChanged(rtc::Optional network_route) { - last_network_route_ = network_route; - if (last_network_route_) { - last_sent_packet_id_ = last_network_route_->last_sent_packet_id; - } + void OnSelectedCandidatePairChanged( + IceTransportInternal* transport_channel, + CandidatePairInterface* selected_candidate_pair, + int last_sent_packet_id, + bool ready_to_send) { + last_selected_candidate_pair_ = selected_candidate_pair; + last_sent_packet_id_ = last_sent_packet_id; ++selected_candidate_pair_switches_; } @@ -3178,6 +3182,9 @@ class P2PTransportChannelPingTest : public testing::Test, channel_state_ = channel->GetState(); } + 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; } @@ -3188,26 +3195,14 @@ class P2PTransportChannelPingTest : public testing::Test, return switches; } - // Return true if the |pair| matches the last network route. - bool CandidatePairMatchesNetworkRoute(CandidatePairInterface* pair) { - if (!pair) { - return !last_network_route_.has_value(); - } else { - return pair->local_candidate().network_id() == - last_network_route_->local_network_id && - pair->remote_candidate().network_id() == - last_network_route_->remote_network_id; - } - } - private: std::unique_ptr vss_; rtc::AutoSocketServerThread thread_; + CandidatePairInterface* last_selected_candidate_pair_ = nullptr; int selected_candidate_pair_switches_ = 0; int last_sent_packet_id_ = -1; bool channel_ready_to_send_ = false; IceTransportState channel_state_ = IceTransportState::STATE_INIT; - rtc::Optional last_network_route_; }; TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { @@ -3585,7 +3580,7 @@ TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) { // The controlled side will select a connection as the "selected connection" // based on priority until the controlling side nominates a connection, at which // point the controlled side will select that connection as the -// "selected connection". Plus, SignalNetworkRouteChanged will be fired if the +// "selected connection". Plus, SignalSelectedCandidatePair will be fired if the // selected connection changes and SignalReadyToSend will be fired if the new // selected connection is writable. TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { @@ -3608,7 +3603,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { // 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)); + EXPECT_EQ(conn1, last_selected_candidate_pair()); EXPECT_EQ(len, SendData(ch, data, len, ++last_packet_id)); // When a higher priority candidate comes in, the new connection is chosen @@ -3618,7 +3613,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { ASSERT_TRUE(conn2 != nullptr); conn2->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_WAIT(conn2, ch.selected_connection(), kDefaultTimeout); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn2, last_selected_candidate_pair()); EXPECT_TRUE(channel_ready_to_send()); EXPECT_EQ(last_packet_id, last_sent_packet_id()); @@ -3636,7 +3631,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { // connection. NominateConnection(conn3); EXPECT_EQ(conn3, ch.selected_connection()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn3)); + EXPECT_EQ(conn3, last_selected_candidate_pair()); EXPECT_EQ(last_packet_id, last_sent_packet_id()); EXPECT_TRUE(channel_ready_to_send()); @@ -3657,7 +3652,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { // The selected connection switches after conn4 becomes writable. conn4->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_WAIT(conn4, ch.selected_connection(), kDefaultTimeout); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn4)); + 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()); @@ -3819,7 +3814,7 @@ TEST_F(P2PTransportChannelPingTest, // Initially, connections are selected based on priority. EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 receives data; it becomes selected. // Advance the clock by 1ms so that the last data receiving timestamp of @@ -3827,12 +3822,12 @@ TEST_F(P2PTransportChannelPingTest, SIMULATED_WAIT(false, 1, clock); conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn2, last_selected_candidate_pair()); // conn1 also receives data; it becomes selected due to priority again. conn1->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 received data more recently; it is selected now because it // received data more recently. @@ -3841,7 +3836,7 @@ TEST_F(P2PTransportChannelPingTest, conn2->ReceivedPingResponse(LOW_RTT, "id"); conn2->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn2, last_selected_candidate_pair()); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 10, clock); @@ -3871,17 +3866,17 @@ TEST_F(P2PTransportChannelPingTest, SIMULATED_WAIT(false, 1, clock); conn1->OnReadPacket("XYZ", 3, rtc::CreatePacketTime(0)); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 is nominated; it becomes the selected connection. NominateConnection(conn2); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn2, last_selected_candidate_pair()); // conn1 is selected because it has higher priority and also nominated. NominateConnection(conn1); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn1, last_selected_candidate_pair()); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 10, clock); @@ -3907,28 +3902,24 @@ TEST_F(P2PTransportChannelPingTest, ASSERT_TRUE(conn2 != nullptr); // conn1 is the selected connection because it has a higher priority, - EXPECT_EQ_SIMULATED_WAIT(conn1, ch.selected_connection(), kDefaultTimeout, - clock); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + EXPECT_EQ_SIMULATED_WAIT(conn1, last_selected_candidate_pair(), + kDefaultTimeout, clock); reset_selected_candidate_pair_switches(); // conn2 is nominated; it becomes selected. NominateConnection(conn2); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_EQ(conn2, ch.selected_connection()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn2, last_selected_candidate_pair()); // conn1 is selected because of its priority. NominateConnection(conn1); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_EQ(conn1, ch.selected_connection()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + EXPECT_EQ(conn1, last_selected_candidate_pair()); // conn2 gets higher remote nomination; it is selected again. NominateConnection(conn2, 2U); EXPECT_EQ(1, reset_selected_candidate_pair_switches()); - EXPECT_EQ(conn2, ch.selected_connection()); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ(conn2, last_selected_candidate_pair()); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 100, clock); @@ -3979,17 +3970,15 @@ TEST_F(P2PTransportChannelPingTest, conn2->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_SIMULATED_WAIT(1, reset_selected_candidate_pair_switches(), kDefaultTimeout, clock); - EXPECT_EQ_SIMULATED_WAIT(conn2, ch.selected_connection(), kDefaultTimeout, - clock); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn2)); + EXPECT_EQ_SIMULATED_WAIT(conn2, last_selected_candidate_pair(), + kDefaultTimeout, clock); // If conn1 is also writable, it will become selected. conn1->ReceivedPingResponse(LOW_RTT, "id"); EXPECT_EQ_SIMULATED_WAIT(1, reset_selected_candidate_pair_switches(), kDefaultTimeout, clock); - EXPECT_EQ_SIMULATED_WAIT(conn1, ch.selected_connection(), kDefaultTimeout, - clock); - EXPECT_TRUE(CandidatePairMatchesNetworkRoute(conn1)); + EXPECT_EQ_SIMULATED_WAIT(conn1, last_selected_candidate_pair(), + kDefaultTimeout, clock); // Make sure sorting won't reselect candidate pair. SIMULATED_WAIT(false, 10, clock); diff --git a/p2p/base/packettransportinternal.h b/p2p/base/packettransportinternal.h index fa9a22c2a8..41e9fbcfb9 100644 --- a/p2p/base/packettransportinternal.h +++ b/p2p/base/packettransportinternal.h @@ -14,15 +14,16 @@ #include #include -#include "api/optional.h" // This is included for PacketOptions. #include "api/ortc/packettransportinterface.h" -#include "p2p/base/port.h" #include "rtc_base/asyncpacketsocket.h" -#include "rtc_base/networkroute.h" #include "rtc_base/sigslot.h" #include "rtc_base/socket.h" +namespace cricket { +class TransportChannel; +} + namespace rtc { struct PacketOptions; struct PacketTime; @@ -31,7 +32,8 @@ struct SentPacket; class PacketTransportInternal : public virtual webrtc::PacketTransportInterface, public sigslot::has_slots<> { public: - virtual const std::string& transport_name() const = 0; + // Identify the object for logging and debug purpose. + virtual std::string debug_name() const = 0; // The transport has been established. virtual bool writable() const = 0; @@ -64,9 +66,6 @@ class PacketTransportInternal : public virtual webrtc::PacketTransportInterface, // Returns the most recent error that occurred on this channel. virtual int GetError() = 0; - // Returns the current network route with transport overhead. - virtual rtc::Optional network_route() const = 0; - // Emitted when the writable state, represented by |writable()|, changes. sigslot::signal1 SignalWritableState; @@ -92,9 +91,6 @@ class PacketTransportInternal : public virtual webrtc::PacketTransportInterface, sigslot::signal2 SignalSentPacket; - // Signalled when the current network route has changed. - sigslot::signal1> SignalNetworkRouteChanged; - protected: PacketTransportInternal(); ~PacketTransportInternal() override; diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 1c316d2cb2..8ffc68866b 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -99,6 +99,11 @@ const char STUN_PORT_TYPE[] = "stun"; const char PRFLX_PORT_TYPE[] = "prflx"; const char RELAY_PORT_TYPE[] = "relay"; +const char UDP_PROTOCOL_NAME[] = "udp"; +const char TCP_PROTOCOL_NAME[] = "tcp"; +const char SSLTCP_PROTOCOL_NAME[] = "ssltcp"; +const char TLS_PROTOCOL_NAME[] = "tls"; + static const char* const PROTO_NAMES[] = {UDP_PROTOCOL_NAME, TCP_PROTOCOL_NAME, SSLTCP_PROTOCOL_NAME, TLS_PROTOCOL_NAME}; diff --git a/p2p/base/port.h b/p2p/base/port.h index 2957fd8b74..3bcfae26be 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -28,7 +28,6 @@ #include "p2p/base/stunrequest.h" #include "rtc_base/asyncpacketsocket.h" #include "rtc_base/checks.h" -#include "rtc_base/nethelper.h" #include "rtc_base/network.h" #include "rtc_base/proxyinfo.h" #include "rtc_base/ratetracker.h" @@ -46,6 +45,11 @@ extern const char STUN_PORT_TYPE[]; extern const char PRFLX_PORT_TYPE[]; extern const char RELAY_PORT_TYPE[]; +extern const char UDP_PROTOCOL_NAME[]; +extern const char TCP_PROTOCOL_NAME[]; +extern const char SSLTCP_PROTOCOL_NAME[]; +extern const char TLS_PROTOCOL_NAME[]; + // RFC 6544, TCP candidate encoding rules. extern const int DISCARD_PORT; extern const char TCPTYPE_ACTIVE_STR[]; diff --git a/p2p/base/udptransport.cc b/p2p/base/udptransport.cc index 8b4ce00689..9f3d7d0990 100644 --- a/p2p/base/udptransport.cc +++ b/p2p/base/udptransport.cc @@ -16,7 +16,6 @@ #include "rtc_base/asyncpacketsocket.h" #include "rtc_base/asyncudpsocket.h" #include "rtc_base/logging.h" -#include "rtc_base/nethelper.h" #include "rtc_base/socketaddress.h" #include "rtc_base/thread.h" #include "rtc_base/thread_checker.h" @@ -64,7 +63,7 @@ rtc::SocketAddress UdpTransport::GetRemoteAddress() const { return remote_address_; } -const std::string& UdpTransport::transport_name() const { +std::string UdpTransport::debug_name() const { return transport_name_; } @@ -96,13 +95,6 @@ int UdpTransport::SendPacket(const char* data, return result; } -rtc::Optional UdpTransport::network_route() const { - rtc::NetworkRoute network_route; - network_route.packet_overhead = - /*kUdpOverhead=*/8 + GetIpOverhead(GetLocalAddress().family()); - return rtc::Optional(network_route); -} - int UdpTransport::SetOption(rtc::Socket::Option opt, int value) { return 0; } diff --git a/p2p/base/udptransport.h b/p2p/base/udptransport.h index 118b596e91..6a9d211ab9 100644 --- a/p2p/base/udptransport.h +++ b/p2p/base/udptransport.h @@ -46,7 +46,7 @@ class UdpTransport : public rtc::PacketTransportInternal, rtc::SocketAddress GetRemoteAddress() const override; // Overrides of PacketTransportInternal, used by webrtc internally. - const std::string& transport_name() const override; + std::string debug_name() const override; bool receiving() const override; @@ -61,8 +61,6 @@ class UdpTransport : public rtc::PacketTransportInternal, int GetError() override; - rtc::Optional network_route() const override; - protected: PacketTransportInternal* GetInternal() override; @@ -75,7 +73,6 @@ class UdpTransport : public rtc::PacketTransportInternal, void OnSocketSentPacket(rtc::AsyncPacketSocket* socket, const rtc::SentPacket& packet); bool IsLocalConsistent(); - std::string transport_name_; int send_error_ = 0; std::unique_ptr socket_; diff --git a/pc/channel.cc b/pc/channel.cc index 4318aaec3a..3950c03a10 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -174,8 +174,6 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread, // channel to signal. rtp_transport_->SignalPacketReceived.connect(this, &BaseChannel::OnPacketReceived); - rtp_transport_->SignalNetworkRouteChanged.connect( - this, &BaseChannel::OnNetworkRouteChanged); RTC_LOG(LS_INFO) << "Created channel for " << content_name; } @@ -303,7 +301,7 @@ void BaseChannel::SetTransports_n( transport_name_ = rtp_dtls_transport->transport_name(); debug_name = transport_name_; } else { - debug_name = rtp_packet_transport->transport_name(); + debug_name = rtp_packet_transport->debug_name(); } if (rtp_packet_transport == rtp_transport_->rtp_packet_transport()) { // Nothing to do if transport isn't changing. @@ -346,9 +344,6 @@ void BaseChannel::SetTransport_n( DtlsTransportInternal* new_dtls_transport, rtc::PacketTransportInternal* new_packet_transport) { RTC_DCHECK(network_thread_->IsCurrent()); - if (new_dtls_transport) { - RTC_DCHECK(new_dtls_transport == new_packet_transport); - } DtlsTransportInternal*& old_dtls_transport = rtcp ? rtcp_dtls_transport_ : rtp_dtls_transport_; rtc::PacketTransportInternal* old_packet_transport = @@ -403,14 +398,21 @@ void BaseChannel::ConnectToDtlsTransport(DtlsTransportInternal* transport) { transport->SignalWritableState.connect(this, &BaseChannel::OnWritableState); transport->SignalDtlsState.connect(this, &BaseChannel::OnDtlsState); transport->SignalSentPacket.connect(this, &BaseChannel::SignalSentPacket_n); + transport->ice_transport()->SignalSelectedCandidatePairChanged.connect( + this, &BaseChannel::OnSelectedCandidatePairChanged); } void BaseChannel::DisconnectFromDtlsTransport( DtlsTransportInternal* transport) { RTC_DCHECK(network_thread_->IsCurrent()); + OnSelectedCandidatePairChanged(transport->ice_transport(), nullptr, -1, + false); + transport->SignalWritableState.disconnect(this); transport->SignalDtlsState.disconnect(this); transport->SignalSentPacket.disconnect(this); + transport->ice_transport()->SignalSelectedCandidatePairChanged.disconnect( + this); } void BaseChannel::ConnectToPacketTransport( @@ -590,24 +592,29 @@ void BaseChannel::OnDtlsState(DtlsTransportInternal* transport, } } -void BaseChannel::OnNetworkRouteChanged( - rtc::Optional network_route) { +void BaseChannel::OnSelectedCandidatePairChanged( + IceTransportInternal* ice_transport, + CandidatePairInterface* selected_candidate_pair, + int last_sent_packet_id, + bool ready_to_send) { + RTC_DCHECK((rtp_dtls_transport_ && + ice_transport == rtp_dtls_transport_->ice_transport()) || + (rtcp_dtls_transport_ && + ice_transport == rtcp_dtls_transport_->ice_transport())); RTC_DCHECK(network_thread_->IsCurrent()); - rtc::NetworkRoute new_route; - if (network_route) { - invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [=] { - media_channel_->OnTransportOverheadChanged( - network_route->packet_overhead); - }); - new_route = *(network_route); - } + selected_candidate_pair_ = selected_candidate_pair; + std::string transport_name = ice_transport->transport_name(); + rtc::NetworkRoute network_route; + if (selected_candidate_pair) { + network_route = rtc::NetworkRoute( + ready_to_send, selected_candidate_pair->local_candidate().network_id(), + selected_candidate_pair->remote_candidate().network_id(), + last_sent_packet_id); - // Note: When the RTCP-muxing is not enabled, RTCP transport and RTP transport - // use the same transport name and MediaChannel::OnNetworkRouteChanged cannot - // work correctly. Intentionally leave it broken to simplify the code and - // encourage the users to stop using non-muxing RTCP. + UpdateTransportOverhead(); + } invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [=] { - media_channel_->OnNetworkRouteChanged(transport_name_, new_route); + media_channel_->OnNetworkRouteChanged(transport_name, network_route); }); } @@ -908,8 +915,9 @@ bool BaseChannel::SetupDtlsSrtp_n(bool rtcp) { if (!ret) { RTC_LOG(LS_WARNING) << "DTLS-SRTP key installation failed"; + } else { + UpdateTransportOverhead(); } - return ret; } @@ -1013,7 +1021,6 @@ void BaseChannel::EnableSrtpTransport_n() { if (srtp_transport_ == nullptr) { rtp_transport_->SignalReadyToSend.disconnect(this); rtp_transport_->SignalPacketReceived.disconnect(this); - rtp_transport_->SignalNetworkRouteChanged.disconnect(this); auto transport = rtc::MakeUnique( std::move(rtp_transport_), content_name_); @@ -1024,8 +1031,6 @@ void BaseChannel::EnableSrtpTransport_n() { this, &BaseChannel::OnTransportReadyToSend); rtp_transport_->SignalPacketReceived.connect( this, &BaseChannel::OnPacketReceived); - rtp_transport_->SignalNetworkRouteChanged.connect( - this, &BaseChannel::OnNetworkRouteChanged); RTC_LOG(LS_INFO) << "Wrapping RtpTransport in SrtpTransport."; } } @@ -1155,7 +1160,7 @@ bool BaseChannel::SetRtcpMux_n(bool enable, // the RTCP transport. std::string debug_name = transport_name_.empty() - ? rtp_transport_->rtp_packet_transport()->transport_name() + ? rtp_transport_->rtp_packet_transport()->debug_name() : transport_name_; RTC_LOG(LS_INFO) << "Enabling rtcp-mux for " << content_name() << "; no longer need RTCP transport for " @@ -1674,6 +1679,47 @@ void BaseChannel::UpdateMediaSendRecvState() { Bind(&BaseChannel::UpdateMediaSendRecvState_w, this)); } +int BaseChannel::GetTransportOverheadPerPacket() const { + RTC_DCHECK(network_thread_->IsCurrent()); + + if (!selected_candidate_pair_) + return 0; + + int transport_overhead_per_packet = 0; + + constexpr int kIpv4Overhaed = 20; + constexpr int kIpv6Overhaed = 40; + transport_overhead_per_packet += + selected_candidate_pair_->local_candidate().address().family() == AF_INET + ? kIpv4Overhaed + : kIpv6Overhaed; + + constexpr int kUdpOverhaed = 8; + constexpr int kTcpOverhaed = 20; + transport_overhead_per_packet += + selected_candidate_pair_->local_candidate().protocol() == + TCP_PROTOCOL_NAME + ? kTcpOverhaed + : kUdpOverhaed; + + if (srtp_active()) { + int srtp_overhead = 0; + if (srtp_transport_->GetSrtpOverhead(&srtp_overhead)) + transport_overhead_per_packet += srtp_overhead; + } + + return transport_overhead_per_packet; +} + +void BaseChannel::UpdateTransportOverhead() { + int transport_overhead_per_packet = GetTransportOverheadPerPacket(); + if (transport_overhead_per_packet) + invoker_.AsyncInvoke( + RTC_FROM_HERE, worker_thread_, + Bind(&MediaChannel::OnTransportOverheadChanged, media_channel_.get(), + transport_overhead_per_packet)); +} + void VoiceChannel::UpdateMediaSendRecvState_w() { // Render incoming data if we're the active call, and we have the local // content. We receive data on the default channel and multiplexed streams. diff --git a/pc/channel.h b/pc/channel.h index 352fda4dcd..ec13f07cf3 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -243,7 +243,11 @@ class BaseChannel void OnDtlsState(DtlsTransportInternal* transport, DtlsTransportState state); - void OnNetworkRouteChanged(rtc::Optional network_route); + void OnSelectedCandidatePairChanged( + IceTransportInternal* ice_transport, + CandidatePairInterface* selected_candidate_pair, + int last_sent_packet_id, + bool ready_to_send); bool PacketIsRtcp(const rtc::PacketTransportInternal* transport, const char* data, @@ -359,6 +363,8 @@ class BaseChannel void SignalSentPacket_w(const rtc::SentPacket& sent_packet); bool IsReadyToSendMedia_n() const; void CacheRtpAbsSendTimeHeaderExtension_n(int rtp_abs_sendtime_extn_id); + int GetTransportOverheadPerPacket() const; + void UpdateTransportOverhead(); // Wraps the existing RtpTransport in an SrtpTransport. void EnableSrtpTransport_n(); diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 36d7b0026e..5736bb6afa 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -1202,8 +1202,6 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { static constexpr uint16_t kLocalNetId = 1; static constexpr uint16_t kRemoteNetId = 2; static constexpr int kLastPacketId = 100; - // Ipv4(20) + UDP(8). - static constexpr int kTransportOverheadPerPacket = 28; CreateChannels(0, 0); @@ -1211,17 +1209,12 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { static_cast(channel1_->media_channel()); ASSERT_TRUE(media_channel1); - // Need to wait for the threads before calling - // |set_num_network_route_changes| because the network route would be set - // when creating the channel. - WaitForThreads(); media_channel1->set_num_network_route_changes(0); network_thread_->Invoke(RTC_FROM_HERE, [this] { - rtc::NetworkRoute network_route; // The transport channel becomes disconnected. - fake_rtp_dtls_transport1_->ice_transport()->SignalNetworkRouteChanged( - - rtc::Optional(network_route)); + fake_rtp_dtls_transport1_->ice_transport() + ->SignalSelectedCandidatePairChanged( + fake_rtp_dtls_transport1_->ice_transport(), nullptr, -1, false); }); WaitForThreads(); EXPECT_EQ(1, media_channel1->num_network_route_changes()); @@ -1229,16 +1222,15 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { media_channel1->set_num_network_route_changes(0); network_thread_->Invoke(RTC_FROM_HERE, [this] { - rtc::NetworkRoute network_route; - network_route.connected = true; - network_route.local_network_id = kLocalNetId; - network_route.remote_network_id = kRemoteNetId; - network_route.last_sent_packet_id = kLastPacketId; - network_route.packet_overhead = kTransportOverheadPerPacket; // The transport channel becomes connected. - fake_rtp_dtls_transport1_->ice_transport()->SignalNetworkRouteChanged( - - rtc::Optional(network_route)); + rtc::SocketAddress local_address("192.168.1.1", 1000 /* port number */); + rtc::SocketAddress remote_address("192.168.1.2", 2000 /* port number */); + auto candidate_pair = cricket::FakeCandidatePair::Create( + local_address, kLocalNetId, remote_address, kRemoteNetId); + fake_rtp_dtls_transport1_->ice_transport() + ->SignalSelectedCandidatePairChanged( + fake_rtp_dtls_transport1_->ice_transport(), candidate_pair.get(), + kLastPacketId, true); }); WaitForThreads(); EXPECT_EQ(1, media_channel1->num_network_route_changes()); @@ -1247,6 +1239,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { EXPECT_EQ(expected_network_route, media_channel1->last_network_route()); EXPECT_EQ(kLastPacketId, media_channel1->last_network_route().last_sent_packet_id); + constexpr int kTransportOverheadPerPacket = 28; // Ipv4(20) + UDP(8). EXPECT_EQ(kTransportOverheadPerPacket, media_channel1->transport_overhead_per_packet()); } diff --git a/pc/rtptransport.cc b/pc/rtptransport.cc index d2604f9eae..1701c4df16 100644 --- a/pc/rtptransport.cc +++ b/pc/rtptransport.cc @@ -11,7 +11,6 @@ #include "pc/rtptransport.h" #include "media/base/rtputils.h" -#include "p2p/base/p2pconstants.h" #include "p2p/base/packettransportinterface.h" #include "rtc_base/checks.h" #include "rtc_base/copyonwritebuffer.h" @@ -32,22 +31,15 @@ void RtpTransport::SetRtpPacketTransport( if (rtp_packet_transport_) { rtp_packet_transport_->SignalReadyToSend.disconnect(this); rtp_packet_transport_->SignalReadPacket.disconnect(this); - rtp_packet_transport_->SignalNetworkRouteChanged.disconnect(this); - // Reset the network route of the old transport. - SignalNetworkRouteChanged(rtc::Optional()); } if (new_packet_transport) { new_packet_transport->SignalReadyToSend.connect( this, &RtpTransport::OnReadyToSend); new_packet_transport->SignalReadPacket.connect(this, &RtpTransport::OnReadPacket); - new_packet_transport->SignalNetworkRouteChanged.connect( - this, &RtpTransport::OnNetworkRouteChange); - // Set the network route for the new transport. - SignalNetworkRouteChanged(new_packet_transport->network_route()); } - rtp_packet_transport_ = new_packet_transport; + // Assumes the transport is ready to send if it is writable. If we are wrong, // ready to send will be updated the next time we try to send. SetReadyToSend(false, @@ -62,19 +54,12 @@ void RtpTransport::SetRtcpPacketTransport( if (rtcp_packet_transport_) { rtcp_packet_transport_->SignalReadyToSend.disconnect(this); rtcp_packet_transport_->SignalReadPacket.disconnect(this); - rtcp_packet_transport_->SignalNetworkRouteChanged.disconnect(this); - // Reset the network route of the old transport. - SignalNetworkRouteChanged(rtc::Optional()); } if (new_packet_transport) { new_packet_transport->SignalReadyToSend.connect( this, &RtpTransport::OnReadyToSend); new_packet_transport->SignalReadPacket.connect(this, &RtpTransport::OnReadPacket); - new_packet_transport->SignalNetworkRouteChanged.connect( - this, &RtpTransport::OnNetworkRouteChange); - // Set the network route for the new transport. - SignalNetworkRouteChanged(new_packet_transport->network_route()); } rtcp_packet_transport_ = new_packet_transport; @@ -176,11 +161,6 @@ void RtpTransport::OnReadyToSend(rtc::PacketTransportInternal* transport) { SetReadyToSend(transport == rtcp_packet_transport_, true); } -void RtpTransport::OnNetworkRouteChange( - rtc::Optional network_route) { - SignalNetworkRouteChanged(network_route); -} - void RtpTransport::SetReadyToSend(bool rtcp, bool ready) { if (rtcp) { rtcp_ready_to_send_ = ready; diff --git a/pc/rtptransport.h b/pc/rtptransport.h index 34ec1b46b9..5e1aa20a1f 100644 --- a/pc/rtptransport.h +++ b/pc/rtptransport.h @@ -11,8 +11,6 @@ #ifndef PC_RTPTRANSPORT_H_ #define PC_RTPTRANSPORT_H_ -#include - #include "pc/bundlefilter.h" #include "pc/rtptransportinternal.h" #include "rtc_base/sigslot.h" @@ -78,7 +76,6 @@ class RtpTransport : public RtpTransportInternal { bool HandlesPacket(const uint8_t* data, size_t len); void OnReadyToSend(rtc::PacketTransportInternal* transport); - void OnNetworkRouteChange(rtc::Optional network_route); // Updates "ready to send" for an individual channel and fires // SignalReadyToSend. diff --git a/pc/rtptransport_unittest.cc b/pc/rtptransport_unittest.cc index d6eb336309..1e1657d20c 100644 --- a/pc/rtptransport_unittest.cc +++ b/pc/rtptransport_unittest.cc @@ -19,10 +19,6 @@ namespace webrtc { constexpr bool kMuxDisabled = false; constexpr bool kMuxEnabled = true; -constexpr uint16_t kLocalNetId = 1; -constexpr uint16_t kRemoteNetId = 2; -constexpr int kLastPacketId = 100; -constexpr int kTransportOverheadPerPacket = 28; // Ipv4(20) + UDP(8). TEST(RtpTransportTest, SetRtcpParametersCantDisableRtcpMux) { RtpTransport transport(kMuxDisabled); @@ -60,21 +56,12 @@ class SignalObserver : public sigslot::has_slots<> { public: explicit SignalObserver(RtpTransport* transport) { transport->SignalReadyToSend.connect(this, &SignalObserver::OnReadyToSend); - transport->SignalNetworkRouteChanged.connect( - this, &SignalObserver::OnNetworkRouteChanged); } - bool ready() const { return ready_; } void OnReadyToSend(bool ready) { ready_ = ready; } - rtc::Optional network_route() { return network_route_; } - void OnNetworkRouteChanged(rtc::Optional network_route) { - network_route_ = network_route; - } - private: bool ready_ = false; - rtc::Optional network_route_; }; TEST(RtpTransportTest, SettingRtcpAndRtpSignalsReady) { @@ -141,61 +128,6 @@ TEST(RtpTransportTest, EnablingRtcpMuxSignalsReady) { EXPECT_TRUE(observer.ready()); } -// Tests the SignalNetworkRoute is fired when setting a packet transport. -TEST(RtpTransportTest, SetRtpTransportWithNetworkRouteChanged) { - RtpTransport transport(kMuxDisabled); - SignalObserver observer(&transport); - rtc::FakePacketTransport fake_rtp("fake_rtp"); - - EXPECT_FALSE(observer.network_route()); - - rtc::NetworkRoute network_route; - // Set a non-null RTP transport with a new network route. - network_route.connected = true; - network_route.local_network_id = kLocalNetId; - network_route.remote_network_id = kRemoteNetId; - network_route.last_sent_packet_id = kLastPacketId; - network_route.packet_overhead = kTransportOverheadPerPacket; - fake_rtp.SetNetworkRoute(rtc::Optional(network_route)); - transport.SetRtpPacketTransport(&fake_rtp); - ASSERT_TRUE(observer.network_route()); - EXPECT_EQ(network_route, *(observer.network_route())); - EXPECT_EQ(kTransportOverheadPerPacket, - observer.network_route()->packet_overhead); - EXPECT_EQ(kLastPacketId, observer.network_route()->last_sent_packet_id); - - // Set a null RTP transport. - transport.SetRtpPacketTransport(nullptr); - EXPECT_FALSE(observer.network_route()); -} - -TEST(RtpTransportTest, SetRtcpTransportWithNetworkRouteChanged) { - RtpTransport transport(kMuxDisabled); - SignalObserver observer(&transport); - rtc::FakePacketTransport fake_rtcp("fake_rtcp"); - - EXPECT_FALSE(observer.network_route()); - - rtc::NetworkRoute network_route; - // Set a non-null RTCP transport with a new network route. - network_route.connected = true; - network_route.local_network_id = kLocalNetId; - network_route.remote_network_id = kRemoteNetId; - network_route.last_sent_packet_id = kLastPacketId; - network_route.packet_overhead = kTransportOverheadPerPacket; - fake_rtcp.SetNetworkRoute(rtc::Optional(network_route)); - transport.SetRtcpPacketTransport(&fake_rtcp); - ASSERT_TRUE(observer.network_route()); - EXPECT_EQ(network_route, *(observer.network_route())); - EXPECT_EQ(kTransportOverheadPerPacket, - observer.network_route()->packet_overhead); - EXPECT_EQ(kLastPacketId, observer.network_route()->last_sent_packet_id); - - // Set a null RTCP transport. - transport.SetRtcpPacketTransport(nullptr); - EXPECT_FALSE(observer.network_route()); -} - class SignalCounter : public sigslot::has_slots<> { public: explicit SignalCounter(RtpTransport* transport) { diff --git a/pc/rtptransportinternal.h b/pc/rtptransportinternal.h index 04d2ef39da..e58961997b 100644 --- a/pc/rtptransportinternal.h +++ b/pc/rtptransportinternal.h @@ -11,11 +11,7 @@ #ifndef PC_RTPTRANSPORTINTERNAL_H_ #define PC_RTPTRANSPORTINTERNAL_H_ -#include - #include "api/ortc/rtptransportinterface.h" -#include "p2p/base/icetransportinternal.h" -#include "rtc_base/networkroute.h" #include "rtc_base/sigslot.h" namespace rtc { @@ -56,10 +52,6 @@ class RtpTransportInternal : public RtpTransportInterface, sigslot::signal3 SignalPacketReceived; - // Called whenever the network route of the P2P layer transport changes. - // The argument is an optional network route. - sigslot::signal1> SignalNetworkRouteChanged; - virtual bool IsWritable(bool rtcp) const = 0; virtual bool SendRtpPacket(rtc::CopyOnWriteBuffer* packet, diff --git a/pc/srtptransport.cc b/pc/srtptransport.cc index bb42ad4d3c..1343fd0cac 100644 --- a/pc/srtptransport.cc +++ b/pc/srtptransport.cc @@ -42,8 +42,6 @@ void SrtpTransport::ConnectToRtpTransport() { this, &SrtpTransport::OnPacketReceived); rtp_transport_->SignalReadyToSend.connect(this, &SrtpTransport::OnReadyToSend); - rtp_transport_->SignalNetworkRouteChanged.connect( - this, &SrtpTransport::OnNetworkRouteChanged); } bool SrtpTransport::SendRtpPacket(rtc::CopyOnWriteBuffer* packet, @@ -172,20 +170,6 @@ void SrtpTransport::OnPacketReceived(bool rtcp, SignalPacketReceived(rtcp, packet, packet_time); } -void SrtpTransport::OnNetworkRouteChanged( - - rtc::Optional network_route) { - // Only append the SRTP overhead when there is a selected network route. - if (network_route) { - int srtp_overhead = 0; - if (IsActive()) { - GetSrtpOverhead(&srtp_overhead); - } - network_route->packet_overhead += srtp_overhead; - } - SignalNetworkRouteChanged(network_route); -} - bool SrtpTransport::SetRtpParams(int send_cs, const uint8_t* send_key, int send_key_len, diff --git a/pc/srtptransport.h b/pc/srtptransport.h index 919d4b95d5..13abd6b47d 100644 --- a/pc/srtptransport.h +++ b/pc/srtptransport.h @@ -16,7 +16,6 @@ #include #include -#include "p2p/base/icetransportinternal.h" #include "pc/rtptransportinternal.h" #include "pc/srtpfilter.h" #include "pc/srtpsession.h" @@ -159,8 +158,8 @@ class SrtpTransport : public RtpTransportInternal { void OnPacketReceived(bool rtcp, rtc::CopyOnWriteBuffer* packet, const rtc::PacketTime& packet_time); + void OnReadyToSend(bool ready) { SignalReadyToSend(ready); } - void OnNetworkRouteChanged(rtc::Optional network_route); bool ProtectRtp(void* data, int in_len, int max_len, int* out_len); diff --git a/pc/transportcontroller.cc b/pc/transportcontroller.cc index 7cf9bbd3bd..57f90b7591 100644 --- a/pc/transportcontroller.cc +++ b/pc/transportcontroller.cc @@ -761,7 +761,7 @@ void TransportController::SetMetricsObserver_n( void TransportController::OnChannelWritableState_n( rtc::PacketTransportInternal* transport) { RTC_DCHECK(network_thread_->IsCurrent()); - RTC_LOG(LS_INFO) << " Transport " << transport->transport_name() + RTC_LOG(LS_INFO) << " TransportChannel " << transport->debug_name() << " writability changed to " << transport->writable() << "."; UpdateAggregateStates_n(); diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 20797cc0ab..971d32d779 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -525,8 +525,6 @@ rtc_static_library("rtc_base_generic") { "messagehandler.h", "messagequeue.cc", "messagequeue.h", - "nethelper.cc", - "nethelper.h", "nethelpers.cc", "nethelpers.h", "network.cc", diff --git a/rtc_base/nethelper.cc b/rtc_base/nethelper.cc deleted file mode 100644 index e654fe3925..0000000000 --- a/rtc_base/nethelper.cc +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright 2017 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. - */ - -#include "rtc_base/nethelper.h" - -#include "rtc_base/checks.h" -#include "rtc_base/ipaddress.h" - -namespace cricket { - -const char UDP_PROTOCOL_NAME[] = "udp"; -const char TCP_PROTOCOL_NAME[] = "tcp"; -const char SSLTCP_PROTOCOL_NAME[] = "ssltcp"; -const char TLS_PROTOCOL_NAME[] = "tls"; - -int GetIpOverhead(int addr_family) { - switch (addr_family) { - case AF_INET: // IPv4 - return 20; - case AF_INET6: // IPv6 - return 40; - default: - RTC_NOTREACHED() << "Invaild address family."; - return 0; - } -} - -int GetProtocolOverhead(const std::string& protocol) { - if (protocol == TCP_PROTOCOL_NAME || protocol == SSLTCP_PROTOCOL_NAME) { - return 20; - } - return 8; -} - -} // namespace cricket diff --git a/rtc_base/nethelper.h b/rtc_base/nethelper.h deleted file mode 100644 index e86d126cbf..0000000000 --- a/rtc_base/nethelper.h +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Copyright 2017 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 RTC_BASE_NETHELPER_H_ -#define RTC_BASE_NETHELPER_H_ - -#include -#include - -// This header contains helper functions and constants used by different types -// of transports. -namespace cricket { - -extern const char UDP_PROTOCOL_NAME[]; -extern const char TCP_PROTOCOL_NAME[]; -extern const char SSLTCP_PROTOCOL_NAME[]; -extern const char TLS_PROTOCOL_NAME[]; - -// Get the network layer overhead per packet based on the IP address family. -int GetIpOverhead(int addr_family); - -// Get the transport layer overhead per packet based on the protocol. -int GetProtocolOverhead(const std::string& protocol); - -} // namespace cricket - -#endif // RTC_BASE_NETHELPER_H_ diff --git a/rtc_base/networkroute.h b/rtc_base/networkroute.h index 07cba6324c..f245cb9dc3 100644 --- a/rtc_base/networkroute.h +++ b/rtc_base/networkroute.h @@ -22,17 +22,14 @@ struct NetworkRoute { uint16_t local_network_id; uint16_t remote_network_id; int last_sent_packet_id; // Last packet id sent on the PREVIOUS route. - int packet_overhead; // The overhead in bytes from IP layer and above. NetworkRoute() : connected(false), local_network_id(0), remote_network_id(0), - last_sent_packet_id(-1), - packet_overhead(0) {} + last_sent_packet_id(-1) {} // The route is connected if the local and remote network ids are provided. - // TODO(zhihuang): Remove this and let the caller set the fields explicitly. NetworkRoute(bool connected, uint16_t local_net_id, uint16_t remote_net_id, @@ -40,11 +37,9 @@ struct NetworkRoute { : connected(connected), local_network_id(local_net_id), remote_network_id(remote_net_id), - last_sent_packet_id(last_packet_id), - packet_overhead(0) {} + last_sent_packet_id(last_packet_id) {} - // |last_sent_packet_id| and |packet_overhead| do not affect the NetworkRoute - // comparison. + // |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 &&