Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377883003:
First step in providing a UdpTransportChannel. (Closed)
DescriptionFirst step in providing a UdpTransportChannel.
Some applications explicitly require RFC3550 style RTP without ICE.
Port number requirement of RFC3550 section 11 will be addressed in a follow-up CL.
BUG=webrtc:6436
Committed: https://crrev.com/6bedebfb7a7688698dc5c26a4361791b7b1b206c
Cr-Commit-Position: refs/heads/master@{#15005}
Patch Set 1 #Patch Set 2 : Fix signed / unsigned comparison in test code. #
Total comments: 26
Patch Set 3 : Add empty override for SetMetricsObserver. #Patch Set 4 : Code style. #Patch Set 5 : Add ThreadChecker, remove Thread member, actually use pointer to SocketServer. #Patch Set 6 : Remove SetRemoteAddrAndPort() method. #
Total comments: 74
Patch Set 7 : Remove several RTC_NOTREACHED() and not-implemented declarations. #Patch Set 8 : Initialiaze in header, where possible and shorten state var names. #Patch Set 9 : Move simple getters into header file. #Patch Set 10 : Remove remote_addr_ member and make some methods private. #Patch Set 11 : Rename candidate_ => parameters_. #Patch Set 12 : Use rtc::Optional for local and remote parameters. #Patch Set 13 : Rename classes RawUdpTransoirt => UdpTransport and move files. #Patch Set 14 : Use only one socket inside UdpTransportChannel. #
Total comments: 1
Patch Set 15 : Remove rtc::UdpTransport. #Patch Set 16 : Rebase. #Patch Set 17 : Fix regression in webrtc/p2p/p2p.gyp. #Patch Set 18 : Comment on origin of overriden public methods. #Patch Set 19 : Rebase UdpTransportChannel on rtc::PacketTransportInterface. #Patch Set 20 : Use rtc::PacketSocketFactory. #Patch Set 21 : Rebase: receiving(). #
Total comments: 50
Patch Set 22 : Resolve TransportChannel dependency, address review comments. #Patch Set 23 : Revert "Remove component_ and debug_name_, just use transport_name_." #
Total comments: 16
Patch Set 24 : Unit test: VirtualSocketServer, constructor and nits. #
Total comments: 3
Patch Set 25 : Revert accidental patch of virtualsocketserver.cc. #Patch Set 26 : Format, remove comments. #Patch Set 27 : Fix gyp build file. #Patch Set 28 : Fix gyp build file, part 2. #
Total comments: 38
Patch Set 29 : Incorporate review feedback. #Patch Set 30 : Fix typo. #
Total comments: 1
Patch Set 31 : Fix typo. #Patch Set 32 : Rename CreateSocket() to Start(). #Patch Set 33 : Fix signed / unsigned assignment in unittest. #Messages
Total messages: 80 (44 generated)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
johan@webrtc.org changed reviewers: + sprang@webrtc.org
@sprang would be good to have a first review before going to owners.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... File webrtc/p2p/rawudp/rawudptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:41: remote_candidate_() { We usually omit default constructors in the initializer list. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:42: network_thread_ = network_thread; Why not in the initializer list? https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:46: RTC_DCHECK_EQ(network_thread_, rtc::Thread::Current()); Please use ThreadChecker utility, here and elsewhere. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:70: LOG(LS_VERBOSE) << "received rtp packet"; This looks extremely verbose. Will it be useful outside of development cycle debugging? If so, probably add some more metadata? https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:96: if (is_rtcp) { Just do if (!is_rtcp) { result = ... } Add the if/else back when it's actually implemented. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:106: const uint16_t maxTry = 100; I'd do static constexpr uint16_t kMaxTries = 100; https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:112: uint16_t rtpport = 2000 + (2 * count); Why 2000? Named constant? https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:203: */ Remove this comment https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:218: */ Remove this comment. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... File webrtc/p2p/rawudp/rawudptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.h:67: // &options, int flags); Did you intend to leave this comment? https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.h:169: // TODO(johan) make this private, once addcandidate is working nit: end all comments with a period. Here and elsewhere. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.h:180: void UpdateWritableState(); Move above field declarations. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.h:186: rtc::SocketServer* ss_; rtc::SocketServer* const ss_; ? On closer look, doesn't even seem to be used? https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... File webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc:37: } nit: empty line between methods. here and elsewhere https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc:170: */ Avoid /* */ blocks in favor of //-prefix for all lines. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc:178: Candidate& cand = ep1_.local_candidates_[0]; Please use only const references, or pointer types if non-const methods are called.
https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... File webrtc/p2p/rawudp/rawudptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:41: remote_candidate_() { On 2016/10/07 09:56:21, språng wrote: > We usually omit default constructors in the initializer list. Acknowledged. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:42: network_thread_ = network_thread; On 2016/10/07 09:56:21, språng wrote: > Why not in the initializer list? Leftover from a different implementation approach. Moving it to the initializer. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:70: LOG(LS_VERBOSE) << "received rtp packet"; On 2016/10/07 09:56:21, språng wrote: > This looks extremely verbose. Will it be useful outside of development cycle > debugging? If so, probably add some more metadata? Only useful for debug. Removing it. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:96: if (is_rtcp) { On 2016/10/07 09:56:21, språng wrote: > Just do > if (!is_rtcp) { result = ... } > Add the if/else back when it's actually implemented. Acknowledged. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:106: const uint16_t maxTry = 100; On 2016/10/07 09:56:21, språng wrote: > I'd do > static constexpr uint16_t kMaxTries = 100; Acknowledged. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:112: uint16_t rtpport = 2000 + (2 * count); On 2016/10/07 09:56:21, språng wrote: > Why 2000? Named constant? Changing this to a constant. Will probably change to a config option in the future. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:203: */ On 2016/10/07 09:56:21, språng wrote: > Remove this comment Acknowledged. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel.cc:218: */ On 2016/10/07 09:56:21, språng wrote: > Remove this comment. Acknowledged. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... File webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc:170: */ On 2016/10/07 09:56:21, språng wrote: > Avoid /* */ blocks in favor of //-prefix for all lines. Acknowledged. https://codereview.webrtc.org/2377883003/diff/20001/webrtc/p2p/rawudp/rawudpt... webrtc/p2p/rawudp/rawudptransportchannel_unittest.cc:178: Candidate& cand = ep1_.local_candidates_[0]; On 2016/10/07 09:56:21, språng wrote: > Please use only const references, or pointer types if non-const methods are > called. Good one. Actually I should have a copy in this place.
Description was changed from ========== First step in providing a RawUdpTransportChannel. Some apllications explicit required RFC3550 section 11 style RTP. BUG=webrtc:6436 ========== to ========== First step in providing a RawUdpTransportChannel. Some apllications explicit require RFC3550 section 11 style RTP. BUG=webrtc:6436 ==========
johan@webrtc.org changed reviewers: + pthatcher@webrtc.org
pthatcher, feedback on this not yet complete udp transport channel would be welcome.
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
Tayor, can you review this once it's gone through another pass, especially the unit tests (which I haven't look at yet). Andrei, this is related to what you're working on with the PacketTransport and breaking up ICE and DTLS. I think it would be good to define the PacketTransportInterface before finishing this CL, whether you do it or whether Johan does it. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/BUILD.gn#newcode440 webrtc/BUILD.gn:440: "p2p/rawudp/rawudptransportchannel_unittest.cc", It doesn't need to go into a separate directory. Just stick it in p2p/base. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/p2p.gyp File webrtc/p2p/p2p.gyp (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/p2p.gyp#newco... webrtc/p2p/p2p.gyp:77: 'rawudp/rawudptransportchannel.h', I'm guessing udptransportchannel.cc and UdpTransportChannel is sufficient (no Raw). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransport.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransport.h:22: class RawUdpTransport : public Transport { I don't think you need a UdpTransport. I think you just need a UdpTransportChannel. Transports are silly things that we should delete and replace with just TransportChannels (and then rename TransportChannels to Transports). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:57: // loop. I don't think this is comment is needed, nor is any thread checking needed. The whole class must be used only on the thread it's constructed with, so it's enough to just say that in a class-level comment. I don't think we have these kind of checks and comments in other transport channels (I could be wrong). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:82: true /*rtcp*/); Please remove these and just have OnSocketReadPacket. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:95: } Again, you don't need any of that. Just do socket_->SendTo. RTP/RTCP difference is handled by channel.cc. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:109: for (uint16_t count = 0; count < kMaxTries; ++count) { It seems like you should use a rtc::PacketSocketFactory and call CreateUdpSocket. The PacketSocketFactory could be passed into the UdpTransportChannel. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:117: rtcp_socket_addr.SetPort(rtpport + 1); How important is this? Cause it's pretty obnoxious. But if we have to do it, I would suggest this logic is at a higher level where some one does code like this: UdpTransportChannel rtp; UdpTransportChannel rtcp; rtp.Start(); int rtcp_port = rtp.local_parameters().port + 1; if (!rtcp.Start({MinPort: rtcp_port; MaxPort: rtcp_port} && rtcp.GetError() == NO_PORT_IN_RANGE)) { // Try again with another port. } Then in this class, you just need to be able to specify the range and have a special error for "no port in range". https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:135: void RawUdpTransportChannel::MaybeStartGathering() { A better name for this would be Start(), which goes to the WAITING or CONNECTED state after opening the socket. Or going to the FAILED state if it couldn't open the socket. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:171: // set. Maybe use an rtc::Optional to indicate unset addr? Yes, rtc::Optional<UdpTransportChannelParameters> would be a good type rather than UdpTransportChannelParameters. The local one would be filled when gathering is complete. The remote one would be filled when SetRemoteParameters is called. I think this whole method could be: void UdpTransportChannel::SetRemoteParameters(const& UdpTransportChannelParameters remote) { remote_parameters_ = new rtc::Optional(remote); UpdateState(); } And UpdateState could just be: UdpTransportChannelState state = local_parameters_ && remote_parameters_; if (state_ == state) { return; } state_ = state; SignalStateChanged(this); https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:185: } These could just go in the .h. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:188: IceGatheringState new_gathering_state) { You don't need "new_" https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:32: // TODO(johan) add Rtcp sockaddr to candidate Style: "TODO(johan): Add RTCP port." Actually, a transport channel should be either RTP or RTCP, not both. If you need two ports, make two transport channels (which is what channel.cc does). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:35: class RawUdpTransportChannel : public TransportChannelImpl { Why bother with TransportChannelImpl? Why not use TransportChannel? Actually, what would make this much more simple is to do the following first: 1. Make a new PacketTransportInterface which has the methods/signals used in channel.cc that are actually relevant: - transport_name - writable - SetOption - SendPacket - GetError - GetStats - SignalWritableState - SignalReadyToSend - SignalReadPacket - SignalSentPacket Don't add these because while they are used in channel.cc, we can refactor channel.cc to know it's not just a PacketTransportInterface but also a crypto thing. - IsDtlsActive - GetSslRole - SetSrtpCryptoSuites - SetSrtpCryptoSuite - ExportKeyingMaterial Do add this one, which even though it's ICE-specific, we can fix that later: - SignalSelectedCandidatePairChange 2. Make TransportChannel subclass PacketTransportInterface and remove the above from TransportChannel. In other words, move these from TransportChannel to PacketTransportInterface. 3. Only implement PacketTransportInterface, not the rest of it. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:39: // when available Style: "TODO(johan): Replace raw pointers by (weak) references to std::shared_ptr when available." https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:64: const rtc::PacketTime& packet_time); Can you put a comment some where that says something like "Implements methods of TransportChannel" and then "Methods unique to UdpTransportChannel"? Or, better yet, make it all just implement PacketTransportInterface. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:65: int SetRemoteAddr(const char* addr); I'd prefer you have a SetRemoteParameters(const UdpTransportChannelParameters& parameters) and then a UdpTransportChannelParameters struct. That would more closely match other transport channels and make it easier to add things in the future. Also, this needs a comment about what it does and what the return value is. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:71: } That's not going to work. SetOption is definitely called by users of transport channels (in particular in channel.cc). You'd be better off without the RTC_NOTREACHED(). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:75: return 0; Same here https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:81: } And here https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:85: return false; And here. Although removing it would be even better (only implement PacketTransportInterface). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:89: RTC_NOTREACHED(); This one should stay, and the next few, unless you do the PacketTransportInteface idea. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:134: void SetIceConfig(const IceConfig& config) override { RTC_NOTREACHED(); } FYI, we're currently refactoring all of this. Can you put a "TODO: Remove all ICE- and DTLS-specific methods"? Either that or just to the PacketTransportInterface idea. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:138: void AddRemoteCandidate(const Candidate& candidate) override; Shouldn't the "remote candidate" be contained in the remote parameters (the remote address)? We have both? A "candidate" is a very ICE-specific thing (we should rename it IceCandidate). I don't think it fits here. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:142: // TODO(johan) implement Why would you implement this? It's an ICE thing. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:167: RTC_NOTREACHED(); I don't think you should have this NOTREACHED(). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:171: bool SetRemoteAddrAndPorts(const char* ip_addr, int rtp_port); I think you should not implement AddCandidate, and instead have this. Except make it SetRemoteParameters. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:179: TransportChannelState tch_state_; Just state_ is enough. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:181: void UpdateWritableState(); I don't think you need UpdateWritableState(), because I don't think you need a separate writable state. And I don't think you need a separate gathering state either. You just need a state, with an enum like: class enum UdpTransportChannelState { INIT // Hasn't started yet (no local parameters) WAITING // No remote parameters CONNECTED // Both local and remote parameters FAILED // Couldn't open a socket } https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:183: std::unique_ptr<rtc::AsyncUDPSocket> rtcp_socket_; As I mentioned, a transport channel should have only one socket. If you want one for RTP and RTCP, the model is to have two transport channels. And that's what "component" represents: 1 is RTP, 2 is RTCP. But all of this is taken care of in channel.cc. You don't need to do it here. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:185: Candidate remote_candidate_; This should be local_parameters_ and remote_parameters_. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:186: rtc::SocketAddress remote_addr_; remote_addr_ should be a part of remote_parameters_.
Description was changed from ========== First step in providing a RawUdpTransportChannel. Some apllications explicit require RFC3550 section 11 style RTP. BUG=webrtc:6436 ========== to ========== First step in providing a RawUdpTransportChannel. Some applications explicit require RFC3550 section 11 style RTP. BUG=webrtc:6436 ==========
Description was changed from ========== First step in providing a RawUdpTransportChannel. Some applications explicit require RFC3550 section 11 style RTP. BUG=webrtc:6436 ========== to ========== First step in providing a RawUdpTransportChannel. Some applications explicitly require RFC3550 section 11 style RTP. BUG=webrtc:6436 ==========
I think Peter, you and I need to get on the same page about the overall design. Mostly, whether RawUdpTransportChannel will inherit from the ICE/DTLS channel base class, or just the PacketTransportInterface as Peter suggests. And whether we'll try to retrofit ICE methods (like "MaybeStartGathering", "AddRemoteIceCandidate"), or add new methods that make more sense for a raw transport. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransport.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransport.h:22: class RawUdpTransport : public Transport { On 2016/10/07 17:36:36, pthatcher1 wrote: > I don't think you need a UdpTransport. I think you just need a > UdpTransportChannel. Transports are silly things that we should delete and > replace with just TransportChannels (and then rename TransportChannels to > Transports). When are we planning on doing that though? We talked about that coming later. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:30: RTC_DCHECK_RUN_ON(&network_thread_checker_); The constructor is where the thread checker is initialized so this is redundant. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:39: gathering_state_(kIceGatheringNew) { Can initialize these to default values in the header. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:40: socket_server_ = socket_server; Can set socket_server_ in initializer list. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:57: // loop. On 2016/10/07 17:36:36, pthatcher1 wrote: > I don't think this is comment is needed, nor is any thread checking needed. The > whole class must be used only on the thread it's constructed with, so it's > enough to just say that in a class-level comment. I don't think we have these > kind of checks and comments in other transport channels (I could be wrong). I see the value of these checks. But since they're DCHECKs they can happen everywhere, since they only happen on debug builds. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:142: TryAllocateSockets(); Why does this return a bool if it's not used? https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:31: class RawUdpCandidate : public Candidate { The Candidate struct already has an IP/port/protocol so I don't see why you need to subclass it. If anything, you could just use an rtc::SocketAddress. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:39: // when available Do we really plan to do this? It's the first I've heard, I'm interested in the justification. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:59: const rtc::PacketTime& packet_time); To match our current design, a transport channel would be just one socket. Two sockets = two transport channels. Also, these shouldn't be public. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:65: int SetRemoteAddr(const char* addr); You could use an rtc::SocketAddress here. And can you document the return value? https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:176: IceGatheringState new_gathering_state); // Set State and Signal nit: "Set state and signal."
We're going to do the PacketTransportInterface very soon anyway. Either Johan does it or Andrei does it. Whoever gets there first. If we had already done the PacketTransportInterface thing, this CL would have been much easier/smaller. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransport.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransport.h:22: class RawUdpTransport : public Transport { On 2016/10/07 18:24:02, Taylor Brandstetter wrote: > On 2016/10/07 17:36:36, pthatcher1 wrote: > > I don't think you need a UdpTransport. I think you just need a > > UdpTransportChannel. Transports are silly things that we should delete and > > replace with just TransportChannels (and then rename TransportChannels to > > Transports). > > When are we planning on doing that though? We talked about that coming later. We'll remove the ICE and DTLS ones later. But we don't need to add a *new* one now. There's no need for it. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:39: // when available On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > Do we really plan to do this? It's the first I've heard, I'm interested in the > justification. There are a few other places in the code with comments like that. I'd rather just leave it as a raw pointer and just remove the TODO, though.
Peter, Taylor, my next steps would be the following: 1) resolve all the "small" issues not depending on PacketTransportInterface or Sockets. 2) Rename RawUdpTansportChannel => UdpTransportChannel and move files to p2p/base. 3) Remove the second socket, use component field. 4) Create example implementation for RTP/RTCP port number requirement. 5) Introduce PacketTransportInterface in dedicated CL. 6) Let PacketTransportInterface directly inherit UdpTransportChannel. 7) Implement CreateUdpTransportChannel for TransportController (dedicated CL). https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:30: RTC_DCHECK_RUN_ON(&network_thread_checker_); On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > The constructor is where the thread checker is initialized so this is redundant. You have a point here. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:39: gathering_state_(kIceGatheringNew) { On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > Can initialize these to default values in the header. Done. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:40: socket_server_ = socket_server; On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > Can set socket_server_ in initializer list. Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:57: // loop. On 2016/10/07 17:36:36, pthatcher1 wrote: > I don't think this is comment is needed, nor is any thread checking needed. The > whole class must be used only on the thread it's constructed with, so it's > enough to just say that in a class-level comment. I don't think we have these > kind of checks and comments in other transport channels (I could be wrong). FYI: similar checks are present in some methods of cricket::P2PTransportChannel. Only difference is, that they are implemented using ASSERT(), not ThreadChecker. Example can be found in https://chromium.googlesource.com/external/webrtc/+/master/webrtc/p2p/base/p2... . https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:109: for (uint16_t count = 0; count < kMaxTries; ++count) { On 2016/10/07 17:36:36, pthatcher1 wrote: > It seems like you should use a rtc::PacketSocketFactory and call > CreateUdpSocket. The PacketSocketFactory could be passed into the > UdpTransportChannel. rtc::PacketSocketFactory seems to be the right thing. Maybe with a patch for the {rtp, rtcp} port number requirements mentioned below. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:117: rtcp_socket_addr.SetPort(rtpport + 1); On 2016/10/07 17:36:36, pthatcher1 wrote: > How important is this? Cause it's pretty obnoxious. Hard requirement. RFC 3550 Section 11 describes "socket pairs" of {rtp port (even number), rtcp port (rtp port + 1, odd)}. SDP will list only the rtp port number, rtcp is derived from that. And yes, this part of RFC 3550 is pretty obnoxious. But for the sake of third party interoperability we have to implement it. >But if we have to do it, I > would suggest this logic is at a higher level where some one does code like > this: I'm fine with placing this logic at a higher level. Probably in the not yet existing cricket::TransportController::CreateUdpTransportChannel(). > > UdpTransportChannel rtp; > UdpTransportChannel rtcp; > rtp.Start(); > int rtcp_port = rtp.local_parameters().port + 1; > if (!rtcp.Start({MinPort: rtcp_port; MaxPort: rtcp_port} && rtcp.GetError() == > NO_PORT_IN_RANGE)) { > // Try again with another port. > } > > > Then in this class, you just need to be able to specify the range and have a > special error for "no port in range". https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:135: void RawUdpTransportChannel::MaybeStartGathering() { On 2016/10/07 17:36:36, pthatcher1 wrote: > A better name for this would be Start(), which goes to the WAITING or CONNECTED > state after opening the socket. Or going to the FAILED state if it couldn't > open the socket. Renaming depends on PacketTransportInterface. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:142: TryAllocateSockets(); On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > Why does this return a bool if it's not used? Should be used, will fix it. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:185: } On 2016/10/07 17:36:36, pthatcher1 wrote: > These could just go in the .h. Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.cc:188: IceGatheringState new_gathering_state) { On 2016/10/07 17:36:36, pthatcher1 wrote: > You don't need "new_" Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... File webrtc/p2p/rawudp/rawudptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:31: class RawUdpCandidate : public Candidate { On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > The Candidate struct already has an IP/port/protocol so I don't see why you need > to subclass it. If anything, you could just use an rtc::SocketAddress. Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:35: class RawUdpTransportChannel : public TransportChannelImpl { On 2016/10/07 17:36:37, pthatcher1 wrote: > Why bother with TransportChannelImpl? Why not use TransportChannel? Currently TransportController::CreateTransportChannel_n() explicit instantiates both cricket::Transport and cricket::TransportChannelImpl. No cricket::TransportChannel in that place. This dependency goes away, if each UdpTransportChannel gets its own TransportController::CreateUdpTransportChannel() factory method. The PacketTransportInterface is tempting. > > Actually, what would make this much more simple is to do the following first: > > 1. Make a new PacketTransportInterface which has the methods/signals used in > channel.cc that are actually relevant: > > - transport_name > - writable > - SetOption > - SendPacket > - GetError > - GetStats > - SignalWritableState > - SignalReadyToSend > - SignalReadPacket > - SignalSentPacket > > Don't add these because while they are used in channel.cc, we can refactor > channel.cc to know it's not just a PacketTransportInterface but also a crypto > thing. > - IsDtlsActive > - GetSslRole > - SetSrtpCryptoSuites > - SetSrtpCryptoSuite > - ExportKeyingMaterial > > Do add this one, which even though it's ICE-specific, we can fix that later: > - SignalSelectedCandidatePairChange > > > 2. Make TransportChannel subclass PacketTransportInterface and remove the above > from TransportChannel. In other words, move these from TransportChannel to > PacketTransportInterface. > > 3. Only implement PacketTransportInterface, not the rest of it. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:39: // when available On 2016/10/07 21:15:39, pthatcher1 wrote: > On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > > Do we really plan to do this? It's the first I've heard, I'm interested in the > > justification. > > There are a few other places in the code with comments like that. > > I'd rather just leave it as a raw pointer and just remove the TODO, though. Here the shared pointer would just by my personal preference. Removing the TODO for now. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:59: const rtc::PacketTime& packet_time); On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > To match our current design, a transport channel would be just one socket. Two > sockets = two transport channels. > > Also, these shouldn't be public. Ack on "should not be public". See my other comments for the two sockets. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:65: int SetRemoteAddr(const char* addr); On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > You could use an rtc::SocketAddress here. And can you document the return value? Blame me, I had already removed this method's implementation. Removing the declaration, too, now. Further discussion below. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:71: } On 2016/10/07 17:36:36, pthatcher1 wrote: > That's not going to work. SetOption is definitely called by users of transport > channels (in particular in channel.cc). You'd be better off without the > RTC_NOTREACHED(). Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:75: return 0; On 2016/10/07 17:36:37, pthatcher1 wrote: > Same here Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:81: } On 2016/10/07 17:36:36, pthatcher1 wrote: > And here Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:85: return false; On 2016/10/07 17:36:37, pthatcher1 wrote: > And here. Although removing it would be even better (only implement > PacketTransportInterface). Replacing the RTC_NOTREACHED() by a method removal todo. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:89: RTC_NOTREACHED(); On 2016/10/07 17:36:37, pthatcher1 wrote: > This one should stay, and the next few, unless you do the > PacketTransportInteface idea. Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:134: void SetIceConfig(const IceConfig& config) override { RTC_NOTREACHED(); } On 2016/10/07 17:36:37, pthatcher1 wrote: > FYI, we're currently refactoring all of this. Can you put a "TODO: Remove all > ICE- and DTLS-specific methods"? > > Either that or just to the PacketTransportInterface idea. Adding the TODO with next patch set. Maybe switching to the PacketTransportInterface with a later patch set. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:138: void AddRemoteCandidate(const Candidate& candidate) override; On 2016/10/07 17:36:36, pthatcher1 wrote: > Shouldn't the "remote candidate" be contained in the remote parameters (the > remote address)? We have both? A "candidate" is a very ICE-specific thing (we > should rename it IceCandidate). I don't think it fits here. Currently AddRemoteCandidate() is TransportController's only method of setting remote endpoint information in a TransportChannel. Remote address is contained as rtc::SocketAddress in Candidate data structure - together with a bunch of ICE specific data fields. Happy to replace this method when PacketTransportInterface is available. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:142: // TODO(johan) implement On 2016/10/07 17:36:36, pthatcher1 wrote: > Why would you implement this? It's an ICE thing. Removing the TODO. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:167: RTC_NOTREACHED(); On 2016/10/07 17:36:37, pthatcher1 wrote: > I don't think you should have this NOTREACHED(). Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:171: bool SetRemoteAddrAndPorts(const char* ip_addr, int rtp_port); On 2016/10/07 17:36:36, pthatcher1 wrote: > I think you should not implement AddCandidate, and instead have this. Except > make it SetRemoteParameters. My fault. This declaration is a left-over from my early experiments. Removing it for now, as there is now implementation. AddCandidate currently has an implementation. Can remove AddCandidate and add SetRemoteParameters, once interaction with TransportController and RtpSender is figured out. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:176: IceGatheringState new_gathering_state); // Set State and Signal On 2016/10/07 18:24:03, Taylor Brandstetter wrote: > nit: "Set state and signal." Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:179: TransportChannelState tch_state_; On 2016/10/07 17:36:37, pthatcher1 wrote: > Just state_ is enough. Acknowledged. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:180: IceGatheringState gathering_state_; Whether gathering_state_ is required depends on inherited interfaces of UdpTransportChannel and the "TransportChannel type awareness" of TransportController and channel.cc . Will leave it in place for now and remove it when PacketTransportInterface is available. https://codereview.webrtc.org/2377883003/diff/100001/webrtc/p2p/rawudp/rawudp... webrtc/p2p/rawudp/rawudptransportchannel.h:181: void UpdateWritableState(); On 2016/10/07 17:36:36, pthatcher1 wrote: > I don't think you need UpdateWritableState(), because I don't think you need a > separate writable state. And I don't think you need a separate gathering state > either. You just need a state, with an enum like: > > class enum UdpTransportChannelState { > INIT // Hasn't started yet (no local parameters) > WAITING // No remote parameters > CONNECTED // Both local and remote parameters > FAILED // Couldn't open a socket > } cricket::TransportChannel has a non virtual method "bool writable() const { return writable_; }". So UdpTransportChannel needs to update the writable_ state variable. Cleaner approach could be making "bool writable()" virtual and implement an override that derives writable from UdpTransportChannel's state.
Most of the "smaller" issues should be resolved. Remaining issues: * Use rtc::PacketSocketFactory in a way that satisfies RFC 3550 section 11's port number requirements for RTP and RTCP * Do modifications that depend on the decision whether to implement cricket::TransportChannel or rtc::PacketTransport. * Integrate with TransportController (could be done in dedicated CL)
Let's land the PacketTransport CL, rebase this one on it, and then take another look. https://codereview.webrtc.org/2377883003/diff/260001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransport.h (right): https://codereview.webrtc.org/2377883003/diff/260001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransport.h:22: class UdpTransport : public Transport { Can you remove this class from this CL? We can decide if you really need it when we use UdpTransportChannel in channel.cc in a later CL. I don't think you will, but there's no point holding up this CL discussing it now.
Peter, Taylor, I would like to bring up a design discussion again, now that the PacketTransportInterface CL is landed. Quote (Taylor, 2016-10-07, https://codereview.webrtc.org/2377883003/#msg20 ): > I think Peter, you and I need to get on the same page about the overall design. > Mostly, whether UdpTransportChannel will inherit from the ICE/DTLS channel > base class, or just the PacketTransportInterface as Peter suggests. And whether > we'll try to retrofit ICE methods (like "MaybeStartGathering", > "AddRemoteIceCandidate"), or add new methods that make more sense for a raw > transport.
On 2016/10/25 22:39:22, johan wrote: > Peter, Taylor, I would like to bring up a design discussion again, now that the > PacketTransportInterface CL is landed. > > Quote (Taylor, 2016-10-07, https://codereview.webrtc.org/2377883003/#msg20 ): > > I think Peter, you and I need to get on the same page about the overall > design. > > Mostly, whether UdpTransportChannel will inherit from the ICE/DTLS channel > > base class, or just the PacketTransportInterface as Peter suggests. And > whether > > we'll try to retrofit ICE methods (like "MaybeStartGathering", > > "AddRemoteIceCandidate"), or add new methods that make more sense for a raw > > transport. OK. Would it make sense to start by rebasing this CL on PacketTransportInterface?
On 2016/10/26 22:41:28, pthatcher1 wrote: > OK. Would it make sense to start by rebasing this CL on > PacketTransportInterface? Well, I can upload a version that directly inherits from PacketTransportInterface. Will be interesting to figure out interaction with channel.cc and VideoRtpSender or AudioRtpSender.
PTAL, rebase done. We can revert it, if necessary. I think the basic question/decision is: a) either make some modifications to channel.cc or TransportController, so that they can work with a PacketTransportInterface implementation b) or let UdpTransportChannel implement cricket::TransportChannel and keep changes to TransportController and channel.cc at a minimum.
Description was changed from ========== First step in providing a RawUdpTransportChannel. Some applications explicitly require RFC3550 section 11 style RTP. BUG=webrtc:6436 ========== to ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 section 11 style RTP. BUG=webrtc:6436 ==========
As I mentioned in email, I prefer a) out of the two options, unless we find something that makes it really difficult. Also, sorry if I'm commenting on things I already commented on before, I got a little lost in the history. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:37: RTC_DCHECK_RUN_ON(&network_thread_checker_); This isn't necessary, since the constructor is where network_thread_checker_ is initialized. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:51: // No thread_checker in high frequency network function. I think a DCHECK should be fine, even if it's high-frequency, since it only affects debug builds. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:90: LOG(LS_WARNING) << "Local Socket already allocated."; nit: Don't need to capitalize Socket https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:127: LOG(INFO) << "socket_ is false"; nit: "null" instead of "false" Also, should this ever be possible given how the class is designed? If not, I'd just use an RTC_DCHECK. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:137: if (!remote_parameters_->address().IsComplete()) { Could you check for this when AddRemoteCandidate is called? If so, then the "IsXConsistent" methods wouldn't be necessary, the class could just prevent anything from ever getting inconsistent. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:138: LOG(INFO) << "remote_addr_ not complete"; Don't have something called "remote_addr_" any more. Could just say "remote address". https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:158: state = STATE_FAILED; This makes the TransportChannelState seem redundant; the caller can figure out what it needs to by a combination of GetLocalCandidate() and writable(). I'd suggest getting rid of state_, and then all you need to keep track of is whether you've done SignalWritableState yet. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:171: return *local_parameters_; I think this will crash if local_parameters_ isn't set yet; can it return a default constructed Candidate (or SocketAddress) instead? https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:19: #include "webrtc/p2p/base/transportchannel.h" It would be nice if this could depend only on PacketTransportInterface and not TransportChannel. Is TransportChannelState the only thing used? https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:30: If this class encapsulates a combination of RTP/RTCP sockets following RFC 3550 rules, I think that should be addressed in a comment above the class declaration. It's not obvious from the name of the class. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:33: UdpTransportChannel(const std::string& transport_name, int component); If this is always RTP+RTCP, it doesn't need a component argument. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:60: TransportChannelState GetState() const { I agree there are a lot of states, but in my opinion it's still worth defining a new enum in order to avoid a dependency on TransportChannel. If it's really necessary. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:68: void AddRemoteCandidate(const Candidate& candidate); As long as this is (now) a new interface, we could just use rtc::SocketAddresses here.
https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:62: debug_name_ = transport_name_ + " " + std::to_string(component_); Just do "return transport_name_". https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:87: void UdpTransportChannel::TryAllocateSocket() { I'd call this CreateSocket, since what it does it call CreateUdpSocket and store it. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:101: // control level. I think it should be done at the RtpSender/RtpReceiver level. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:107: LOG(INFO) << "Allocated Rtp socket with local port " << port; I'd say "Created UDP socket with port " https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:142: } The above two methods are only used once. I'd prefer to just inline them in UpdateState(). https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:178: state_ = state; Please use early returns. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:19: #include "webrtc/p2p/base/transportchannel.h" On 2016/11/01 21:32:49, Taylor Brandstetter wrote: > It would be nice if this could depend only on PacketTransportInterface and not > TransportChannel. Is TransportChannelState the only thing used? I agree. Just make a new state enum. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:30: On 2016/11/01 21:32:49, Taylor Brandstetter wrote: > If this class encapsulates a combination of RTP/RTCP sockets following RFC 3550 > rules, I think that should be addressed in a comment above the class > declaration. It's not obvious from the name of the class. It doesn't. This should be one UDP socket/port and having two ports should be handled up higher, such as in RtpSender/RtpReceiver. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:33: UdpTransportChannel(const std::string& transport_name, int component); On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > If this is always RTP+RTCP, it doesn't need a component argument. As far as I can tell, the component is only used for the debug string. So why not just use the transport_name for that and drop the component? https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:43: return true; Might as well implement it. It's basically just recording a single class field of the last received time and compare that to the current time. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:60: TransportChannelState GetState() const { On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > I agree there are a lot of states, but in my opinion it's still worth defining a > new enum in order to avoid a dependency on TransportChannel. If it's really > necessary. I agree. Make a new enum. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:68: void AddRemoteCandidate(const Candidate& candidate); On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > As long as this is (now) a new interface, we could just use rtc::SocketAddresses > here. Yeah, we shouldn't use candidates. They are specific to ICE. I would call this SetRemoteParameters. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:72: const Candidate& GetLocalCandidate(); I would call this local_parameters() https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/p2p.gyp File webrtc/p2p/p2p.gyp (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/p2p.gyp#newco... webrtc/p2p/p2p.gyp:74: 'base/udptransport.h', These files don't exist.
I should have addressed all review comments. Apologies, if I missed one. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:37: RTC_DCHECK_RUN_ON(&network_thread_checker_); On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > This isn't necessary, since the constructor is where network_thread_checker_ is > initialized. Done. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:51: // No thread_checker in high frequency network function. On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > I think a DCHECK should be fine, even if it's high-frequency, since it only > affects debug builds. The thread checker's CalledOnValidThread() method uses a CritScope lock. I do not want to hunt down potential network timing differences between debug builds and release builds. Especially not in a function that is called every 10 ms (shortest reasonable audio packet length). Otherwise DCHECKs are great! https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:62: debug_name_ = transport_name_ + " " + std::to_string(component_); On 2016/11/02 06:19:37, pthatcher1 wrote: > Just do "return transport_name_". Done. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:87: void UdpTransportChannel::TryAllocateSocket() { On 2016/11/02 06:19:37, pthatcher1 wrote: > I'd call this CreateSocket, since what it does it call CreateUdpSocket and store > it. Done. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:90: LOG(LS_WARNING) << "Local Socket already allocated."; On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > nit: Don't need to capitalize Socket Acknowledged. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:107: LOG(INFO) << "Allocated Rtp socket with local port " << port; On 2016/11/02 06:19:37, pthatcher1 wrote: > I'd say "Created UDP socket with port " Done. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:127: LOG(INFO) << "socket_ is false"; On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > nit: "null" instead of "false" > Also, should this ever be possible given how the class is designed? If not, I'd > just use an RTC_DCHECK. Inlined into UpdateState() as Peter suggests. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:137: if (!remote_parameters_->address().IsComplete()) { On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > Could you check for this when AddRemoteCandidate is called? If so, then the > "IsXConsistent" methods wouldn't be necessary, the class could just prevent > anything from ever getting inconsistent. Can do that. Side effect: state does not change to STATE_FAIL anymore, when caller tries to set an invalid remote address. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:138: LOG(INFO) << "remote_addr_ not complete"; On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > Don't have something called "remote_addr_" any more. Could just say "remote > address". Acknowledged. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:142: } On 2016/11/02 06:19:37, pthatcher1 wrote: > The above two methods are only used once. I'd prefer to just inline them in > UpdateState(). Inlining IsLocalConsistent() in UpdateState(). Doing the remote addr check on SetRemoteParameters(). https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:158: state = STATE_FAILED; On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > This makes the TransportChannelState seem redundant; the caller can figure out > what it needs to by a combination of GetLocalCandidate() and writable(). I'd > suggest getting rid of state_, and then all you need to keep track of is whether > you've done SignalWritableState yet. May be redundant, but usually state dependent logic in the controlling instance will be way cleaner, if there are distinct states, represented by single enum. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:171: return *local_parameters_; On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > I think this will crash if local_parameters_ isn't set yet; can it return a > default constructed Candidate (or SocketAddress) instead? Oups, adding default + unit test. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:178: state_ = state; On 2016/11/02 06:19:37, pthatcher1 wrote: > Please use early returns. Ok, returning early if (state_ == state) and resolving the nested if. Sticking to "if (state == STATE_COMPLETED) {Signal...()}" as this is not the default case. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:19: #include "webrtc/p2p/base/transportchannel.h" On 2016/11/02 06:19:38, pthatcher1 wrote: > On 2016/11/01 21:32:49, Taylor Brandstetter wrote: > > It would be nice if this could depend only on PacketTransportInterface and not > > TransportChannel. Is TransportChannelState the only thing used? > > I agree. Just make a new state enum. Ok https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:33: UdpTransportChannel(const std::string& transport_name, int component); On 2016/11/02 06:19:37, pthatcher1 wrote: > On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > > If this is always RTP+RTCP, it doesn't need a component argument. > > As far as I can tell, the component is only used for the debug string. So why > not just use the transport_name for that and drop the component? Can do that. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:43: return true; On 2016/11/02 06:19:37, pthatcher1 wrote: > Might as well implement it. It's basically just recording a single class field > of the last received time and compare that to the current time. Yes, receiving() is no rocket science, but I would still prefer to implement it in a dedicated smaller CL. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:60: TransportChannelState GetState() const { On 2016/11/02 06:19:37, pthatcher1 wrote: > On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > > I agree there are a lot of states, but in my opinion it's still worth defining > a > > new enum in order to avoid a dependency on TransportChannel. If it's really > > necessary. > > I agree. Make a new enum. Done. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:68: void AddRemoteCandidate(const Candidate& candidate); On 2016/11/02 06:19:37, pthatcher1 wrote: > On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > > As long as this is (now) a new interface, we could just use > rtc::SocketAddresses > > here. > > Yeah, we shouldn't use candidates. They are specific to ICE. I would call this > SetRemoteParameters. Done. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:72: const Candidate& GetLocalCandidate(); On 2016/11/02 06:19:37, pthatcher1 wrote: > I would call this local_parameters() Ok, the naming pattern for cheap functions from the style guide takes precedence over the convinience of having similar name patterns for Set and Get methods. https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/p2p.gyp File webrtc/p2p/p2p.gyp (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/p2p.gyp#newco... webrtc/p2p/p2p.gyp:74: 'base/udptransport.h', On 2016/11/02 06:19:38, pthatcher1 wrote: > These files don't exist. Acknowledged.
https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:30: On 2016/11/02 06:19:37, pthatcher1 wrote: > On 2016/11/01 21:32:49, Taylor Brandstetter wrote: > > If this class encapsulates a combination of RTP/RTCP sockets following RFC > 3550 > > rules, I think that should be addressed in a comment above the class > > declaration. It's not obvious from the name of the class. > > It doesn't. This should be one UDP socket/port and having two ports should be > handled up higher, such as in RtpSender/RtpReceiver. It's not clear to me how that will work, or how it *could* work given this class as implemented in this CL. If "TryAllocateSocket" takes no arguments and provides no guarantees on port number, how do you create two UdpTransportChannels with compatible port numbers? When I reviewed this CL, my assumption was that it would implement what's described in the design doc, which is a combined RTP/RTCP transport. If that's not the case, I think we need to go back to the design doc and flesh this out. But I think the combined design is fine, personally. Also, if we have separate UdpTransportChannels for RTP/RTCP, then we *do* need component() to identify them after all...
Ok, as far as I can tell, this CL is only blocked by a missing agreement on the design decision regarding the RTP/RTCP port number requirement from RFC 3550 section 11. My suggestion would be to postpone that decision to a dedicated smaller follow-up CL. Landing this CL would be very helpful for follow-up tasks. They are currently blocked. On 2016/11/02 19:15:20, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... > File webrtc/p2p/base/udptransportchannel.h (right): > > https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... > webrtc/p2p/base/udptransportchannel.h:30: > On 2016/11/02 06:19:37, pthatcher1 wrote: > > On 2016/11/01 21:32:49, Taylor Brandstetter wrote: > > > If this class encapsulates a combination of RTP/RTCP sockets following RFC > > 3550 > > > rules, I think that should be addressed in a comment above the class > > > declaration. It's not obvious from the name of the class. > > > > It doesn't. This should be one UDP socket/port and having two ports should be > > handled up higher, such as in RtpSender/RtpReceiver. > > It's not clear to me how that will work, or how it *could* work given this class > as implemented in this CL. If "TryAllocateSocket" takes no arguments and > provides no guarantees on port number, how do you create two > UdpTransportChannels with compatible port numbers? > > When I reviewed this CL, my assumption was that it would implement what's > described in the design doc, which is a combined RTP/RTCP transport. If that's > not the case, I think we need to go back to the design doc and flesh this out. > But I think the combined design is fine, personally. Until patch set 13 this CL matched the original design doc. See comments on patch set 6 for reasons of removal. I'm in the process of incorporating review comments into the doc. But as I wrote above, we should handle the port number requirement in a dedicated CL. > > Also, if we have separate UdpTransportChannels for RTP/RTCP, then we *do* need > component() to identify them after all... Bringing component back for now for the sake of consistency.
Description was changed from ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 section 11 style RTP. BUG=webrtc:6436 ========== to ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 style RTP without ICE. Port number requirement of RFC3550 section 11 will be addressed in a follow-up CL. BUG=webrtc:6436 ==========
I'm ok with landing this CL and working out the RTP/RTCP details later. I just reviewed the test, and noticed some minor things that should be changed, however (mainly just using VirtualSocketServer). Thanks for bearing with us; you've done great for someone new to this codebase. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:51: // No thread_checker in high frequency network function. On 2016/11/02 15:14:04, johan wrote: > On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > > I think a DCHECK should be fine, even if it's high-frequency, since it only > > affects debug builds. > > The thread checker's CalledOnValidThread() method uses a CritScope lock. > I do not want to hunt down potential network timing differences between debug > builds and release builds. > Especially not in a function that is called every 10 ms (shortest reasonable > audio packet length). > Otherwise DCHECKs are great! Ok, fair enough. https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:158: state = STATE_FAILED; On 2016/11/02 15:14:04, johan wrote: > On 2016/11/01 21:32:48, Taylor Brandstetter wrote: > > This makes the TransportChannelState seem redundant; the caller can figure out > > what it needs to by a combination of GetLocalCandidate() and writable(). I'd > > suggest getting rid of state_, and then all you need to keep track of is > whether > > you've done SignalWritableState yet. > > May be redundant, but usually state dependent logic in the controlling instance > will be way cleaner, if there are distinct states, represented by single enum. I guess it's a matter of preference, so I'll accept it. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:26: class UdpTransPortChannelTest : public testing::Test, nit: remove capital 'P' https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:34: rtc::SocketServer::CreateDefault().release()); Can you use the VirtualSocketServer (+ SocketServerScope) instead, so the test doesn't use any real network resources? You could look at other tests for an example, such as port_unittest.cc. Also, generally we just put setup code in the constructor. See: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:42: network_thread_ = nullptr; This isn't necessary; can just let the destructor run. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:123: // local_channel1 <==> remote_chanel1 nit: chanel->channel https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:124: EXPECT_EQ_WAIT(len, ep1_.SendData(data, len), 1000); Could you use a default timeout of something like 10 seconds, and make it a constant at the top of the file? If this is a functional test and not a performance test, I like to use very generous timeouts since it may run in a slow environment. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:125: // rtc::Thread::Current()->ProcessMessages(1000); nit: Can these commented-out lines be removed? https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:149: TEST_F(UdpTransPortChannelTest, DefaultLocal) { nit: Rename to DefaultLocalParameters https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:169: // keep port, but explicit set IP. nit: Capitalize "keep", "explicit"->"explicitly"
Thanks! Great! VirtualSocketServer is in place. Test fixture and nits should be cleaned up. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:26: class UdpTransPortChannelTest : public testing::Test, On 2016/11/04 00:17:41, Taylor Brandstetter wrote: > nit: remove capital 'P' Done. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:34: rtc::SocketServer::CreateDefault().release()); On 2016/11/04 00:17:42, Taylor Brandstetter wrote: > Can you use the VirtualSocketServer (+ SocketServerScope) instead, so the test > doesn't use any real network resources? You could look at other tests for an > example, such as port_unittest.cc. > Done. Tricky to figure out how VirtualSocketServer works with sockets bound to ipv4 inaddr_any ("0.0.0.0"). > Also, generally we just put setup code in the constructor. See: > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Thanks for the text fixture doc link! Removing SetUp, TearDown and obsolete reset code. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:42: network_thread_ = nullptr; On 2016/11/04 00:17:41, Taylor Brandstetter wrote: > This isn't necessary; can just let the destructor run. Acknowledged. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:123: // local_channel1 <==> remote_chanel1 On 2016/11/04 00:17:42, Taylor Brandstetter wrote: > nit: chanel->channel Done. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:124: EXPECT_EQ_WAIT(len, ep1_.SendData(data, len), 1000); On 2016/11/04 00:17:41, Taylor Brandstetter wrote: > Could you use a default timeout of something like 10 seconds, and make it a > constant at the top of the file? If this is a functional test and not a > performance test, I like to use very generous timeouts since it may run in a > slow environment. Done. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:125: // rtc::Thread::Current()->ProcessMessages(1000); On 2016/11/04 00:17:41, Taylor Brandstetter wrote: > nit: Can these commented-out lines be removed? Done. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:149: TEST_F(UdpTransPortChannelTest, DefaultLocal) { On 2016/11/04 00:17:42, Taylor Brandstetter wrote: > nit: Rename to DefaultLocalParameters Done. https://codereview.webrtc.org/2377883003/diff/440001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:169: // keep port, but explicit set IP. On 2016/11/04 00:17:41, Taylor Brandstetter wrote: > nit: Capitalize "keep", "explicit"->"explicitly" Done.
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gyp_dbg/builds/...)
lgtm, just some things I think can be cleaned up now. https://codereview.webrtc.org/2377883003/diff/460001/webrtc/base/virtualsocke... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/2377883003/diff/460001/webrtc/base/virtualsocke... webrtc/base/virtualsocketserver.cc:1175: } I don't think this is necessary, is it? https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:99: // rtc::SocketAddress socket_addr("127.0.0.1", 0); Can remove this comment https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel_unittest.cc:47: // only, nit: comment formatting
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/11/04 17:51:07, Taylor Brandstetter wrote: > lgtm, just some things I think can be cleaned up now. Thanks! Cleaning up. > > https://codereview.webrtc.org/2377883003/diff/460001/webrtc/base/virtualsocke... > File webrtc/base/virtualsocketserver.cc (right): > > https://codereview.webrtc.org/2377883003/diff/460001/webrtc/base/virtualsocke... > webrtc/base/virtualsocketserver.cc:1175: } > I don't think this is necessary, is it? No, that one was not intended for upload. > > https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... > File webrtc/p2p/base/udptransportchannel.cc (right): > > https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... > webrtc/p2p/base/udptransportchannel.cc:99: // rtc::SocketAddress > socket_addr("127.0.0.1", 0); > Can remove this comment done. > > https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... > File webrtc/p2p/base/udptransportchannel_unittest.cc (right): > > https://codereview.webrtc.org/2377883003/diff/460001/webrtc/p2p/base/udptrans... > webrtc/p2p/base/udptransportchannel_unittest.cc:47: // only, > nit: comment formatting done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gyp_dbg/builds/...)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/02 19:15:20, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... > File webrtc/p2p/base/udptransportchannel.h (right): > > https://codereview.webrtc.org/2377883003/diff/400001/webrtc/p2p/base/udptrans... > webrtc/p2p/base/udptransportchannel.h:30: > On 2016/11/02 06:19:37, pthatcher1 wrote: > > On 2016/11/01 21:32:49, Taylor Brandstetter wrote: > > > If this class encapsulates a combination of RTP/RTCP sockets following RFC > > 3550 > > > rules, I think that should be addressed in a comment above the class > > > declaration. It's not obvious from the name of the class. > > > > It doesn't. This should be one UDP socket/port and having two ports should be > > handled up higher, such as in RtpSender/RtpReceiver. > > It's not clear to me how that will work, or how it *could* work given this class > as implemented in this CL. If "TryAllocateSocket" takes no arguments and > provides no guarantees on port number, how do you create two > UdpTransportChannels with compatible port numbers? With this CL, you can't. A future CL would presumably have a way of setting a min and max port (which can easily be passed to the AsyncSocketFactory). > > When I reviewed this CL, my assumption was that it would implement what's > described in the design doc, which is a combined RTP/RTCP transport. If that's > not the case, I think we need to go back to the design doc and flesh this out. > But I think the combined design is fine, personally. I asked to not go with that part of the design in this CL. Before doing the next CL, we can go back to the design. I'm in favor have 2 transports for 2 ports. But we can talk about that after this CL is laneded. > > Also, if we have separate UdpTransportChannels for RTP/RTCP, then we *do* need > component() to identify them after all... Why? The only place we really use component is in ICE candidates, to route them to the right transport channel and to calculate priority. Other than that, there isn't anything that I know of. And since this doesn't use ICE candidates or priorities, then there's nothing left.
https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:39: UpdateDebugName(); Might as well just inline this. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:58: RTC_DCHECK_EQ(socket_.get(), socket); This is just as high frequency as OnSocketReadPacket, so the thread checker high frequency argument is equally valid here. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:94: } Why wouldn't this just check if socket_ is set or not? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:103: // control level. Can you please remove this comment? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:116: local_parameters_ = rtc::Optional<rtc::SocketAddress>(socket_addr); Do this only have a port in it? Then why put in a whole SocketAddress? Why not just store a port? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:116: local_parameters_ = rtc::Optional<rtc::SocketAddress>(socket_addr); Why do we even store local_parameters? Why not just make local_parameters() use this logic from the socket_? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:127: if (local_parameters_ && !socket_) { Can this even happen? The local_parameters_ aren't set until the socket_ is set. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:140: } Also, I think it could be more clear as: if (!local_parameters_) { SetState(INIT); } else if (!remote_parameters_) { SetState(CONNECTING); } else { SetState(CONNECTED); } Honestly, I don't even see how you get to the failed.... if you call SetRemoteParameters and you don't have a local socket yet? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:153: const rtc::SocketAddress& UdpTransportChannel::local_parameters() { Why not just make this rtc::Optional<rtc::SocketAddress>? That seems better than returning the default address. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:158: void UdpTransportChannel::SetTransportChannelState(UdpTransportState state) { Just SetState is a better method name. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:164: if (state == UDPTRANSPORT_STATE_COMPLETED) { I think CONNECTED would be better than COMPLETED. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:36: }; Can you use enum class, in which case you don't need the "UDPTRANSPORT_STATE" prefix? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:42: int component, I thought you said you removed the components. Can you please remove them? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:64: UdpTransportState GetState() const { Why not just "state()"? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:69: // Check GetState() for resulting state. I'm not sure what meaning this comment has. And why is this method public? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:74: // Returns default value, if local candidate is not set. "value, if" => "value if" And what does "local candidate not set" mean? Why not just say "Returns default value if in the INIT or FAILED state"? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:88: UdpTransportState state_ = UDPTRANSPORT_STATE_INIT; Fields should go below methods https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:100: rtc::ThreadChecker network_thread_checker_; The fields filled by the constructor should go first, then the rest.
On 2016/11/04 22:33:58, pthatcher1 wrote: > On 2016/11/02 19:15:20, Taylor Brandstetter wrote: > > > > Also, if we have separate UdpTransportChannels for RTP/RTCP, then we *do* need > > component() to identify them after all... > > Why? The only place we really use component is in ICE candidates, to route them > to the right transport channel and to calculate priority. Other than that, > there isn't anything that I know of. And since this doesn't use ICE candidates > or priorities, then there's nothing left. The RTP/RTCP UdpTransportChannels for a given media description will have the same "debug name" unless they're differentiated by component. Unless its transport name includes the component, which I guess should work fine.
https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.cc (right): https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:39: UpdateDebugName(); On 2016/11/04 22:58:58, pthatcher1 wrote: > Might as well just inline this. Ack, but removed anyway due to the removal of component. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:58: RTC_DCHECK_EQ(socket_.get(), socket); On 2016/11/04 22:58:58, pthatcher1 wrote: > This is just as high frequency as OnSocketReadPacket, so the thread checker high > frequency argument is equally valid here. Yes this is high frequency. But I see less potential timing impact, as unlike the ThreadChecker, std::unique_ptr<>::get() does not use rtc::CritScope. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:94: } On 2016/11/04 22:58:58, pthatcher1 wrote: > Why wouldn't this just check if socket_ is set or not? Done. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:103: // control level. On 2016/11/04 22:58:58, pthatcher1 wrote: > Can you please remove this comment? Removing last sentence. Keeping the Todo. That is, what you meant, right? https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:116: local_parameters_ = rtc::Optional<rtc::SocketAddress>(socket_addr); On 2016/11/04 22:58:58, pthatcher1 wrote: > Do this only have a port in it? Then why put in a whole SocketAddress? Why not > just store a port? No, this contains both local address (which is currently the "any addr") and port. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:116: local_parameters_ = rtc::Optional<rtc::SocketAddress>(socket_addr); On 2016/11/04 22:58:58, pthatcher1 wrote: > Why do we even store local_parameters? Why not just make local_parameters() use > this logic from the socket_? socket's GetLocalAddress seems to be a bit expensive with (posix) implementation details ::getsockname() and SocketAddressFromSockStorage(). Not sure about the windows socket implementation. Probably a question of personal preference. But if you think, that the benefit of simplified logic in UdpTransportChannel still outweighs the cost of calling rtc::Socket::GetLocalAddress, removing local_parameters_ storage would be ok for me. Going with a minor cleanup for now and removing the temporary port variable. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:127: if (local_parameters_ && !socket_) { On 2016/11/04 22:58:58, pthatcher1 wrote: > Can this even happen? The local_parameters_ aren't set until the socket_ is > set. With the current implementation the situation (local_parameters_ && !socket_) should not occur. I would suggest changing this check to an RTC_DCHECK, so we do not rely on current CreateSocket implementation. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:140: } On 2016/11/04 22:58:58, pthatcher1 wrote: > Also, I think it could be more clear as: > > if (!local_parameters_) { > SetState(INIT); > } else if (!remote_parameters_) { > SetState(CONNECTING); > } else { > SetState(CONNECTED); > } > > > Honestly, I don't even see how you get to the failed.... if you call > SetRemoteParameters and you don't have a local socket yet? Ok, that's clean. FAILED state for this situation is not strictly necessary. We could even consider to remove FAILED from the State enum for now. As FAILED would be very similar to INIT. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:153: const rtc::SocketAddress& UdpTransportChannel::local_parameters() { On 2016/11/04 22:58:58, pthatcher1 wrote: > Why not just make this rtc::Optional<rtc::SocketAddress>? That seems better > than returning the default address. That means a copy instead of a reference. But I agree, an rtc::Optional could be better here. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:158: void UdpTransportChannel::SetTransportChannelState(UdpTransportState state) { On 2016/11/04 22:58:58, pthatcher1 wrote: > Just SetState is a better method name. ok https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.cc:164: if (state == UDPTRANSPORT_STATE_COMPLETED) { On 2016/11/04 22:58:58, pthatcher1 wrote: > I think CONNECTED would be better than COMPLETED. Renaming according to your preference. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:36: }; On 2016/11/04 22:58:58, pthatcher1 wrote: > Can you use enum class, in which case you don't need the "UDPTRANSPORT_STATE" > prefix? > Sounds good. Doing the enum class, and declaring that one as part of UdpTransportChannel. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:42: int component, On 2016/11/04 22:58:58, pthatcher1 wrote: > I thought you said you removed the components. Can you please remove them? Done. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:64: UdpTransportState GetState() const { On 2016/11/04 22:58:58, pthatcher1 wrote: > Why not just "state()"? Method name is an artifact from the TransportChannel implementation. Renaming is ok for me. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:69: // Check GetState() for resulting state. On 2016/11/04 22:58:58, pthatcher1 wrote: > I'm not sure what meaning this comment has. > > And why is this method public? It is public, because it is not called from any other method or constructor in this class. We could consider making it private. In that case either the constructor would have to invoke CreateSocket(), which I think is a bad idea. Or we would need something like a public start() method that only calls CreateSocket() to separate control logic and implementation details. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:74: // Returns default value, if local candidate is not set. On 2016/11/04 22:58:58, pthatcher1 wrote: > "value, if" => "value if" > > And what does "local candidate not set" mean? Why not just say "Returns default > value if in the INIT or FAILED state"? Rephrasing the comment, as the return type changed to rtc::Optional<rtc::SocketAddress> https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:88: UdpTransportState state_ = UDPTRANSPORT_STATE_INIT; On 2016/11/04 22:58:58, pthatcher1 wrote: > Fields should go below methods Acknowledged. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:100: rtc::ThreadChecker network_thread_checker_; On 2016/11/04 22:58:58, pthatcher1 wrote: > The fields filled by the constructor should go first, then the rest. Ok, transport_name_ and socket_server_ first.
ptal
Still lgtm https://codereview.webrtc.org/2377883003/diff/580001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/580001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:67: // Returned optional does not hav a value if in the INIT or FAILED state. hav -> have
lgtm, assuming you make the method name "Start", with the appropriate comment. https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:69: // Check GetState() for resulting state. On 2016/11/07 17:51:53, johan wrote: > On 2016/11/04 22:58:58, pthatcher1 wrote: > > I'm not sure what meaning this comment has. > > > > And why is this method public? > > It is public, because it is not called from any other method or constructor in > this class. We could consider making it private. > In that case either the constructor would have to invoke CreateSocket(), which I > think is a bad idea. Or we would need something like a public start() method > that only calls CreateSocket() to separate control logic and implementation > details. I'd be in favor of a Start method.
https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... File webrtc/p2p/base/udptransportchannel.h (right): https://codereview.webrtc.org/2377883003/diff/540001/webrtc/p2p/base/udptrans... webrtc/p2p/base/udptransportchannel.h:69: // Check GetState() for resulting state. On 2016/11/09 16:34:59, pthatcher1 wrote: > On 2016/11/07 17:51:53, johan wrote: > > On 2016/11/04 22:58:58, pthatcher1 wrote: > > > I'm not sure what meaning this comment has. > > > > > > And why is this method public? > > > > It is public, because it is not called from any other method or constructor in > > this class. We could consider making it private. > > In that case either the constructor would have to invoke CreateSocket(), which > I > > think is a bad idea. Or we would need something like a public start() method > > that only calls CreateSocket() to separate control logic and implementation > > details. > > I'd be in favor of a Start method. Renamed CreateSocket to Start and rephrased comment.
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/12920) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/19827) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19441)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/6665)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by johan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2377883003/#ps640001 (title: "Fix signed / unsigned assignment in unittest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 style RTP without ICE. Port number requirement of RFC3550 section 11 will be addressed in a follow-up CL. BUG=webrtc:6436 ========== to ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 style RTP without ICE. Port number requirement of RFC3550 section 11 will be addressed in a follow-up CL. BUG=webrtc:6436 ==========
Message was sent while issue was closed.
Committed patchset #33 (id:640001)
Message was sent while issue was closed.
Description was changed from ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 style RTP without ICE. Port number requirement of RFC3550 section 11 will be addressed in a follow-up CL. BUG=webrtc:6436 ========== to ========== First step in providing a UdpTransportChannel. Some applications explicitly require RFC3550 style RTP without ICE. Port number requirement of RFC3550 section 11 will be addressed in a follow-up CL. BUG=webrtc:6436 Committed: https://crrev.com/6bedebfb7a7688698dc5c26a4361791b7b1b206c Cr-Commit-Position: refs/heads/master@{#15005} ==========
Message was sent while issue was closed.
Patchset 33 (id:??) landed as https://crrev.com/6bedebfb7a7688698dc5c26a4361791b7b1b206c Cr-Commit-Position: refs/heads/master@{#15005} |
