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(); } }