Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1844803002:
Modify PeerConnection for end-to-end QuicDataChannel usage (Closed)
|
Created:
4 years, 8 months ago by mikescarlett Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionModify PeerConnection for end-to-end QuicDataChannel usage
To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.
PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.
WebRtcSession has ben modified to use a QuicTransportChannel
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used.
Patch Set 1 #Patch Set 2 : #
Total comments: 160
Patch Set 3 : Apply changes to p2p directory and QuicDataChannel/QuicDataTransport #Patch Set 4 : Remove transport.h from this CL #Patch Set 5 : Still in progress but works end to end now #Patch Set 6 : Created a QuicDataChannel::MessageHelper class and improved end-to-end testing #Patch Set 7 : Remove webrtcsdp.cc from this CL #
Total comments: 38
Patch Set 8 : Update QuicDataChannel and QuicDataTransport #Patch Set 9 : Sync with other CLs (will be much smaller once other CLs are landed) #Patch Set 10 : Sync to upstream after landing QUIC data channel and QUIC transport CLs #
Messages
Total messages: 16 (7 generated)
Description was changed from ========== Initial QuicDataChannel implementation BUG= ========== to ========== Initial QuicDataChannel implementation A QUIC data channel that implements DataChannelInterface. It replaces SCTP data channels by using a QuicTransportChannel to create a ReliableQuicStream for each message. QuicDataChannel only implements unordered, reliable delivery for the initial implementation. A QuicTransport helper class demuxes each incoming ReliableQuicStream to a QuicDataChannel when the remote peer receives a message. To allow end-to-end QuicDataChannel usage with a PeerConnection, session descriptions have been modified to include a boolean for whether to do QUIC, which serializes to SDP including an "a=quic" attribute. It is currently assumed that if the offerer specifies QUIC in the session description, then both peers must use QUIC. WebRtcSession and TransportController have been modified to allow QuicTransportChannel usage by the QuicDataChannel. BUG= ==========
mikescarlett@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org
I'm mailing this so you can at least see what I've done. It's not ready to be submitted and I'm still working on testing.
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
Even though it needs some changes, in general this looks very good. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:849: } It seems like this should logically go a few lines up, just before "new QuicDataChannel". https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection.h File webrtc/api/peerconnection.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.h:28: #ifdef USE_QUIC I think our naming convention is HAVE_QUIC, but I could be wrong. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:1885: initializing_client()->CreateDataChannel(); I assume this will do more in the future. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:150: rtc::ByteBuffer* remaining_bytes) { The order should be remaining_bytes, stream, since that's the order chronologically. And it should probably be first_bytes, not remaining_bytes. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:168: signaling_thread_->Invoke<void>( These needs to be an AsyncInvoke https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:170: OnMessage_s(message); Why do we hop to the signalling thread and then call OnMessage_s? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:183: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream->id()]; When does the buffer get added to the queud_data_? Is it done implicitly by reading it? If so, we should probably be more explicit about adding to it. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:183: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream->id()]; I think we just want a normal rtc::Buffer, not an rtc::CopyOnWriteBuffer here, and everywhere in this file. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:188: } Can you use an early return in this code? Perhaps something like: if (!observer) { LOG(LS_WARNING) << ...; stream->close(); return; } if (!stream->fin_received()) { queued_data_[stream->id] = rtc::ByteBuffer(first_bytes->Data(), first_bytes->Length()); incoming_quic_streams_[stream->id()] = stream; stream->SignalDataReceived.connect(this, QuicDataChannel::OnDataReceived); stream->SignalClosed.connect(this, &QuicDataChannel::OnStreamClosed); return; } rtc::ByteBuffer(first_bytes->Data(), first_bytes->Length()); signaling_thread_->AsyncInvoke<void>(QuicDataChannel::OnMessage_s); stream->close(); https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:200: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream_id]; We should use .find() and check to see if it's in the map. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:207: DataBuffer message(received_data, false); We should probably use a map of rtc::Buffer* so that we can pass ownership to OnMessage_s here and avoid copying here. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:215: } Same here with early returns. Something like this: cricket::ReliableQuicStream* stream = incoming_quic_streams_.find(stream_id).second; queued_data_[stream_id].Append(data, len); if (!stream->fin_received()) { return; } ... signaling_thread_->AsyncInvoke<void>(QuicDataChannel::OnMessage_s); stream->close(); https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:216: } Now that I see this more completely, I think that instead of having two maps, one for the stream and one for the queued data, instead we should encapsulate the "read and buffer until the stream is done" into a separate QuicDataChannelMessage class that handles that, and then have a map of of stream id => QuicDataChannelMessage. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:223: SendOpenMessage_w(); I though we weren't doing the open message in this CL. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:239: } Early return here please. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachannel.h File webrtc/api/quicdatachannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:22: #include "webrtc/base/scoped_ref_ptr.h" If these aren't used in the .h, please move them to the .cc. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:49: // data. The Funny formatting here. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.cc File webrtc/api/quictransport.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:19: 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. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:31: data_channel_map_.erase(id); 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? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:49: quic_stream_map_.erase(id); 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? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:54: byte_buffer.ReadVarint(&data_channel_id); Shouldn't we check the return value? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:67: data_channel->OnIncomingStream(stream, &byte_buffer); It seems like we should parse the message ID here as well and call something like data_channel->OnIncomingMessage(id, buffer, stream). https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.h File webrtc/api/quictransport.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:20: #include "webrtc/base/scoped_ref_ptr.h" If these aren't used in this file, please move them to the .cc. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:35: // remote peer to its corresponding QuicDataChannel, and encapsulates a Might be more clear as "by reading the data channel ID from the stream and looking it up in a map of id => QuicDataChannel." https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:41: // existing QuicDataChannel, which reads the remaining data. Might be more clear as "if the QuicDataChannel exists, it sends the stream to the QuicDataChannel. Otherwise, it creates a QuicDataChannel with the given ID" https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:53: void UnregisterDataChannel(int id); I think AddDataChannel and RemoveDataChannel would be slightly better names. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:54: bool has_data_channel(int id) const { HasDataChannel https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:68: // Map of <data channel id, QuicDataChannel*> values. Might be easier to read as "data channel ID => QuicDataChannel*" https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:69: std::unordered_map<int, QuicDataChannel*> data_channel_map_; I think a better name would be data_channel_by_id_. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:70: // Map of <QUIC stream id, ReliableQuicStream*> values. Similarly, "QUIC stream ID => ReliableQuicStream" https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:72: quic_stream_map_; quic_stream_by_id_ https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport_... File webrtc/api/quictransport_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport_... webrtc/api/quictransport_unittest.cc:223: byte_buffer2.WriteString("Hello, World!"); It's probably worth making a WriteMessage(dcid, msgid, payload) method that writes these three. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:2184: } And does this need the ifdef as well? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.h File webrtc/api/webrtcsession.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.h:317: cricket::QuicTransportChannel* quic_transport_channel() const; Doesn't this need USE_QUIC or HAVE_QUIC as well (and the class reference above)? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc File webrtc/base/bytebuffer.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:237: Since this is basically what is described in https://developers.google.com/protocol-buffers/docs/encoding#varints, we should probably add a comment with a reference to that, with the caveat that we only go to 64 bits, not 128 bits. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:238: bool ByteBuffer::ReadVarint(uint64_t* val) { These should be ReadUvarint and WriteUvarint, since they are unsigned. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:238: bool ByteBuffer::ReadVarint(uint64_t* val) { It might be a good idea to pull this varint stuff into its own CL and land it separately. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:239: if (!val) {}s please https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:250: v |= static_cast<uint64_t>(numeric_value & (0x80 - 1)) << i; This might be more clear as: v |= (static_cast<uint64_t>(byte) & 0x7F) << i; if (byte < 0x80) { *val = v; return true; } In other words, use 0x7F instead of 0x80-1 and remove "numeric_value" if it's not needed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:253: *val = (byte_order_ == ORDER_NETWORK) ? NetworkToHost64(v) : v; I don't think this is correct. Varints are always LSB first, regardless of the byte_order_, so we shouldn't do this swap at the end. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:267: char byte = static_cast<char>(numeric_value); Does this work? It would be a bit shorter: char byte = static_cast<char>(val) | 0x80; Or this: char byte = static_cast<char>(val | 0x80); In other words, the mask with 0x7FF isn't necessary because you truncate right after anyway. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:271: char byte = static_cast<char>(val); Might as well call this "last_byte". https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... File webrtc/base/bytebuffer_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... webrtc/base/bytebuffer_unittest.cc:164: // Write and read varint. What does this mean? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... webrtc/base/bytebuffer_unittest.cc:214: TEST(ByteBufferTest, TestReadWriteVarint) { Uvarint https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... webrtc/base/bytebuffer_unittest.cc:240: rtc::ByteBuffer::ByteOrder::ORDER_HOST); Test with ORDER_NETWORK also. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransport.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransport.h:44: bool SetLocalCertificate( I think it would help to move all of the code changes in the p2p/ directory into its own CL and land that by itself (like the varint one). https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:671: #endif // USE_QUIC This doesn't need USE_QUIC, does it? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:680: return nullptr; We should probably do an RTC_DCHECK if we don't put the whole method behind USE_QUIC. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:96: // macro is defined. Just say "if USE_QUIC is defined" https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:99: QuicTransportChannel* quic_transport_channel() const; Doesn't this need to be behind USE_QUIC? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quicsessi... File webrtc/p2p/quic/quicsession.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quicsessi... webrtc/p2p/quic/quicsession.cc:82: // write_blocked_streams()->RegisterStream(stream->id(), priority); What is this commented-out line? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:126: Transport("", nullptr), Why not use channel->transport_name() for the transport name? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:629: // active as client. Can you just delete this whole comment and write the following: TODO(pthatcher): Move all the negotiation up into PeerConnection and out of the Transport code. Just have a SetRole or similar method here. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:681: ReliableQuicStream* QuicTransportChannel::CreateQuicStream() { Can you make it clear that the 0 here is the priority? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.h:222: // Creates a new QUIC stream that can send or receive data. Isn't this just for sending data (because each message gets its own stream)?
pthatcher@google.com changed reviewers: + pthatcher@google.com
https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1052: } An alternative to the TransportChannel storing a transport channel: quic_transport_channel = static_cast<QuicDataChannel>(session_->data_channel()->transport_channel()); quic_data_transport_->SetTransportChannel(quic_transport_channel); for (auto dc : quic_data_channels_) { dc->SetTransportChannel(quic_transport_channel); }
https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:838: // before QUIC is specified in the session description. Can't "use quic" be a parameter in RTCConfiguration, at the PeerConnection initialization time? Or maybe it could be a parameter in the DataChannelInit. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:842: RTC_DCHECK(quic_transport_channel != nullptr); It should be possible to create a data channel even if the underlying QUIC transport channel doesn't exist. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1943: return !rtp_data_channels_.empty() || !sctp_data_channels_.empty(); || !quic_data_channels_.empty() This is used so that a data m= section is offered if there's at least one data channel. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:26: const DataChannelInit* config) What should happen if the data channel init is something QUIC doesn't support (ordered)? Maybe PeerConnection should check that and throw an error. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:45: if (state_ == kOpen && send_open_message_) { If send_open_message_ is always false I'd just remove it and SendOpenMessage_w from this CL. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:94: // QUIC streams from being created or the data channel state is invalid. If this can only occur due to an internal error, would an RTC_DCHECK be appropriate? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:117: buffered_amount_ += len; I don't see buffered_amount_ ever decrease. Also where is the stream closed in this situation? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:120: return false; If the data is buffered successfully, this method shouldn't return false. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:155: remaining_bytes->ReadVarint(&message_id); Should probably handle failures of ReadVarint. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:183: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream->id()]; On 2016/03/30 20:34:48, pthatcher1 wrote: > When does the buffer get added to the queud_data_? Is it done implicitly by > reading it? If so, we should probably be more explicit about adding to it. Do you mean, "when does the object get constructed"? I think it's pretty standard that operator[] on a map creates an object, so this wasn't surprising to me. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:200: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream_id]; On 2016/03/30 20:34:48, pthatcher1 wrote: > We should use .find() and check to see if it's in the map. It always *should* be. So if you do that, put it in an RTC_DCHECK. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:210: OnMessage_s(message); Again, OnMessage_s shouldn't be called twice. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:216: } On 2016/03/30 20:34:48, pthatcher1 wrote: > Now that I see this more completely, I think that instead of having two maps, > one for the stream and one for the queued data, instead we should encapsulate > the "read and buffer until the stream is done" into a separate > QuicDataChannelMessage class that handles that, and then have a map of of stream > id => QuicDataChannelMessage. +1 https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:234: if (observer_ != nullptr) { If it's OnMessage_s that checks for an observer, you don't need to in OnIncomingStream etc. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachannel.h File webrtc/api/quicdatachannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:90: rtc::ByteBuffer* remaining_bytes); Maybe make this private and declare "friend QuicTransport" (QuicDataTransport if it gets renamed). Also in the comment above the class, explain the interaction between QuicDataChannel and QuicTransport/QuicDataTransport https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:95: } What is this used for, and does it need to be public? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:55: // Too large message (> 16 KB) that violates the QUIC stream flow control limit. nit: maybe say "oversized" instead of "too large" https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:145: // Called when the first QUIC stream frame is received for incoming data. I'd add a comment saying that this is simulating the role of QuicTransport/QuicDataTransport. This wasn't clear to me at first. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:339: ASSERT_FALSE(peer1_data_channel->Send(kTooLargeBuffer)); Shouldn't the test also wait until the message is received? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.cc File webrtc/api/quictransport.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:49: quic_stream_map_.erase(id); On 2016/03/30 20:34:48, pthatcher1 wrote: > 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? This is handled in QuicDataChannel. QuicTransport just needs to pull the data channel ID out of the first chunk of data. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp.cc File webrtc/api/webrtcsdp.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:154: static const char kAttributeQuic[] = "quic"; Does it make sense for this to be an attribute? I think it would make more sense as a profile string. Instead of "UDP/TLS/RTP/SAVPF" we could use "UDP/QUIC". https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp.cc#n... webrtc/api/webrtcsdp.cc:826: AddLine("a=quic", &message); Replace this with: InitAttrLine(kAttributeQuic, &os); AddLine(os.str(), &message); https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp_unit... File webrtc/api/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp_unit... webrtc/api/webrtcsdp_unittest.cc:3147: TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutQuic) { Aren't the "WithoutQuic" tests already covered by the tests above, since the sample session description doesn't contain "a=quic"? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp_unit... webrtc/api/webrtcsdp_unittest.cc:3148: std::string sdp_with_quic = kSdpSessionString; Should be sdp_without_quic? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1743: transport_controller_->set_quic(true); This should fail if a transport channel has already been created. You can't switch back and forth between QUIC and non-QUIC. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.h File webrtc/api/webrtcsession.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.h:317: cricket::QuicTransportChannel* quic_transport_channel() const; It's possible that this is NULL when it's needed. So you need an equivalent of DataChannelProviderInterface in order to tell a QuicDataChannel when the underlying transport channel is created or destroyed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:229: ice_transport_channel_.reset( If you decide to force "one QUIC channel per PeerConnection", this should fail if you try to create a second one. If you decide to allow multiple QUIC channels, then you shouldn't store ice_transport_channel_ and quic_transport_channel_. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.h:73: public Transport, As discussed, I think it will be confusing for a QuicTransportChannel to also be a Transport. Can you make a separate "QuicTransport" class, which is essentially just a QuicTransportChannel factory? Look at P2PTransport for example. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.h:234: const TransportDescription* local_description_for_test() const { I don't like having "for test" methods that expose the internals of a class. Is there a way to do without these?
I split the p2p directory changes into 3 CLs: https://codereview.webrtc.org/1844333006/ https://codereview.webrtc.org/1856513002/ https://codereview.webrtc.org/1856943002/ so please look at those first. I'm still in the process of changing QuicDataChannel/QuicDataTransport unit tests and haven't fixed the issues with WebRtcSession/PeerConnection. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:838: // before QUIC is specified in the session description. On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > Can't "use quic" be a parameter in RTCConfiguration, at the PeerConnection > initialization time? Or maybe it could be a parameter in the DataChannelInit. This seems necessary since QUIC won't be negotiated. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:842: RTC_DCHECK(quic_transport_channel != nullptr); On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > It should be possible to create a data channel even if the underlying QUIC > transport channel doesn't exist. Fixed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:1943: return !rtp_data_channels_.empty() || !sctp_data_channels_.empty(); On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > || !quic_data_channels_.empty() > > This is used so that a data m= section is offered if there's at least one data > channel. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection.h File webrtc/api/peerconnection.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection.h:28: #ifdef USE_QUIC On 2016/03/30 20:34:48, pthatcher1 wrote: > I think our naming convention is HAVE_QUIC, but I could be wrong. I renamed it HAVE_QUIC because HAVE_SCTP is used for building SCTP. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:1885: initializing_client()->CreateDataChannel(); On 2016/03/30 20:34:48, pthatcher1 wrote: > I assume this will do more in the future. Yes it will. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:26: const DataChannelInit* config) On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > What should happen if the data channel init is something QUIC doesn't support > (ordered)? Maybe PeerConnection should check that and throw an error. By error do you mean something like "RTC_DCHECK(config->reliable && !config->ordered)" in PeerConnection so that the app crashes with bad parameters? I can do that to prevent unintended usage. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:45: if (state_ == kOpen && send_open_message_) { On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > If send_open_message_ is always false I'd just remove it and SendOpenMessage_w > from this CL. Removed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:94: // QUIC streams from being created or the data channel state is invalid. On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > If this can only occur due to an internal error, would an RTC_DCHECK be > appropriate? I agree now. A minor issue is this will crash if an app sends 2^31 messages among all data channels (due to 32-bit integer overflow), but that behavior isn't supported yet so it can crash in that case. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:117: buffered_amount_ += len; On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > I don't see buffered_amount_ ever decrease. Also where is the stream closed in > this situation? I implemented this by adding a new ReliableQuicStream callback method. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:120: return false; On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > If the data is buffered successfully, this method shouldn't return false. Okay buffering returns true now. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:150: rtc::ByteBuffer* remaining_bytes) { On 2016/03/30 20:34:48, pthatcher1 wrote: > The order should be remaining_bytes, stream, since that's the order > chronologically. And it should probably be first_bytes, not remaining_bytes. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:155: remaining_bytes->ReadVarint(&message_id); On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > Should probably handle failures of ReadVarint. Done. ReadVarint is in QuicDataTransport now. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:168: signaling_thread_->Invoke<void>( On 2016/03/30 20:34:48, pthatcher1 wrote: > These needs to be an AsyncInvoke Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:170: OnMessage_s(message); On 2016/03/30 20:34:48, pthatcher1 wrote: > Why do we hop to the signalling thread and then call OnMessage_s? That clearly shouldn't be there. Fixed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:183: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream->id()]; On 2016/03/30 20:34:48, pthatcher1 wrote: > I think we just want a normal rtc::Buffer, not an rtc::CopyOnWriteBuffer here, > and everywhere in this file. It needs to be rtc::CopyOnWriteBuffer due to this CL: https://codereview.webrtc.org/1823503002/ https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:183: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream->id()]; On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > On 2016/03/30 20:34:48, pthatcher1 wrote: > > When does the buffer get added to the queud_data_? Is it done implicitly by > > reading it? If so, we should probably be more explicit about adding to it. > > Do you mean, "when does the object get constructed"? I think it's pretty > standard that operator[] on a map creates an object, so this wasn't surprising > to me. I changed it to not use operator[] to create the buffer, since that is more readable. Yes that was creating the object (using the default constructor). https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:188: } On 2016/03/30 20:34:48, pthatcher1 wrote: > Can you use an early return in this code? Perhaps something like: > > > if (!observer) { > LOG(LS_WARNING) << ...; > stream->close(); > return; > } > > if (!stream->fin_received()) { > queued_data_[stream->id] = rtc::ByteBuffer(first_bytes->Data(), > first_bytes->Length()); > incoming_quic_streams_[stream->id()] = stream; > stream->SignalDataReceived.connect(this, QuicDataChannel::OnDataReceived); > stream->SignalClosed.connect(this, &QuicDataChannel::OnStreamClosed); > return; > } > > rtc::ByteBuffer(first_bytes->Data(), first_bytes->Length()); > signaling_thread_->AsyncInvoke<void>(QuicDataChannel::OnMessage_s); > stream->close(); > Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:200: rtc::CopyOnWriteBuffer& received_data = queued_data_[stream_id]; On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > On 2016/03/30 20:34:48, pthatcher1 wrote: > > We should use .find() and check to see if it's in the map. > > It always *should* be. So if you do that, put it in an RTC_DCHECK. I put an RTC_DCHECK. OnDataReceived is only called when the stream belongs to this data channel's incoming QUIC streams. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:207: DataBuffer message(received_data, false); On 2016/03/30 20:34:48, pthatcher1 wrote: > We should probably use a map of rtc::Buffer* so that we can pass ownership to > OnMessage_s here and avoid copying here. What about using std::move to avoid copying for DataBuffer and OnMessage_s? Will that still be bad for performance? https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:210: OnMessage_s(message); On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > Again, OnMessage_s shouldn't be called twice. Removed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:215: } On 2016/03/30 20:34:48, pthatcher1 wrote: > Same here with early returns. Something like this: > > cricket::ReliableQuicStream* stream = > incoming_quic_streams_.find(stream_id).second; > queued_data_[stream_id].Append(data, len); > if (!stream->fin_received()) { > return; > } > > ... > signaling_thread_->AsyncInvoke<void>(QuicDataChannel::OnMessage_s); > stream->close(); > > > Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:216: } On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > On 2016/03/30 20:34:48, pthatcher1 wrote: > > Now that I see this more completely, I think that instead of having two maps, > > one for the stream and one for the queued data, instead we should encapsulate > > the "read and buffer until the stream is done" into a separate > > QuicDataChannelMessage class that handles that, and then have a map of of > stream > > id => QuicDataChannelMessage. > > +1 I revised it. QuicDataChannelMessage is a struct and I find that name verbose for an implementation detail, so I made it QuicDataChannel::Message. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:223: SendOpenMessage_w(); On 2016/03/30 20:34:48, pthatcher1 wrote: > I though we weren't doing the open message in this CL. Right I'm not implementing it. Fixed https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:234: if (observer_ != nullptr) { On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > If it's OnMessage_s that checks for an observer, you don't need to in > OnIncomingStream etc. Replaced with RTC_DCHECK. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:239: } On 2016/03/30 20:34:48, pthatcher1 wrote: > Early return here please. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachannel.h File webrtc/api/quicdatachannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:22: #include "webrtc/base/scoped_ref_ptr.h" On 2016/03/30 20:34:48, pthatcher1 wrote: > If these aren't used in the .h, please move them to the .cc. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:49: // data. The On 2016/03/30 20:34:48, pthatcher1 wrote: > Funny formatting here. I'll fix the formatting https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:90: rtc::ByteBuffer* remaining_bytes); On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > Maybe make this private and declare "friend QuicTransport" (QuicDataTransport if > it gets renamed). Also in the comment above the class, explain the interaction > between QuicDataChannel and QuicTransport/QuicDataTransport Is this to prevent the method from being exposed directly to PeerConnection? What if I use Peter's suggestion of having QuicDataTransport create/destroy channels, then have QuicDataTransport return channels of type DataChannelInterface? I also changed the method signature to not used ByteBuffer if that was a concern. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:95: } On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > What is this used for, and does it need to be public? This isn't needed. I removed it. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:55: // Too large message (> 16 KB) that violates the QUIC stream flow control limit. On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > nit: maybe say "oversized" instead of "too large" Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:145: // Called when the first QUIC stream frame is received for incoming data. On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > I'd add a comment saying that this is simulating the role of > QuicTransport/QuicDataTransport. This wasn't clear to me at first. I'm redoing this anyway by having it use QuicDataTransport in the unittest. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.cc File webrtc/api/quictransport.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:19: On 2016/03/30 20:34:48, pthatcher1 wrote: > 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. Done. This is necessary anyway because I'm implementing a "QuicTransport" similar to "DtlsTransport". https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:31: data_channel_map_.erase(id); On 2016/03/30 20:34:48, pthatcher1 wrote: > 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? 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. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:49: quic_stream_map_.erase(id); On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > On 2016/03/30 20:34:48, pthatcher1 wrote: > > 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? > > This is handled in QuicDataChannel. QuicTransport just needs to pull the data > channel ID out of the first chunk of data. 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. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:54: byte_buffer.ReadVarint(&data_channel_id); On 2016/03/30 20:34:49, pthatcher1 wrote: > Shouldn't we check the return value? We should. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.cc:67: data_channel->OnIncomingStream(stream, &byte_buffer); On 2016/03/30 20:34:48, pthatcher1 wrote: > It seems like we should parse the message ID here as well and call something > like data_channel->OnIncomingMessage(id, buffer, stream). > 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. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.h File webrtc/api/quictransport.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:20: #include "webrtc/base/scoped_ref_ptr.h" On 2016/03/30 20:34:49, pthatcher1 wrote: > If these aren't used in this file, please move them to the .cc. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:35: // remote peer to its corresponding QuicDataChannel, and encapsulates a On 2016/03/30 20:34:49, pthatcher1 wrote: > Might be more clear as "by reading the data channel ID from the stream and > looking it up in a map of id => QuicDataChannel." Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:41: // existing QuicDataChannel, which reads the remaining data. On 2016/03/30 20:34:49, pthatcher1 wrote: > Might be more clear as "if the QuicDataChannel exists, it sends the stream to > the QuicDataChannel. Otherwise, it creates a QuicDataChannel with the given ID" Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:53: void UnregisterDataChannel(int id); On 2016/03/30 20:34:49, pthatcher1 wrote: > I think AddDataChannel and RemoveDataChannel would be slightly better names. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:54: bool has_data_channel(int id) const { On 2016/03/30 20:34:49, pthatcher1 wrote: > HasDataChannel Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:68: // Map of <data channel id, QuicDataChannel*> values. On 2016/03/30 20:34:49, pthatcher1 wrote: > Might be easier to read as "data channel ID => QuicDataChannel*" Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:69: std::unordered_map<int, QuicDataChannel*> data_channel_map_; On 2016/03/30 20:34:49, pthatcher1 wrote: > I think a better name would be data_channel_by_id_. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:70: // Map of <QUIC stream id, ReliableQuicStream*> values. On 2016/03/30 20:34:49, pthatcher1 wrote: > Similarly, "QUIC stream ID => ReliableQuicStream" Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport.... webrtc/api/quictransport.h:72: quic_stream_map_; On 2016/03/30 20:34:49, pthatcher1 wrote: > quic_stream_by_id_ Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport_... File webrtc/api/quictransport_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quictransport_... webrtc/api/quictransport_unittest.cc:223: byte_buffer2.WriteString("Hello, World!"); On 2016/03/30 20:34:49, pthatcher1 wrote: > It's probably worth making a WriteMessage(dcid, msgid, payload) method that > writes these three. Added a helper method. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp_unit... File webrtc/api/webrtcsdp_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/webrtcsdp_unit... webrtc/api/webrtcsdp_unittest.cc:3147: TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutQuic) { On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > Aren't the "WithoutQuic" tests already covered by the tests above, since the > sample session description doesn't contain "a=quic"? They are. You're correct. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc File webrtc/base/bytebuffer.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:237: On 2016/03/30 20:34:49, pthatcher1 wrote: > Since this is basically what is described in > https://developers.google.com/protocol-buffers/docs/encoding#varints, we should > probably add a comment with a reference to that, with the caveat that we only go > to 64 bits, not 128 bits. I added a comment. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:238: bool ByteBuffer::ReadVarint(uint64_t* val) { On 2016/03/30 20:34:49, pthatcher1 wrote: > These should be ReadUvarint and WriteUvarint, since they are unsigned. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:238: bool ByteBuffer::ReadVarint(uint64_t* val) { On 2016/03/30 20:34:49, pthatcher1 wrote: > It might be a good idea to pull this varint stuff into its own CL and land it > separately. Will make this a separate CL. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:239: if (!val) On 2016/03/30 20:34:49, pthatcher1 wrote: > {}s please Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:250: v |= static_cast<uint64_t>(numeric_value & (0x80 - 1)) << i; On 2016/03/30 20:34:50, pthatcher1 wrote: > This might be more clear as: > > v |= (static_cast<uint64_t>(byte) & 0x7F) << i; > if (byte < 0x80) { > *val = v; > return true; > } > > In other words, use 0x7F instead of 0x80-1 and remove "numeric_value" if it's > not needed. > Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:253: *val = (byte_order_ == ORDER_NETWORK) ? NetworkToHost64(v) : v; On 2016/03/30 20:34:49, pthatcher1 wrote: > I don't think this is correct. Varints are always LSB first, regardless of the > byte_order_, so we shouldn't do this swap at the end. Correct. I removed this. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:267: char byte = static_cast<char>(numeric_value); On 2016/03/30 20:34:49, pthatcher1 wrote: > Does this work? It would be a bit shorter: > > char byte = static_cast<char>(val) | 0x80; > > Or this: > > char byte = static_cast<char>(val | 0x80); > > > In other words, the mask with 0x7FF isn't necessary because you truncate right > after anyway. > > That works. I changed it. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer.cc... webrtc/base/bytebuffer.cc:271: char byte = static_cast<char>(val); On 2016/03/30 20:34:50, pthatcher1 wrote: > Might as well call this "last_byte". Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... File webrtc/base/bytebuffer_unittest.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... webrtc/base/bytebuffer_unittest.cc:164: // Write and read varint. On 2016/03/30 20:34:50, pthatcher1 wrote: > What does this mean? This doesn't belong here, removed https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... webrtc/base/bytebuffer_unittest.cc:214: TEST(ByteBufferTest, TestReadWriteVarint) { On 2016/03/30 20:34:50, pthatcher1 wrote: > Uvarint Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/base/bytebuffer_un... webrtc/base/bytebuffer_unittest.cc:240: rtc::ByteBuffer::ByteOrder::ORDER_HOST); On 2016/03/30 20:34:50, pthatcher1 wrote: > Test with ORDER_NETWORK also. Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransport.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransport.h:44: bool SetLocalCertificate( On 2016/03/30 20:34:50, pthatcher1 wrote: > I think it would help to move all of the code changes in the p2p/ directory into > its own CL and land that by itself (like the varint one). I will. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transport.h:210: } Note: I originally modified Transport to let QuicTransportChannel subclass it, but that is no longer necessary. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:229: ice_transport_channel_.reset( On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > If you decide to force "one QUIC channel per PeerConnection", this should fail > if you try to create a second one. > > If you decide to allow multiple QUIC channels, then you shouldn't store > ice_transport_channel_ and quic_transport_channel_. This uses QuicTransport now. QuicTransport owns the channels. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:671: #endif // USE_QUIC On 2016/03/30 20:34:50, pthatcher1 wrote: > This doesn't need USE_QUIC, does it? Fixed. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:680: return nullptr; On 2016/03/30 20:34:50, pthatcher1 wrote: > We should probably do an RTC_DCHECK if we don't put the whole method behind > USE_QUIC. This is behind HAVE_QUIC. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:96: // macro is defined. On 2016/03/30 20:34:50, pthatcher1 wrote: > Just say "if USE_QUIC is defined" I made this simpler by putting it behind HAVE_QUIC and setting "quic_ = true" instead of having a use_quic parameter https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:99: QuicTransportChannel* quic_transport_channel() const; On 2016/03/30 20:34:50, pthatcher1 wrote: > Doesn't this need to be behind USE_QUIC? Yes. Done. (Also removed quic_transport_channel() method) https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quicsessi... File webrtc/p2p/quic/quicsession.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quicsessi... webrtc/p2p/quic/quicsession.cc:82: // write_blocked_streams()->RegisterStream(stream->id(), priority); On 2016/03/30 20:34:50, pthatcher1 wrote: > What is this commented-out line? Removed. I was originally intending to allow setting write priority for QUIC streams, but am leaving it out of this CL. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:126: Transport("", nullptr), On 2016/03/30 20:34:50, pthatcher1 wrote: > Why not use channel->transport_name() for the transport name? Transport isn't subclassed anymore https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:629: // active as client. On 2016/03/30 20:34:50, pthatcher1 wrote: > Can you just delete this whole comment and write the following: > > TODO(pthatcher): Move all the negotiation up into PeerConnection and out of the > Transport code. Just have a SetRole or similar method here. Done. See QuicTransport in https://codereview.webrtc.org/1856943002. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:681: ReliableQuicStream* QuicTransportChannel::CreateQuicStream() { On 2016/03/30 20:34:50, pthatcher1 wrote: > Can you make it clear that the 0 here is the priority? Done. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.h:73: public Transport, On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > As discussed, I think it will be confusing for a QuicTransportChannel to also be > a Transport. Can you make a separate "QuicTransport" class, which is essentially > just a QuicTransportChannel factory? Look at P2PTransport for example. Done and split into a new CL. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.h:222: // Creates a new QUIC stream that can send or receive data. On 2016/03/30 20:34:50, pthatcher1 wrote: > Isn't this just for sending data (because each message gets its own stream)? Changed comment. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.h:234: const TransportDescription* local_description_for_test() const { On 2016/04/01 23:23:42, Taylor Brandstetter wrote: > I don't like having "for test" methods that expose the internals of a class. Is > there a way to do without these? This class doesn't need these anyway since it no longer subclasses Transport.
Meant to say that https://codereview.webrtc.org/1844333006/ is the bytebuffer CL and other two CLs are for the p2p directory.
https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:26: const DataChannelInit* config) On 2016/04/05 19:58:50, mikescarlett wrote: > On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > > What should happen if the data channel init is something QUIC doesn't support > > (ordered)? Maybe PeerConnection should check that and throw an error. > > By error do you mean something like "RTC_DCHECK(config->reliable && > !config->ordered)" in PeerConnection so that the app crashes with bad > parameters? I can do that to prevent unintended usage. It definitely shouldn't crash, but the API should probably should return a null data channel. https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachannel.h File webrtc/api/quicdatachannel.h (right): https://codereview.webrtc.org/1844803002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.h:90: rtc::ByteBuffer* remaining_bytes); On 2016/04/05 19:58:50, mikescarlett wrote: > On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > > Maybe make this private and declare "friend QuicTransport" (QuicDataTransport > if > > it gets renamed). Also in the comment above the class, explain the interaction > > between QuicDataChannel and QuicTransport/QuicDataTransport > > Is this to prevent the method from being exposed directly to PeerConnection? > What if I use Peter's suggestion of having QuicDataTransport create/destroy > channels, then have QuicDataTransport return channels of type > DataChannelInterface? > > I also changed the method signature to not used ByteBuffer if that was a > concern. Peter's suggestion sounds good. But even then, making this method private makes it clear that the method is an implementation detail, in case anything besides PeerConnection tries to use it at some point.
Working through this again (the parts not covered in the 3 breakout CLs) https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:1529: session_options->data_channel_type = cricket::DCT_QUIC; Why not just do session_options->data_channel_type = session_->data_channel_type()? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:1561: } Same here. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:57: if (state_ != kOpen) { Since the state is set on the signaling, I think we need to read this on the signaling thread and not the worker thread. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:57: if (state_ != kOpen) { Since state_ is set on the signaling thread, I think we need to check it on the signaling thread, and not on the worker thread (in Send, not Send_w) https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:66: } Why? If we prepend the message with the data channel ID and message ID, won't that still create a non-empty message? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:69: message_helper_.Encode(buffer, id_, ++num_sent_messages_, &payload); Instead of copying the whole message (which may be large), can we instead call stream->Write twice, once for the header and once for the payload? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:70: return WriteData_w(payload.data<char>(), payload.size()); Rather than having WriteData_w be a separate method, can you just combine the two methods? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:70: return WriteData_w(payload.data<char>(), payload.size()); Instead of making WriteData_w its own method, can you just inline all that code here? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:76: // block bytes exceeding this limit. So we can't send data messages larger than 16KB? That's going to be a problem. Another option is to buffer in this layer of the code and then send out more as SignalQueuedBytesWritten goes down to a low number (perhaps not wait until 0). Can you put a TODO to support higher than 16KB by buffering here if we can't change the QUIC maximum size message? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:110: return false; We should probably log what the result was, since it was unexpected. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:116: observer_->OnBufferedAmountChange(buffered_amount_); What thread is this called on? Because I'm pretty sure the observer_ needs to be called on the signaling thread, and buffered_amount_ needs to be changed on the signaling thread. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:123: RTC_DCHECK(channel != nullptr); RTC_DCHECK(channel) is sufficient. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:129: this, &QuicDataChannel::OnConnectionClosed); We also need to disconnect from the previous quic_transport_channel_ if it's not null. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:173: stream->Close(); I think flipping this around to if (stream->fin_received()) { ... } ... Would be slightly nicer, since the fin_received case is shorter. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.cc:216: SetState(kOpen); Actually, you need to check channel->writable(). This event fires every time it changes, and it might change to unwritable. However, you *don't* want to go back from the kOpen state once you're there. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... File webrtc/api/quicdatachannel.h (right): https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.h:30: typedef uint32_t QuicStreamId; Put a TODO to make this uint64_t. The QUIC guys said they'll make it 64-bit. https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.h:72: class MessageHelper { Why is this public? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.h:92: } Why is this protected? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.h:128: void OnMessage_s(const DataBuffer& received_data); Can you provide some comments on these? Why are the protected instead of private? https://codereview.webrtc.org/1844803002/diff/120001/webrtc/api/quicdatachann... webrtc/api/quicdatachannel.h:170: const MessageHelper& message_helper_; Hold a const ref???
Description was changed from ========== Initial QuicDataChannel implementation A QUIC data channel that implements DataChannelInterface. It replaces SCTP data channels by using a QuicTransportChannel to create a ReliableQuicStream for each message. QuicDataChannel only implements unordered, reliable delivery for the initial implementation. A QuicTransport helper class demuxes each incoming ReliableQuicStream to a QuicDataChannel when the remote peer receives a message. To allow end-to-end QuicDataChannel usage with a PeerConnection, session descriptions have been modified to include a boolean for whether to do QUIC, which serializes to SDP including an "a=quic" attribute. It is currently assumed that if the offerer specifies QUIC in the session description, then both peers must use QUIC. WebRtcSession and TransportController have been modified to allow QuicTransportChannel usage by the QuicDataChannel. BUG= ========== to ========== Initial QuicDataChannel implementation A QUIC data channel that implements DataChannelInterface. It replaces SCTP data channels by using a QuicTransportChannel to create a ReliableQuicStream for each message. QuicDataChannel only implements unordered, reliable delivery for the initial implementation. A QuicDataTransport helper class dispatches each incoming ReliableQuicStream to a QuicDataChannel when the remote peer receives a message. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. WebRtcSession and TransportController have been modified to allow QuicTransportChannel usage by the QuicDataChannel. BUG= ==========
Description was changed from ========== Initial QuicDataChannel implementation A QUIC data channel that implements DataChannelInterface. It replaces SCTP data channels by using a QuicTransportChannel to create a ReliableQuicStream for each message. QuicDataChannel only implements unordered, reliable delivery for the initial implementation. A QuicDataTransport helper class dispatches each incoming ReliableQuicStream to a QuicDataChannel when the remote peer receives a message. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. WebRtcSession and TransportController have been modified to allow QuicTransportChannel usage by the QuicDataChannel. BUG= ========== to ========== Modify PeerConnection for end-to-end QuicDataChannel usage To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicTransportChannel instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used. BUG= ==========
I split the QuicDataChannel and QuicDataTransport to a new CL: https://codereview.chromium.org/1886623002/ so that we can use this CL for discussing the PeerConnection and WebRtcSession changes. https://codereview.chromium.org/1844803002/diff/20001/webrtc/api/quicdatachan... File webrtc/api/quicdatachannel.h (right): https://codereview.chromium.org/1844803002/diff/20001/webrtc/api/quicdatachan... webrtc/api/quicdatachannel.h:90: rtc::ByteBuffer* remaining_bytes); On 2016/04/05 22:31:17, Taylor Brandstetter wrote: > On 2016/04/05 19:58:50, mikescarlett wrote: > > On 2016/04/01 23:23:41, Taylor Brandstetter wrote: > > > Maybe make this private and declare "friend QuicTransport" > (QuicDataTransport > > if > > > it gets renamed). Also in the comment above the class, explain the > interaction > > > between QuicDataChannel and QuicTransport/QuicDataTransport > > > > Is this to prevent the method from being exposed directly to PeerConnection? > > What if I use Peter's suggestion of having QuicDataTransport create/destroy > > channels, then have QuicDataTransport return channels of type > > DataChannelInterface? > > > > I also changed the method signature to not used ByteBuffer if that was a > > concern. > > Peter's suggestion sounds good. But even then, making this method private makes > it clear that the method is an implementation detail, in case anything besides > PeerConnection tries to use it at some point. I made this method private. For testing I have a fake QuicDataTransport that writes/reads everything as a string, so instead of making QuicDataTransport be a friend directly, I thought there should be a QuicMEssageDispatcher base class that handles both message encoding and decoding/dispatching, so that QuicDataTransport and the testing class can subclass it. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/peerconnect... File webrtc/api/peerconnection.cc (right): https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/peerconnect... webrtc/api/peerconnection.cc:1529: session_options->data_channel_type = cricket::DCT_QUIC; On 2016/04/12 00:58:38, pthatcher1 wrote: > Why not just do session_options->data_channel_type = > session_->data_channel_type()? Sure I'll do that. I didn't know if RTP data channels were intentionally being excluded. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/peerconnect... webrtc/api/peerconnection.cc:1561: } On 2016/04/12 00:58:38, pthatcher1 wrote: > Same here. Done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... File webrtc/api/quicdatachannel.cc (right): https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:57: if (state_ != kOpen) { On 2016/04/12 00:58:38, pthatcher1 wrote: > Since state_ is set on the signaling thread, I think we need to check it on the > signaling thread, and not on the worker thread (in Send, not Send_w) Ok done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:66: } On 2016/04/12 00:58:38, pthatcher1 wrote: > Why? If we prepend the message with the data channel ID and message ID, won't > that still create a non-empty message? I wanted to be consistent with the SCTP data channel behavior. I don't think the data channel API specifies what should happen in that case, but there's nothing preventing the message from being sent. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:69: message_helper_.Encode(buffer, id_, ++num_sent_messages_, &payload); On 2016/04/12 00:58:38, pthatcher1 wrote: > Instead of copying the whole message (which may be large), can we instead call > stream->Write twice, once for the header and once for the payload? Yes that's fine. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:70: return WriteData_w(payload.data<char>(), payload.size()); On 2016/04/12 00:58:39, pthatcher1 wrote: > Rather than having WriteData_w be a separate method, can you just combine the > two methods? I'll combine them. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:76: // block bytes exceeding this limit. On 2016/04/12 00:58:38, pthatcher1 wrote: > So we can't send data messages larger than 16KB? That's going to be a problem. > > Another option is to buffer in this layer of the code and then send out more as > SignalQueuedBytesWritten goes down to a low number (perhaps not wait until 0). > Can you put a TODO to support higher than 16KB by buffering here if we can't > change the QUIC maximum size message? > I meant to say that the session buffers messages larger than 16KB, not that they get blocked totally. But now I'm observing that increasing the limit is not required to send large messages because every time the 16KB limit is reached, the remote peer automatically sends "window update frame" packets that tell the QUIC stream to send the next 16KB. QUIC allows adjusting the QUIC flow control window size, but 16KB is the default for streams and sessions. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:110: return false; On 2016/04/12 00:58:39, pthatcher1 wrote: > We should probably log what the result was, since it was unexpected. Done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:116: observer_->OnBufferedAmountChange(buffered_amount_); On 2016/04/12 00:58:39, pthatcher1 wrote: > What thread is this called on? Because I'm pretty sure the observer_ needs to > be called on the signaling thread, and buffered_amount_ needs to be changed on > the signaling thread. OnQueuedBytesWritten is on the worker thread. I fixed it so OnBufferedAmountChange occurs on the signaling thread. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:123: RTC_DCHECK(channel != nullptr); On 2016/04/12 00:58:38, pthatcher1 wrote: > RTC_DCHECK(channel) is sufficient. Done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:129: this, &QuicDataChannel::OnConnectionClosed); On 2016/04/12 00:58:39, pthatcher1 wrote: > We also need to disconnect from the previous quic_transport_channel_ if it's not > null. This function should only be called once. I added RTC_DCHECK(!quic_transport_channel_) and updated the comments. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:173: stream->Close(); On 2016/04/12 00:58:39, pthatcher1 wrote: > I think flipping this around to > > if (stream->fin_received()) { > ... > } > ... > > Would be slightly nicer, since the fin_received case is shorter. Done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.cc:216: SetState(kOpen); On 2016/04/12 00:58:38, pthatcher1 wrote: > Actually, you need to check channel->writable(). This event fires every time it > changes, and it might change to unwritable. > > However, you *don't* want to go back from the kOpen state once you're there. Done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... File webrtc/api/quicdatachannel.h (right): https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.h:30: typedef uint32_t QuicStreamId; On 2016/04/12 00:58:39, pthatcher1 wrote: > Put a TODO to make this uint64_t. The QUIC guys said they'll make it 64-bit. Done. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.h:72: class MessageHelper { On 2016/04/12 00:58:39, pthatcher1 wrote: > Why is this public? I made it public so that my unit test and QuicDataTransport can subclass it for calling OnIncomingStream. Hopefully this class isn't confusing otherwise I'll redo it (and I moved it outside of QuicDataChannel with a new name to clarify it is a standalone class that QuicDataChannel depends on), but my intention is to allow this class to be a friend of QuicDataChannel so that OnIncomingStream can be kept private, and have subclasses of this external to the QuicDataChannel responsible for encoding/decoding the message headers. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.h:92: } On 2016/04/12 00:58:39, pthatcher1 wrote: > Why is this protected? I made this protected so that only subclasses can call QuicDataChannel::OnIncomingStream. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.h:128: void OnMessage_s(const DataBuffer& received_data); On 2016/04/12 00:58:39, pthatcher1 wrote: > Can you provide some comments on these? Why are the protected instead of > private? I added comments. They should be private since QuicDataChannel isn't designed to be subclassed. https://codereview.chromium.org/1844803002/diff/120001/webrtc/api/quicdatacha... webrtc/api/quicdatachannel.h:170: const MessageHelper& message_helper_; On 2016/04/12 00:58:39, pthatcher1 wrote: > Hold a const ref??? I didn't see the need for QuicDataChannel to modify MessageHelper, if QuicDataChannel uses this for encoding the header. It is also a ref because MessageHelper exists outside this class and is a constructor parameter.
Message was sent while issue was closed.
Description was changed from ========== Modify PeerConnection for end-to-end QuicDataChannel usage To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicTransportChannel instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used. BUG= ========== to ========== Modify PeerConnection for end-to-end QuicDataChannel usage To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicTransportChannel instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used. ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
