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 <sukhanov@webrtc.org>
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28146}
diff --git a/p2p/base/datagram_dtls_adaptor.cc b/p2p/base/datagram_dtls_adaptor.cc
index f62cfbf..a6e7d30 100644
--- a/p2p/base/datagram_dtls_adaptor.cc
+++ b/p2p/base/datagram_dtls_adaptor.cc
@@ -45,12 +45,12 @@
DatagramDtlsAdaptor::DatagramDtlsAdaptor(
IceTransportInternal* ice_transport,
- std::unique_ptr<webrtc::DatagramTransportInterface> 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 @@
// 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 @@
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 c1014cf..c685890 100644
--- a/p2p/base/datagram_dtls_adaptor.h
+++ b/p2p/base/datagram_dtls_adaptor.h
@@ -42,11 +42,10 @@
// 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<webrtc::DatagramTransportInterface> 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 @@
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 @@
webrtc::CryptoOptions crypto_options_;
IceTransportInternal* ice_transport_;
- std::unique_ptr<webrtc::DatagramTransportInterface> 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 16e8b81..07a669a 100644
--- a/p2p/base/dtls_transport_internal.h
+++ b/p2p/base/dtls_transport_internal.h
@@ -18,7 +18,6 @@
#include <string>
#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 @@
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 3098681..8c614a9 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -100,7 +100,8 @@
std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport,
std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
- std::unique_ptr<webrtc::MediaTransportInterface> media_transport)
+ std::unique_ptr<webrtc::MediaTransportInterface> media_transport,
+ std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport)
: network_thread_(rtc::Thread::Current()),
mid_(mid),
local_certificate_(local_certificate),
@@ -118,14 +119,15 @@
? new rtc::RefCountedObject<webrtc::DtlsTransport>(
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() {
// 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 @@
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 9add2e5..8e00793 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 @@
std::unique_ptr<webrtc::DtlsSrtpTransport> dtls_srtp_transport,
std::unique_ptr<DtlsTransportInternal> rtp_dtls_transport,
std::unique_ptr<DtlsTransportInternal> rtcp_dtls_transport,
- std::unique_ptr<webrtc::MediaTransportInterface> media_transport);
+ std::unique_ptr<webrtc::MediaTransportInterface> media_transport,
+ std::unique_ptr<webrtc::DatagramTransportInterface> datagram_transport);
~JsepTransport() override;
@@ -222,7 +224,7 @@
// 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 @@
std::unique_ptr<webrtc::MediaTransportInterface> media_transport_
RTC_GUARDED_BY(accessor_lock_);
+ // Optional datagram transport (experimental).
+ std::unique_ptr<webrtc::DatagramTransportInterface> 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 6adecb3..93949c6 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -475,7 +475,7 @@
std::unique_ptr<cricket::DtlsTransportInternal>
JsepTransportController::CreateDtlsTransport(
cricket::IceTransportInternal* ice,
- std::unique_ptr<DatagramTransportInterface> datagram_transport) {
+ DatagramTransportInterface* datagram_transport) {
RTC_DCHECK(network_thread_->IsCurrent());
std::unique_ptr<cricket::DtlsTransportInternal> dtls;
@@ -485,8 +485,7 @@
// Create DTLS wrapper around DatagramTransportInterface.
dtls = absl::make_unique<cricket::DatagramDtlsAdaptor>(
- 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 @@
}
std::unique_ptr<cricket::DtlsTransportInternal> rtp_dtls_transport =
- CreateDtlsTransport(ice.get(), std::move(datagram_transport));
+ CreateDtlsTransport(ice.get(), datagram_transport.get());
std::unique_ptr<cricket::DtlsTransportInternal> rtcp_dtls_transport;
std::unique_ptr<RtpTransport> unencrypted_rtp_transport;
@@ -1217,7 +1216,8 @@
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 aaaab48..995c703 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -346,7 +346,7 @@
std::unique_ptr<cricket::DtlsTransportInternal> CreateDtlsTransport(
cricket::IceTransportInternal* ice,
- std::unique_ptr<DatagramTransportInterface> datagram_transport);
+ DatagramTransportInterface* datagram_transport);
std::unique_ptr<cricket::IceTransportInternal> CreateIceTransport(
const std::string transport_name,
bool rtcp);
diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc
index ec64379..7e2f80e 100644
--- a/pc/jsep_transport_unittest.cc
+++ b/pc/jsep_transport_unittest.cc
@@ -109,7 +109,8 @@
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(
