Move ready to send logic from BaseChannel to RtpTransport.

webrtc/pc/channel.cc

Index: webrtc/pc/channel.cc
diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc
index d45e220d5c7843b9eab9099d7ccdd58d6b4e9dc6..c4f86cee548a2b41c09c8909fe9817b3928d5901 100644
--- a/webrtc/pc/channel.cc
+++ b/webrtc/pc/channel.cc
@@ -168,6 +168,7 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread,
       network_thread_(network_thread),
       signaling_thread_(signaling_thread),
       content_name_(content_name),
+      rtcp_mux_required_(rtcp_mux_required),
       rtp_transport_(rtcp_mux_required),
       srtp_required_(srtp_required),
       media_channel_(media_channel),
@@ -176,6 +177,8 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread,
 #if defined(ENABLE_EXTERNAL_AUTH)
   srtp_filter_.EnableExternalAuth();
 #endif
+  rtp_transport_.SignalReadyToSend.connect(
+      this, &BaseChannel::OnTransportReadyToSend);
   LOG(LS_INFO) << "Created channel for " << content_name;
 }
 
@@ -250,7 +253,7 @@ bool BaseChannel::InitNetwork_n(
       !SetDtlsSrtpCryptoSuites_n(rtcp_dtls_transport_, true)) {
     return false;
   }
-  if (rtp_transport_.rtcp_mux_required()) {
+  if (rtcp_mux_required_) {
     rtcp_mux_filter_.SetActive();
   }
   return true;
@@ -343,19 +346,6 @@ void BaseChannel::SetTransports_n(
   // 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, rtp_packet_transport && rtp_packet_transport->writable());
-  SetTransportChannelReadyToSend(
-      true, rtcp_packet_transport && rtcp_packet_transport->writable());
 }
 
 void BaseChannel::SetTransport_n(
@@ -382,9 +372,9 @@ void BaseChannel::SetTransport_n(
   }
 
   if (rtcp) {
-    rtp_transport_.set_rtcp_packet_transport(new_packet_transport);
+    rtp_transport_.SetRtcpPacketTransport(new_packet_transport);
   } else {
-    rtp_transport_.set_rtp_packet_transport(new_packet_transport);
+    rtp_transport_.SetRtpPacketTransport(new_packet_transport);
   }
   old_dtls_transport = new_dtls_transport;
 
@@ -398,6 +388,7 @@ void BaseChannel::SetTransport_n(
         << "Setting RTCP for DTLS/SRTP after SrtpFilter is active "
         << "should never happen.";
   }
+
   if (new_dtls_transport) {
     ConnectToDtlsTransport(new_dtls_transport);
   } else {
@@ -412,9 +403,9 @@ void BaseChannel::SetTransport_n(
 void BaseChannel::ConnectToDtlsTransport(DtlsTransportInternal* transport) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
+  // TODO(zstein): de-dup with ConnectToPacketTransport
   transport->SignalWritableState.connect(this, &BaseChannel::OnWritableState);
   transport->SignalReadPacket.connect(this, &BaseChannel::OnPacketRead);
-  transport->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend);
   transport->SignalDtlsState.connect(this, &BaseChannel::OnDtlsState);
   transport->SignalSentPacket.connect(this, &BaseChannel::SignalSentPacket_n);
   transport->ice_transport()->SignalSelectedCandidatePairChanged.connect(
@@ -429,7 +420,6 @@ void BaseChannel::DisconnectFromDtlsTransport(
 
   transport->SignalWritableState.disconnect(this);
   transport->SignalReadPacket.disconnect(this);
-  transport->SignalReadyToSend.disconnect(this);
   transport->SignalDtlsState.disconnect(this);
   transport->SignalSentPacket.disconnect(this);
   transport->ice_transport()->SignalSelectedCandidatePairChanged.disconnect(
@@ -441,7 +431,6 @@ void BaseChannel::ConnectToPacketTransport(
   RTC_DCHECK_RUN_ON(network_thread_);
   transport->SignalWritableState.connect(this, &BaseChannel::OnWritableState);
   transport->SignalReadPacket.connect(this, &BaseChannel::OnPacketRead);
-  transport->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend);
   transport->SignalSentPacket.connect(this, &BaseChannel::SignalSentPacket_n);
 }
 
@@ -450,7 +439,6 @@ void BaseChannel::DisconnectFromPacketTransport(
   RTC_DCHECK_RUN_ON(network_thread_);
   transport->SignalWritableState.disconnect(this);
   transport->SignalReadPacket.disconnect(this);
-  transport->SignalReadyToSend.disconnect(this);
   transport->SignalSentPacket.disconnect(this);
 }
 
@@ -530,8 +518,7 @@ bool BaseChannel::GetConnectionStats(ConnectionInfos* infos) {
 bool BaseChannel::NeedsRtcpTransport() {
   // If this BaseChannel doesn't require RTCP mux and we haven't fully
   // negotiated RTCP mux, we need an RTCP transport.
-  return !rtp_transport_.rtcp_mux_required() &&
-         !rtcp_mux_filter_.IsFullyActive();
+  return !rtcp_mux_required_ && !rtcp_mux_filter_.IsFullyActive();
 }
 
 bool BaseChannel::IsReadyToReceiveMedia_w() const {
@@ -618,13 +605,6 @@ void BaseChannel::OnPacketRead(rtc::PacketTransportInternal* transport,
   HandlePacket(rtcp, &packet, packet_time);
 }
 
-void BaseChannel::OnReadyToSend(rtc::PacketTransportInternal* transport) {
-  RTC_DCHECK(transport == rtp_transport_.rtp_packet_transport() ||
-             transport == rtp_transport_.rtcp_packet_transport());
-  SetTransportChannelReadyToSend(
-      transport == rtp_transport_.rtcp_packet_transport(), true);
-}
-
 void BaseChannel::OnDtlsState(DtlsTransportInternal* transport,
                               DtlsTransportState state) {
   if (!ShouldSetupDtlsSrtp_n()) {
@@ -668,22 +648,10 @@ void BaseChannel::OnSelectedCandidatePairChanged(
            network_route));
 }
 
-void BaseChannel::SetTransportChannelReadyToSend(bool rtcp, bool ready) {
-  RTC_DCHECK(network_thread_->IsCurrent());
-  if (rtcp) {
-    rtcp_ready_to_send_ = ready;
-  } else {
-    rtp_ready_to_send_ = ready;
-  }
-
-  bool ready_to_send =
-      (rtp_ready_to_send_ &&
-       // In the case of rtcp mux |rtcp_packet_transport_| will be null.
-       (rtcp_ready_to_send_ || !rtp_transport_.rtcp_packet_transport()));
-
+void BaseChannel::OnTransportReadyToSend(bool ready) {
   invoker_.AsyncInvoke<void>(
       RTC_FROM_HERE, worker_thread_,
-      Bind(&MediaChannel::OnReadyToSend, media_channel_, ready_to_send));
+      Bind(&MediaChannel::OnReadyToSend, media_channel_, ready));
 }
 
 bool BaseChannel::PacketIsRtcp(const rtc::PacketTransportInternal* transport,
@@ -718,11 +686,8 @@ bool BaseChannel::SendPacket(bool rtcp,
   // packet before doing anything. (We might get RTCP packets that we don't
   // intend to send.) If we've negotiated RTCP mux, send RTCP over the RTP
   // transport.
-  rtc::PacketTransportInternal* transport =
-      (!rtcp || rtcp_mux_filter_.IsActive())
-          ? rtp_transport_.rtp_packet_transport()
-          : rtp_transport_.rtcp_packet_transport();
-  if (!transport || !transport->writable()) {
+  const bool send_on_rtcp = rtcp && !rtcp_mux_filter_.IsActive();

[Taylor Brandstetter, 2017/04/19 05:56:33]

This could be moved into RtpTransport, if rtcp_mux_filter_.IsActive() always
matched "rtcp_mux_enabled()"

[Zach Stein, 2017/04/20 19:59:10]

Done.

+  if (!rtp_transport_.IsWritable(send_on_rtcp)) {
     return false;
   }
 
@@ -818,16 +783,8 @@ bool BaseChannel::SendPacket(bool rtcp,
 
   // Bon voyage.
   int flags = (secure() && secure_dtls()) ? PF_SRTP_BYPASS : PF_NORMAL;
-  int ret = transport->SendPacket(packet->data<char>(), packet->size(),
-                                  updated_options, flags);
-  if (ret != static_cast<int>(packet->size())) {
-    if (transport->GetError() == ENOTCONN) {
-      LOG(LS_WARNING) << "Got ENOTCONN from transport.";
-      SetTransportChannelReadyToSend(rtcp, false);
-    }
-    return false;
-  }
-  return true;
+  return rtp_transport_.SendPacket(send_on_rtcp, packet, updated_options,
+                                   flags);
 }
 
 bool BaseChannel::WantsPacket(bool rtcp, const rtc::CopyOnWriteBuffer* packet) {
@@ -1259,7 +1216,7 @@ bool BaseChannel::SetRtcpMux_n(bool enable,
                                std::string* error_desc) {
   // Provide a more specific error message for the RTCP mux "require" policy
   // case.
-  if (rtp_transport_.rtcp_mux_required() && !enable) {
+  if (rtcp_mux_required_ && !enable) {
     SafeSetError(
         "rtcpMuxPolicy is 'require', but media description does not "
         "contain 'a=rtcp-mux'.",
@@ -1270,11 +1227,13 @@ bool BaseChannel::SetRtcpMux_n(bool enable,
   switch (action) {
     case CA_OFFER:
       ret = rtcp_mux_filter_.SetOffer(enable, src);
+      // TODO(zstein): rtp_transport_.set_rtcp_mux_enabled here?

[Taylor Brandstetter, 2017/04/19 05:56:33]

I think you could do
"rtp_transport_.set_rtcp_mux_enabled(rtcp_mux_filter_.IsActive());" after the
switch statement, that seems simplest.

[Zach Stein, 2017/04/20 19:59:10]

I think that is just what I want - thanks!

       break;
     case CA_PRANSWER:
       // This may activate RTCP muxing, but we don't yet destroy the transport
       // because the final answer may deactivate it.
       ret = rtcp_mux_filter_.SetProvisionalAnswer(enable, src);
+      // TODO(zstein): rtp_transport_.set_rtcp_mux_enabled here?
       break;
     case CA_ANSWER:
       ret = rtcp_mux_filter_.SetAnswer(enable, src);
@@ -1293,7 +1252,8 @@ bool BaseChannel::SetRtcpMux_n(bool enable,
           SignalRtcpMuxFullyActive(transport_name_);
         }
         UpdateWritableState_n();
-        SetTransportChannelReadyToSend(true, false);
+        // TODO(zstein): Maybe rtcp_mux_filter should signal these changes.

[Taylor Brandstetter, 2017/04/19 05:56:33]

Since its state will always change synchronously, I don't think that's necessary

[Zach Stein, 2017/04/20 19:59:10]

Acknowledged.

+        rtp_transport_.set_rtcp_mux_enabled(true);
       }
       break;
     case CA_UPDATE: