Refactoring on QUIC

webrtc/api/webrtcsession.cc

Index: webrtc/api/webrtcsession.cc
diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc
index b9a3fc8453389dc040a28f51a47d0a4649e0eb49..1fbf82c00a41aaac25d9301eaaa8741fe473a6d8 100644
--- a/webrtc/api/webrtcsession.cc
+++ b/webrtc/api/webrtcsession.cc
@@ -24,6 +24,7 @@
 #include "webrtc/api/webrtcsessiondescriptionfactory.h"
 #include "webrtc/audio_sink.h"
 #include "webrtc/base/basictypes.h"
+#include "webrtc/base/bind.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/logging.h"
@@ -38,6 +39,10 @@
 #include "webrtc/pc/channelmanager.h"
 #include "webrtc/pc/mediasession.h"
 
+#ifdef HAVE_QUIC
+#include "webrtc/p2p/quic/quictransportchannel.h"
+#endif  // HAVE_QUIC
+
 using cricket::ContentInfo;
 using cricket::ContentInfos;
 using cricket::MediaContentDescription;
@@ -458,7 +463,8 @@ WebRtcSession::WebRtcSession(webrtc::MediaControllerInterface* media_controller,
                              rtc::Thread* worker_thread,
                              rtc::Thread* signaling_thread,
                              cricket::PortAllocator* port_allocator)
-    : worker_thread_(worker_thread),
+    : network_thread_(network_thread),
+      worker_thread_(worker_thread),
       signaling_thread_(signaling_thread),
       // RFC 3264: The numeric value of the session id and version in the
       // o line MUST be representable with a "64 bit signed integer".
@@ -545,7 +551,17 @@ bool WebRtcSession::Initialize(
   // PeerConnectionFactoryInterface::Options.
   if (rtc_configuration.enable_rtp_data_channel) {
     data_channel_type_ = cricket::DCT_RTP;
-  } else {
+  }
+#ifdef HAVE_QUIC
+  else if (rtc_configuration.enable_quic && dtls_enabled_) {

[pthatcher1, 2016/06/28 22:39:03]

I don't think we need to check for dtls_enabled_.  

+    // Use QUIC instead of DTLS when |enable_quic| is true and encryption is
+    // enabled.
+    data_channel_type_ = cricket::DCT_QUIC;
+    transport_controller_->use_quic();
+    LOG(LS_INFO) << "Using QUIC instead of DTLS";

[pthatcher1, 2016/06/28 22:39:03]

We could log this if dtls_enabled_.  But otherwise, I don't think we need to.

+  }
+#endif  // HAVE_QUIC
+  else {
     // DTLS has to be enabled to use SCTP.
     if (!options.disable_sctp_data_channels && dtls_enabled_) {
       data_channel_type_ = cricket::DCT_SCTP;
@@ -1803,6 +1819,27 @@ bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content,
 
 bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content,
                                       const std::string* bundle_transport) {
+#ifdef HAVE_QUIC
+  if (data_channel_type_ == cricket::DCT_QUIC) {
+    // The TransportController may not have a QuicTransportChannel, so force
+    // creation of it.
+    LOG(LS_INFO) << "Getting or creating QuicTransportChannel";
+    RTC_DCHECK(transport_controller_->quic());
+    cricket::TransportChannel* transport_channel =
+        network_thread_->Invoke<cricket::TransportChannel*>(
+            RTC_FROM_HERE,
+            rtc::Bind(&cricket::TransportController::CreateTransportChannel_n,
+                      transport_controller_.get(), content->name,
+                      cricket::ICE_CANDIDATE_COMPONENT_RTP));
+    if (!transport_channel) {
+      return false;
+    }
+    quic_transport_channel_ =
+        static_cast<cricket::QuicTransportChannel*>(transport_channel);
+    SignalQuicTransportChannelCreated(quic_transport_channel_);
+    return true;
+  }
+#endif  // HAVE_QUIC

[Taylor Brandstetter, 2016/06/29 17:50:04]

I think this needs to be rethought. This design means that when using QUIC,
WebRtcSession creates the transport channel instead of BaseChannel. But that
means WebRtcSession should be doing the same stuff that happens when using
BaseChannels, such as:

* If BUNDLE is negotiated, may need to switch which TransportChannel is used for
data
* If a media description is rejected, the TransportChannel should be destroyed.

It may be easier to just use BaseChannel, and signal when its TransportChannel
is changed and when it's created/destroyed. For example, when bundling is
negotiated, the PeerConnection's QuicDataTransport should start using the
QuicTransportChannel negotiated for bundling data.

Or, QuicDataTransport could just be a subclass of BaseChannel. Then there are
much less wires to hook up.

It's probably a good idea to have a whiteboard discussion when Peter is here.

[pthatcher1, 2016/06/29 18:35:28]

Yeah, we can discuss it in person.  This was the design Michael and I came up
with while he was here.  I was trying to avoid the complexity of BaseChannel by
requiring that QUIC only be used with max-bundle, at least in the early version
of support for QUIC.

Similarly, this CL doesn't do any negotiation.  It just assumes you will do QUIC
and won't reject QUIC (kind of a like a max-quic setting would be).  Doing
negotiation can come in a later CL (at which point, we may need to do something
with BaseChannel, but there are actually harder things to whiteboard and figure
out around offering either DTLS or QUIC and then reacting to whichever comes
back either on the signaling or media path).

   bool sctp = (data_channel_type_ == cricket::DCT_SCTP);
   bool require_rtcp_mux =
       rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
@@ -2001,7 +2038,7 @@ bool WebRtcSession::ReadyToUseRemoteCandidate(
     const IceCandidateInterface* candidate,
     const SessionDescriptionInterface* remote_desc,
     bool* valid) {
-  *valid = true;;
+  *valid = true;
 
   const SessionDescriptionInterface* current_remote_desc =
       remote_desc ? remote_desc : remote_desc_.get();
@@ -2026,6 +2063,14 @@ bool WebRtcSession::ReadyToUseRemoteCandidate(
       current_remote_desc->description()->contents()[mediacontent_index];
   cricket::BaseChannel* channel = GetChannel(content.name);
   if (!channel) {
+#ifdef HAVE_QUIC
+    if (data_channel_type_ == cricket::DCT_QUIC && quic_transport_channel_ &&
+        content.name == quic_transport_channel_->transport_name()) {
+      // QUIC data channels do not have a BaseChannel unless bundle is enabled.
+      return transport_controller_->ReadyForRemoteCandidates(
+          quic_transport_channel_->transport_name());
+    }
+#endif  // HAVE_QUIC
     return false;
   }