Unify and de-duplicate BaseChannel deletion in PeerConnection

This refactoring reduces code duplication in PeerConnection and
will make it easier to use these methods with the Unified Plan
implementation.

Bug: webrtc:8587
Change-Id: I6afd44fff702290903555cbe7703198b6b091da6
Reviewed-on: https://webrtc-review.googlesource.com/26822
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
Reviewed-by: Zhi Huang <zhihuang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21052}

pc/peerconnection.cc

@@ -845,33 +845,16 @@ PeerConnection::~PeerConnection() {
   // Destroy video channels first since they may have a pointer to a voice
   // channel.
   for (auto transceiver : transceivers_) {
-    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_VIDEO &&
+    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_VIDEO) {
-        transceiver->internal()->channel()) {
+      DestroyTransceiverChannel(transceiver);
-      DestroyVideoChannel(static_cast<cricket::VideoChannel*>(
-          transceiver->internal()->channel()));
     }
   }
   for (auto transceiver : transceivers_) {
-    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_AUDIO &&
+    if (transceiver->internal()->media_type() == cricket::MEDIA_TYPE_AUDIO) {
-        transceiver->internal()->channel()) {
+      DestroyTransceiverChannel(transceiver);
-      DestroyVoiceChannel(static_cast<cricket::VoiceChannel*>(
-          transceiver->internal()->channel()));
     }
   }
-  if (rtp_data_channel_) {
   DestroyDataChannel();
-  }
-
-  // Note: Cannot use rtc::Bind to create a functor to invoke because it will
-  // grab a reference to this PeerConnection. The RefCountedObject vtable will
-  // have already been destroyed (since it is a subclass of PeerConnection) and
-  // using rtc::Bind will cause "Pure virtual function called" error to appear.
-
-  if (sctp_transport_) {
-    OnDataChannelDestroyed();
-    network_thread()->Invoke<void>(RTC_FROM_HERE,
-                                   [this] { DestroySctpTransport_n(); });
-  }
 
   RTC_LOG(LS_INFO) << "Session: " << session_id() << " is destroyed.";
 
@@ -4267,31 +4250,22 @@ bool PeerConnection::UseCandidate(const IceCandidateInterface* candidate) {
 }
 
 void PeerConnection::RemoveUnusedChannels(const SessionDescription* desc) {
-  // TODO(steveanton): Add support for multiple audio/video channels.
   // Destroy video channel first since it may have a pointer to the
   // voice channel.
   const cricket::ContentInfo* video_info = cricket::GetFirstVideoContent(desc);
-  if ((!video_info || video_info->rejected) && video_channel()) {
+  if (!video_info || video_info->rejected) {
-    DestroyVideoChannel(video_channel());
+    DestroyTransceiverChannel(GetVideoTransceiver());
   }
 
-  const cricket::ContentInfo* voice_info = cricket::GetFirstAudioContent(desc);
+  const cricket::ContentInfo* audio_info = cricket::GetFirstAudioContent(desc);
-  if ((!voice_info || voice_info->rejected) && voice_channel()) {
+  if (!audio_info || audio_info->rejected) {
-    DestroyVoiceChannel(voice_channel());
+    DestroyTransceiverChannel(GetAudioTransceiver());
   }
 
   const cricket::ContentInfo* data_info = cricket::GetFirstDataContent(desc);
   if (!data_info || data_info->rejected) {
-    if (rtp_data_channel_) {
     DestroyDataChannel();
   }
-    if (sctp_transport_) {
-      OnDataChannelDestroyed();
-      network_thread()->Invoke<void>(
-          RTC_FROM_HERE,
-          rtc::Bind(&PeerConnection::DestroySctpTransport_n, this));
-    }
-  }
 }
 
 std::string PeerConnection::GetTransportNameForMediaSection(
@@ -4951,7 +4925,9 @@ void PeerConnection::OnSentPacket_w(const rtc::SentPacket& sent_packet) {
 const std::string PeerConnection::GetTransportName(
     const std::string& content_name) {
   cricket::BaseChannel* channel = GetChannel(content_name);
-  if (!channel) {
+  if (channel) {
+    return channel->transport_name();
+  }
   if (sctp_transport_) {
     RTC_DCHECK(sctp_content_name_);
     RTC_DCHECK(sctp_transport_name_);
@@ -4961,8 +4937,6 @@ const std::string PeerConnection::GetTransportName(
   }
   // Return an empty string if failed to retrieve the transport name.
   return "";
-  }
-  return channel->transport_name();
 }
 
 void PeerConnection::DestroyRtcpTransport_n(const std::string& transport_name) {
@@ -4971,57 +4945,68 @@ void PeerConnection::DestroyRtcpTransport_n(const std::string& transport_name) {
       transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
 }
 
-// TODO(steveanton): Perhaps this should be managed by the RtpTransceiver.
+void PeerConnection::DestroyTransceiverChannel(
-void PeerConnection::DestroyVideoChannel(cricket::VideoChannel* video_channel) {
+    rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>
-  RTC_DCHECK(video_channel);
+        transceiver) {
-  RTC_DCHECK(video_channel->rtp_dtls_transport());
+  RTC_DCHECK(transceiver);
-  RTC_DCHECK_EQ(GetVideoTransceiver()->internal()->channel(), video_channel);
-  GetVideoTransceiver()->internal()->SetChannel(nullptr);
-  const std::string transport_name =
-      video_channel->rtp_dtls_transport()->transport_name();
-  const bool need_to_delete_rtcp =
-      (video_channel->rtcp_dtls_transport() != nullptr);
-  // The above need to be cached before destroying the video channel so that we
-  // do not access uninitialized memory.
-  channel_manager()->DestroyVideoChannel(video_channel);
-  transport_controller_->DestroyDtlsTransport(
-      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
-  if (need_to_delete_rtcp) {
-    transport_controller_->DestroyDtlsTransport(
-        transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
-  }
-}
 
-// TODO(steveanton): Perhaps this should be managed by the RtpTransceiver.
+  cricket::BaseChannel* channel = transceiver->internal()->channel();
-void PeerConnection::DestroyVoiceChannel(cricket::VoiceChannel* voice_channel) {
+  if (channel) {
-  RTC_DCHECK(voice_channel);
+    transceiver->internal()->SetChannel(nullptr);
-  RTC_DCHECK(voice_channel->rtp_dtls_transport());
+    DestroyBaseChannel(channel);
-  RTC_DCHECK_EQ(GetAudioTransceiver()->internal()->channel(), voice_channel);
-  GetAudioTransceiver()->internal()->SetChannel(nullptr);
-  const std::string transport_name =
-      voice_channel->rtp_dtls_transport()->transport_name();
-  const bool need_to_delete_rtcp =
-      (voice_channel->rtcp_dtls_transport() != nullptr);
-  // The above need to be cached before destroying the video channel so that we
-  // do not access uninitialized memory.
-  channel_manager()->DestroyVoiceChannel(voice_channel);
-  transport_controller_->DestroyDtlsTransport(
-      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
-  if (need_to_delete_rtcp) {
-    transport_controller_->DestroyDtlsTransport(
-        transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
   }
 }
 
 void PeerConnection::DestroyDataChannel() {
+  if (rtp_data_channel_) {
     OnDataChannelDestroyed();
-  RTC_DCHECK(rtp_data_channel_->rtp_dtls_transport());
+    DestroyBaseChannel(rtp_data_channel_);
-  std::string transport_name;
-  transport_name = rtp_data_channel_->rtp_dtls_transport()->transport_name();
-  bool need_to_delete_rtcp =
-      (rtp_data_channel_->rtcp_dtls_transport() != nullptr);
-  channel_manager()->DestroyRtpDataChannel(rtp_data_channel_);
     rtp_data_channel_ = nullptr;
+  }
+
+  // Note: Cannot use rtc::Bind to create a functor to invoke because it will
+  // grab a reference to this PeerConnection. If this is called from the
+  // PeerConnection destructor, the RefCountedObject vtable will have already
+  // been destroyed (since it is a subclass of PeerConnection) and using
+  // rtc::Bind will cause "Pure virtual function called" error to appear.
+
+  if (sctp_transport_) {
+    OnDataChannelDestroyed();
+    network_thread()->Invoke<void>(RTC_FROM_HERE,
+                                   [this] { DestroySctpTransport_n(); });
+  }
+}
+
+void PeerConnection::DestroyBaseChannel(cricket::BaseChannel* channel) {
+  RTC_DCHECK(channel);
+  RTC_DCHECK(channel->rtp_dtls_transport());
+
+  // Need to cache these before destroying the base channel so that we do not
+  // access uninitialized memory.
+  const std::string transport_name =
+      channel->rtp_dtls_transport()->transport_name();
+  const bool need_to_delete_rtcp = (channel->rtcp_dtls_transport() != nullptr);
+
+  switch (channel->media_type()) {
+    case cricket::MEDIA_TYPE_AUDIO:
+      channel_manager()->DestroyVoiceChannel(
+          static_cast<cricket::VoiceChannel*>(channel));
+      break;
+    case cricket::MEDIA_TYPE_VIDEO:
+      channel_manager()->DestroyVideoChannel(
+          static_cast<cricket::VideoChannel*>(channel));
+      break;
+    case cricket::MEDIA_TYPE_DATA:
+      channel_manager()->DestroyRtpDataChannel(
+          static_cast<cricket::RtpDataChannel*>(channel));
+      break;
+    default:
+      RTC_NOTREACHED() << "Unknown media type: " << channel->media_type();
+      break;
+  }
+
+  // |channel| can no longer be used.
+
   transport_controller_->DestroyDtlsTransport(
       transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
   if (need_to_delete_rtcp) {

pc/peerconnection.h

@@ -766,12 +766,20 @@ class PeerConnection : public PeerConnectionInterface,
   const std::string GetTransportName(const std::string& content_name);
 
   void DestroyRtcpTransport_n(const std::string& transport_name);
-  void RemoveAndDestroyVideoChannel(cricket::VideoChannel* video_channel);
+
-  void DestroyVideoChannel(cricket::VideoChannel* video_channel);
+  // Destroys and clears the BaseChannel associated with the given transceiver,
-  void RemoveAndDestroyVoiceChannel(cricket::VoiceChannel* voice_channel);
+  // if such channel is set.
-  void DestroyVoiceChannel(cricket::VoiceChannel* voice_channel);
+  void DestroyTransceiverChannel(
+      rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>
+          transceiver);
+
+  // Destroys the RTP data channel and/or the SCTP data channel and clears it.
   void DestroyDataChannel();
 
+  // Destroys the given BaseChannel. The channel cannot be accessed after this
+  // method is called.
+  void DestroyBaseChannel(cricket::BaseChannel* channel);
+
   // Storing the factory as a scoped reference pointer ensures that the memory
   // in the PeerConnectionFactoryImpl remains available as long as the
   // PeerConnection is running. It is passed to PeerConnection as a raw pointer.