From 292ce4ef2569f499faa405ddde03c0a91b60a3cd Mon Sep 17 00:00:00 2001 From: Anton Sukhanov Date: Mon, 3 Jun 2019 13:00:24 -0700 Subject: [PATCH] Move datagram transport to JsepTransport - This makes it consistent with ICE and MediaTransport ownership. - Removes unnecessary datagram_transport() getter in DtlsTransportInternal As a side effect this fixes bug in JsepTransportController, which moved datagram_transport to Dtls after creating it, then checked if (datagram_transport) to decide which RTP transport to create. As a result of this bug we were creating Sded instead of Unencrypted RTP with datagram transport. Bug: webrtc:9719 Change-Id: Ic5b13a450ce6ac5b2a20d388657e3949aabef079 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139620 Commit-Queue: Anton Sukhanov Reviewed-by: Bjorn Mellem Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#28146} --- p2p/base/datagram_dtls_adaptor.cc | 11 ++--------- p2p/base/datagram_dtls_adaptor.h | 12 +++++------- p2p/base/dtls_transport_internal.h | 9 --------- pc/jsep_transport.cc | 15 +++++++++++---- pc/jsep_transport.h | 10 ++++++++-- pc/jsep_transport_controller.cc | 10 +++++----- pc/jsep_transport_controller.h | 2 +- pc/jsep_transport_unittest.cc | 3 ++- 8 files changed, 34 insertions(+), 38 deletions(-) diff --git a/p2p/base/datagram_dtls_adaptor.cc b/p2p/base/datagram_dtls_adaptor.cc index f62cfbf91c..a6e7d30908 100644 --- a/p2p/base/datagram_dtls_adaptor.cc +++ b/p2p/base/datagram_dtls_adaptor.cc @@ -45,12 +45,12 @@ namespace cricket { DatagramDtlsAdaptor::DatagramDtlsAdaptor( IceTransportInternal* ice_transport, - std::unique_ptr datagram_transport, + webrtc::DatagramTransportInterface* datagram_transport, const webrtc::CryptoOptions& crypto_options, webrtc::RtcEventLog* event_log) : crypto_options_(crypto_options), ice_transport_(ice_transport), - datagram_transport_(std::move(datagram_transport)), + datagram_transport_(datagram_transport), event_log_(event_log) { RTC_DCHECK(ice_transport_); RTC_DCHECK(datagram_transport_); @@ -88,9 +88,6 @@ DatagramDtlsAdaptor::~DatagramDtlsAdaptor() { // Unsubscribe from Datagram Transport dinks. datagram_transport_->SetDatagramSink(nullptr); datagram_transport_->SetTransportStateCallback(nullptr); - - // Make sure datagram transport is destroyed before ICE. - datagram_transport_.reset(); } const webrtc::CryptoOptions& DatagramDtlsAdaptor::crypto_options() const { @@ -247,10 +244,6 @@ IceTransportInternal* DatagramDtlsAdaptor::ice_transport() { return ice_transport_; } -webrtc::DatagramTransportInterface* DatagramDtlsAdaptor::datagram_transport() { - return datagram_transport_.get(); -} - // Similar implementaton as in p2p/base/dtls_transport.cc. void DatagramDtlsAdaptor::OnReadyToSend( rtc::PacketTransportInternal* transport) { diff --git a/p2p/base/datagram_dtls_adaptor.h b/p2p/base/datagram_dtls_adaptor.h index c1014cfff8..c68589060f 100644 --- a/p2p/base/datagram_dtls_adaptor.h +++ b/p2p/base/datagram_dtls_adaptor.h @@ -42,11 +42,10 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal, // TODO(sukhanov): Taking crypto options, because DtlsTransportInternal // has a virtual getter crypto_options(). Consider removing getter and // removing crypto_options from DatagramDtlsAdaptor. - DatagramDtlsAdaptor( - IceTransportInternal* ice_transport, - std::unique_ptr datagram_transport, - const webrtc::CryptoOptions& crypto_options, - webrtc::RtcEventLog* event_log); + DatagramDtlsAdaptor(IceTransportInternal* ice_transport, + webrtc::DatagramTransportInterface* datagram_transport, + const webrtc::CryptoOptions& crypto_options, + webrtc::RtcEventLog* event_log); ~DatagramDtlsAdaptor() override; @@ -89,7 +88,6 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal, size_t digest_len) override; bool SetSslMaxProtocolVersion(rtc::SSLProtocolVersion version) override; IceTransportInternal* ice_transport() override; - webrtc::DatagramTransportInterface* datagram_transport() override; const std::string& transport_name() const override; bool writable() const override; @@ -132,7 +130,7 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal, webrtc::CryptoOptions crypto_options_; IceTransportInternal* ice_transport_; - std::unique_ptr datagram_transport_; + webrtc::DatagramTransportInterface* datagram_transport_; // Current ICE writable state. Must be modified by calling set_ice_writable(), // which propagates change notifications. diff --git a/p2p/base/dtls_transport_internal.h b/p2p/base/dtls_transport_internal.h index 16e8b8151a..07a669af2e 100644 --- a/p2p/base/dtls_transport_internal.h +++ b/p2p/base/dtls_transport_internal.h @@ -18,7 +18,6 @@ #include #include "api/crypto/crypto_options.h" -#include "api/datagram_transport_interface.h" #include "api/dtls_transport_interface.h" #include "api/scoped_refptr.h" #include "p2p/base/ice_transport_internal.h" @@ -65,14 +64,6 @@ class DtlsTransportInternal : public rtc::PacketTransportInternal { virtual const webrtc::CryptoOptions& crypto_options() const = 0; - // Returns datagram transport or nullptr if not using datagram transport. - // TODO(sukhanov): Make pure virtual. - // TODO(sukhanov): Consider moving ownership of datagram transport and ICE - // to JsepTransport. - virtual webrtc::DatagramTransportInterface* datagram_transport() { - return nullptr; - } - virtual DtlsTransportState dtls_state() const = 0; virtual int component() const = 0; diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 3098681cb9..8c614a9db5 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -100,7 +100,8 @@ JsepTransport::JsepTransport( std::unique_ptr dtls_srtp_transport, std::unique_ptr rtp_dtls_transport, std::unique_ptr rtcp_dtls_transport, - std::unique_ptr media_transport) + std::unique_ptr media_transport, + std::unique_ptr datagram_transport) : network_thread_(rtc::Thread::Current()), mid_(mid), local_certificate_(local_certificate), @@ -118,14 +119,15 @@ JsepTransport::JsepTransport( ? new rtc::RefCountedObject( std::move(rtcp_dtls_transport)) : nullptr), - media_transport_(std::move(media_transport)) { + media_transport_(std::move(media_transport)), + datagram_transport_(std::move(datagram_transport)) { RTC_DCHECK(ice_transport_); RTC_DCHECK(rtp_dtls_transport_); // |rtcp_ice_transport_| must be present iff |rtcp_dtls_transport_| is // present. RTC_DCHECK_EQ((rtcp_ice_transport_ != nullptr), (rtcp_dtls_transport_ != nullptr)); - RTC_DCHECK(!datagram_transport() || !media_transport_); + RTC_DCHECK(!datagram_transport_ || !media_transport_); // Verify the "only one out of these three can be set" invariant. if (unencrypted_rtp_transport_) { RTC_DCHECK(!sdes_transport); @@ -146,7 +148,7 @@ JsepTransport::JsepTransport( JsepTransport::~JsepTransport() { // Disconnect media transport state callbacks and make sure we delete media - // transports before ICE. + // transport before ICE. if (media_transport_) { media_transport_->SetMediaTransportStateCallback(nullptr); media_transport_.reset(); @@ -158,6 +160,11 @@ JsepTransport::~JsepTransport() { if (rtcp_dtls_transport_) { rtcp_dtls_transport_->Clear(); } + + // Delete datagram transport before ICE, but after DTLS transport. + datagram_transport_.reset(); + + // ICE will be the last transport to be deleted. } webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index 9add2e5778..8e00793bf9 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -18,6 +18,7 @@ #include "absl/types/optional.h" #include "api/candidate.h" +#include "api/datagram_transport_interface.h" #include "api/jsep.h" #include "api/media_transport_interface.h" #include "p2p/base/dtls_transport.h" @@ -93,7 +94,8 @@ class JsepTransport : public sigslot::has_slots<>, std::unique_ptr dtls_srtp_transport, std::unique_ptr rtp_dtls_transport, std::unique_ptr rtcp_dtls_transport, - std::unique_ptr media_transport); + std::unique_ptr media_transport, + std::unique_ptr datagram_transport); ~JsepTransport() override; @@ -222,7 +224,7 @@ class JsepTransport : public sigslot::has_slots<>, // Returns datagram transport, if available. webrtc::DatagramTransportInterface* datagram_transport() const { rtc::CritScope scope(&accessor_lock_); - return rtp_dtls_transport_->internal()->datagram_transport(); + return datagram_transport_.get(); } // Returns the latest media transport state. @@ -343,6 +345,10 @@ class JsepTransport : public sigslot::has_slots<>, std::unique_ptr media_transport_ RTC_GUARDED_BY(accessor_lock_); + // Optional datagram transport (experimental). + std::unique_ptr datagram_transport_ + RTC_GUARDED_BY(accessor_lock_); + // If |media_transport_| is provided, this variable represents the state of // media transport. // diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 6adecb3be3..93949c6657 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -475,7 +475,7 @@ JsepTransportController::CreateIceTransport(const std::string transport_name, std::unique_ptr JsepTransportController::CreateDtlsTransport( cricket::IceTransportInternal* ice, - std::unique_ptr datagram_transport) { + DatagramTransportInterface* datagram_transport) { RTC_DCHECK(network_thread_->IsCurrent()); std::unique_ptr dtls; @@ -485,8 +485,7 @@ JsepTransportController::CreateDtlsTransport( // Create DTLS wrapper around DatagramTransportInterface. dtls = absl::make_unique( - ice, std::move(datagram_transport), config_.crypto_options, - config_.event_log); + ice, datagram_transport, config_.crypto_options, config_.event_log); } else if (config_.media_transport_factory && config_.use_media_transport_for_media && config_.use_media_transport_for_data_channels) { @@ -1166,7 +1165,7 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( } std::unique_ptr rtp_dtls_transport = - CreateDtlsTransport(ice.get(), std::move(datagram_transport)); + CreateDtlsTransport(ice.get(), datagram_transport.get()); std::unique_ptr rtcp_dtls_transport; std::unique_ptr unencrypted_rtp_transport; @@ -1217,7 +1216,8 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( content_info.name, certificate_, std::move(ice), std::move(rtcp_ice), std::move(unencrypted_rtp_transport), std::move(sdes_transport), std::move(dtls_srtp_transport), std::move(rtp_dtls_transport), - std::move(rtcp_dtls_transport), std::move(media_transport)); + std::move(rtcp_dtls_transport), std::move(media_transport), + std::move(datagram_transport)); jsep_transport->SignalRtcpMuxActive.connect( this, &JsepTransportController::UpdateAggregateStates_n); diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index aaaab4881e..995c703ce1 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -346,7 +346,7 @@ class JsepTransportController : public sigslot::has_slots<> { std::unique_ptr CreateDtlsTransport( cricket::IceTransportInternal* ice, - std::unique_ptr datagram_transport); + DatagramTransportInterface* datagram_transport); std::unique_ptr CreateIceTransport( const std::string transport_name, bool rtcp); diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index ec64379618..7e2f80ef5e 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc @@ -109,7 +109,8 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> { std::move(rtcp_ice), std::move(unencrypted_rtp_transport), std::move(sdes_transport), std::move(dtls_srtp_transport), std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport), - /*media_transport=*/nullptr); + /*media_transport=*/nullptr, + /*datagram_transport=*/nullptr); signal_rtcp_mux_active_received_ = false; jsep_transport->SignalRtcpMuxActive.connect(