First step in providing a UdpTransportChannel.

webrtc/p2p/base/udptransportchannel.h

+/*
+ *  Copyright 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_P2P_BASE_UDPTRANSPORTCHANNEL_H_
+#define WEBRTC_P2P_BASE_UDPTRANSPORTCHANNEL_H_
+
+#include <memory>
+#include <string>
+
+#include "webrtc/base/socketaddress.h"
+#include "webrtc/base/thread_checker.h"
+#include "webrtc/p2p/base/transportchannel.h"

[Taylor Brandstetter, 2016/11/01 21:32:49]

It would be nice if this could depend only on PacketTransportInterface and not
TransportChannel. Is TransportChannelState the only thing used?

[pthatcher1, 2016/11/02 06:19:38]

I agree.  Just make a new state enum.

[johan, 2016/11/02 15:14:04]

Ok

+#include "webrtc/p2p/base/transportchannelimpl.h"
+
+namespace rtc {
+class AsyncUDPSocket;
+class PhysicalSocketServer;
+class SocketServer;
+class Thread;
+}
+
+namespace cricket {
+

[Taylor Brandstetter, 2016/11/01 21:32:49]

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.

[pthatcher1, 2016/11/02 06:19:37]

It doesn't.  This should be one UDP socket/port and having two ports should be
handled up higher, such as in RtpSender/RtpReceiver.

[Taylor Brandstetter, 2016/11/02 19:15:20]

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...

+class UdpTransportChannel : public rtc::PacketTransportInterface {
+ public:
+  UdpTransportChannel(const std::string& transport_name, int component);

[Taylor Brandstetter, 2016/11/01 21:32:48]

If this is always RTP+RTCP, it doesn't need a component argument.

[pthatcher1, 2016/11/02 06:19:37]

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?

[johan, 2016/11/02 15:14:04]

Can do that.

+  UdpTransportChannel(const std::string& transport_name,
+                      int component,
+                      rtc::SocketServer* ss);
+  ~UdpTransportChannel();
+
+  const std::string debug_name() const override { return debug_name_; }
+
+  bool receiving() const override {
+    // TODO(johan): Implement method and signal.
+    return true;

[pthatcher1, 2016/11/02 06:19:37]

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.

[johan, 2016/11/02 15:14:04]

Yes, receiving() is no rocket science, but I would still prefer to implement it
in a dedicated smaller CL. 

+  }
+
+  bool writable() const override;
+
+  int SendPacket(const char* data,
+                 size_t len,
+                 const rtc::PacketOptions& options,
+                 int flags) override;
+
+  int SetOption(rtc::Socket::Option opt, int value) override { return 0; }
+
+  int GetError() override { return send_error_; }
+
+  // TODO(johan): Reusing TransportChannelState is ugly. On the other hand there
+  // are already too many state enums for very similar states troughout the code
+  // base.
+  TransportChannelState GetState() const {

[Taylor Brandstetter, 2016/11/01 21:32:48]

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.

[pthatcher1, 2016/11/02 06:19:37]

I agree.  Make a new enum.

[johan, 2016/11/02 15:14:04]

Done.

+    RTC_DCHECK_RUN_ON(&network_thread_checker_);
+    return state_;
+  }
+
+  // Check GetState() for resulting state.
+  void TryAllocateSocket();
+
+  void AddRemoteCandidate(const Candidate& candidate);

[Taylor Brandstetter, 2016/11/01 21:32:48]

As long as this is (now) a new interface, we could just use rtc::SocketAddresses
here.

[pthatcher1, 2016/11/02 06:19:37]

Yeah, we shouldn't use candidates.  They are specific to ICE. I would call this
SetRemoteParameters.

[johan, 2016/11/02 15:14:04]

Done.

+
+  // Returns default values, if local candidate is not set.
+  // Consider checking GetState() before calling GetLocalCandidate().
+  const Candidate& GetLocalCandidate();

[pthatcher1, 2016/11/02 06:19:37]

I would call this local_parameters()

[johan, 2016/11/02 15:14:04]

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

+
+ private:
+  void OnSocketReadPacket(rtc::AsyncPacketSocket* socket,
+                          const char* data,
+                          size_t len,
+                          const rtc::SocketAddress& remote_addr,
+                          const rtc::PacketTime& packet_time);
+  void OnSocketSentPacket(rtc::AsyncPacketSocket* socket,
+                          const rtc::SentPacket& packet);
+  void SetTransportChannelState(
+      TransportChannelState state);  // Set State and Signal.
+  TransportChannelState state_ = STATE_INIT;
+  bool IsLocalConsistent();
+  bool IsRemoteConsistent();
+  void UpdateState();
+  void UpdateDebugName();
+  int send_error_ = 0;
+  std::string transport_name_;
+  std::string debug_name_;
+  int component_;
+  std::unique_ptr<rtc::AsyncPacketSocket> socket_;
+  // TODO(johan): use a non-ICE datatype for parameters with
+  // PacketTransportInterface.
+  rtc::Optional<Candidate> local_parameters_;
+  rtc::Optional<Candidate> remote_parameters_;
+  rtc::SocketServer* socket_server_;
+  rtc::ThreadChecker network_thread_checker_;
+};
+}  // namespace cricket
+
+#endif  // WEBRTC_P2P_BASE_UDPTRANSPORTCHANNEL_H_