Combining "SetTransportChannel" and "SetRtcpTransportChannel".

webrtc/pc/channel.cc

 /*
  *  Copyright 2004 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 282 matching lines @@
     // DTLS-SRTP when |writable_| becomes true again.
     writable_ = false;
     srtp_filter_.ResetParams();
   }
 
   // If this BaseChannel uses RTCP and we haven't fully negotiated RTCP mux,
   // we need an RTCP channel.
   if (rtcp_enabled_ && !rtcp_mux_filter_.IsFullyActive()) {
     LOG(LS_INFO) << "Create RTCP TransportChannel for " << content_name()
                  << " on " << transport_name << " transport ";
-    // TODO(deadbeef): Remove this grossness when we remove non-muxed RTCP.
-    SetRtcpTransportChannel_n(
-        transport_controller_->CreateTransportChannel_n(
-            transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP),
-        false /* update_writablity */);
+    SetTransportChannel_n(
+        true, transport_controller_->CreateTransportChannel_n(
+                  transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP));
     if (!rtcp_transport_channel_) {
       return false;
     }
   }
 
-  // We're not updating the writablity during the transition state.
-  SetTransportChannel_n(transport_controller_->CreateTransportChannel_n(
-      transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  LOG(LS_INFO) << "Create non-RTCP TransportChannel for " << content_name()
+               << " on " << transport_name << " transport ";
+  SetTransportChannel_n(
+      false, transport_controller_->CreateTransportChannel_n(
+                 transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP));
   if (!transport_channel_) {
     return false;
   }
 
-  // TODO(deadbeef): Remove this grossness when we remove non-muxed RTCP.
-  if (rtcp_transport_channel_) {
-    // We can only update the RTCP ready to send after set_transport_channel has
-    // handled channel writability.
-    SetTransportChannelReadyToSend(true, rtcp_transport_channel_->writable());
-  }
   transport_name_ = transport_name;
-  return true;
-}
-
-void BaseChannel::SetTransportChannel_n(TransportChannel* new_tc) {
-  RTC_DCHECK(network_thread_->IsCurrent());
-
-  TransportChannel* old_tc = transport_channel_;
-  if (!old_tc && !new_tc) {
-    // Nothing to do.
-    return;
-  }
-  RTC_DCHECK(old_tc != new_tc);
-
-  if (old_tc) {
-    DisconnectFromTransportChannel(old_tc);
-    transport_controller_->DestroyTransportChannel_n(
-        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 between RTP and RTCP upon
   // setting new transport channels.
   UpdateWritableState_n();
+  // We can only update ready-to-send after updating writability.
+  //
   // On setting a new channel, assume it's ready to send if it's writable,
   // because we have no way of knowing otherwise (the channel doesn't give us
   // "was last send successful?").
   //
   // This won't always be accurate (the last SendPacket call from another
   // BaseChannel could have resulted in an error), but even so, we'll just
   // encounter the error again and update "ready to send" accordingly.
-  SetTransportChannelReadyToSend(false, new_tc && new_tc->writable());
+  SetTransportChannelReadyToSend(
+      false, transport_channel_ && transport_channel_->writable());
+  SetTransportChannelReadyToSend(
+      true, rtcp_transport_channel_ && rtcp_transport_channel_->writable());
+  return true;
 }
 
-void BaseChannel::SetRtcpTransportChannel_n(TransportChannel* new_tc,
-                                            bool update_writablity) {
+void BaseChannel::SetTransportChannel_n(bool rtcp,
+                                        TransportChannel* new_channel) {
   RTC_DCHECK(network_thread_->IsCurrent());
+  TransportChannel*& old_channel =
+      rtcp ? rtcp_transport_channel_ : transport_channel_;
 
-  TransportChannel* old_tc = rtcp_transport_channel_;
-  if (!old_tc && !new_tc) {
+  if (!old_channel && !new_channel) {
     // Nothing to do.
     return;
   }
-  RTC_DCHECK(old_tc != new_tc);
+  RTC_DCHECK(old_channel != new_channel);
 
-  if (old_tc) {
-    DisconnectFromTransportChannel(old_tc);
+  if (old_channel) {
+    DisconnectFromTransportChannel(old_channel);
     transport_controller_->DestroyTransportChannel_n(
-        transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
+        transport_name_, rtcp ? cricket::ICE_CANDIDATE_COMPONENT_RTCP
+                              : cricket::ICE_CANDIDATE_COMPONENT_RTP);
   }
 
-  rtcp_transport_channel_ = new_tc;
+  old_channel = new_channel;
 
-  if (new_tc) {
-    RTC_CHECK(!(ShouldSetupDtlsSrtp_n() && srtp_filter_.IsActive()))
-        << "Setting RTCP for DTLS/SRTP after SrtpFilter is active "
-        << "should never happen.";
-    ConnectToTransportChannel(new_tc);
+  if (new_channel) {
+    if (rtcp) {
+      RTC_CHECK(!(ShouldSetupDtlsSrtp_n() && srtp_filter_.IsActive()))
+          << "Setting RTCP for DTLS/SRTP after SrtpFilter is active "
+          << "should never happen.";
+    }
+    ConnectToTransportChannel(new_channel);
     for (const auto& pair : rtcp_socket_options_) {
-      new_tc->SetOption(pair.first, pair.second);
+      new_channel->SetOption(pair.first, pair.second);
     }
   }
-
-  if (update_writablity) {
-    // Update aggregate writable/ready-to-send state between RTP and RTCP upon
-    // setting new channel
-    UpdateWritableState_n();
-    // On setting a new channel, assume it's ready to send if it's writable,
-    // because we have no way of knowing otherwise (the channel doesn't give us
-    // "was last send successful?").
-    //
-    // This won't always be accurate (the last SendPacket call from another
-    // BaseChannel could have resulted in an error), but even so, we'll just
-    // encounter the error again and update "ready to send" accordingly.
-    SetTransportChannelReadyToSend(true, new_tc && new_tc->writable());
-  }
 }
 
 void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   tc->SignalWritableState.connect(this, &BaseChannel::OnWritableState);
   tc->SignalReadPacket.connect(this, &BaseChannel::OnChannelRead);
   tc->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend);
   tc->SignalDtlsState.connect(this, &BaseChannel::OnDtlsState);
   tc->SignalSelectedCandidatePairChanged.connect(
@@ skipping 785 matching lines @@
 }
 
 void BaseChannel::ActivateRtcpMux() {
   network_thread_->Invoke<void>(RTC_FROM_HERE,
                                 Bind(&BaseChannel::ActivateRtcpMux_n, this));
 }
 
 void BaseChannel::ActivateRtcpMux_n() {
   if (!rtcp_mux_filter_.IsActive()) {
     rtcp_mux_filter_.SetActive();
-    SetRtcpTransportChannel_n(nullptr, true);
+    SetTransportChannel_n(true, nullptr);
+    // Update aggregate writable/ready-to-send state between RTP and RTCP upon
+    // removing channel.
+    UpdateWritableState_n();
+    SetTransportChannelReadyToSend(true, false);
   }
 }
 
 bool BaseChannel::SetRtcpMux_n(bool enable,
                                ContentAction action,
                                ContentSource src,
                                std::string* error_desc) {
   bool ret = false;
   switch (action) {
     case CA_OFFER:
       ret = rtcp_mux_filter_.SetOffer(enable, src);
       break;
     case CA_PRANSWER:
       // This may activate RTCP muxing, but we don't yet destroy the channel
       // because the final answer may deactivate it.
       ret = rtcp_mux_filter_.SetProvisionalAnswer(enable, src);
       break;
     case CA_ANSWER:
       ret = rtcp_mux_filter_.SetAnswer(enable, src);
       if (ret && rtcp_mux_filter_.IsActive()) {
         // We activated RTCP mux, close down the RTCP transport.
         LOG(LS_INFO) << "Enabling rtcp-mux for " << content_name()
                      << " by destroying RTCP transport channel for "
                      << transport_name();
-        SetRtcpTransportChannel_n(nullptr, true);
+        SetTransportChannel_n(true, nullptr);
+        UpdateWritableState_n();
+        SetTransportChannelReadyToSend(true, false);

[skvlad, 2016/08/26 23:23:13]

Please add comments for what the "true" /* rtcp */ and "false" /* ready_to_send
*/ parameters mean.

In general the use of a boolean to choose between the RTP and RTCP channels in
this file makes it very hard to read. Perhaps we can use an enum, or group the
channel states together into a struct (one for RTP and one for RTCP) and pass
around a pointer to that struct. But this is not related to this CL, please
don't spend time on it yet.

[Taylor Brandstetter, 2016/08/26 23:29:39]

I agree. I was planning to use an enum; the struct idea is good too.

       }
       break;
     case CA_UPDATE:
       // No RTCP mux info.
       ret = true;
       break;
     default:
       break;
   }
   if (!ret) {
@@ skipping 1189 matching lines @@
 }
 
 void DataChannel::OnStreamClosedRemotely(uint32_t sid) {
   rtc::TypedMessageData<uint32_t>* message =
       new rtc::TypedMessageData<uint32_t>(sid);
   signaling_thread()->Post(RTC_FROM_HERE, this, MSG_STREAMCLOSEDREMOTELY,
                            message);
 }
 
 }  // namespace cricket