admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Move datagram transport to JsepTransport

Browse Source 
- 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}
This commit is contained in:
Anton Sukhanov 2019-06-03 13:00:24 -07:00 committed by Commit Bot
parent 9005e23a26
commit 292ce4ef25
8 changed files with 34 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
p2p/base/datagram_dtls_adaptor.cc
View File

@ -45,12 +45,12 @@ namespace cricket {
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 @@ 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) {

12
p2p/base/datagram_dtls_adaptor.h
View File

@ -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<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 @@ 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<webrtc::DatagramTransportInterface> datagram_transport_;
webrtc::DatagramTransportInterface* datagram_transport_;
// Current ICE writable state. Must be modified by calling set_ice_writable(),
// which propagates change notifications.

9
p2p/base/dtls_transport_internal.h
View File

@ -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 @@ 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;

15
pc/jsep_transport.cc
View File

@ -100,7 +100,8 @@ JsepTransport::JsepTransport(
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 @@ JsepTransport::JsepTransport(
? 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(
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(

10
pc/jsep_transport.h
View File

@ -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<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 @@ 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<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.
//

10
pc/jsep_transport_controller.cc
View File

@ -475,7 +475,7 @@ JsepTransportController::CreateIceTransport(const std::string transport_name,
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 @@ JsepTransportController::CreateDtlsTransport(
// 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 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
}
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 @@ 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);

2
pc/jsep_transport_controller.h
View File

@ -346,7 +346,7 @@ class JsepTransportController : public sigslot::has_slots<> {
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);

3
pc/jsep_transport_unittest.cc
View File

@ -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(