Separate setting a cricket::Channel from clearing the channel.

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 <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36695}

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_) {

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() {

pc/rtp_transceiver.cc

@@ -164,14 +164,13 @@ void RtpTransceiver::SetChannel(
     cricket::ChannelInterface* channel,
     std::function<RtpTransportInternal*(const std::string&)> 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<void>(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<void>(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<void>(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<RtpSenderProxyWithInternal<RtpSenderInternal>> sender) {
   RTC_DCHECK_RUN_ON(thread_);

pc/rtp_transceiver.h

@@ -134,6 +134,9 @@ class RtpTransceiver : public RtpTransceiverInterface,
                   std::function<RtpTransportInternal*(const std::string&)>
                       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<RtpCodecCapability> codec_preferences_;

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<MockRtpReceiverInternal> receiver_ = MockReceiver();

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

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