datachannel: Make SendDataParams reliability fields optional<int>

It doesn't make sense to use negative values or 0 to disable the
feature, so we use an optional int value.
Values bigger than 65535 are clamped down.

Bug: webrtc:12730
Change-Id: I6bd9cd92f7d0a70a78cf5a7c91dca52c28d08ba1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217760
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33954}

api/data_channel_interface.h

@@ -44,11 +44,13 @@ struct DataChannelInit {
   //
   // Cannot be set along with |maxRetransmits|.
   // This is called |maxPacketLifeTime| in the WebRTC JS API.
+  // Negative values are ignored, and positive values are clamped to [0-65535]
   absl::optional<int> maxRetransmitTime;
 
   // The max number of retransmissions.
   //
   // Cannot be set along with |maxRetransmitTime|.
+  // Negative values are ignored, and positive values are clamped to [0-65535]
   absl::optional<int> maxRetransmits;
 
   // This is set by the application and opaque to the WebRTC implementation.

api/transport/data_channel_transport_interface.h

@@ -47,15 +47,15 @@ struct SendDataParams {
   // If set, the maximum number of times this message may be
   // retransmitted by the transport before it is dropped.
   // Setting this value to zero disables retransmission.
-  // Must be non-negative. |max_rtx_count| and |max_rtx_ms| may not be set
+  // Valid values are in the range [0-UINT16_MAX].
-  // simultaneously.
+  // |max_rtx_count| and |max_rtx_ms| may not be set simultaneously.
   absl::optional<int> max_rtx_count;
 
   // If set, the maximum number of milliseconds for which the transport
   // may retransmit this message before it is dropped.
   // Setting this value to zero disables retransmission.
-  // Must be non-negative. |max_rtx_count| and |max_rtx_ms| may not be set
+  // Valid values are in the range [0-UINT16_MAX].
-  // simultaneously.
+  // |max_rtx_count| and |max_rtx_ms| may not be set simultaneously.
   absl::optional<int> max_rtx_ms;
 };
 

media/base/media_channel.h

@@ -919,21 +919,15 @@ struct SendDataParams {
   // The type of message (binary, text, or control).
   DataMessageType type = DMT_TEXT;
 
-  // TODO(pthatcher): Make |ordered| and |reliable| true by default?
   // For SCTP, whether to send messages flagged as ordered or not.
   // If false, messages can be received out of order.
   bool ordered = false;
-  // For SCTP, whether the messages are sent reliably or not.
+  // Provide partial reliability by resending up to this many times. Either
-  // If false, messages may be lost.
+  // count or millis is supported, not both at the same time.
-  bool reliable = false;
+  absl::optional<int> max_rtx_count;
-  // For SCTP, if reliable == false, provide partial reliability by
+  // Provide partial reliability by resending for up to this many milliseconds.
-  // resending up to this many times.  Either count or millis
+  // Either count or millis is supported, not both at the same time.
-  // is supported, not both at the same time.
+  absl::optional<int> max_rtx_ms;
-  int max_rtx_count = 0;
-  // For SCTP, if reliable == false, provide partial reliability by
-  // resending for up to this many milliseconds.  Either count or millis
-  // is supported, not both at the same time.
-  int max_rtx_ms = 0;
 };
 
 enum SendDataResult { SDR_SUCCESS, SDR_ERROR, SDR_BLOCK };

media/sctp/dcsctp_transport.cc

@@ -11,6 +11,7 @@
 #include "media/sctp/dcsctp_transport.h"
 
 #include <cstdint>
+#include <limits>
 #include <utility>
 #include <vector>
 
@@ -221,11 +222,16 @@ bool DcSctpTransport::SendData(const cricket::SendDataParams& params,
 
   dcsctp::SendOptions send_options;
   send_options.unordered = dcsctp::IsUnordered(!params.ordered);
-  if (params.max_rtx_ms > 0)
+  if (params.max_rtx_ms.has_value()) {
-    send_options.lifetime = dcsctp::DurationMs(params.max_rtx_ms);
+    RTC_DCHECK(*params.max_rtx_ms >= 0 &&
-  if (params.max_rtx_count > 0)
+               *params.max_rtx_ms <= std::numeric_limits<uint16_t>::max());
-    send_options.max_retransmissions =
+    send_options.lifetime = dcsctp::DurationMs(*params.max_rtx_ms);
-        static_cast<size_t>(params.max_rtx_count);
+  }
+  if (params.max_rtx_count.has_value()) {
+    RTC_DCHECK(*params.max_rtx_count >= 0 &&
+               *params.max_rtx_count <= std::numeric_limits<uint16_t>::max());
+    send_options.max_retransmissions = *params.max_rtx_count;
+  }
 
   auto error = socket_->Send(std::move(message), send_options);
   switch (error) {

media/sctp/usrsctp_transport.cc

@@ -225,18 +225,22 @@ sctp_sendv_spa CreateSctpSendParams(const cricket::SendDataParams& params,
   // example.
   spa.sendv_sndinfo.snd_flags |= SCTP_EOR;
 
-  // Ordered implies reliable.
   if (!params.ordered) {
     spa.sendv_sndinfo.snd_flags |= SCTP_UNORDERED;
-    if (params.max_rtx_count >= 0 || params.max_rtx_ms == 0) {
+  }
-      spa.sendv_flags |= SCTP_SEND_PRINFO_VALID;
+  if (params.max_rtx_count.has_value()) {
-      spa.sendv_prinfo.pr_policy = SCTP_PR_SCTP_RTX;
+    RTC_DCHECK(*params.max_rtx_count >= 0 &&
-      spa.sendv_prinfo.pr_value = params.max_rtx_count;
+               *params.max_rtx_count <= std::numeric_limits<uint16_t>::max());
-    } else {
+    spa.sendv_flags |= SCTP_SEND_PRINFO_VALID;
-      spa.sendv_flags |= SCTP_SEND_PRINFO_VALID;
+    spa.sendv_prinfo.pr_policy = SCTP_PR_SCTP_RTX;
-      spa.sendv_prinfo.pr_policy = SCTP_PR_SCTP_TTL;
+    spa.sendv_prinfo.pr_value = *params.max_rtx_count;
-      spa.sendv_prinfo.pr_value = params.max_rtx_ms;
+  }
-    }
+  if (params.max_rtx_ms.has_value()) {
+    RTC_DCHECK(*params.max_rtx_ms >= 0 &&
+               *params.max_rtx_ms <= std::numeric_limits<uint16_t>::max());
+    spa.sendv_flags |= SCTP_SEND_PRINFO_VALID;
+    spa.sendv_prinfo.pr_policy = SCTP_PR_SCTP_TTL;
+    spa.sendv_prinfo.pr_value = *params.max_rtx_ms;
   }
   return spa;
 }

media/sctp/usrsctp_transport_reliability_unittest.cc

@@ -646,9 +646,6 @@ TEST_F(UsrSctpReliabilityTest,
   cricket::SendDataParams send_params;
   send_params.sid = -1;
   send_params.ordered = true;
-  send_params.reliable = true;
-  send_params.max_rtx_count = 0;
-  send_params.max_rtx_ms = 0;
 
   SctpPingPong test(1, kTransport1Port, kTransport2Port, thread1.get(),
                     thread2.get(), messages_count, packet_loss_percents,
@@ -684,9 +681,6 @@ TEST_F(UsrSctpReliabilityTest,
   cricket::SendDataParams send_params;
   send_params.sid = -1;
   send_params.ordered = true;
-  send_params.reliable = true;
-  send_params.max_rtx_count = 0;
-  send_params.max_rtx_ms = 0;
 
   SctpPingPong test(1, kTransport1Port, kTransport2Port, thread1.get(),
                     thread2.get(), messages_count, packet_loss_percents,
@@ -723,9 +717,8 @@ TEST_F(UsrSctpReliabilityTest,
   cricket::SendDataParams send_params;
   send_params.sid = -1;
   send_params.ordered = false;
-  send_params.reliable = false;
+  send_params.max_rtx_count = std::numeric_limits<uint16_t>::max();
-  send_params.max_rtx_count = INT_MAX;
+  send_params.max_rtx_ms = std::numeric_limits<uint16_t>::max();
-  send_params.max_rtx_ms = INT_MAX;
 
   SctpPingPong test(1, kTransport1Port, kTransport2Port, thread1.get(),
                     thread2.get(), messages_count, packet_loss_percents,
@@ -760,9 +753,6 @@ TEST_F(UsrSctpReliabilityTest,
   cricket::SendDataParams send_params;
   send_params.sid = -1;
   send_params.ordered = true;
-  send_params.reliable = true;
-  send_params.max_rtx_count = 0;
-  send_params.max_rtx_ms = 0;
   constexpr uint32_t base_sctp_port = 5000;
 
   // The constants value below were experimentally chosen

pc/data_channel_controller.cc

@@ -398,11 +398,8 @@ bool DataChannelController::DataChannelSendData(
   SendDataParams send_params;
   send_params.type = ToWebrtcDataMessageType(params.type);
   send_params.ordered = params.ordered;
-  if (params.max_rtx_count >= 0) {
+  send_params.max_rtx_count = params.max_rtx_count;
-    send_params.max_rtx_count = params.max_rtx_count;
+  send_params.max_rtx_ms = params.max_rtx_ms;
-  } else if (params.max_rtx_ms >= 0) {
-    send_params.max_rtx_ms = params.max_rtx_ms;
-  }
 
   RTCError error = network_thread()->Invoke<RTCError>(
       RTC_FROM_HERE, [this, params, send_params, payload] {

pc/sctp_data_channel.cc

@@ -10,6 +10,7 @@
 
 #include "pc/sctp_data_channel.h"
 
+#include <limits>
 #include <memory>
 #include <string>
 #include <utility>
@@ -78,17 +79,27 @@ InternalDataChannelInit::InternalDataChannelInit(const DataChannelInit& base)
     // Specified in createDataChannel, WebRTC spec section 6.1 bullet 13.
     id = -1;
   }
-  // Backwards compatibility: If base.maxRetransmits or base.maxRetransmitTime
+  // Backwards compatibility: If maxRetransmits or maxRetransmitTime
-  // have been set to -1, unset them.
+  // are negative, the feature is not enabled.
-  if (maxRetransmits && *maxRetransmits == -1) {
+  // Values are clamped to a 16bit range.
-    RTC_LOG(LS_ERROR)
+  if (maxRetransmits) {
-        << "Accepting maxRetransmits = -1 for backwards compatibility";
+    if (*maxRetransmits < 0) {
-    maxRetransmits = absl::nullopt;
+      RTC_LOG(LS_ERROR)
+          << "Accepting maxRetransmits < 0 for backwards compatibility";
+      maxRetransmits = absl::nullopt;
+    } else if (*maxRetransmits > std::numeric_limits<uint16_t>::max()) {
+      maxRetransmits = std::numeric_limits<uint16_t>::max();
+    }
   }
-  if (maxRetransmitTime && *maxRetransmitTime == -1) {
+
-    RTC_LOG(LS_ERROR)
+  if (maxRetransmitTime) {
-        << "Accepting maxRetransmitTime = -1 for backwards compatibility";
+    if (*maxRetransmitTime < 0) {
-    maxRetransmitTime = absl::nullopt;
+      RTC_LOG(LS_ERROR)
+          << "Accepting maxRetransmitTime < 0 for backwards compatibility";
+      maxRetransmitTime = absl::nullopt;
+    } else if (*maxRetransmitTime > std::numeric_limits<uint16_t>::max()) {
+      maxRetransmitTime = std::numeric_limits<uint16_t>::max();
+    }
   }
 }
 
@@ -620,10 +631,8 @@ bool SctpDataChannel::SendDataMessage(const DataBuffer& buffer,
            "because the OPEN_ACK message has not been received.";
   }
 
-  send_params.max_rtx_count =
+  send_params.max_rtx_count = config_.maxRetransmits;
-      config_.maxRetransmits ? *config_.maxRetransmits : -1;
+  send_params.max_rtx_ms = config_.maxRetransmitTime;
-  send_params.max_rtx_ms =
-      config_.maxRetransmitTime ? *config_.maxRetransmitTime : -1;
   send_params.sid = config_.id;
   send_params.type = buffer.binary ? cricket::DMT_BINARY : cricket::DMT_TEXT;
 

pc/sctp_data_channel_transport.cc

@@ -45,9 +45,8 @@ RTCError SctpDataChannelTransport::SendData(
   sd_params.sid = channel_id;
   sd_params.type = ToCricketDataMessageType(params.type);
   sd_params.ordered = params.ordered;
-  sd_params.reliable = !(params.max_rtx_count || params.max_rtx_ms);
+  sd_params.max_rtx_count = params.max_rtx_count;
-  sd_params.max_rtx_count = params.max_rtx_count.value_or(-1);
+  sd_params.max_rtx_ms = params.max_rtx_ms;
-  sd_params.max_rtx_ms = params.max_rtx_ms.value_or(-1);
 
   cricket::SendDataResult result;
   sctp_transport_->SendData(sd_params, buffer, &result);