Remove BaseChannel's dependency on TransportController.

webrtc/api/webrtcsession.cc

 /*
  *  Copyright 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 488 matching lines @@
       this, &WebRtcSession::OnTransportControllerCandidatesRemoved);
   transport_controller_->SignalDtlsHandshakeError.connect(
       this, &WebRtcSession::OnTransportControllerDtlsHandshakeError);
 }
 
 WebRtcSession::~WebRtcSession() {
   RTC_DCHECK(signaling_thread()->IsCurrent());
   // Destroy video_channel_ first since it may have a pointer to the
   // voice_channel_.
   if (video_channel_) {
-    SignalVideoChannelDestroyed();
-    channel_manager_->DestroyVideoChannel(video_channel_.release());
+    DestroyVideoChannel();
   }
   if (voice_channel_) {
-    SignalVoiceChannelDestroyed();
-    channel_manager_->DestroyVoiceChannel(voice_channel_.release());
+    DestroyVoiceChannel();
   }
   if (rtp_data_channel_) {
-    SignalDataChannelDestroyed();
-    channel_manager_->DestroyRtpDataChannel(rtp_data_channel_.release());
+    DestroyDataChannel();
   }
   if (sctp_transport_) {
     SignalDataChannelDestroyed();
     network_thread_->Invoke<void>(
         RTC_FROM_HERE, rtc::Bind(&WebRtcSession::DestroySctpTransport_n, this));
   }
 #ifdef HAVE_QUIC
   if (quic_data_transport_) {
     quic_data_transport_.reset();
   }
@@ skipping 200 matching lines @@
     // is applied. Restore back to old description.
     return BadLocalSdp(desc->type(), kCreateChannelFailed, err_desc);
   }
 
   // Remove unused channels if MediaContentDescription is rejected.
   RemoveUnusedChannels(local_description()->description());
 
   if (!UpdateSessionState(action, cricket::CS_LOCAL, err_desc)) {
     return false;
   }
-
   if (remote_description()) {
     // Now that we have a local description, we can push down remote candidates.
     UseCandidatesInSessionDescription(remote_description());
   }
 
   pending_ice_restarts_.clear();
-
   if (error() != ERROR_NONE) {
     return BadLocalSdp(desc->type(), GetSessionErrorMsg(), err_desc);
   }
   return true;
 }
 
 bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc,
                                          std::string* err_desc) {
   RTC_DCHECK(signaling_thread()->IsCurrent());
 
@@ skipping 310 matching lines @@
   const std::string& transport_name = *first_content_name;
 
 #ifdef HAVE_QUIC
   if (quic_data_transport_ &&
       bundle.HasContentName(quic_data_transport_->content_name()) &&
       quic_data_transport_->transport_name() != transport_name) {
     LOG(LS_ERROR) << "Unable to BUNDLE " << quic_data_transport_->content_name()
                   << " on " << transport_name << "with QUIC.";
   }
 #endif
-
   auto maybe_set_transport = [this, bundle,
                               transport_name](cricket::BaseChannel* ch) {
     if (!ch || !bundle.HasContentName(ch->content_name())) {
       return true;
     }
 
-    if (ch->transport_name() == transport_name) {
+    std::string old_transport_name = ch->transport_name();
+    if (old_transport_name == transport_name) {
       LOG(LS_INFO) << "BUNDLE already enabled for " << ch->content_name()
                    << " on " << transport_name << ".";
       return true;
     }
 
-    if (!ch->SetTransport(transport_name)) {
+    cricket::TransportChannel* rtp_transport =
+        transport_controller_->CreateTransportChannel(
+            transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+    bool need_rtcp = (ch->rtcp_transport() != nullptr);
+    cricket::TransportChannel* rtcp_transport = nullptr;
+    if (need_rtcp) {
+      rtcp_transport = transport_controller_->CreateTransportChannel_n(
+          transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+    }
+
+    if (!ch->SetTransport(rtp_transport, rtcp_transport)) {
       LOG(LS_WARNING) << "Failed to enable BUNDLE for " << ch->content_name();
       return false;

[pthatcher1, 2017/01/13 21:51:07]

If it failed, do you need to destroy the new transports?  

[Taylor Brandstetter, 2017/01/13 23:46:48]

It won't ever fail unless invalid arguments are supplied. I'll change it from a
method that returns false to one that just DCHECKs.

     }
     LOG(LS_INFO) << "Enabled BUNDLE for " << ch->content_name() << " on "
                  << transport_name << ".";
+    DestroyTransport(old_transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+    // If the channel needs rtcp, it means that the channel used to have a
+    // rtcp transport which needs to be deleted now.
+    if (need_rtcp) {
+      DestroyTransport(old_transport_name,
+                       cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+    }
     return true;
   };
 
   if (!maybe_set_transport(voice_channel()) ||
       !maybe_set_transport(video_channel()) ||
       !maybe_set_transport(rtp_data_channel())) {
     return false;
   }
   // For SCTP, transport creation/deletion happens here instead of in the
   // object itself.
@@ skipping 567 matching lines @@
   }
   return true;
 }
 
 void WebRtcSession::RemoveUnusedChannels(const SessionDescription* desc) {
   // 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_) {
-    SignalVideoChannelDestroyed();
-    channel_manager_->DestroyVideoChannel(video_channel_.release());
+    DestroyVideoChannel();
   }
 
   const cricket::ContentInfo* voice_info =
       cricket::GetFirstAudioContent(desc);
   if ((!voice_info || voice_info->rejected) && voice_channel_) {
-    SignalVoiceChannelDestroyed();
-    channel_manager_->DestroyVoiceChannel(voice_channel_.release());
+    DestroyVoiceChannel();
   }
 
   const cricket::ContentInfo* data_info =
       cricket::GetFirstDataContent(desc);
   if (!data_info || data_info->rejected) {
     if (rtp_data_channel_) {
-      SignalDataChannelDestroyed();
-      channel_manager_->DestroyRtpDataChannel(rtp_data_channel_.release());
+      DestroyDataChannel();
     }
     if (sctp_transport_) {
       SignalDataChannelDestroyed();
       network_thread_->Invoke<void>(
           RTC_FROM_HERE,
           rtc::Bind(&WebRtcSession::DestroySctpTransport_n, this));
     }
 #ifdef HAVE_QUIC
     // Clean up the existing QuicDataTransport and its QuicTransportChannels.
     if (quic_data_transport_) {
@@ skipping 62 matching lines @@
   }
 
   return true;
 }
 
 bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content,
                                        const std::string* bundle_transport) {
   bool require_rtcp_mux =
       rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
   bool create_rtcp_transport_channel = !require_rtcp_mux;
+
+  std::string transport_name =
+      bundle_transport ? *bundle_transport : content->name;
+
+  cricket::TransportChannel* rtp_transport =
+      transport_controller_->CreateTransportChannel(
+          transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  cricket::TransportChannel* rtcp_transport = nullptr;
+  if (create_rtcp_transport_channel) {
+    rtcp_transport = transport_controller_->CreateTransportChannel(
+        transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+  }
+
   voice_channel_.reset(channel_manager_->CreateVoiceChannel(
-      media_controller_, transport_controller_.get(), content->name,
+      media_controller_, rtp_transport, rtcp_transport,
+      transport_controller_->signaling_thread(), content->name,
       bundle_transport, create_rtcp_transport_channel, SrtpRequired(),
       audio_options_));
   if (!voice_channel_) {
     return false;

[pthatcher1, 2017/01/13 21:51:07]

If the constructor failed, do we have to destroy the transports?

[Taylor Brandstetter, 2017/01/13 23:46:48]

Done in follow up CL.

   }
+
+  voice_channel_->SignalDestroyRtcpTransport.connect(
+      this, &WebRtcSession::OnDestroyRtcpTransport_n);

[pthatcher1, 2017/01/13 21:51:07]

This is funky.  I think it would be better to pass the BaseChannel something it
can call rather than have a callback like this.  See my
RtpTransportPairInterface idea below.

[Taylor Brandstetter, 2017/01/13 23:46:48]

I think a signal is the right approach here for minimizing interdependencies.
But I changed the signal name to "SignalRtcpMuxFullyActive", which makes this
code make more sense.

   if (require_rtcp_mux) {
     voice_channel_->ActivateRtcpMux();
   }
 
   voice_channel_->SignalDtlsSrtpSetupFailure.connect(
       this, &WebRtcSession::OnDtlsSrtpSetupFailure);
 
   SignalVoiceChannelCreated();
   voice_channel_->SignalSentPacket.connect(this,
                                            &WebRtcSession::OnSentPacket_w);
   return true;
 }
 
 bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content,
                                        const std::string* bundle_transport) {
   bool require_rtcp_mux =
       rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
   bool create_rtcp_transport_channel = !require_rtcp_mux;
+
+  std::string transport_name =
+      bundle_transport ? *bundle_transport : content->name;
+
+  cricket::TransportChannel* rtp_transport =
+      transport_controller_->CreateTransportChannel(
+          transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  cricket::TransportChannel* rtcp_transport = nullptr;
+  if (create_rtcp_transport_channel) {
+    rtcp_transport = transport_controller_->CreateTransportChannel(
+        transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+  }

[pthatcher1, 2017/01/13 21:51:07]

To make it easier to construct and destroy and pass around, would it make sense
to do something like this?

class RtpTransportPairInterface {
  virtual ~RtpTransportPairInterface();
  virtual TransportChannel* rtp_transport() = 0;
  virtual TransportChannel* rtcp_transport() = 0;
  virtual void DestroyRtcpTransport();
}

class WebRtcSession::RtpTransportPair : RtpTransportPairInterface {
  RtpTransportPair(TransportController* troller, const std::string&
transport_name, create_rtcp) : transport_controller_(troller),
transport_name_(transport_name) {
    rtp_transport_ =
transport_controller_->CreateTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTP);
    if(create_rtcp) {
      rtcp_transport_ =
transport_controller_->CreateTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
    }    
  }

  ~RtpTransportPair() { 
    transport_controller_->DestroyTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTP);
    if(rtcp_transport_) {
      transport_controller_->DestroyTransportChannel(transport_name_,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
    }
  }

  TransportChannel* rtp_transport() override {
    return rtp_transport_;
  }
  TransportChannel* rtcp_transport() override {
    return rtcp_transport_;
  }

  void DestroyRtcpTransport() override {
  }

  TransportController* transport_controller_ = nullptr;
  std::string transport_name_;
  TransportChannel* rtp_transport_ = nullptr;
  TransportChannel* rtcp_transport_ = nullptr;
}

std::unique_ptr<RtpTransportPairInterface> transports(new
WebRtcSession::RtpTransportPair(transport_controller_, transport_name));
video_channel_.reset(channel_manager->CreateVideoChannel(media_controller_,
std::move(transport)));


Now:
1.  You only have to pass in one thing, not two.
2.  That thing is clearly owned by BaseChannel, and once deleted cleans up the
transports.
3.  If video_channel_ fails to be created, the transports are automatically
deleted.
4.  There's no code duplication.
5.  You don't have this weird "SignalDestroyRtcpTransport".  The BaseChannel can
just call that on the RtpTransportPairInterface that it owns.

[Taylor Brandstetter, 2017/01/13 23:46:47]

This is something that would be handled elegantly when BaseChannel is refactored
into RtpTransceiver/RtpTransport, but for now I thought we just wanted to do the
minimum changes that will let us implement the shim objects.

+
   video_channel_.reset(channel_manager_->CreateVideoChannel(
-      media_controller_, transport_controller_.get(), content->name,
+      media_controller_, rtp_transport, rtcp_transport,
+      transport_controller_->signaling_thread(), content->name,
       bundle_transport, create_rtcp_transport_channel, SrtpRequired(),
       video_options_));
+
   if (!video_channel_) {
     return false;
   }
+
+  video_channel_->SignalDestroyRtcpTransport.connect(
+      this, &WebRtcSession::OnDestroyRtcpTransport_n);
   if (require_rtcp_mux) {
     video_channel_->ActivateRtcpMux();
   }
   video_channel_->SignalDtlsSrtpSetupFailure.connect(
       this, &WebRtcSession::OnDtlsSrtpSetupFailure);
 
   SignalVideoChannelCreated();
   video_channel_->SignalSentPacket.connect(this,
                                            &WebRtcSession::OnSentPacket_w);
   return true;
@@ skipping 20 matching lines @@
     }
     if (!network_thread_->Invoke<bool>(
             RTC_FROM_HERE, rtc::Bind(&WebRtcSession::CreateSctpTransport_n,
                                      this, content->name, transport_name))) {
       return false;
     };
   } else {
     bool require_rtcp_mux =
         rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
     bool create_rtcp_transport_channel = !sctp && !require_rtcp_mux;
+
+    std::string transport_name =
+        bundle_transport ? *bundle_transport : content->name;
+    cricket::TransportChannel* rtp_transport =
+        transport_controller_->CreateTransportChannel(
+            transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+    cricket::TransportChannel* rtcp_transport = nullptr;
+    if (create_rtcp_transport_channel) {
+      rtcp_transport = transport_controller_->CreateTransportChannel(
+          transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+    }
+
     rtp_data_channel_.reset(channel_manager_->CreateRtpDataChannel(
-        media_controller_, transport_controller_.get(), content->name,
+        media_controller_, rtp_transport, rtcp_transport,
+        transport_controller_->signaling_thread(), content->name,
         bundle_transport, create_rtcp_transport_channel, SrtpRequired()));
+
     if (!rtp_data_channel_) {
       return false;
     }
+
+    rtp_data_channel_->SignalDestroyRtcpTransport.connect(
+        this, &WebRtcSession::OnDestroyRtcpTransport_n);
+
     if (require_rtcp_mux) {
       rtp_data_channel_->ActivateRtcpMux();
     }
     rtp_data_channel_->SignalDtlsSrtpSetupFailure.connect(
         this, &WebRtcSession::OnDtlsSrtpSetupFailure);
     rtp_data_channel_->SignalSentPacket.connect(this,
                                                 &WebRtcSession::OnSentPacket_w);
   }
 
   SignalDataChannelCreated();
+
   return true;
 }
 
 std::unique_ptr<SessionStats> WebRtcSession::GetStats_n(
     const ChannelNamePairs& channel_name_pairs) {
   RTC_DCHECK(network_thread()->IsCurrent());
   std::unique_ptr<SessionStats> session_stats(new SessionStats());
   for (const auto channel_name_pair : { &channel_name_pairs.voice,
                                         &channel_name_pairs.video,
                                         &channel_name_pairs.data }) {
@@ skipping 431 matching lines @@
       if (content_name == *sctp_content_name_) {
         return *sctp_transport_name_;
       }
     }
     // Return an empty string if failed to retrieve the transport name.
     return "";
   }
   return channel->transport_name();
 }
 
+void WebRtcSession::DestroyTransport(const std::string& transport_name,
+                                     int component) {
+  network_thread_->Invoke<void>(
+      RTC_FROM_HERE,
+      rtc::Bind(&cricket::TransportController::DestroyTransportChannel_n,
+                transport_controller_.get(), transport_name, component));

[pthatcher1, 2017/01/13 21:51:07]

You added code to TransportController to do this thread hop.  You might as well
add DestroyTransportChannel as well.  Then they are both in the same place.  Or
remove both and pass the network_thread_ into the RtpTransportPair and let it do
the thread hop. 

[Taylor Brandstetter, 2017/01/13 23:46:48]

I think this was an artifact of changing things back and forth, I'll make it
consistent with CreateTransportChannel.

+}
+
+void WebRtcSession::OnDestroyRtcpTransport_n(
+    const std::string& transport_name) {
+  ASSERT(network_thread()->IsCurrent());
+  transport_controller_->DestroyTransportChannel_n(
+      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+}
+
+void WebRtcSession::DestroyVideoChannel() {
+  SignalVideoChannelDestroyed();
+  RTC_DCHECK(video_channel_->rtp_transport());
+  std::string transport_name;
+  transport_name = video_channel_->rtp_transport()->transport_name();
+  bool need_to_delete_rtcp = (video_channel_->rtcp_transport() != nullptr);
+  channel_manager_->DestroyVideoChannel(video_channel_.release());
+  DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  if (need_to_delete_rtcp) {
+    DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+  }
+}
+
+void WebRtcSession::DestroyVoiceChannel() {
+  SignalVoiceChannelDestroyed();
+  RTC_DCHECK(voice_channel_->rtp_transport());
+  std::string transport_name;
+  transport_name = voice_channel_->rtp_transport()->transport_name();
+  bool need_to_delete_rtcp = (voice_channel_->rtcp_transport() != nullptr);
+  channel_manager_->DestroyVoiceChannel(voice_channel_.release());
+  DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  if (need_to_delete_rtcp) {
+    DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+  }
+}
+
+void WebRtcSession::DestroyDataChannel() {
+  SignalDataChannelDestroyed();
+  RTC_DCHECK(rtp_data_channel_->rtp_transport());
+  std::string transport_name;
+  transport_name = rtp_data_channel_->rtp_transport()->transport_name();
+  bool need_to_delete_rtcp = (rtp_data_channel_->rtcp_transport() != nullptr);
+  channel_manager_->DestroyRtpDataChannel(rtp_data_channel_.release());
+  DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  if (need_to_delete_rtcp) {
+    DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+  }

[pthatcher1, 2017/01/13 21:51:07]

I think most of this goes away if the BaseChannel just deletes the thing you
passed it, and that deletes the transports (such as my RtpTransporPair idea).

+}
 }  // namespace webrtc