diff --git a/api/data_channel_interface.h b/api/data_channel_interface.h index 5b2b1263ab..56bb6c98fb 100644 --- a/api/data_channel_interface.h +++ b/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 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 maxRetransmits; // This is set by the application and opaque to the WebRTC implementation. diff --git a/api/transport/data_channel_transport_interface.h b/api/transport/data_channel_transport_interface.h index 7b8c653c39..550fabaacd 100644 --- a/api/transport/data_channel_transport_interface.h +++ b/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 - // simultaneously. + // Valid values are in the range [0-UINT16_MAX]. + // |max_rtx_count| and |max_rtx_ms| may not be set simultaneously. absl::optional 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 - // simultaneously. + // Valid values are in the range [0-UINT16_MAX]. + // |max_rtx_count| and |max_rtx_ms| may not be set simultaneously. absl::optional max_rtx_ms; }; diff --git a/media/base/media_channel.h b/media/base/media_channel.h index 1c25a90194..8b1d7ad657 100644 --- a/media/base/media_channel.h +++ b/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. - // If false, messages may be lost. - bool reliable = false; - // For SCTP, if reliable == false, provide partial reliability by - // resending up to this many times. Either count or millis - // is supported, not both at the same time. - 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; + // Provide partial reliability by resending up to this many times. Either + // count or millis is supported, not both at the same time. + absl::optional max_rtx_count; + // Provide partial reliability by resending for up to this many milliseconds. + // Either count or millis is supported, not both at the same time. + absl::optional max_rtx_ms; }; enum SendDataResult { SDR_SUCCESS, SDR_ERROR, SDR_BLOCK }; diff --git a/media/sctp/dcsctp_transport.cc b/media/sctp/dcsctp_transport.cc index 6cf1c693e5..15d1742eb0 100644 --- a/media/sctp/dcsctp_transport.cc +++ b/media/sctp/dcsctp_transport.cc @@ -11,6 +11,7 @@ #include "media/sctp/dcsctp_transport.h" #include +#include #include #include @@ -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) - send_options.lifetime = dcsctp::DurationMs(params.max_rtx_ms); - if (params.max_rtx_count > 0) - send_options.max_retransmissions = - static_cast(params.max_rtx_count); + if (params.max_rtx_ms.has_value()) { + RTC_DCHECK(*params.max_rtx_ms >= 0 && + *params.max_rtx_ms <= std::numeric_limits::max()); + send_options.lifetime = dcsctp::DurationMs(*params.max_rtx_ms); + } + if (params.max_rtx_count.has_value()) { + RTC_DCHECK(*params.max_rtx_count >= 0 && + *params.max_rtx_count <= std::numeric_limits::max()); + send_options.max_retransmissions = *params.max_rtx_count; + } auto error = socket_->Send(std::move(message), send_options); switch (error) { diff --git a/media/sctp/usrsctp_transport.cc b/media/sctp/usrsctp_transport.cc index 18e0384876..cfd5d6f7f5 100644 --- a/media/sctp/usrsctp_transport.cc +++ b/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; - spa.sendv_prinfo.pr_policy = SCTP_PR_SCTP_RTX; - spa.sendv_prinfo.pr_value = params.max_rtx_count; - } else { - 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; - } + } + if (params.max_rtx_count.has_value()) { + RTC_DCHECK(*params.max_rtx_count >= 0 && + *params.max_rtx_count <= std::numeric_limits::max()); + spa.sendv_flags |= SCTP_SEND_PRINFO_VALID; + spa.sendv_prinfo.pr_policy = SCTP_PR_SCTP_RTX; + spa.sendv_prinfo.pr_value = *params.max_rtx_count; + } + if (params.max_rtx_ms.has_value()) { + RTC_DCHECK(*params.max_rtx_ms >= 0 && + *params.max_rtx_ms <= std::numeric_limits::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; } diff --git a/media/sctp/usrsctp_transport_reliability_unittest.cc b/media/sctp/usrsctp_transport_reliability_unittest.cc index ddc8419a11..407dd8d134 100644 --- a/media/sctp/usrsctp_transport_reliability_unittest.cc +++ b/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 = INT_MAX; - send_params.max_rtx_ms = INT_MAX; + send_params.max_rtx_count = std::numeric_limits::max(); + send_params.max_rtx_ms = std::numeric_limits::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 diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 6b3500cbff..7e6dc4b0ae 100644 --- a/pc/data_channel_controller.cc +++ b/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; - } else if (params.max_rtx_ms >= 0) { - send_params.max_rtx_ms = params.max_rtx_ms; - } + send_params.max_rtx_count = params.max_rtx_count; + send_params.max_rtx_ms = params.max_rtx_ms; RTCError error = network_thread()->Invoke( RTC_FROM_HERE, [this, params, send_params, payload] { diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc index f67a0e7ea7..2c9066155c 100644 --- a/pc/sctp_data_channel.cc +++ b/pc/sctp_data_channel.cc @@ -10,6 +10,7 @@ #include "pc/sctp_data_channel.h" +#include #include #include #include @@ -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 - // have been set to -1, unset them. - if (maxRetransmits && *maxRetransmits == -1) { - RTC_LOG(LS_ERROR) - << "Accepting maxRetransmits = -1 for backwards compatibility"; - maxRetransmits = absl::nullopt; + // Backwards compatibility: If maxRetransmits or maxRetransmitTime + // are negative, the feature is not enabled. + // Values are clamped to a 16bit range. + if (maxRetransmits) { + if (*maxRetransmits < 0) { + RTC_LOG(LS_ERROR) + << "Accepting maxRetransmits < 0 for backwards compatibility"; + maxRetransmits = absl::nullopt; + } else if (*maxRetransmits > std::numeric_limits::max()) { + maxRetransmits = std::numeric_limits::max(); + } } - if (maxRetransmitTime && *maxRetransmitTime == -1) { - RTC_LOG(LS_ERROR) - << "Accepting maxRetransmitTime = -1 for backwards compatibility"; - maxRetransmitTime = absl::nullopt; + + if (maxRetransmitTime) { + if (*maxRetransmitTime < 0) { + RTC_LOG(LS_ERROR) + << "Accepting maxRetransmitTime < 0 for backwards compatibility"; + maxRetransmitTime = absl::nullopt; + } else if (*maxRetransmitTime > std::numeric_limits::max()) { + maxRetransmitTime = std::numeric_limits::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 = - config_.maxRetransmits ? *config_.maxRetransmits : -1; - send_params.max_rtx_ms = - config_.maxRetransmitTime ? *config_.maxRetransmitTime : -1; + send_params.max_rtx_count = config_.maxRetransmits; + send_params.max_rtx_ms = config_.maxRetransmitTime; send_params.sid = config_.id; send_params.type = buffer.binary ? cricket::DMT_BINARY : cricket::DMT_TEXT; diff --git a/pc/sctp_data_channel_transport.cc b/pc/sctp_data_channel_transport.cc index 135c14424a..786931c609 100644 --- a/pc/sctp_data_channel_transport.cc +++ b/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.value_or(-1); - sd_params.max_rtx_ms = params.max_rtx_ms.value_or(-1); + sd_params.max_rtx_count = params.max_rtx_count; + sd_params.max_rtx_ms = params.max_rtx_ms; cricket::SendDataResult result; sctp_transport_->SendData(sd_params, buffer, &result);