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..22ede29cf1adcc665a6c19b81a548d291861c64a
--- /dev/null
+++ b/webrtc/p2p/base/udptransportchannel.cc
@@ -0,0 +1,169 @@
+/*
+ *  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/asyncpacketsocket.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"
+#include "webrtc/p2p/base/packettransportinterface.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) {
+  UpdateDebugName();

[pthatcher1, 2016/11/04 22:58:58]

Might as well just inline this.

[johan, 2016/11/07 17:51:52]

Ack, but removed anyway due to the removal of component.

+}
+
+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.
+  SignalReadPacket(this, data, len, packet_time, 0);
+}
+
+void UdpTransportChannel::OnSocketSentPacket(rtc::AsyncPacketSocket* socket,
+                                             const rtc::SentPacket& packet) {
+  RTC_DCHECK_EQ(socket_.get(), socket);

[pthatcher1, 2016/11/04 22:58:58]

This is just as high frequency as OnSocketReadPacket, so the thread checker high
frequency argument is equally valid here.

[johan, 2016/11/07 17:51:52]

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.

+  SignalSentPacket(this, packet);
+}
+
+void UdpTransportChannel::UpdateDebugName() {
+  debug_name_ = transport_name_ + " " + std::to_string(component_);
+}
+
+bool UdpTransportChannel::writable() const {
+  return state_ == UDPTRANSPORT_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_;
+  int result = socket_->SendTo((const void*)data, len, remote_addr_, options);
+  if (result <= 0) {
+    LOG(LS_VERBOSE) << "SendPacket() " << result;
+  }
+  return result;
+}
+
+void UdpTransportChannel::CreateSocket() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if ((state_ == UDPTRANSPORT_STATE_CONNECTING) ||
+      (state_ == UDPTRANSPORT_STATE_COMPLETED)) {
+    LOG(LS_WARNING) << "Local socket already allocated.";
+    return;
+  }

[pthatcher1, 2016/11/04 22:58:58]

Why wouldn't this just check if socket_ is set or not?

[johan, 2016/11/07 17:51:53]

Done.

+  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/04 22:58:58]

Can you please remove this comment?

[johan, 2016/11/07 17:51:53]

Removing last sentence. Keeping the Todo. That is, what you meant, right?

+  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) << "Created UDP socket with addr "
+              << socket_->GetLocalAddress().ipaddr() << " port " << port << ".";
+    socket_->SignalReadPacket.connect(this,
+                                      &UdpTransportChannel::OnSocketReadPacket);
+    socket_->SignalSentPacket.connect(this,
+                                      &UdpTransportChannel::OnSocketSentPacket);
+    socket_addr.SetPort(port);
+    local_parameters_ = rtc::Optional<rtc::SocketAddress>(socket_addr);

[pthatcher1, 2016/11/04 22:58:58]

Why do we even store local_parameters?  Why not just make local_parameters() use
this logic from the socket_?

[pthatcher1, 2016/11/04 22:58:58]

Do this only have a port in it?  Then why put in a whole SocketAddress?  Why not
just store a port?

[johan, 2016/11/07 17:51:53]

No, this contains both local address (which is currently the "any addr") and
port. 

[johan, 2016/11/07 17:51:53]

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.

+  } else {
+    LOG(INFO) << "Local socket allocation failure";
+  }
+  UpdateState();
+  return;
+}
+
+void UdpTransportChannel::UpdateState() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  UdpTransportState state;
+  if (local_parameters_ && !socket_) {

[pthatcher1, 2016/11/04 22:58:58]

Can this even happen?  The local_parameters_ aren't set until the socket_ is
set.

[johan, 2016/11/07 17:51:52]

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.

+    LOG(INFO) << "local address is set, but socket is null";
+    state = UDPTRANSPORT_STATE_FAILED;
+  } else if (!local_parameters_ && !remote_parameters_) {
+    state = UDPTRANSPORT_STATE_INIT;
+  } else if (local_parameters_ && !remote_parameters_) {
+    state = UDPTRANSPORT_STATE_CONNECTING;
+  } else if (local_parameters_ && remote_parameters_) {
+    state = UDPTRANSPORT_STATE_COMPLETED;
+  } else {
+    state = UDPTRANSPORT_STATE_FAILED;
+  }
+  SetTransportChannelState(state);
+}

[pthatcher1, 2016/11/04 22:58:58]

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?

[johan, 2016/11/07 17:51:53]

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.

+
+void UdpTransportChannel::SetRemoteParameters(const rtc::SocketAddress& addr) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if (!addr.IsComplete()) {
+    LOG(INFO) << "remote address not complete";
+    return;
+  }
+  // TODO(johan) check for ipv4, other settings.
+  remote_parameters_ = rtc::Optional<rtc::SocketAddress>(addr);
+  UpdateState();
+}
+
+const rtc::SocketAddress& UdpTransportChannel::local_parameters() {

[pthatcher1, 2016/11/04 22:58:58]

Why not just make this rtc::Optional<rtc::SocketAddress>?  That seems better
than returning the default address.

[johan, 2016/11/07 17:51:52]

That means a copy instead of a reference. But I agree, an rtc::Optional could be
better here.

+  static const rtc::SocketAddress default_addr;
+  return local_parameters_.value_or(default_addr);
+}
+
+void UdpTransportChannel::SetTransportChannelState(UdpTransportState state) {

[pthatcher1, 2016/11/04 22:58:58]

Just SetState is a better method name.

[johan, 2016/11/07 17:51:53]

ok

+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if (state_ == state) {
+    return;
+  }
+  state_ = state;
+  if (state == UDPTRANSPORT_STATE_COMPLETED) {

[pthatcher1, 2016/11/04 22:58:58]

I think CONNECTED would be better than COMPLETED.

[johan, 2016/11/07 17:51:53]

Renaming according to your preference.

+    SignalWritableState(this);
+    SignalReadyToSend(this);
+  }
+}
+}  // namespace cricket