First step in providing a UdpTransportChannel.

webrtc/p2p/base/udptransportchannel.cc

Index: webrtc/p2p/base/udptransportchannel.cc
diff --git a/webrtc/p2p/base/udptransportchannel.cc b/webrtc/p2p/base/udptransportchannel.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a6252f5564f57a3c50d217ddce09f9dc589bba0f
--- /dev/null
+++ b/webrtc/p2p/base/udptransportchannel.cc
@@ -0,0 +1,185 @@
+/*
+ *  Copyright 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/p2p/base/udptransportchannel.h"
+
+#include <string>
+
+#include "webrtc/base/asyncudpsocket.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/base/physicalsocketserver.h"
+#include "webrtc/base/socketaddress.h"
+#include "webrtc/base/thread.h"
+#include "webrtc/base/thread_checker.h"
+#include "webrtc/p2p/base/basicpacketsocketfactory.h"
+
+namespace cricket {
+
+UdpTransportChannel::UdpTransportChannel(const std::string& transport_name,
+                                         int component)
+    : UdpTransportChannel(transport_name,
+                          component,
+                          rtc::Thread::Current()->socketserver()) {}
+
+UdpTransportChannel::UdpTransportChannel(const std::string& transport_name,
+                                         int component,
+                                         rtc::SocketServer* socket_server)
+    : transport_name_(transport_name),
+      component_(component),
+      socket_server_(socket_server) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);

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

This isn't necessary, since the constructor is where network_thread_checker_ is
initialized.

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

Done.

+  UpdateDebugName();
+}
+
+UdpTransportChannel::~UdpTransportChannel() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+}
+
+void UdpTransportChannel::OnSocketReadPacket(
+    rtc::AsyncPacketSocket* socket,
+    const char* data,
+    size_t len,
+    const rtc::SocketAddress& remote_addr,
+    const rtc::PacketTime& packet_time) {
+  // No thread_checker in high frequency network function.

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

I think a DCHECK should be fine, even if it's high-frequency, since it only
affects debug builds.

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

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!

[Taylor Brandstetter, 2016/11/04 00:17:41]

Ok, fair enough.

+  SignalReadPacket(this, data, len, packet_time, 0);
+}
+
+void UdpTransportChannel::OnSocketSentPacket(rtc::AsyncPacketSocket* socket,
+                                             const rtc::SentPacket& packet) {
+  RTC_DCHECK_EQ(socket_.get(), socket);
+  SignalSentPacket(this, packet);
+}
+
+void UdpTransportChannel::UpdateDebugName() {
+  debug_name_ = transport_name_ + " " + std::to_string(component_);

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

Just do "return transport_name_".

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

Done.

+}
+
+bool UdpTransportChannel::writable() const {
+  return state_ == STATE_COMPLETED;
+}
+
+int UdpTransportChannel::SendPacket(const char* data,
+                                    size_t len,
+                                    const rtc::PacketOptions& options,
+                                    int flags) {
+  // No thread_checker in high frequency network function.
+  if (!remote_parameters_) {
+    LOG(LS_WARNING) << "Remote parameters not set.";
+    send_error_ = ENOTCONN;
+    return -1;
+  }
+  const rtc::SocketAddress& remote_addr_ = remote_parameters_->address();
+  int result = socket_->SendTo((const void*)data, len, remote_addr_, options);
+  if (result <= 0) {
+    LOG(LS_VERBOSE) << "SendPacket() " << result;
+  }
+  return result;
+}
+
+void UdpTransportChannel::TryAllocateSocket() {

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

I'd call this CreateSocket, since what it does it call CreateUdpSocket and store
it.

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

Done.

+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if ((state_ == STATE_CONNECTING) || (state_ == STATE_COMPLETED)) {
+    LOG(LS_WARNING) << "Local Socket already allocated.";

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

nit: Don't need to capitalize Socket

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

Acknowledged.

+    return;
+  }
+  static constexpr uint16_t kMaxTries = 100;
+  static constexpr uint16_t kMinPortNumber = 2000;
+  // TODO(johan) provide configuration option for kMinPortNumber.
+  rtc::SocketAddress socket_addr("0.0.0.0", 0);
+  // TODO(johan): Replace BasicPacketSocketFactory by something that honors RFC
+  // 3550 Section 11 port number requirements like
+  // {port_{RTP} is even, port_{RTCP} := port{RTP} + 1}.
+  // This could be a SetSocket(...) method for sockets allocated at a higher
+  // control level.

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

I think it should be done at the RtpSender/RtpReceiver level.

+  rtc::BasicPacketSocketFactory socket_factory(socket_server_);
+  socket_.reset(socket_factory.CreateUdpSocket(socket_addr, kMinPortNumber,
+                                               kMinPortNumber + kMaxTries));
+  if (socket_) {
+    uint16_t port = socket_->GetLocalAddress().port();
+    LOG(INFO) << "Allocated Rtp socket with local port " << port;

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

I'd say "Created UDP socket with port "

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

Done.

+    socket_->SignalReadPacket.connect(this,
+                                      &UdpTransportChannel::OnSocketReadPacket);
+    socket_->SignalSentPacket.connect(this,
+                                      &UdpTransportChannel::OnSocketSentPacket);
+    socket_addr.SetPort(port);
+    local_parameters_ = rtc::Optional<Candidate>(Candidate());
+    local_parameters_->set_address(socket_addr);
+  } else {
+    LOG(INFO) << "Local socket allocation failure";
+  }
+  UpdateState();
+  return;
+}
+
+bool UdpTransportChannel::IsLocalConsistent() {
+  if (!local_parameters_) {
+    return true;
+  }
+  if (!socket_) {
+    LOG(INFO) << "socket_ is false";

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

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.

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

Inlined into UpdateState() as Peter suggests.

+    return false;
+  }
+  return true;
+}
+
+bool UdpTransportChannel::IsRemoteConsistent() {
+  if (!remote_parameters_) {
+    return true;
+  }
+  if (!remote_parameters_->address().IsComplete()) {

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

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.

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

Can do that.
Side effect: state does not change to STATE_FAIL anymore, when caller tries to
set an invalid remote address.

+    LOG(INFO) << "remote_addr_ not complete";

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

Don't have something called "remote_addr_" any more. Could just say "remote
address".

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

Acknowledged.

+    return false;
+  }
+  return true;
+}

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

The above two methods are only used once.  I'd prefer to just inline them in
UpdateState().

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

Inlining IsLocalConsistent() in UpdateState().
Doing the remote addr check on SetRemoteParameters().

+
+void UdpTransportChannel::UpdateState() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  TransportChannelState state;
+  if (!IsLocalConsistent()) {
+    state = STATE_FAILED;
+  } else if (!IsRemoteConsistent()) {
+    state = STATE_FAILED;
+  } else if (!local_parameters_ && !remote_parameters_) {
+    state = STATE_INIT;
+  } else if (local_parameters_ && !remote_parameters_) {
+    state = STATE_CONNECTING;
+  } else if (local_parameters_ && remote_parameters_) {
+    state = STATE_COMPLETED;
+  } else {
+    state = STATE_FAILED;

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

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.

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

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.

[Taylor Brandstetter, 2016/11/04 00:17:41]

I guess it's a matter of preference, so I'll accept it.

+  }
+  SetTransportChannelState(state);
+}
+
+void UdpTransportChannel::AddRemoteCandidate(const Candidate& candidate) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  // TODO(johan) check for ipv4, other settings.
+  remote_parameters_ = rtc::Optional<Candidate>(candidate);
+  UpdateState();
+}
+
+const Candidate& UdpTransportChannel::GetLocalCandidate() {
+  return *local_parameters_;

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

I think this will crash if local_parameters_ isn't set yet; can it return a
default constructed Candidate (or SocketAddress) instead?

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

Oups, adding default + unit test.

+}
+
+void UdpTransportChannel::SetTransportChannelState(
+    TransportChannelState state) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if (state_ != state) {
+    state_ = state;

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

Please use early returns.

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

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.

+    if (state == STATE_COMPLETED) {
+      SignalWritableState(this);
+      SignalReadyToSend(this);
+    }
+  }
+}
+}  // namespace cricket