From f3736ed3d8d4e6660ad8b82a8deff7aca8389e5c Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 8 Apr 2019 13:09:30 +0200 Subject: [PATCH] Datachannel: Use absl::optional for maxRetransmits and maxRetransmitTime. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Karl Wiberg Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#27507} --- api/data_channel_interface.cc | 8 ++++ api/data_channel_interface.h | 13 ++++-- pc/data_channel.cc | 43 +++++++++++++++---- pc/data_channel.h | 28 ++++++------ pc/peer_connection_integrationtest.cc | 1 + pc/peer_connection_interface_unittest.cc | 18 +++++++- pc/sctp_utils.cc | 22 +++++----- pc/sctp_utils_unittest.cc | 23 +++++----- .../RTCDataChannelConfiguration.mm | 8 +++- 9 files changed, 112 insertions(+), 52 deletions(-) diff --git a/api/data_channel_interface.cc b/api/data_channel_interface.cc index 240ccbe7a2..d299cedf45 100644 --- a/api/data_channel_interface.cc +++ b/api/data_channel_interface.cc @@ -24,6 +24,14 @@ uint16_t DataChannelInterface::maxRetransmits() const { return 0; } +absl::optional DataChannelInterface::maxRetransmitsOpt() const { + return absl::nullopt; +} + +absl::optional 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 1bd874e936..f7032ec069 100644 --- a/api/data_channel_interface.h +++ b/api/data_channel_interface.h @@ -18,6 +18,7 @@ #include #include +#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 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 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 maxRetransmitsOpt() const; + virtual absl::optional maxPacketLifeTime() const; virtual std::string protocol() const; virtual bool negotiated() const; diff --git a/pc/data_channel.cc b/pc/data_channel.cc index e4727f25de..cd4ddedf34 100644 --- a/pc/data_channel.cc +++ b/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 || - 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 @@ 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_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 3e58c2b020..e4166dd3b1 100644 --- a/pc/data_channel.h +++ b/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) - : 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 @@ 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(-1); + } + virtual uint16_t maxRetransmits() const { + return config_.maxRetransmits ? *config_.maxRetransmits + : static_cast(-1); + } + virtual absl::optional maxPacketLifeTime() const { return config_.maxRetransmitTime; } - virtual uint16_t maxRetransmits() const { return config_.maxRetransmits; } + virtual absl::optional 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, maxRetransmitsOpt) +PROXY_CONSTMETHOD0(absl::optional, 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 218a8bb50f..58c343942e 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/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(); diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 371cdc150d..edede253fa 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -2082,7 +2082,6 @@ TEST_P(PeerConnectionInterfaceTest, CreateSctpDataChannel) { CreatePeerConnection(rtc_config); webrtc::DataChannelInit config; - rtc::scoped_refptr 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 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 aa7b6c11c0..7b67fc1839 100644 --- a/pc/sctp_utils.cc +++ b/pc/sctp_utils.cc @@ -108,8 +108,8 @@ bool ParseDataChannelOpenMessage(const rtc::CopyOnWriteBuffer& payload, 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 @@ 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; - } 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 72db9521bb..01f54349ea 100644 --- a/pc/sctp_utils_unittest.cc +++ b/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 - ? 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(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(-1, output_config.maxRetransmits); + EXPECT_EQ(*config.maxRetransmitTime, *output_config.maxRetransmitTime); + 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) { diff --git a/sdk/objc/api/peerconnection/RTCDataChannelConfiguration.mm b/sdk/objc/api/peerconnection/RTCDataChannelConfiguration.mm index 1208b6dbed..198bfbbaed 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 {