Wire up RTP keep-alive in ortc api.

webrtc/api/ortc/rtptransportinterface.h

Index: webrtc/api/ortc/rtptransportinterface.h
diff --git a/webrtc/api/ortc/rtptransportinterface.h b/webrtc/api/ortc/rtptransportinterface.h
index 119d662edde283815170dbab234103ea8c763f7b..3e67525bc93020eb6d8ef76395f142b5fe73e367 100644
--- a/webrtc/api/ortc/rtptransportinterface.h
+++ b/webrtc/api/ortc/rtptransportinterface.h
@@ -15,13 +15,14 @@
 
 #include "webrtc/api/ortc/packettransportinterface.h"
 #include "webrtc/api/rtcerror.h"
+#include "webrtc/common_types.h"
 #include "webrtc/rtc_base/optional.h"
 
 namespace webrtc {
 
 class RtpTransportAdapter;
 
-struct RtcpParameters {
+struct RtcpParameters final {
   // The SSRC to be used in the "SSRC of packet sender" field. If not set, one
   // will be chosen by the implementation.
   // TODO(deadbeef): Not implemented.
@@ -51,6 +52,19 @@ struct RtcpParameters {
   bool operator!=(const RtcpParameters& o) const { return !(*this == o); }
 };
 
+struct RtpTransportParameters final {
+  // Enabled periodic sending of keep-alive packets, that help prevent timeouts
+  // on the network level, such as NAT bindings. See RFC6263 section 4.6.
+  RtpKeepAliveConfig keepalive_config;

[pthatcher1, 2017/07/11 18:25:09]

Can we just call this "keepalive" (or keep_alive?)?

[sprang_webrtc, 2017/07/11 19:54:05]

Sure.

+
+  bool operator==(const RtpTransportParameters& o) const {
+    return keepalive_config == o.keepalive_config;
+  }
+  bool operator!=(const RtpTransportParameters& o) const {
+    return !(*this == o);
+  }
+};
+
 // Base class for different types of RTP transports that can be created by an
 // OrtcFactory. Used by RtpSenders/RtpReceivers.
 //
@@ -85,6 +99,10 @@ class RtpTransportInterface {
   // parameters (see above).
   virtual RtcpParameters GetRtcpParameters() const = 0;
 
+  virtual RTCError SetRtpTransportParameters(
+      const RtpTransportParameters& parameters) = 0;
+  virtual RtpTransportParameters GetRtpTransportParameters() const = 0;

[pthatcher1, 2017/07/11 18:25:09]

I think we should merge these into one set of get/set:

SetParameters(RtpTransportParameters)
RtpTransportParameters GetParameters()


Where RtpTransportParameters has RtcpParameters:

struct RtpTrnsportParameters {
  RtcpParameters rtcp;
  RtpKeepAliveParameters keepalive;
}

[sprang_webrtc, 2017/07/11 19:54:05]

I pondered that, but then we're mixing a transport level feature with a
stream-level one. It got strange to be forced to pass along unused
RtcpParameters in places were just the RtpTransportParameters made sense.
Having a third "RtpRtcpParameters" or whatever struct joining these two also
feels a bit overkill.

[pthatcher1, 2017/07/12 22:45:21]

Ah, that's a good point.  I was thrown off by the fact that we use it here as
transport-level parameters.  I presume we will change that in the future to be
per-stream.

In that case, I'm fine with an additional getter/setter.  But can you call it
GetParameters and SetParameters? 
RtpTransport::SetParameters(RtpTransportParameters) makes a lot more sense than
RtpTransport::SetRtpTransportParameters(RtpTransportParameters).

[Taylor Brandstetter, 2017/07/12 23:15:50]

Wait, what feature here is stream-level? The keepalives? But I thought the
reason we're putting it here (as opposed to on RtpSender) is that it's
conceptually transport-level.

I'd generally agree with Peter about merging the two structs. It's more
consistent with the ORTC programming model, where if you need to change
something you're expected to do "getParameters, modify  stuff, setParameters".
And all the objects defined thus far have used a single "parameters" structure.

This seems sensible to me, and consistent with other things:

struct RtpTransportParameters {
  RtcpParameters rtcp;
  RtpKeepAliveConfig keepalive_config;
};

+
  protected:
   // Only for internal use. Returns a pointer to an internal interface, for use
   // by the implementation.