TransportController refactoring

talk/session/media/channel.cc

Index: talk/session/media/channel.cc
diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc
index dc7a478510ccd0fb1eb482005b715467bf64081f..8fb26b9491289d1724b1350485d96aad96320b76 100644
--- a/talk/session/media/channel.cc
+++ b/talk/session/media/channel.cc
@@ -60,11 +60,6 @@ static const char kDtlsSrtpExporterLabel[] = "EXTRACTOR-dtls_srtp";
 
 static const int kAgcMinus10db = -10;
 
-static void SetSessionError(BaseSession* session, BaseSession::Error error,
-                            const std::string& error_desc) {
-  session->SetError(error, error_desc);
-}
-
 static void SafeSetError(const std::string& message, std::string* error_desc) {
   if (error_desc) {
     *error_desc = message;
@@ -151,10 +146,12 @@ static const MediaContentDescription* GetContentDescription(
 }
 
 BaseChannel::BaseChannel(rtc::Thread* thread,
-                         MediaChannel* media_channel, BaseSession* session,
-                         const std::string& content_name, bool rtcp)
+                         MediaChannel* media_channel,
+                         TransportController* transport_controller,
+                         const std::string& content_name,
+                         bool rtcp)
     : worker_thread_(thread),
-      session_(session),
+      transport_controller_(transport_controller),
       media_channel_(media_channel),
       content_name_(content_name),
       rtcp_(rtcp),
@@ -191,7 +188,7 @@ BaseChannel::~BaseChannel() {
 }
 
 bool BaseChannel::Init() {
-  if (!SetTransportChannels(session(), rtcp())) {
+  if (!SetTransportChannels(content_name(), rtcp())) {
     return false;
   }
 
@@ -212,28 +209,42 @@ void BaseChannel::Deinit() {
   media_channel_->SetInterface(NULL);
 }
 
-bool BaseChannel::SetTransportChannels(BaseSession* session, bool rtcp) {
+bool BaseChannel::SetTransport(const std::string& transport_name) {
+  // Only enable an RTCP transport channel if we already had one.
+  bool rtcp = (rtcp_transport_channel() != nullptr);
+  if (!SetTransportChannels(transport_name, rtcp)) {
+    return false;
+  }
+  return true;
+}
+
+bool BaseChannel::SetTransportChannels(
+    const std::string& transport_name, bool rtcp) {
   return worker_thread_->Invoke<bool>(Bind(
-      &BaseChannel::SetTransportChannels_w, this, session, rtcp));
+      &BaseChannel::SetTransportChannels_w, this, transport_name, rtcp));
 }
 
-bool BaseChannel::SetTransportChannels_w(BaseSession* session, bool rtcp) {
+bool BaseChannel::SetTransportChannels_w(
+    const std::string& transport_name, bool rtcp) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
-  set_transport_channel(session->CreateChannel(
-      content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  set_transport_channel(transport_controller_->CreateTransportChannel_w(
+      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
   if (!transport_channel()) {
     return false;
   }
   if (rtcp) {
-    set_rtcp_transport_channel(session->CreateChannel(
-        content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTCP));
+    LOG(LS_INFO) << "Create RTCP TransportChannel for " << content_name()
+                 << " on " << transport_name << " transport ";
+    set_rtcp_transport_channel(transport_controller_->CreateTransportChannel_w(
+        transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP));
     if (!rtcp_transport_channel()) {
       return false;
     }
   } else {
     set_rtcp_transport_channel(nullptr);
   }
+  transport_name_ = transport_name;
 
   return true;
 }
@@ -244,18 +255,29 @@ void BaseChannel::set_transport_channel(TransportChannel* new_tc) {
   TransportChannel* old_tc = transport_channel_;
 
   if (old_tc == new_tc) {
+    if (old_tc) {
+      // Need to make sure ref count is decremented
+      transport_controller_->DestroyTransportChannel_w(
+          transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+    }

[pthatcher1, 2015/08/10 20:40:17]

This doesn't seem right.  If we set_transport_channel(transport_channel()), it
will decrement the ref count?  Then you could keep doing that until it explodes.

[Taylor Brandstetter, 2015/08/11 01:20:07]

I did this because "CreateTransportChannel_w" above will increment the ref
count. But probably a better solution would be to not call
CreateTransportChannel_w in the first place unless necessary. I added that logic
to SetTransportChannels_w.

[pthatcher1, 2015/08/18 22:32:46]

I think it's right to decrement the pointer of the old_tc if old_tc != new_tc. 
But if they are the same and we return early, we shouldn't change the ref count.

     return;
   }
   if (old_tc) {
     DisconnectFromTransportChannel(old_tc);
-    session()->DestroyChannel(
-        content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTP);
+    transport_controller_->DestroyTransportChannel_w(
+        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);
   }
 
   transport_channel_ = new_tc;
 
   if (new_tc) {
     ConnectToTransportChannel(new_tc);
+    for (const auto& pair : socket_options_) {
+      new_tc->SetOption(pair.first, pair.second);
+    }
+
+    OnWritableState(new_tc);
+    SetReadyToSend(new_tc, new_tc->writable());

[pthatcher1, 2015/08/10 20:40:17]

Can you put in a nice comment about why we need to set the socket options and
update the writeable and "ready to send" states when we get a new transport
channel?

[Taylor Brandstetter, 2015/08/11 01:20:07]

Done.

   }
 }
 
@@ -269,14 +291,20 @@ void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc) {
   }
   if (old_tc) {
     DisconnectFromTransportChannel(old_tc);
-    session()->DestroyChannel(
-        content_name(), cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+    transport_controller_->DestroyTransportChannel_w(
+        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
   }
 
   rtcp_transport_channel_ = new_tc;
 
   if (new_tc) {
     ConnectToTransportChannel(new_tc);
+    for (const auto& pair : rtcp_socket_options_) {
+      new_tc->SetOption(pair.first, pair.second);
+    }
+
+    OnWritableState(new_tc);
+    SetReadyToSend(new_tc, new_tc->writable());
   }
 }
 
@@ -396,9 +424,13 @@ int BaseChannel::SetOption(SocketType type, rtc::Socket::Option opt,
   switch (type) {
     case ST_RTP:
       channel = transport_channel_;
+      socket_options_.push_back(
+          std::pair<rtc::Socket::Option, int>(opt, value));
       break;
     case ST_RTCP:
       channel = rtcp_transport_channel_;
+      rtcp_socket_options_.push_back(
+          std::pair<rtc::Socket::Option, int>(opt, value));
       break;
   }
   return channel ? channel->SetOption(opt, value) : -1;
@@ -745,13 +777,13 @@ void BaseChannel::ChannelWritable_w() {
   // If we're doing DTLS-SRTP, now is the time.
   if (!was_ever_writable_ && ShouldSetupDtlsSrtp()) {
     if (!SetupDtlsSrtp(false)) {
-      SignalDtlsSetupFailure(this, false);
+      SignalDtlsSetupFailure_w(false);
       return;
     }
 
     if (rtcp_transport_channel_) {
       if (!SetupDtlsSrtp(true)) {
-        SignalDtlsSetupFailure(this, true);
+        SignalDtlsSetupFailure_w(true);
         return;
       }
     }
@@ -794,8 +826,8 @@ bool BaseChannel::ShouldSetupDtlsSrtp() const {
 bool BaseChannel::SetupDtlsSrtp(bool rtcp_channel) {
   bool ret = false;
 
-  TransportChannel *channel = rtcp_channel ?
-      rtcp_transport_channel_ : transport_channel_;
+  TransportChannel* channel = rtcp_channel ? rtcp_transport_channel_
+                                           : transport_channel_;
 
   // No DTLS
   if (!channel->IsDtlsActive())
@@ -1001,7 +1033,7 @@ void BaseChannel::ActivateRtcpMux() {
 void BaseChannel::ActivateRtcpMux_w() {
   if (!rtcp_mux_filter_.IsActive()) {
     rtcp_mux_filter_.SetActive();
-    set_rtcp_transport_channel(NULL);
+    set_rtcp_transport_channel(nullptr);
   }
 }
 
@@ -1020,7 +1052,10 @@ bool BaseChannel::SetRtcpMux_w(bool enable, ContentAction action,
       ret = rtcp_mux_filter_.SetAnswer(enable, src);
       if (ret && rtcp_mux_filter_.IsActive()) {
         // We activated RTCP mux, close down the RTCP transport.
-        set_rtcp_transport_channel(NULL);
+        LOG(LS_INFO) << "Enabling rtcp-mux for " << content_name()
+                     << " by destroying RTCP transport channel for "
+                     << transport_name();
+        set_rtcp_transport_channel(nullptr);
       }
       break;
     case CA_UPDATE:
@@ -1290,12 +1325,11 @@ void BaseChannel::FlushRtcpMessages() {
 VoiceChannel::VoiceChannel(rtc::Thread* thread,
                            MediaEngineInterface* media_engine,
                            VoiceMediaChannel* media_channel,
-                           BaseSession* session,
+                           TransportController* transport_controller,
                            const std::string& content_name,
                            bool rtcp)
-    : BaseChannel(thread, media_channel, session, content_name,
+    : BaseChannel(thread, media_channel, transport_controller, content_name,
                   rtcp),
-      media_engine_(media_engine),

[pthatcher1, 2015/08/10 20:40:17]

Do we not need to set the media engine any more?

[Taylor Brandstetter, 2015/08/11 01:20:07]

Good catch... I must have deleted that line on accident when handling a merge
conflict.

       received_media_(false) {
 }
 
@@ -1685,10 +1719,10 @@ void VoiceChannel::GetSrtpCiphers(std::vector<std::string>* ciphers) const {
 
 VideoChannel::VideoChannel(rtc::Thread* thread,
                            VideoMediaChannel* media_channel,
-                           BaseSession* session,
+                           TransportController* transport_controller,
                            const std::string& content_name,
                            bool rtcp)
-    : BaseChannel(thread, media_channel, session, content_name,
+    : BaseChannel(thread, media_channel, transport_controller, content_name,
                   rtcp),
       renderer_(NULL),
       previous_we_(rtc::WE_CLOSE) {
@@ -2123,10 +2157,11 @@ void VideoChannel::GetSrtpCiphers(std::vector<std::string>* ciphers) const {
 
 DataChannel::DataChannel(rtc::Thread* thread,
                          DataMediaChannel* media_channel,
-                         BaseSession* session,
+                         TransportController* transport_controller,
                          const std::string& content_name,
                          bool rtcp)
-    : BaseChannel(thread, media_channel, session, content_name, rtcp),
+    : BaseChannel(thread, media_channel, transport_controller, content_name,
+                  rtcp),
       data_channel_type_(cricket::DCT_NONE),
       ready_to_send_data_(false) {
 }