Modify PeerConnection for end-to-end QuicDataChannel usage

webrtc/api/quictransport.cc

+/*
+ *  Copyright 2016 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.
+ */
+
+#include "webrtc/api/quictransport.h"
+
+#include "webrtc/base/bytebuffer.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/p2p/quic/quictransportchannel.h"
+#include "webrtc/p2p/quic/reliablequicstream.h"
+
+namespace webrtc {
+

[pthatcher1, 2016/03/30 20:34:48]

The more I think about it, the more I think this class should be
QuicDataTransport instead of QuicTransport, since eventually
QuicTransportChannel will be renamed QuicTransport.  Someday, we may decided to
combine QuicDataTransport and QuicTransport, but for now it's probably best to
keep them separate.

[mikescarlett, 2016/04/05 19:58:50]

Done. This is necessary anyway because I'm implementing a "QuicTransport"
similar to "DtlsTransport".

+QuicTransport::QuicTransport(
+    cricket::QuicTransportChannel* quic_transport_channel) {
+  quic_transport_channel->SignalIncomingStream.connect(
+      this, &QuicTransport::OnIncomingStream);
+}
+
+void QuicTransport::RegisterDataChannel(QuicDataChannel* data_channel) {
+  data_channel_map_[data_channel->id()] = data_channel;
+}
+
+void QuicTransport::UnregisterDataChannel(int id) {
+  data_channel_map_.erase(id);

[pthatcher1, 2016/03/30 20:34:48]

Hmmm.... it seems to me like the QuicDataTransport should own the
QuicDataChannels, and the PeerConnection should only hold references to them,
just like how a TransportController hows the TransportChannels. 

In that case, we'd have CreateDataChannel and DestroyDataChannel instead of
AddDataChannel and RemoveDataChannel.  I think this would especially make more
sense once we handle the OPEN message.

What do you think?

[mikescarlett, 2016/04/05 19:58:50]

That's a good idea. For the OPEN message, it would be cleaner than having
PeerConnection/WebRtcSession handle a callback from this class then having
PeerConnection call "AddDataChannel". I'm also thinking it simplifies the logic
for when the QuicTransportChannel does not exist yet and making sure the
QuicDataChannel is added to the QuicDataTransport.

+}
+
+// Called when a QUIC stream is created for incoming data.
+void QuicTransport::OnIncomingStream(cricket::ReliableQuicStream* stream) {
+  RTC_DCHECK(stream != nullptr);
+  quic_stream_map_[stream->id()] = stream;
+  stream->SignalDataReceived.connect(this, &QuicTransport::OnDataReceived);
+  // stream->SignalClosed.connect(this, &QuicTransport::OnStreamClosed);
+}
+
+// Called when the first QUIC stream frame is received for incoming data.
+void QuicTransport::OnDataReceived(net::QuicStreamId id,
+                                   const char* data,
+                                   size_t len) {
+  RTC_DCHECK(quic_stream_map_.find(id) != quic_stream_map_.end());
+  cricket::ReliableQuicStream* stream = quic_stream_map_[id];
+  stream->SignalDataReceived.disconnect(this);
+  quic_stream_map_.erase(id);

[pthatcher1, 2016/03/30 20:34:48]

Are we guaranteed to always get all the data all at once?  Or do we need to
buffer the incoming data until it's all received?

[Taylor Brandstetter, 2016/04/01 23:23:42]

This is handled in QuicDataChannel. QuicTransport just needs to pull the data
channel ID out of the first chunk of data.

[mikescarlett, 2016/04/05 19:58:51]

I prefer to have QuicDataChannel buffer the data itself as currently designed,
so that this class only handles QUIC streams that don't have a data channel yet.

An alternative would be having this class deal with incoming streams instead of
the QuicDataChannel, but then it would be responsible for maintaining which data
channel each of the streams are associated with and closing the streams when the
data channel is closing.

+
+  // Read data channel id
+  rtc::ByteBuffer byte_buffer(data, len, rtc::ByteBuffer::ORDER_HOST);
+  uint64_t data_channel_id;
+  byte_buffer.ReadVarint(&data_channel_id);

[pthatcher1, 2016/03/30 20:34:49]

Shouldn't we check the return value?

[mikescarlett, 2016/04/05 19:58:51]

We should.

+
+  // Retrieve the data channel which will handle the message.
+  const auto& kv = data_channel_map_.find(data_channel_id);
+  if (kv == data_channel_map_.end()) {
+    // TODO(mikescarlett): Implement listening for an OPEN message to create a
+    // new QuicDataChannel when messages are received for a nonexistent ID.
+    LOG(LS_ERROR) << "Data was received for QUIC data channel "
+                  << data_channel_id
+                  << " but it is not registered to the QuicTransport.";
+  } else {
+    // Otherwise get the existing data channel.
+    QuicDataChannel* data_channel = kv->second;
+    data_channel->OnIncomingStream(stream, &byte_buffer);

[pthatcher1, 2016/03/30 20:34:48]

It seems like we should parse the message ID here as well and call something
like data_channel->OnIncomingMessage(id, buffer, stream).

[mikescarlett, 2016/04/05 19:58:50]

Sure this could parse the message ID. That also allows us to use "const char*
data, size_t len" instead of a ByteBuffer to the QuicDataChannel.

+  }
+}
+
+}  // namespace webrtc