Wire up RTP keep-alive in ortc api.

webrtc/api/ortc/rtptransportinterface.h

 /*
  *  Copyright 2017 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_API_ORTC_RTPTRANSPORTINTERFACE_H_
 #define WEBRTC_API_ORTC_RTPTRANSPORTINTERFACE_H_
 
 #include <string>
 
 #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.
   rtc::Optional<uint32_t> ssrc;
 
   // The Canonical Name (CNAME) used by RTCP (e.g. in SDES messages).
   //
   // If empty in the construction of the RtpTransport, one will be generated by
   // the implementation, and returned in GetRtcpParameters. Multiple
   // RtpTransports created by the same OrtcFactory will use the same generated
   // CNAME.
   //
   // If empty when passed into SetRtcpParameters, the CNAME simply won't be
   // modified.
   std::string cname;
 
   // Send reduced-size RTCP?
   bool reduced_size = false;
 
   // Send RTCP multiplexed on the RTP transport?
   bool mux = true;
 
   bool operator==(const RtcpParameters& o) const {
     return ssrc == o.ssrc && cname == o.cname &&
            reduced_size == o.reduced_size && mux == o.mux;
   }
   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.
 //
 // This is not present in the standard ORTC API, but exists here for a few
 // reasons. Firstly, it allows different types of RTP transports to be used:
 // DTLS-SRTP (which is required for the web), but also SDES-SRTP and
 // unencrypted RTP. It also simplifies the handling of RTCP muxing, and
 // provides a better API point for it.
 //
 // Note that Edge's implementation of ORTC provides a similar API point, called
@@ skipping 14 matching lines @@
   // RTCP if initially not enabled.
   //
   // Changing |mux| from "true" to "false" is not allowed, and changing the
   // CNAME is currently unsupported.
   virtual RTCError SetRtcpParameters(const RtcpParameters& parameters) = 0;
   // Returns last set or constructed-with parameters. If |cname| was empty in
   // construction, the generated CNAME will be present in the returned
   // 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.
   virtual RtpTransportAdapter* GetInternal() = 0;
 
   // Classes that can use this internal interface.
   friend class OrtcFactory;
   friend class OrtcRtpSenderAdapter;
   friend class OrtcRtpReceiverAdapter;
   friend class RtpTransportControllerAdapter;
   friend class RtpTransportAdapter;
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_API_ORTC_RTPTRANSPORTINTERFACE_H_