From daee870a354dff0595150dfd97b20b79d76ffa3d Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Fri, 29 Apr 2022 11:42:55 +0000 Subject: [PATCH] Remove ability to do SetChannel() without ClearChannel() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This calls out the fact that SetChannel() is only used on M-section activation; ClearChannel is called on deactivation, and we never change the channel while a transceiver is active. Bug: webrtc:13931 Change-Id: I3a3bfeec7c1d27d98c3f94a9401bee2130754ed7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260461 Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#36709} --- pc/rtp_transceiver.cc | 38 +++++++++++----------------------- pc/rtp_transceiver.h | 14 ++++++++----- pc/rtp_transceiver_unittest.cc | 9 ++++---- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index 958a785809..e2e69f6670 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -164,25 +164,19 @@ void RtpTransceiver::SetChannel( cricket::ChannelInterface* channel, std::function transport_lookup) { RTC_DCHECK_RUN_ON(thread_); - RTC_DCHECK(channel); // Use ClearChannel() if clearing. + RTC_DCHECK(channel); RTC_DCHECK(transport_lookup); + RTC_DCHECK(!channel_); // Cannot set a channel on a stopped transceiver. - if (stopped_ || channel == channel_) { + if (stopped_) { return; } RTC_LOG_THREAD_BLOCK_COUNT(); - if (channel_) { - signaling_thread_safety_->SetNotAlive(); - signaling_thread_safety_ = nullptr; - } - RTC_DCHECK_EQ(media_type(), channel->media_type()); signaling_thread_safety_ = PendingTaskSafetyFlag::Create(); - cricket::ChannelInterface* channel_to_delete = nullptr; - // An alternative to this, could be to require SetChannel to be called // on the network thread. The channel object operates for the most part // on the network thread, as part of its initialization being on the network @@ -193,12 +187,6 @@ void RtpTransceiver::SetChannel( // helps with keeping the channel implementation requirements being met and // avoids synchronization for accessing the pointer or network related state. channel_manager_->network_thread()->Invoke(RTC_FROM_HERE, [&]() { - if (channel_) { - channel_->SetFirstPacketReceivedCallback(nullptr); - channel_->SetRtpTransport(nullptr); - channel_to_delete = channel_; - } - channel_ = channel; channel_->SetRtpTransport(transport_lookup(channel_->mid())); @@ -208,12 +196,7 @@ void RtpTransceiver::SetChannel( [this]() { OnFirstPacketReceived(); })); }); }); - - RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); - - if (channel_to_delete || !senders_.empty() || !receivers_.empty()) { - DeleteChannel(channel_to_delete); - } + PushNewMediaChannelAndDeleteChannel(nullptr); RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2); } @@ -244,17 +227,20 @@ void RtpTransceiver::ClearChannel() { }); RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1); - - if (channel_to_delete || !senders_.empty() || !receivers_.empty()) { - DeleteChannel(channel_to_delete); - } + PushNewMediaChannelAndDeleteChannel(channel_to_delete); RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(2); } -void RtpTransceiver::DeleteChannel( +void RtpTransceiver::PushNewMediaChannelAndDeleteChannel( cricket::ChannelInterface* channel_to_delete) { + // The clumsy combination of pushing down media channel and deleting + // the channel is due to the desire to do both things in one Invoke(). + if (!channel_to_delete && senders_.empty() && receivers_.empty()) { + return; + } channel_manager_->worker_thread()->Invoke(RTC_FROM_HERE, [&]() { + // Push down the new media_channel, if any, otherwise clear it. auto* media_channel = channel_ ? channel_->media_channel() : nullptr; for (const auto& sender : senders_) { sender->internal()->SetMediaChannel(media_channel); diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index 57d527f9b3..a42cae771e 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -107,18 +107,19 @@ class RtpTransceiver : public RtpTransceiverInterface, // furthermore be made on the signaling thread. // // `channel`: The channel instance to be associated with the transceiver. - // When a valid pointer is passed for `channel`, the state of the object + // This must be a valid pointer. + // The state of the object // is expected to be newly constructed and not initalized for network // activity (see next parameter for more). // // NOTE: For all practical purposes, the ownership of the channel // object should be considered to lie with the transceiver until - // `SetChannel()` is called again with nullptr set as the new channel. + // `ClearChannel()` is called. // Moving forward, this parameter will change to either be a // std::unique_ptr<> or the full construction of the channel object will // be moved to happen within the context of the transceiver class. // - // `transport_lookup`: When `channel` points to a valid channel object, this + // `transport_lookup`: This // callback function will be used to look up the `RtpTransport` object // to associate with the channel via `BaseChannel::SetRtpTransport`. // The lookup function will be called on the network thread, synchronously @@ -130,6 +131,7 @@ class RtpTransceiver : public RtpTransceiverInterface, // synchronously to the network thread from the signaling thread. // The callback allows us to combine the transport lookup with network // state initialization of the channel object. + // ClearChannel() must be used before calling SetChannel() again. void SetChannel(cricket::ChannelInterface* channel, std::function transport_lookup); @@ -280,8 +282,10 @@ class RtpTransceiver : public RtpTransceiverInterface, private: void OnFirstPacketReceived(); void StopSendingAndReceiving(); - // Delete a channel. Used by SetChannel and ClearChannel. - void DeleteChannel(cricket::ChannelInterface* channel_to_delete); + // Delete a channel, and ensure that references to its media channel + // are updated before deleting it. + void PushNewMediaChannelAndDeleteChannel( + cricket::ChannelInterface* channel_to_delete); // Enforce that this object is created, used and destroyed on one thread. TaskQueueBase* const thread_; diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index a65be9ba15..29828a045c 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -75,14 +75,15 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { EXPECT_CALL(channel2, media_type()) .WillRepeatedly(Return(cricket::MediaType::MEDIA_TYPE_AUDIO)); + // Clear the current channel - required to allow SetChannel() + EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); + EXPECT_CALL(cm, DestroyChannel(&channel1)).WillRepeatedly(testing::Return()); + transceiver->ClearChannel(); // Channel can no longer be set, so this call should be a no-op. transceiver->SetChannel(&channel2, [](const std::string&) { return nullptr; }); - EXPECT_EQ(&channel1, transceiver->channel()); + EXPECT_EQ(nullptr, transceiver->channel()); - // Clear the current channel before `transceiver` goes out of scope. - EXPECT_CALL(channel1, SetFirstPacketReceivedCallback(_)); - EXPECT_CALL(cm, DestroyChannel(&channel1)).WillRepeatedly(testing::Return()); transceiver->ClearChannel(); }