First step in providing a UdpTransportChannel.

webrtc/p2p/rawudp/rawudptransportchannel.cc

+/*
+ *  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/rawudp/rawudptransportchannel.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"
+
+namespace cricket {
+
+RawUdpTransportChannel::RawUdpTransportChannel(
+    const std::string& transport_name,
+    int component)
+    : RawUdpTransportChannel(transport_name,
+                             component,
+                             rtc::Thread::Current()->socketserver()) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);

[Taylor Brandstetter, 2016/10/07 18:24:03]

The constructor is where the thread checker is initialized so this is redundant.

[johan, 2016/10/11 13:36:09]

You have a point here.

+}
+
+RawUdpTransportChannel::RawUdpTransportChannel(
+    const std::string& transport_name,
+    int component,
+    rtc::SocketServer* socket_server)
+    : TransportChannelImpl(transport_name, component),
+      tch_state_(STATE_INIT),
+      gathering_state_(kIceGatheringNew) {

[Taylor Brandstetter, 2016/10/07 18:24:03]

Can initialize these to default values in the header.

[johan, 2016/10/11 13:36:09]

Done.

+  socket_server_ = socket_server;

[Taylor Brandstetter, 2016/10/07 18:24:03]

Can set socket_server_ in initializer list.

[johan, 2016/10/11 13:36:09]

Acknowledged.

+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+}
+
+RawUdpTransportChannel::~RawUdpTransportChannel() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+}
+
+void RawUdpTransportChannel::OnSocketReadPacket(
+    rtc::AsyncPacketSocket* socket,
+    const char* data,
+    size_t len,
+    const rtc::SocketAddress& remote_addr,
+    const rtc::PacketTime& packet_time,
+    bool is_rtcp) {
+  // No thread_checker in high frequency network function.
+  // Serialisation of processing RTP and RTCP socket is implicit done by event
+  // loop.

[pthatcher1, 2016/10/07 17:36:36]

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

[Taylor Brandstetter, 2016/10/07 18:24:03]

I see the value of these checks. But since they're DCHECKs they can happen
everywhere, since they only happen on debug builds.

[johan, 2016/10/11 13:36:09]

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

+  SignalReadPacket(this, data, len, packet_time, 0);
+}
+
+void RawUdpTransportChannel::OnRtpSocketReadPacket(
+    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.
+  // TODO(johan) check, if remote_addr is valid and legit.
+  OnSocketReadPacket(socket, data, len, remote_addr, packet_time,
+                     false /*rtcp*/);
+}
+
+void RawUdpTransportChannel::OnRtcpSocketReadPacket(
+    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.
+  // TODO(johan) check, if remote_addr is valid and legit.
+  OnSocketReadPacket(socket, data, len, remote_addr, packet_time,
+                     true /*rtcp*/);

[pthatcher1, 2016/10/07 17:36:36]

Please remove these and just have OnSocketReadPacket.

+}
+
+int RawUdpTransportChannel::SendPacket(const char* data,
+                                       size_t len,
+                                       const rtc::PacketOptions& options,
+                                       int flags) {
+  // No thread_checker in high frequency network function.
+  int32_t is_rtcp = 0;
+  // TODO(johan): add logic to send rtcp packets over rtcp socket.
+  int result = -1;
+  if (!is_rtcp) {
+    result = rtp_socket_->SendTo((const void*)data, len, remote_addr_, options);
+  }

[pthatcher1, 2016/10/07 17:36:36]

Again, you don't need any of that.  Just do socket_->SendTo.  RTP/RTCP
difference is handled by channel.cc.

+  LOG(LS_VERBOSE) << "SendPacket() " << result;
+  return result;
+}
+
+bool RawUdpTransportChannel::TryAllocateSockets() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  static constexpr uint16_t kMaxTries = 100;
+  static constexpr uint16_t kMinPortNumber = 2000;
+  // TODO(johan) provide configuration option for kMinPortNumber.
+  rtc::SocketAddress rtp_socket_addr("0.0.0.0", 0);
+  rtc::SocketAddress rtcp_socket_addr("0.0.0.0", 1);
+  // TODO(johan) allocate sockets only once, and loop over
+  // rtc::AsyncSocketAdapter::bind().
+  for (uint16_t count = 0; count < kMaxTries; ++count) {

[pthatcher1, 2016/10/07 17:36:36]

It seems like you should use a rtc::PacketSocketFactory and call
CreateUdpSocket.  The PacketSocketFactory could be passed into the
UdpTransportChannel.

[johan, 2016/10/11 13:36:09]

rtc::PacketSocketFactory seems to be the right thing. Maybe with a patch for the
{rtp, rtcp} port number requirements mentioned below.

+    uint16_t rtpport = kMinPortNumber + (2 * count);
+    rtp_socket_addr.SetPort(rtpport);
+    rtp_socket_.reset(
+        rtc::AsyncUDPSocket::Create(socket_server_, rtp_socket_addr));
+    if (!rtp_socket_) {
+      continue;
+    }
+    rtcp_socket_addr.SetPort(rtpport + 1);

[pthatcher1, 2016/10/07 17:36:36]

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

[johan, 2016/10/11 13:36:09]

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.
I'm fine with placing this logic at a higher level. Probably in the not yet
existing cricket::TransportController::CreateUdpTransportChannel().

+    rtcp_socket_.reset(
+        rtc::AsyncUDPSocket::Create(socket_server_, rtcp_socket_addr));
+    if (rtp_socket_ && rtcp_socket_) {
+      LOG(INFO) << "Allocated Rtp socket with local port " << rtpport;
+      rtp_socket_->SignalReadPacket.connect(
+          this, &RawUdpTransportChannel::OnRtpSocketReadPacket);
+      rtcp_socket_->SignalReadPacket.connect(
+          this, &RawUdpTransportChannel::OnRtcpSocketReadPacket);
+      local_candidate_.set_address(rtp_socket_addr);
+      return true;
+    }
+  }
+  rtp_socket_.reset();
+  rtcp_socket_.reset();
+  return false;
+}
+
+void RawUdpTransportChannel::MaybeStartGathering() {

[pthatcher1, 2016/10/07 17:36:36]

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.

[johan, 2016/10/11 13:36:09]

Renaming depends on PacketTransportInterface.

+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if (gathering_state_ != kIceGatheringNew) {
+    LOG(INFO) << "candidates gathering already done, early return";
+    return;
+  }
+  SetGatheringState(kIceGatheringGathering);
+  TryAllocateSockets();

[Taylor Brandstetter, 2016/10/07 18:24:03]

Why does this return a bool if it's not used?

[johan, 2016/10/11 13:36:09]

Should be used, will fix it.

+  SignalCandidateGathered(this, local_candidate_);
+  SetGatheringState(kIceGatheringComplete);
+}
+
+void RawUdpTransportChannel::UpdateWritableState() {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  bool writable = true;
+  if (!rtp_socket_) {
+    writable = false;
+    LOG(INFO) << "rtp_socket_ is false";
+  }
+  if (!remote_addr_.IsComplete()) {
+    writable = false;
+    LOG(INFO) << "remote_addr_ not complete";
+  }
+  if (gathering_state_ != kIceGatheringComplete) {
+    writable = false;
+    LOG(INFO) << "gathering_state_ " << gathering_state_;
+  }
+  set_writable(writable);
+}
+
+void RawUdpTransportChannel::AddRemoteCandidate(const Candidate& candidate) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  // TODO(johan) check for ipv4, other settings.
+  remote_candidate_ = candidate;
+  remote_addr_ = candidate.address();
+  // TODO(johan) - only set STATE_COMPLETED if *both* local and remote addr are
+  // set. Maybe use an rtc::Optional to indicate unset addr?

[pthatcher1, 2016/10/07 17:36:36]

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);

 

+  SetTransportChannelState(STATE_COMPLETED);
+  // TODO(johan) - set STATE_FAILED, if remote addr is invalid.
+  UpdateWritableState();
+}
+
+IceGatheringState RawUdpTransportChannel::gathering_state() const {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  return gathering_state_;
+}
+
+TransportChannelState RawUdpTransportChannel::GetState() const {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  return tch_state_;
+}

[pthatcher1, 2016/10/07 17:36:36]

These could just go in the .h.

[johan, 2016/10/11 13:36:09]

Acknowledged.

+
+void RawUdpTransportChannel::SetGatheringState(
+    IceGatheringState new_gathering_state) {

[pthatcher1, 2016/10/07 17:36:36]

You don't need "new_"

[johan, 2016/10/11 13:36:10]

Acknowledged.

+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if (gathering_state_ != new_gathering_state) {
+    gathering_state_ = new_gathering_state;
+    SignalGatheringState(this);
+  }
+}
+
+void RawUdpTransportChannel::SetTransportChannelState(
+    TransportChannelState new_tch_state) {
+  RTC_DCHECK_RUN_ON(&network_thread_checker_);
+  if (tch_state_ != new_tch_state) {
+    tch_state_ = new_tch_state;
+    SignalStateChanged(this);
+  }
+}
+}  // namespace cricket