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}diff --git a/api/data_channel_interface.cc b/api/data_channel_interface.cc
index 240ccbe..d299ced 100644
--- a/api/data_channel_interface.cc
+++ b/api/data_channel_interface.cc
@@ -24,6 +24,14 @@
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();
}
diff --git a/api/data_channel_interface.h b/api/data_channel_interface.h
index 1bd874e..f7032ec 100644
--- a/api/data_channel_interface.h
+++ b/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 @@
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 @@
// 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;
diff --git a/pc/data_channel.cc b/pc/data_channel.cc
index e4727f2..cd4dded 100644
--- a/pc/data_channel.cc
+++ b/pc/data_channel.cc
@@ -28,6 +28,30 @@
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 @@
bool DataChannel::Init(const InternalDataChannelInit& config) {
if (data_channel_type_ == cricket::DCT_RTP) {
- if (config.reliable || config.id != -1 || config.maxRetransmits != -1 ||
- config.maxRetransmitTime != -1) {
+ if (config.reliable || config.id != -1 || config.maxRetransmits ||
+ 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 ||
- config.maxRetransmitTime < -1) {
+ if (config.id < -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 @@
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 @@
"because the OPEN_ACK message has not been received.";
}
- send_params.max_rtx_count = config_.maxRetransmits;
- send_params.max_rtx_ms = config_.maxRetransmitTime;
+ send_params.max_rtx_count =
+ 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_;
diff --git a/pc/data_channel.h b/pc/data_channel.h
index 3e58c2b..e4166dd 100644
--- a/pc/data_channel.h
+++ b/pc/data_channel.h
@@ -57,18 +57,7 @@
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)
- : 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;
- }
- }
-
+ explicit InternalDataChannelInit(const DataChannelInit& base);
OpenHandshakeRole open_handshake_role;
};
@@ -135,10 +124,21 @@
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, 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)
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 218a8bb..58c3439 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -3207,6 +3207,7 @@
CreatePeerConnectionWrappersWithConfig(rtc_config_1, rtc_config_2));
ConnectFakeSignaling();
caller()->CreateDataChannel();
+ ASSERT_TRUE(caller()->data_channel() != nullptr);
caller()->AddAudioVideoTracks();
callee()->AddAudioVideoTracks();
caller()->CreateAndSetAndSignalOffer();
diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc
index 371cdc1..edede25 100644
--- a/pc/peer_connection_interface_unittest.cc
+++ b/pc/peer_connection_interface_unittest.cc
@@ -2082,7 +2082,6 @@
CreatePeerConnection(rtc_config);
webrtc::DataChannelInit config;
-
rtc::scoped_refptr<DataChannelInterface> channel =
pc_->CreateDataChannel("1", &config);
EXPECT_TRUE(channel != NULL);
@@ -2103,7 +2102,7 @@
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 @@
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,
diff --git a/pc/sctp_utils.cc b/pc/sctp_utils.cc
index aa7b6c1..7b67fc1 100644
--- a/pc/sctp_utils.cc
+++ b/pc/sctp_utils.cc
@@ -108,8 +108,8 @@
config->ordered = false;
}
- config->maxRetransmits = -1;
- config->maxRetransmitTime = -1;
+ config->maxRetransmits = absl::nullopt;
+ config->maxRetransmitTime = absl::nullopt;
switch (channel_type) {
case DCOMCT_ORDERED_PARTIAL_RTXS:
case DCOMCT_UNORDERED_PARTIAL_RTXS:
@@ -142,27 +142,27 @@
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;
- } else if (config.maxRetransmitTime > -1) {
+ reliability_param = *config.maxRetransmits;
+ } 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;
- } else if (config.maxRetransmitTime > -1) {
+ reliability_param = *config.maxRetransmits;
+ } else if (config.maxRetransmitTime) {
channel_type = DCOMCT_UNORDERED_PARTIAL_TIME;
- reliability_param = config.maxRetransmitTime;
+ reliability_param = *config.maxRetransmitTime;
} else {
channel_type = DCOMCT_UNORDERED_RELIABLE;
}
diff --git a/pc/sctp_utils_unittest.cc b/pc/sctp_utils_unittest.cc
index 72db952..01f5434 100644
--- a/pc/sctp_utils_unittest.cc
+++ b/pc/sctp_utils_unittest.cc
@@ -34,23 +34,22 @@
ASSERT_TRUE(buffer.ReadUInt8(&channel_type));
if (config.ordered) {
- EXPECT_EQ(config.maxRetransmits > -1
- ? 0x01
- : (config.maxRetransmitTime > -1 ? 0x02 : 0),
- channel_type);
+ EXPECT_EQ(
+ config.maxRetransmits ? 0x01 : (config.maxRetransmitTime ? 0x02 : 0),
+ 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) {
- EXPECT_EQ(config.maxRetransmits > -1 ? config.maxRetransmits
- : config.maxRetransmitTime,
+ if (config.maxRetransmits || config.maxRetransmitTime) {
+ EXPECT_EQ(config.maxRetransmits ? *config.maxRetransmits
+ : *config.maxRetransmitTime,
static_cast<int>(reliability));
}
@@ -110,8 +109,8 @@
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(-1, output_config.maxRetransmits);
+ EXPECT_EQ(*config.maxRetransmitTime, *output_config.maxRetransmitTime);
+ EXPECT_FALSE(output_config.maxRetransmits);
}
TEST_F(SctpUtilsTest, WriteParseOpenMessageWithMaxRetransmits) {
@@ -134,7 +133,7 @@
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) {
diff --git a/sdk/objc/api/peerconnection/RTCDataChannelConfiguration.mm b/sdk/objc/api/peerconnection/RTCDataChannelConfiguration.mm
index 1208b6d..198bfbb 100644
--- a/sdk/objc/api/peerconnection/RTCDataChannelConfiguration.mm
+++ b/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 {
