Datachannel: Use absl::optional for maxRetransmits and maxRetransmitTime.

These parameters are nullable in the JS API.
This allows cleaner handling of "unset" vs "set" in Chrome.

Backwards compatibility note: Behavior should not change, even for users
who set the values explicitly to -1 in the DataChannelInit struct.
Those who try to read back the value will get a compile-time error.

Bug: chromium:854385
Change-Id: Ib488ca5f70bc24ba8b4a3f71b506434c4d2c60b2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/131381
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27507}

api/data_channel_interface.cc

@@ -24,6 +24,14 @@ uint16_t DataChannelInterface::maxRetransmits() const {
   return 0;
 }
 
+absl::optional<int> DataChannelInterface::maxRetransmitsOpt() const {
+  return absl::nullopt;
+}
+
+absl::optional<int> DataChannelInterface::maxPacketLifeTime() const {
+  return absl::nullopt;
+}
+
 std::string DataChannelInterface::protocol() const {
   return std::string();
 }

api/data_channel_interface.h

@@ -18,6 +18,7 @@
 #include <stdint.h>
 #include <string>
 
+#include "absl/types/optional.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/copy_on_write_buffer.h"
 #include "rtc_base/ref_count.h"
@@ -35,15 +36,16 @@ struct DataChannelInit {
   bool ordered = true;
 
   // The max period of time in milliseconds in which retransmissions will be
-  // sent. After this time, no more retransmissions will be sent. -1 if unset.
+  // sent. After this time, no more retransmissions will be sent.
   //
   // Cannot be set along with |maxRetransmits|.
-  int maxRetransmitTime = -1;
+  // This is called |maxPacketLifeTime| in the WebRTC JS API.
+  absl::optional<int> maxRetransmitTime;
 
-  // The max number of retransmissions. -1 if unset.
+  // The max number of retransmissions.
   //
   // Cannot be set along with |maxRetransmitTime|.
-  int maxRetransmits = -1;
+  absl::optional<int> maxRetransmits;
 
   // This is set by the application and opaque to the WebRTC implementation.
   std::string protocol;
@@ -137,8 +139,11 @@ class DataChannelInterface : public rtc::RefCountInterface {
   // implemented these APIs. They should all just return the values the
   // DataChannel was created with.
   virtual bool ordered() const;
+  // TODO(hta): Deprecate and remove the following two functions.
   virtual uint16_t maxRetransmitTime() const;
   virtual uint16_t maxRetransmits() const;
+  virtual absl::optional<int> maxRetransmitsOpt() const;
+  virtual absl::optional<int> maxPacketLifeTime() const;
   virtual std::string protocol() const;
   virtual bool negotiated() const;
 

pc/data_channel.cc

@@ -28,6 +28,30 @@ namespace webrtc {
 static size_t kMaxQueuedReceivedDataBytes = 16 * 1024 * 1024;
 static size_t kMaxQueuedSendDataBytes = 16 * 1024 * 1024;
 
+InternalDataChannelInit::InternalDataChannelInit(const DataChannelInit& base)
+    : DataChannelInit(base), open_handshake_role(kOpener) {
+  // If the channel is externally negotiated, do not send the OPEN message.
+  if (base.negotiated) {
+    open_handshake_role = kNone;
+  } else {
+    // Datachannel is externally negotiated. Ignore the id value.
+    // Specified in createDataChannel, WebRTC spec section 6.1 bullet 13.
+    id = -1;
+  }
+  // Backwards compatibility: If base.maxRetransmits or base.maxRetransmitTime
+  // have been set to -1, unset them.
+  if (maxRetransmits && *maxRetransmits == -1) {
+    RTC_LOG(LS_ERROR)
+        << "Accepting maxRetransmits = -1 for backwards compatibility";
+    maxRetransmits = absl::nullopt;
+  }
+  if (maxRetransmitTime && *maxRetransmitTime == -1) {
+    RTC_LOG(LS_ERROR)
+        << "Accepting maxRetransmitTime = -1 for backwards compatibility";
+    maxRetransmitTime = absl::nullopt;
+  }
+}
+
 bool SctpSidAllocator::AllocateSid(rtc::SSLRole role, int* sid) {
   int potential_sid = (role == rtc::SSL_CLIENT) ? 0 : 1;
   while (!IsSidAvailable(potential_sid)) {
@@ -140,21 +164,22 @@ DataChannel::DataChannel(DataChannelProviderInterface* provider,
 
 bool DataChannel::Init(const InternalDataChannelInit& config) {
   if (data_channel_type_ == cricket::DCT_RTP) {
-    if (config.reliable || config.id != -1 || config.maxRetransmits != -1 ||
+    if (config.reliable || config.id != -1 || config.maxRetransmits ||
-        config.maxRetransmitTime != -1) {
+        config.maxRetransmitTime) {
       RTC_LOG(LS_ERROR) << "Failed to initialize the RTP data channel due to "
                            "invalid DataChannelInit.";
       return false;
     }
     handshake_state_ = kHandshakeReady;
   } else if (IsSctpLike(data_channel_type_)) {
-    if (config.id < -1 || config.maxRetransmits < -1 ||
+    if (config.id < -1 ||
-        config.maxRetransmitTime < -1) {
+        (config.maxRetransmits && *config.maxRetransmits < 0) ||
+        (config.maxRetransmitTime && *config.maxRetransmitTime < 0)) {
       RTC_LOG(LS_ERROR) << "Failed to initialize the SCTP data channel due to "
                            "invalid DataChannelInit.";
       return false;
     }
-    if (config.maxRetransmits != -1 && config.maxRetransmitTime != -1) {
+    if (config.maxRetransmits && config.maxRetransmitTime) {
       RTC_LOG(LS_ERROR)
           << "maxRetransmits and maxRetransmitTime should not be both set.";
       return false;
@@ -206,7 +231,7 @@ bool DataChannel::reliable() const {
   if (data_channel_type_ == cricket::DCT_RTP) {
     return false;
   } else {
-    return config_.maxRetransmits == -1 && config_.maxRetransmitTime == -1;
+    return !config_.maxRetransmits && !config_.maxRetransmitTime;
   }
 }
 
@@ -575,8 +600,10 @@ bool DataChannel::SendDataMessage(const DataBuffer& buffer,
              "because the OPEN_ACK message has not been received.";
     }
 
-    send_params.max_rtx_count = config_.maxRetransmits;
+    send_params.max_rtx_count =
-    send_params.max_rtx_ms = config_.maxRetransmitTime;
+        config_.maxRetransmits ? *config_.maxRetransmits : -1;
+    send_params.max_rtx_ms =
+        config_.maxRetransmitTime ? *config_.maxRetransmitTime : -1;
     send_params.sid = config_.id;
   } else {
     send_params.ssrc = send_ssrc_;

pc/data_channel.h

@@ -57,18 +57,7 @@ struct InternalDataChannelInit : public DataChannelInit {
   enum OpenHandshakeRole { kOpener, kAcker, kNone };
   // The default role is kOpener because the default |negotiated| is false.
   InternalDataChannelInit() : open_handshake_role(kOpener) {}
-  explicit InternalDataChannelInit(const DataChannelInit& base)
+  explicit InternalDataChannelInit(const DataChannelInit& base);
-      : DataChannelInit(base), open_handshake_role(kOpener) {
-    // If the channel is externally negotiated, do not send the OPEN message.
-    if (base.negotiated) {
-      open_handshake_role = kNone;
-    } else {
-      // Datachannel is externally negotiated. Ignore the id value.
-      // Specified in createDataChannel, WebRTC spec section 6.1 bullet 13.
-      id = -1;
-    }
-  }
-
   OpenHandshakeRole open_handshake_role;
 };
 
@@ -135,10 +124,21 @@ class DataChannel : public DataChannelInterface, public sigslot::has_slots<> {
   virtual std::string label() const { return label_; }
   virtual bool reliable() const;
   virtual bool ordered() const { return config_.ordered; }
+  // Backwards compatible accessors
   virtual uint16_t maxRetransmitTime() const {
+    return config_.maxRetransmitTime ? *config_.maxRetransmitTime
+                                     : static_cast<uint16_t>(-1);
+  }
+  virtual uint16_t maxRetransmits() const {
+    return config_.maxRetransmits ? *config_.maxRetransmits
+                                  : static_cast<uint16_t>(-1);
+  }
+  virtual absl::optional<int> maxPacketLifeTime() const {
     return config_.maxRetransmitTime;
   }
-  virtual uint16_t maxRetransmits() const { return config_.maxRetransmits; }
+  virtual absl::optional<int> maxRetransmitsOpt() const {
+    return config_.maxRetransmits;
+  }
   virtual std::string protocol() const { return config_.protocol; }
   virtual bool negotiated() const { return config_.negotiated; }
   virtual int id() const { return config_.id; }
@@ -307,6 +307,8 @@ PROXY_CONSTMETHOD0(bool, reliable)
 PROXY_CONSTMETHOD0(bool, ordered)
 PROXY_CONSTMETHOD0(uint16_t, maxRetransmitTime)
 PROXY_CONSTMETHOD0(uint16_t, maxRetransmits)
+PROXY_CONSTMETHOD0(absl::optional<int>, maxRetransmitsOpt)
+PROXY_CONSTMETHOD0(absl::optional<int>, maxPacketLifeTime)
 PROXY_CONSTMETHOD0(std::string, protocol)
 PROXY_CONSTMETHOD0(bool, negotiated)
 PROXY_CONSTMETHOD0(int, id)

pc/peer_connection_integrationtest.cc

@@ -3207,6 +3207,7 @@ TEST_P(PeerConnectionIntegrationTest, RtpDataChannelsRejectedByCallee) {
       CreatePeerConnectionWrappersWithConfig(rtc_config_1, rtc_config_2));
   ConnectFakeSignaling();
   caller()->CreateDataChannel();
+  ASSERT_TRUE(caller()->data_channel() != nullptr);
   caller()->AddAudioVideoTracks();
   callee()->AddAudioVideoTracks();
   caller()->CreateAndSetAndSignalOffer();

pc/peer_connection_interface_unittest.cc

@@ -2082,7 +2082,6 @@ TEST_P(PeerConnectionInterfaceTest, CreateSctpDataChannel) {
   CreatePeerConnection(rtc_config);
 
   webrtc::DataChannelInit config;
-
   rtc::scoped_refptr<DataChannelInterface> channel =
       pc_->CreateDataChannel("1", &config);
   EXPECT_TRUE(channel != NULL);
@@ -2103,7 +2102,7 @@ TEST_P(PeerConnectionInterfaceTest, CreateSctpDataChannel) {
   EXPECT_FALSE(channel->reliable());
   EXPECT_FALSE(observer_.renegotiation_needed_);
 
-  config.maxRetransmits = -1;
+  config.maxRetransmits = absl::nullopt;
   config.maxRetransmitTime = 0;
   channel = pc_->CreateDataChannel("4", &config);
   EXPECT_TRUE(channel != NULL);
@@ -2111,6 +2110,21 @@ TEST_P(PeerConnectionInterfaceTest, CreateSctpDataChannel) {
   EXPECT_FALSE(observer_.renegotiation_needed_);
 }
 
+// For backwards compatibility, we want people who "unset" maxRetransmits
+// and maxRetransmitTime by setting them to -1 to get what they want.
+TEST_P(PeerConnectionInterfaceTest, CreateSctpDataChannelWithMinusOne) {
+  RTCConfiguration rtc_config;
+  rtc_config.enable_dtls_srtp = true;
+  CreatePeerConnection(rtc_config);
+
+  webrtc::DataChannelInit config;
+  config.maxRetransmitTime = -1;
+  config.maxRetransmits = -1;
+  rtc::scoped_refptr<DataChannelInterface> channel =
+      pc_->CreateDataChannel("1", &config);
+  EXPECT_TRUE(channel != NULL);
+}
+
 // This tests that no data channel is returned if both maxRetransmits and
 // maxRetransmitTime are set for SCTP data channels.
 TEST_P(PeerConnectionInterfaceTest,

pc/sctp_utils.cc

@@ -108,8 +108,8 @@ bool ParseDataChannelOpenMessage(const rtc::CopyOnWriteBuffer& payload,
       config->ordered = false;
   }
 
-  config->maxRetransmits = -1;
+  config->maxRetransmits = absl::nullopt;
-  config->maxRetransmitTime = -1;
+  config->maxRetransmitTime = absl::nullopt;
   switch (channel_type) {
     case DCOMCT_ORDERED_PARTIAL_RTXS:
     case DCOMCT_UNORDERED_PARTIAL_RTXS:
@@ -142,27 +142,27 @@ bool WriteDataChannelOpenMessage(const std::string& label,
                                  const DataChannelInit& config,
                                  rtc::CopyOnWriteBuffer* payload) {
   // Format defined at
-  // http://tools.ietf.org/html/draft-ietf-rtcweb-data-protocol-00#section-6.1
+  // http://tools.ietf.org/html/draft-ietf-rtcweb-data-protocol-09#section-5.1
   uint8_t channel_type = 0;
   uint32_t reliability_param = 0;
   uint16_t priority = 0;
   if (config.ordered) {
-    if (config.maxRetransmits > -1) {
+    if (config.maxRetransmits) {
       channel_type = DCOMCT_ORDERED_PARTIAL_RTXS;
-      reliability_param = config.maxRetransmits;
+      reliability_param = *config.maxRetransmits;
-    } else if (config.maxRetransmitTime > -1) {
+    } else if (config.maxRetransmitTime) {
       channel_type = DCOMCT_ORDERED_PARTIAL_TIME;
-      reliability_param = config.maxRetransmitTime;
+      reliability_param = *config.maxRetransmitTime;
     } else {
       channel_type = DCOMCT_ORDERED_RELIABLE;
     }
   } else {
-    if (config.maxRetransmits > -1) {
+    if (config.maxRetransmits) {
       channel_type = DCOMCT_UNORDERED_PARTIAL_RTXS;
-      reliability_param = config.maxRetransmits;
+      reliability_param = *config.maxRetransmits;
-    } else if (config.maxRetransmitTime > -1) {
+    } else if (config.maxRetransmitTime) {
       channel_type = DCOMCT_UNORDERED_PARTIAL_TIME;
-      reliability_param = config.maxRetransmitTime;
+      reliability_param = *config.maxRetransmitTime;
     } else {
       channel_type = DCOMCT_UNORDERED_RELIABLE;
     }

pc/sctp_utils_unittest.cc

@@ -34,23 +34,22 @@ class SctpUtilsTest : public testing::Test {
 
     ASSERT_TRUE(buffer.ReadUInt8(&channel_type));
     if (config.ordered) {
-      EXPECT_EQ(config.maxRetransmits > -1
+      EXPECT_EQ(
-                    ? 0x01
+          config.maxRetransmits ? 0x01 : (config.maxRetransmitTime ? 0x02 : 0),
-                    : (config.maxRetransmitTime > -1 ? 0x02 : 0),
+          channel_type);
-                channel_type);
     } else {
-      EXPECT_EQ(config.maxRetransmits > -1
+      EXPECT_EQ(config.maxRetransmits
                     ? 0x81
-                    : (config.maxRetransmitTime > -1 ? 0x82 : 0x80),
+                    : (config.maxRetransmitTime ? 0x82 : 0x80),
                 channel_type);
     }
 
     ASSERT_TRUE(buffer.ReadUInt16(&priority));
 
     ASSERT_TRUE(buffer.ReadUInt32(&reliability));
-    if (config.maxRetransmits > -1 || config.maxRetransmitTime > -1) {
+    if (config.maxRetransmits || config.maxRetransmitTime) {
-      EXPECT_EQ(config.maxRetransmits > -1 ? config.maxRetransmits
+      EXPECT_EQ(config.maxRetransmits ? *config.maxRetransmits
-                                           : config.maxRetransmitTime,
+                                      : *config.maxRetransmitTime,
                 static_cast<int>(reliability));
     }
 
@@ -110,8 +109,8 @@ TEST_F(SctpUtilsTest, WriteParseOpenMessageWithMaxRetransmitTime) {
   EXPECT_EQ(label, output_label);
   EXPECT_EQ(config.protocol, output_config.protocol);
   EXPECT_EQ(config.ordered, output_config.ordered);
-  EXPECT_EQ(config.maxRetransmitTime, output_config.maxRetransmitTime);
+  EXPECT_EQ(*config.maxRetransmitTime, *output_config.maxRetransmitTime);
-  EXPECT_EQ(-1, output_config.maxRetransmits);
+  EXPECT_FALSE(output_config.maxRetransmits);
 }
 
 TEST_F(SctpUtilsTest, WriteParseOpenMessageWithMaxRetransmits) {
@@ -134,7 +133,7 @@ TEST_F(SctpUtilsTest, WriteParseOpenMessageWithMaxRetransmits) {
   EXPECT_EQ(config.protocol, output_config.protocol);
   EXPECT_EQ(config.ordered, output_config.ordered);
   EXPECT_EQ(config.maxRetransmits, output_config.maxRetransmits);
-  EXPECT_EQ(-1, output_config.maxRetransmitTime);
+  EXPECT_FALSE(output_config.maxRetransmitTime);
 }
 
 TEST_F(SctpUtilsTest, WriteParseAckMessage) {

sdk/objc/api/peerconnection/RTCDataChannelConfiguration.mm

@@ -33,7 +33,7 @@
 }
 
 - (int)maxPacketLifeTime {
-  return _nativeDataChannelInit.maxRetransmitTime;
+  return *_nativeDataChannelInit.maxRetransmitTime;
 }
 
 - (void)setMaxPacketLifeTime:(int)maxPacketLifeTime {
@@ -41,7 +41,11 @@
 }
 
 - (int)maxRetransmits {
-  return _nativeDataChannelInit.maxRetransmits;
+  if (_nativeDataChannelInit.maxRetransmits) {
+    return *_nativeDataChannelInit.maxRetransmits;
+  } else {
+    return -1;
+  }
 }
 
 - (void)setMaxRetransmits:(int)maxRetransmits {