First step in providing a UdpTransportChannel.

webrtc/p2p/rawudp/rawudptransportchannel.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_RAWUDP_RAWUDPTRANSPORTCHANNEL_H_
+#define WEBRTC_P2P_RAWUDP_RAWUDPTRANSPORTCHANNEL_H_
+
+#include <memory>
+#include <string>
+
+#include "webrtc/base/socketaddress.h"
+#include "webrtc/base/thread_checker.h"
+#include "webrtc/p2p/base/transportchannel.h"
+#include "webrtc/p2p/base/transportchannelimpl.h"
+
+namespace rtc {
+class AsyncUDPSocket;
+class PhysicalSocketServer;
+class SocketServer;
+class Thread;
+}
+
+namespace cricket {
+
+class RawUdpCandidate : public Candidate {

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

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.

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

Acknowledged.

+  // TODO(johan) add Rtcp sockaddr to candidate

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

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

+};
+
+class RawUdpTransportChannel : public TransportChannelImpl {

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

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.  

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

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.

+ public:
+  RawUdpTransportChannel(const std::string& transport_name, int component);
+  // TODO(johan): replace raw pointers by (weak) references to std::shared_ptr,
+  // when available

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

Style: "TODO(johan): Replace raw pointers by (weak) references to
std::shared_ptr when available."

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

Do we really plan to do this? It's the first I've heard, I'm interested in the
justification.

[pthatcher1, 2016/10/07 21:15:39]

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.

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

Here the shared pointer would just by my personal preference. Removing the TODO
for now.

+  RawUdpTransportChannel(const std::string& transport_name,
+                         int component,
+                         rtc::SocketServer* ss);
+  ~RawUdpTransportChannel();
+  int SendPacket(const char* data,
+                 size_t len,
+                 const rtc::PacketOptions& options,
+                 int flags) override;
+  int Recv();
+  void OnSocketReadPacket(rtc::AsyncPacketSocket* socket,
+                          const char* data,
+                          size_t len,
+                          const rtc::SocketAddress& remote_addr,
+                          const rtc::PacketTime& packet_time,
+                          bool is_rtcp);
+  void OnRtpSocketReadPacket(rtc::AsyncPacketSocket* socket,
+                             const char* data,
+                             size_t len,
+                             const rtc::SocketAddress& remote_addr,
+                             const rtc::PacketTime& packet_time);

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

To match our current design, a transport channel would be just one socket. Two
sockets = two transport channels.

Also, these shouldn't be public.

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

Ack on "should not be public".
See my other comments for the two sockets.

+  void OnRtcpSocketReadPacket(rtc::AsyncPacketSocket* socket,
+                              const char* data,
+                              size_t len,
+                              const rtc::SocketAddress& remote_addr,
+                              const rtc::PacketTime& packet_time);

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

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.

+  int SetRemoteAddr(const char* addr);

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

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.

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

You could use an rtc::SocketAddress here. And can you document the return value?

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

Blame me, I had already removed this method's implementation. Removing the
declaration, too, now.
Further discussion below.

+  TransportChannelState GetState() const override;
+
+  int SetOption(rtc::Socket::Option opt, int value) override {
+    RTC_NOTREACHED();
+    return 0;
+  }

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

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

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

Acknowledged.

+
+  int GetError() override {
+    RTC_NOTREACHED();
+    return 0;

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

Same here

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

Acknowledged.

+  }
+
+  bool GetStats(ConnectionInfos* infos) override {
+    RTC_NOTREACHED();
+    return false;
+  }

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

And here

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

Acknowledged.

+
+  bool IsDtlsActive() const override {
+    RTC_NOTREACHED();
+    return false;

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

And here.  Although removing it would be even better (only implement
PacketTransportInterface).

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

Replacing the RTC_NOTREACHED() by a method removal todo.

+  }
+
+  bool GetSslRole(rtc::SSLRole* role) const override {
+    RTC_NOTREACHED();

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

This one should stay, and the next few, unless you do the
PacketTransportInteface idea.

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

Acknowledged.

+    return false;
+  }
+
+  rtc::scoped_refptr<rtc::RTCCertificate> GetLocalCertificate() const override {
+    RTC_NOTREACHED();
+    return nullptr;
+  }
+
+  std::unique_ptr<rtc::SSLCertificate> GetRemoteSSLCertificate()
+      const override {
+    RTC_NOTREACHED();
+    return nullptr;
+  }
+
+  bool ExportKeyingMaterial(const std::string& label,
+                            const uint8_t* context,
+                            size_t context_len,
+                            bool use_context,
+                            uint8_t* result,
+                            size_t result_len) override {
+    RTC_NOTREACHED();
+    return false;
+  }
+
+  // TransportChannelImpl overrides
+  IceRole GetIceRole() const override {
+    RTC_NOTREACHED();
+    return ICEROLE_UNKNOWN;
+  }
+
+  void SetIceRole(IceRole role) override { RTC_NOTREACHED(); }
+
+  void SetIceTiebreaker(uint64_t tiebreaker) override { RTC_NOTREACHED(); }
+
+  void SetIceParameters(const IceParameters& ice_params) override {
+    RTC_NOTREACHED();
+  }
+
+  void SetRemoteIceParameters(const IceParameters& ice_params) override {
+    RTC_NOTREACHED();
+  }
+
+  void SetRemoteIceMode(IceMode mode) override { RTC_NOTREACHED(); }
+
+  void SetIceConfig(const IceConfig& config) override { RTC_NOTREACHED(); }

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

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.

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

Adding the TODO with next patch set. Maybe switching to the
PacketTransportInterface with a later patch set.

+
+  void MaybeStartGathering() override;
+
+  void AddRemoteCandidate(const Candidate& candidate) override;

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

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.

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

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.

+
+  // TransportChannelImpl overrides
+  void RemoveRemoteCandidate(const Candidate& candidate) override {
+    // TODO(johan) implement

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

Why would you implement this?  It's an ICE thing.

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

Removing the TODO.

+    RTC_NOTREACHED();
+  }
+
+  IceGatheringState gathering_state() const override;
+
+  bool SetLocalCertificate(
+      const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) override {
+    RTC_NOTREACHED();
+    return false;
+  }
+
+  bool SetRemoteFingerprint(const std::string& digest_alg,
+                            const uint8_t* digest,
+                            size_t digest_len) override {
+    RTC_NOTREACHED();
+    return false;
+  }
+
+  bool SetSslRole(rtc::SSLRole role) override {
+    RTC_NOTREACHED();
+    return false;
+  }
+
+  void SetMetricsObserver(webrtc::MetricsObserverInterface* observer) override {
+    RTC_NOTREACHED();

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

I don't think you should have this NOTREACHED().

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

Acknowledged.

+  }
+
+  // TODO(johan) make this private, once addcandidate is working
+  bool SetRemoteAddrAndPorts(const char* ip_addr, int rtp_port);

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

I think you should not implement AddCandidate, and instead have this.  Except
make it SetRemoteParameters.

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

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.

+
+ private:
+  bool TryAllocateSockets();
+  void SetGatheringState(
+      IceGatheringState new_gathering_state);  // Set State and Signal

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

nit: "Set state and signal."

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

Acknowledged.

+  void SetTransportChannelState(
+      TransportChannelState new_tch_state);  // Set State and Signal
+  TransportChannelState tch_state_;

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

Just state_ is enough.

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

Acknowledged.

+  IceGatheringState gathering_state_;

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

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.

+  void UpdateWritableState();

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

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
}

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

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.

+  std::unique_ptr<rtc::AsyncUDPSocket> rtp_socket_;
+  std::unique_ptr<rtc::AsyncUDPSocket> rtcp_socket_;

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

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.

+  Candidate local_candidate_;
+  Candidate remote_candidate_;

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

This should be local_parameters_ and remote_parameters_.

+  rtc::SocketAddress remote_addr_;

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

remote_addr_ should be a part of remote_parameters_.

+  rtc::SocketServer* socket_server_;
+  rtc::ThreadChecker network_thread_checker_;
+};
+}  // namespace cricket
+
+#endif  // WEBRTC_P2P_RAWUDP_RAWUDPTRANSPORTCHANNEL_H_