Set/clear the data channel pointers on the network thread
Bug: webrtc:9987
Change-Id: I8fa1b675a54729a26ee55926c6f27bb59981d379
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/213665
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33640}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index df35d0c..818e0ad 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -505,6 +505,19 @@
data_channel_type_ = type;
}
+cricket::RtpDataChannel* DataChannelController::rtp_data_channel() const {
+ // TODO(bugs.webrtc.org/9987): Only allow this accessor to be called on the
+ // network thread.
+ // RTC_DCHECK_RUN_ON(network_thread());
+ return rtp_data_channel_;
+}
+
+void DataChannelController::set_rtp_data_channel(
+ cricket::RtpDataChannel* channel) {
+ RTC_DCHECK_RUN_ON(network_thread());
+ rtp_data_channel_ = channel;
+}
+
DataChannelTransportInterface* DataChannelController::data_channel_transport()
const {
// TODO(bugs.webrtc.org/11547): Only allow this accessor to be called on the
diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
index f6d4409..854da59 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -115,12 +115,8 @@
// Accessors
cricket::DataChannelType data_channel_type() const;
void set_data_channel_type(cricket::DataChannelType type);
- cricket::RtpDataChannel* rtp_data_channel() const {
- return rtp_data_channel_;
- }
- void set_rtp_data_channel(cricket::RtpDataChannel* channel) {
- rtp_data_channel_ = channel;
- }
+ cricket::RtpDataChannel* rtp_data_channel() const;
+ void set_rtp_data_channel(cricket::RtpDataChannel* channel);
DataChannelTransportInterface* data_channel_transport() const;
void set_data_channel_transport(DataChannelTransportInterface* transport);
const std::map<std::string, rtc::scoped_refptr<RtpDataChannel>>*
@@ -203,9 +199,9 @@
// |rtp_data_channel_| is used if in RTP data channel mode,
// |data_channel_transport_| when using SCTP.
+ // TODO(bugs.webrtc.org/9987): Accessed on both signaling and network
+ // thread.
cricket::RtpDataChannel* rtp_data_channel_ = nullptr;
- // TODO(bugs.webrtc.org/9987): Accessed on both
- // signaling and some other thread.
SctpSidAllocator sid_allocator_ /* RTC_GUARDED_BY(signaling_thread()) */;
std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 84803da..aaf7232 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -522,11 +522,13 @@
// should be destroyed there.
network_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
RTC_DCHECK_RUN_ON(network_thread());
+ TeardownDataChannelTransport_n();
transport_controller_.reset();
port_allocator_.reset();
if (network_thread_safety_)
network_thread_safety_->SetNotAlive();
});
+
// call_ and event_log_ must be destroyed on the worker thread.
worker_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
RTC_DCHECK_RUN_ON(worker_thread());
@@ -1737,6 +1739,12 @@
rtp_manager_->Close();
network_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
+ // Data channels will already have been unset via the DestroyAllChannels()
+ // call above, which triggers a call to TeardownDataChannelTransport_n().
+ // TODO(tommi): ^^ That's not exactly optimal since this is yet another
+ // blocking hop to the network thread during Close(). Further still, the
+ // voice/video/data channels will be cleared on the worker thread.
+ RTC_DCHECK(!data_channel_controller_.rtp_data_channel());
transport_controller_.reset();
port_allocator_->DiscardCandidatePool();
if (network_thread_safety_) {
@@ -2410,16 +2418,30 @@
return true;
}
-void PeerConnection::TeardownDataChannelTransport_n() {
- if (!sctp_mid_n_ && !data_channel_controller_.data_channel_transport()) {
+void PeerConnection::SetupRtpDataChannelTransport_n(
+ cricket::RtpDataChannel* data_channel) {
+ data_channel_controller_.set_rtp_data_channel(data_channel);
+ if (!data_channel)
return;
- }
- RTC_LOG(LS_INFO) << "Tearing down data channel transport for mid="
- << *sctp_mid_n_;
- // |sctp_mid_| may still be active through an SCTP transport. If not, unset
- // it.
- sctp_mid_n_.reset();
+ // TODO(bugs.webrtc.org/9987): OnSentPacket_w needs to be changed to
+ // OnSentPacket_n (and be called on the network thread).
+ data_channel->SignalSentPacket().connect(this,
+ &PeerConnection::OnSentPacket_w);
+}
+
+void PeerConnection::TeardownDataChannelTransport_n() {
+ // Clear the RTP data channel if any.
+ data_channel_controller_.set_rtp_data_channel(nullptr);
+
+ if (sctp_mid_n_) {
+ // |sctp_mid_| may still be active through an SCTP transport. If not, unset
+ // it.
+ RTC_LOG(LS_INFO) << "Tearing down data channel transport for mid="
+ << *sctp_mid_n_;
+ sctp_mid_n_.reset();
+ }
+
data_channel_controller_.TeardownDataChannelTransport_n();
}
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index bd2e80a..7818e7b 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -442,6 +442,8 @@
bool SetupDataChannelTransport_n(const std::string& mid)
RTC_RUN_ON(network_thread());
+ void SetupRtpDataChannelTransport_n(cricket::RtpDataChannel* data_channel)
+ RTC_RUN_ON(network_thread());
void TeardownDataChannelTransport_n() RTC_RUN_ON(network_thread());
cricket::ChannelInterface* GetChannel(const std::string& content_name);
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 3499e4c..a8cd3c3 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -4671,19 +4671,18 @@
case cricket::DCT_RTP:
default:
RtpTransportInternal* rtp_transport = pc_->GetRtpTransport(mid);
- // TODO(bugs.webrtc.org/9987): set_rtp_data_channel() should be called on
- // the network thread like set_data_channel_transport is.
- data_channel_controller()->set_rtp_data_channel(
+ cricket::RtpDataChannel* data_channel =
channel_manager()->CreateRtpDataChannel(
pc_->configuration()->media_config, rtp_transport,
signaling_thread(), mid, pc_->SrtpRequired(),
- pc_->GetCryptoOptions(), &ssrc_generator_));
-
- if (!data_channel_controller()->rtp_data_channel()) {
+ pc_->GetCryptoOptions(), &ssrc_generator_);
+ if (!data_channel)
return false;
- }
- data_channel_controller()->rtp_data_channel()->SignalSentPacket().connect(
- pc_, &PeerConnection::OnSentPacket_w);
+
+ pc_->network_thread()->Invoke<void>(RTC_FROM_HERE, [this, data_channel] {
+ RTC_DCHECK_RUN_ON(pc_->network_thread());
+ pc_->SetupRtpDataChannelTransport_n(data_channel);
+ });
have_pending_rtp_data_channel_ = true;
return true;
}
@@ -4722,21 +4721,22 @@
void SdpOfferAnswerHandler::DestroyDataChannelTransport() {
RTC_DCHECK_RUN_ON(signaling_thread());
- if (data_channel_controller()->rtp_data_channel()) {
- data_channel_controller()->OnTransportChannelClosed();
- DestroyChannelInterface(data_channel_controller()->rtp_data_channel());
- data_channel_controller()->set_rtp_data_channel(nullptr);
- }
+ const bool has_sctp = pc_->sctp_mid().has_value();
+ auto* rtp_data_channel = data_channel_controller()->rtp_data_channel();
- if (pc_->sctp_mid()) {
- RTC_DCHECK_RUN_ON(pc_->signaling_thread());
+ if (has_sctp || rtp_data_channel)
data_channel_controller()->OnTransportChannelClosed();
- pc_->network_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
- RTC_DCHECK_RUN_ON(pc_->network_thread());
- pc_->TeardownDataChannelTransport_n();
- });
+
+ pc_->network_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
+ RTC_DCHECK_RUN_ON(pc_->network_thread());
+ pc_->TeardownDataChannelTransport_n();
+ });
+
+ if (has_sctp)
pc_->ResetSctpDataMid();
- }
+
+ if (rtp_data_channel)
+ DestroyChannelInterface(rtp_data_channel);
}
void SdpOfferAnswerHandler::DestroyChannelInterface(
