From d69e0709c857dc6bee18e6db0eff2828e5a63087 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Wed, 7 Apr 2021 15:14:43 +0200 Subject: [PATCH] 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 Reviewed-by: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#33640} --- pc/data_channel_controller.cc | 13 +++++++++++ pc/data_channel_controller.h | 12 ++++------ pc/peer_connection.cc | 38 ++++++++++++++++++++++++------- pc/peer_connection.h | 2 ++ pc/sdp_offer_answer.cc | 42 +++++++++++++++++------------------ 5 files changed, 70 insertions(+), 37 deletions(-) diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index df35d0c8d3..818e0ad775 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -505,6 +505,19 @@ void DataChannelController::set_data_channel_type( 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 f6d4409f55..854da59a43 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -115,12 +115,8 @@ class DataChannelController : public RtpDataChannelProviderInterface, // 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>* @@ -203,9 +199,9 @@ class DataChannelController : public RtpDataChannelProviderInterface, // |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> sctp_data_channels_ diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 84803dafdb..aaf72324c7 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -522,11 +522,13 @@ PeerConnection::~PeerConnection() { // should be destroyed there. network_thread()->Invoke(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(RTC_FROM_HERE, [this] { RTC_DCHECK_RUN_ON(worker_thread()); @@ -1737,6 +1739,12 @@ void PeerConnection::Close() { rtp_manager_->Close(); network_thread()->Invoke(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 @@ bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) { 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 bd2e80a47b..7818e7bf4c 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -442,6 +442,8 @@ class PeerConnection : public PeerConnectionInternal, 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 3499e4c432..a8cd3c39fe 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -4671,19 +4671,18 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) { 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(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::DestroyTransceiverChannel( 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(RTC_FROM_HERE, [this] { - RTC_DCHECK_RUN_ON(pc_->network_thread()); - pc_->TeardownDataChannelTransport_n(); - }); + + pc_->network_thread()->Invoke(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(