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..882948185a2ebdcf711b6d3a7f848137d566a9c7 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),
@@ -185,13 +182,21 @@ BaseChannel::~BaseChannel() {
   // the media channel may try to send on the dead transport channel. NULLing
   // is not an effective strategy since the sends will come on another thread.
   delete media_channel_;
-  set_transport_channel(nullptr);
-  set_rtcp_transport_channel(nullptr);
+  if (transport_channel_) {
+    DisconnectFromTransportChannel(transport_channel_);
+    transport_controller_->DestroyTransportChannel_w(
+        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP);
+  }
+  if (rtcp_transport_channel_) {
+    DisconnectFromTransportChannel(rtcp_transport_channel_);
+    transport_controller_->DestroyTransportChannel_w(
+        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+  }
   LOG(LS_INFO) << "Destroyed channel";
 }
 
 bool BaseChannel::Init() {
-  if (!SetTransportChannels(session(), rtcp())) {
+  if (!SetTransportChannels(content_name(), rtcp())) {
     return false;
   }
 
@@ -212,28 +217,47 @@ void BaseChannel::Deinit() {
   media_channel_->SetInterface(NULL);
 }
 
-bool BaseChannel::SetTransportChannels(BaseSession* session, bool rtcp) {
-  return worker_thread_->Invoke<bool>(Bind(
-      &BaseChannel::SetTransportChannels_w, this, session, 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, 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));
-  if (!transport_channel()) {
-    return false;
+  if (transport_name != transport_name_) {
+    set_transport_channel(transport_controller_->CreateTransportChannel_w(

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

Even though it's called "create", it's more like "create if necessary; if
already created, increment the ref count".  We need all of the BaseChannels to
increment the refcount.  Otherwise, it might get deleted while one of them is
still using it.

In other words, I think this if statement is wrong and we should always call
CreateTransportChannel.

[Taylor Brandstetter, 2015/08/25 01:04:05]

This just avoids incrementing the ref count pointlessly. If transport_name ==
transport_name_, we can be sure that we already HAVE a transport channel
identical to the one about to be given to us, and we ALREADY added to its ref
count. Imagine if you called:

SetTransportChannels("audio", true);
SetTransportChannels("audio", false);

On the first call we would create the RTP and RTCP transport channels. On the
second, we'd destroy the RTCP channel. But there'd be no need to add a ref count
to the RTP channel since it's not changing. Or at least, that's how I understood
it.

[pthatcher1, 2015/08/25 18:40:38]

If you call:

SetTransportChannels("audio", true);
SetTransportChannels("audio", false);

Assuming we are not bundling and not using RTCP-mux, then two separate transport
channels will be created, and each will need their ref count incremented.  It's
the transport channel getting incremented, not the transport.  

[Taylor Brandstetter, 2015/08/25 20:39:54]

*Initially* we'll add a ref count to each channel, yes. But on the second call
('SetTransportChannels("audio", false);'), there's no need to add a ref count to
the RTP channel, since we already have a reference to it, and already
incremented its ref count. If we did that, then when destroying the BaseChannel,
the RTP channel would be left with a ref count of 1, and wouldn't be destroyed
even though nothing is referencing it.

[pthatcher1, 2015/08/25 21:19:49]

OK, I think I get what you're saying now.  I misread your example as something
else the last time.  And I must have misread the code the first time through,
because it makes sense to me this time.

What you're saying is if we basically enable bundle twice (with the same
transport name), we need to make sure we don't increment the refcount twice.  

I think we could write that in a slightly more readable way (that a slow code
reader like myself can handle) with an early return.  How about something like
this?


if (!rtcp_transport_channel_ && rtcp) {
  LOG(LS_WARNING) << "Ignoring request for RTCP transport channel because we
don't support disabling RTCP-mux once enabled."
  return false;
}

if (transport_name == transport_name_) {
  // Nothing to do
  return true;
}

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

[Taylor Brandstetter, 2015/08/27 22:15:57]

I followed your suggestion and also removed the "rtcp" parameter from this
function so it becomes even more clear (I hope).

+        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));
-    if (!rtcp_transport_channel()) {
-      return false;
+    if (!rtcp_transport_channel_ || transport_name != transport_name_) {
+      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;
 }
@@ -242,42 +266,48 @@ void BaseChannel::set_transport_channel(TransportChannel* new_tc) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   TransportChannel* old_tc = transport_channel_;
-
-  if (old_tc == new_tc) {
-    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);
+    }
   }
+
+  // Update aggregate writable/ready-to-send state upon setting new channel

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

It's not really aggregate.  It's more like inherited.

[Taylor Brandstetter, 2015/08/25 01:04:05]

Well we have to aggregate the writable state from RTP and RTCP channels (at
least until RTCP-mux is required).

[pthatcher1, 2015/08/25 18:40:38]

That's a good point.  Maybe mention that in the comment, so we can update it
when we remove RTCP-non-mux (just say "aggregate writable/ready-to-send state
between RTP and RTCP").

[Taylor Brandstetter, 2015/08/25 20:39:54]

Done.

+  UpdateWritableState_w();
+  SetReadyToSend(false, new_tc && new_tc->writable());
 }
 
 void BaseChannel::set_rtcp_transport_channel(TransportChannel* new_tc) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   TransportChannel* old_tc = rtcp_transport_channel_;
-
-  if (old_tc == new_tc) {
-    return;
-  }
   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);
+    }
   }
+
+  // Update aggregate writable/ready-to-send state upon setting new channel
+  UpdateWritableState_w();
+  SetReadyToSend(true, new_tc && new_tc->writable());
 }
 
 void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) {
@@ -396,9 +426,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;
@@ -406,12 +440,7 @@ int BaseChannel::SetOption(SocketType type, rtc::Socket::Option opt,
 
 void BaseChannel::OnWritableState(TransportChannel* channel) {
   ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_);
-  if (transport_channel_->writable()
-      && (!rtcp_transport_channel_ || rtcp_transport_channel_->writable())) {
-    ChannelWritable_w();
-  } else {
-    ChannelNotWritable_w();
-  }
+  UpdateWritableState_w();
 }
 
 void BaseChannel::OnChannelRead(TransportChannel* channel,
@@ -429,26 +458,25 @@ void BaseChannel::OnChannelRead(TransportChannel* channel,
 }
 
 void BaseChannel::OnReadyToSend(TransportChannel* channel) {
-  SetReadyToSend(channel, true);
+  ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_);
+  SetReadyToSend(channel == rtcp_transport_channel_, true);
 }
 
-void BaseChannel::SetReadyToSend(TransportChannel* channel, bool ready) {
-  ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_);
-  if (channel == transport_channel_) {
-    rtp_ready_to_send_ = ready;
-  }
-  if (channel == rtcp_transport_channel_) {
+void BaseChannel::SetReadyToSend(bool rtcp, bool ready) {
+  if (rtcp) {
     rtcp_ready_to_send_ = ready;
+  } else {
+    rtp_ready_to_send_ = ready;
   }
 
-  if (!ready) {
-    // Notify the MediaChannel when either rtp or rtcp channel can't send.
-    media_channel_->OnReadyToSend(false);
-  } else if (rtp_ready_to_send_ &&
-             // In the case of rtcp mux |rtcp_transport_channel_| will be null.
-             (rtcp_ready_to_send_ || !rtcp_transport_channel_)) {
+  if (rtp_ready_to_send_ &&
+      // In the case of rtcp mux |rtcp_transport_channel_| will be null.
+      (rtcp_ready_to_send_ || !rtcp_transport_channel_)) {
     // Notify the MediaChannel when both rtp and rtcp channel can send.
     media_channel_->OnReadyToSend(true);
+  } else {
+    // Notify the MediaChannel when either rtp or rtcp channel can't send.
+    media_channel_->OnReadyToSend(false);
   }
 }
 
@@ -570,7 +598,7 @@ bool BaseChannel::SendPacket(bool rtcp, rtc::Buffer* packet,
   if (ret != static_cast<int>(packet->size())) {
     if (channel->GetError() == EWOULDBLOCK) {
       LOG(LS_WARNING) << "Got EWOULDBLOCK from socket.";
-      SetReadyToSend(channel, false);
+      SetReadyToSend(rtcp, false);
     }
     return false;
   }
@@ -721,14 +749,21 @@ bool BaseChannel::IsStreamMuted_w(uint32 ssrc) {
   return muted_streams_.find(ssrc) != muted_streams_.end();
 }
 
+void BaseChannel::UpdateWritableState_w() {
+  if (transport_channel_ && transport_channel_->writable() &&
+      (!rtcp_transport_channel_ || rtcp_transport_channel_->writable())) {
+    ChannelWritable_w();
+  } else {
+    ChannelNotWritable_w();
+  }
+}
+
 void BaseChannel::ChannelWritable_w() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
   if (writable_)
     return;
 
-  LOG(LS_INFO) << "Channel socket writable ("
-               << transport_channel_->content_name() << ", "
-               << transport_channel_->component() << ")"
+  LOG(LS_INFO) << "Channel writable (" << content_name_ << ")"
                << (was_ever_writable_ ? "" : " for the first time");
 
   std::vector<ConnectionInfo> infos;
@@ -745,13 +780,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 +829,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())
@@ -890,9 +925,7 @@ void BaseChannel::ChannelNotWritable_w() {
   if (!writable_)
     return;
 
-  LOG(LS_INFO) << "Channel socket not writable ("
-               << transport_channel_->content_name() << ", "
-               << transport_channel_->component() << ")";
+  LOG(LS_INFO) << "Channel not writable (" << content_name_ << ")";
   writable_ = false;
   ChangeState();
 }
@@ -1001,7 +1034,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 +1053,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,10 +1326,13 @@ 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),
       received_media_(false) {
@@ -1685,10 +1724,13 @@ 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 +2165,14 @@ 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) {
 }