From 19ebabc90459cf238eadeb13e81bcdbc225c8845 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 28 Apr 2022 13:31:17 +0000 Subject: [PATCH] Separate setting a cricket::Channel from clearing the channel. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it clearer which modules set the channel. Also remove GetChannel() from PeerConnection public API This was only used once, internally, and can better be inlined. Part of reducing the exposure of Channel. Bug: webrtc:13931 Change-Id: I5f44865230a0d8314d269c85afb91d4b503e8de0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260187 Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#36695} --- pc/peer_connection.cc | 18 ++-- pc/peer_connection.h | 2 - pc/rtp_transceiver.cc | 100 +++++++++++++++-------- pc/rtp_transceiver.h | 8 ++ pc/rtp_transceiver_unittest.cc | 6 +- pc/sdp_offer_answer.cc | 14 ++-- pc/test/fake_peer_connection_for_stats.h | 2 +- 7 files changed, 90 insertions(+), 60 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index bf8bd961d9..f13d5a911f 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2141,16 +2141,6 @@ void PeerConnection::StopRtcEventLog_w() { } } -cricket::ChannelInterface* PeerConnection::GetChannel(const std::string& mid) { - for (const auto& transceiver : rtp_manager()->transceivers()->UnsafeList()) { - cricket::ChannelInterface* channel = transceiver->internal()->channel(); - if (channel && channel->mid() == mid) { - return channel; - } - } - return nullptr; -} - bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) { RTC_DCHECK_RUN_ON(signaling_thread()); if (!local_description() || !remote_description()) { @@ -2864,9 +2854,11 @@ bool PeerConnection::OnTransportChanged( DataChannelTransportInterface* data_channel_transport) { RTC_DCHECK_RUN_ON(network_thread()); bool ret = true; - auto base_channel = GetChannel(mid); - if (base_channel) { - ret = base_channel->SetRtpTransport(rtp_transport); + for (const auto& transceiver : rtp_manager()->transceivers()->UnsafeList()) { + cricket::ChannelInterface* channel = transceiver->internal()->channel(); + if (channel && channel->mid() == mid) { + ret = channel->SetRtpTransport(rtp_transport); + } } if (mid == sctp_mid_n_) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 84a881083f..ca43fdf248 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -427,8 +427,6 @@ class PeerConnection : public PeerConnectionInternal, bool SetupDataChannelTransport_n(const std::string& mid) override RTC_RUN_ON(network_thread()); void TeardownDataChannelTransport_n() override RTC_RUN_ON(network_thread()); - cricket::ChannelInterface* GetChannel(const std::string& mid) - RTC_RUN_ON(network_thread()); // Functions made public for testing. void ReturnHistogramVeryQuicklyForTesting() { diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 29bf947eee..958a785809 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -164,14 +164,13 @@ void RtpTransceiver::SetChannel( cricket::ChannelInterface* channel, std::function transport_lookup) { RTC_DCHECK_RUN_ON(thread_); - // Cannot set a non-null channel on a stopped transceiver. - if ((stopped_ && channel) || channel == channel_) { + RTC_DCHECK(channel); // Use ClearChannel() if clearing. + RTC_DCHECK(transport_lookup); + // Cannot set a channel on a stopped transceiver. + if (stopped_ || channel == channel_) { return; } - RTC_DCHECK(channel || channel_); - RTC_DCHECK(!channel || transport_lookup) << "lookup function not supplied"; - RTC_LOG_THREAD_BLOCK_COUNT(); if (channel_) { @@ -179,10 +178,8 @@ void RtpTransceiver::SetChannel( signaling_thread_safety_ = nullptr; } - if (channel) { - RTC_DCHECK_EQ(media_type(), channel->media_type()); - signaling_thread_safety_ = PendingTaskSafetyFlag::Create(); - } + RTC_DCHECK_EQ(media_type(), channel->media_type()); + signaling_thread_safety_ = PendingTaskSafetyFlag::Create(); cricket::ChannelInterface* channel_to_delete = nullptr; @@ -204,40 +201,77 @@ void RtpTransceiver::SetChannel( channel_ = channel; - if (channel_) { - channel_->SetRtpTransport(transport_lookup(channel_->mid())); - channel_->SetFirstPacketReceivedCallback( - [thread = thread_, flag = signaling_thread_safety_, this]() mutable { - thread->PostTask(ToQueuedTask( - std::move(flag), [this]() { OnFirstPacketReceived(); })); - }); - } + channel_->SetRtpTransport(transport_lookup(channel_->mid())); + channel_->SetFirstPacketReceivedCallback( + [thread = thread_, flag = signaling_thread_safety_, this]() mutable { + thread->PostTask(ToQueuedTask(std::move(flag), + [this]() { OnFirstPacketReceived(); })); + }); }); RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); if (channel_to_delete || !senders_.empty() || !receivers_.empty()) { - channel_manager_->worker_thread()->Invoke(RTC_FROM_HERE, [&]() { - auto* media_channel = channel_ ? channel_->media_channel() : nullptr; - for (const auto& sender : senders_) { - sender->internal()->SetMediaChannel(media_channel); - } - - for (const auto& receiver : receivers_) { - receiver->internal()->SetMediaChannel(media_channel); - } - - // Destroy the channel, if we had one, now _after_ updating the receivers - // who might have had references to the previous channel. - if (channel_to_delete) { - channel_manager_->DestroyChannel(channel_to_delete); - } - }); + DeleteChannel(channel_to_delete); } RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2); } +void RtpTransceiver::ClearChannel() { + RTC_DCHECK_RUN_ON(thread_); + + if (!channel_) { + return; + } + + RTC_LOG_THREAD_BLOCK_COUNT(); + + if (channel_) { + signaling_thread_safety_->SetNotAlive(); + signaling_thread_safety_ = nullptr; + } + cricket::ChannelInterface* channel_to_delete = nullptr; + + channel_manager_->network_thread()->Invoke(RTC_FROM_HERE, [&]() { + if (channel_) { + channel_->SetFirstPacketReceivedCallback(nullptr); + channel_->SetRtpTransport(nullptr); + channel_to_delete = channel_; + } + + channel_ = nullptr; + }); + + RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); + + if (channel_to_delete || !senders_.empty() || !receivers_.empty()) { + DeleteChannel(channel_to_delete); + } + + RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2); +} + +void RtpTransceiver::DeleteChannel( + cricket::ChannelInterface* channel_to_delete) { + channel_manager_->worker_thread()->Invoke(RTC_FROM_HERE, [&]() { + auto* media_channel = channel_ ? channel_->media_channel() : nullptr; + for (const auto& sender : senders_) { + sender->internal()->SetMediaChannel(media_channel); + } + + for (const auto& receiver : receivers_) { + receiver->internal()->SetMediaChannel(media_channel); + } + + // Destroy the channel, if we had one, now _after_ updating the receivers + // who might have had references to the previous channel. + if (channel_to_delete) { + channel_manager_->DestroyChannel(channel_to_delete); + } + }); +} + void RtpTransceiver::AddSender( rtc::scoped_refptr> sender) { RTC_DCHECK_RUN_ON(thread_); diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index e71fdc1695..57d527f9b3 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -134,6 +134,9 @@ class RtpTransceiver : public RtpTransceiverInterface, std::function transport_lookup); + // Clear the association between the transceiver and the channel. + void ClearChannel(); + // Adds an RtpSender of the appropriate type to be owned by this transceiver. // Must not be null. void AddSender( @@ -277,6 +280,8 @@ class RtpTransceiver : public RtpTransceiverInterface, private: void OnFirstPacketReceived(); void StopSendingAndReceiving(); + // Delete a channel. Used by SetChannel and ClearChannel. + void DeleteChannel(cricket::ChannelInterface* channel_to_delete); // Enforce that this object is created, used and destroyed on one thread. TaskQueueBase* const thread_; @@ -301,6 +306,9 @@ class RtpTransceiver : public RtpTransceiverInterface, bool reused_for_addtrack_ = false; bool has_ever_been_used_to_send_ = false; + // Accessed on both thread_ and the network thread. Considered safe + // because all access on the network thread is within an invoke() + // from thread_. cricket::ChannelInterface* channel_ = nullptr; cricket::ChannelManager* channel_manager_ = nullptr; std::vector codec_preferences_; diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 9c2a0461b3..a65be9ba15 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -83,7 +83,7 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { // Clear the current channel before `transceiver` goes out of scope. EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); EXPECT_CALL(cm, DestroyChannel(&channel1)).WillRepeatedly(testing::Return()); - transceiver->SetChannel(nullptr, nullptr); + transceiver->ClearChannel(); } // Checks that a channel can be unset on a stopped `RtpTransceiver` @@ -112,7 +112,7 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { EXPECT_EQ(&channel, transceiver->channel()); // Set the channel to `nullptr`. - transceiver->SetChannel(nullptr, nullptr); + transceiver->ClearChannel(); EXPECT_EQ(nullptr, transceiver->channel()); } @@ -217,7 +217,7 @@ class RtpTransceiverTestForHeaderExtensions : public ::testing::Test { EXPECT_CALL(mock_channel, SetFirstPacketReceivedCallback(_)); EXPECT_CALL(channel_manager_, DestroyChannel(&mock_channel)) .WillRepeatedly(testing::Return()); - transceiver_->SetChannel(nullptr, nullptr); + transceiver_->ClearChannel(); } rtc::scoped_refptr receiver_ = MockReceiver(); diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 325e033ea5..9014852c15 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -2924,7 +2924,7 @@ RTCError SdpOfferAnswerHandler::Rollback(SdpType desc_type) { } RTC_DCHECK(transceiver->internal()->mid().has_value()); - transceiver->internal()->SetChannel(nullptr, nullptr); + transceiver->internal()->ClearChannel(); if (signaling_state() == PeerConnectionInterface::kHaveRemoteOffer && transceiver->receiver()) { @@ -3609,7 +3609,7 @@ RTCError SdpOfferAnswerHandler::UpdateTransceiverChannel( cricket::ChannelInterface* channel = transceiver->internal()->channel(); if (content.rejected) { if (channel) { - transceiver->internal()->SetChannel(nullptr, nullptr); + transceiver->internal()->ClearChannel(); } } else { if (!channel) { @@ -4607,14 +4607,12 @@ void SdpOfferAnswerHandler::RemoveUnusedChannels( // voice channel. const cricket::ContentInfo* video_info = cricket::GetFirstVideoContent(desc); if (!video_info || video_info->rejected) { - rtp_manager()->GetVideoTransceiver()->internal()->SetChannel(nullptr, - nullptr); + rtp_manager()->GetVideoTransceiver()->internal()->ClearChannel(); } const cricket::ContentInfo* audio_info = cricket::GetFirstAudioContent(desc); if (!audio_info || audio_info->rejected) { - rtp_manager()->GetAudioTransceiver()->internal()->SetChannel(nullptr, - nullptr); + rtp_manager()->GetAudioTransceiver()->internal()->ClearChannel(); } const cricket::ContentInfo* data_info = cricket::GetFirstDataContent(desc); @@ -4923,12 +4921,12 @@ void SdpOfferAnswerHandler::DestroyAllChannels() { for (const auto& transceiver : list) { if (transceiver->media_type() == cricket::MEDIA_TYPE_VIDEO) { - transceiver->internal()->SetChannel(nullptr, nullptr); + transceiver->internal()->ClearChannel(); } } for (const auto& transceiver : list) { if (transceiver->media_type() == cricket::MEDIA_TYPE_AUDIO) { - transceiver->internal()->SetChannel(nullptr, nullptr); + transceiver->internal()->ClearChannel(); } } diff --git a/pc/test/fake_peer_connection_for_stats.h b/pc/test/fake_peer_connection_for_stats.h index 44b2f5b33b..a263c1eed2 100644 --- a/pc/test/fake_peer_connection_for_stats.h +++ b/pc/test/fake_peer_connection_for_stats.h @@ -155,7 +155,7 @@ class FakePeerConnectionForStats : public FakePeerConnectionBase { ~FakePeerConnectionForStats() { for (auto transceiver : transceivers_) { - transceiver->internal()->SetChannel(nullptr, nullptr); + transceiver->internal()->ClearChannel(); } }