From b78e6a93050f637148fd208a3743ee009615d2eb Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 20 Oct 2023 13:16:35 +0200 Subject: [PATCH] dcsctp: Use TimeDelta in TX path This commit replaces the internal use of DurationMs, with millisecond precision, to webrtc::TimeDelta, which uses microsecond precision. This is just a refactoring. The only change to the public API is convenience methods to convert between DurationMs and webrtc::TimeDelta. Bug: webrtc:15593 Change-Id: Ida861bf585c716be5f898d0e7ef98da2c15268b7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325402 Commit-Queue: Victor Boivie Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#41062} --- net/dcsctp/public/BUILD.gn | 1 + net/dcsctp/public/types.h | 10 ++++++++++ net/dcsctp/socket/context.h | 4 ++-- net/dcsctp/socket/heartbeat_handler.cc | 2 +- net/dcsctp/socket/heartbeat_handler_test.cc | 3 ++- net/dcsctp/socket/mock_context.h | 2 +- net/dcsctp/socket/stream_reset_handler_test.cc | 5 +++-- net/dcsctp/socket/transmission_control_block.cc | 7 ++++--- net/dcsctp/socket/transmission_control_block.h | 2 +- net/dcsctp/tx/BUILD.gn | 1 + net/dcsctp/tx/outstanding_data.cc | 9 +++++---- net/dcsctp/tx/outstanding_data.h | 4 ++-- net/dcsctp/tx/outstanding_data_test.cc | 10 +++++----- net/dcsctp/tx/retransmission_queue.cc | 10 +++++----- net/dcsctp/tx/retransmission_queue.h | 4 ++-- net/dcsctp/tx/retransmission_queue_test.cc | 5 +++-- 16 files changed, 48 insertions(+), 31 deletions(-) diff --git a/net/dcsctp/public/BUILD.gn b/net/dcsctp/public/BUILD.gn index 6cb289bf5b..032ffe54fa 100644 --- a/net/dcsctp/public/BUILD.gn +++ b/net/dcsctp/public/BUILD.gn @@ -11,6 +11,7 @@ import("../../../webrtc.gni") rtc_source_set("types") { deps = [ "../../../api:array_view", + "../../../api/units:time_delta", "../../../rtc_base:strong_alias", ] sources = [ diff --git a/net/dcsctp/public/types.h b/net/dcsctp/public/types.h index 7d69875d1a..02e2ce1e5e 100644 --- a/net/dcsctp/public/types.h +++ b/net/dcsctp/public/types.h @@ -14,6 +14,7 @@ #include #include +#include "api/units/time_delta.h" #include "rtc_base/strong_alias.h" namespace dcsctp { @@ -41,6 +42,10 @@ class DurationMs : public webrtc::StrongAlias { constexpr explicit DurationMs(const UnderlyingType& v) : webrtc::StrongAlias(v) {} + constexpr explicit DurationMs(webrtc::TimeDelta v) + : webrtc::StrongAlias( + v.IsInfinite() ? InfiniteDuration() : DurationMs(v.ms())) {} + static constexpr DurationMs InfiniteDuration() { return DurationMs(std::numeric_limits::max()); } @@ -58,6 +63,11 @@ class DurationMs : public webrtc::StrongAlias { value_ *= factor; return *this; } + constexpr webrtc::TimeDelta ToTimeDelta() const { + return *this == DurationMs::InfiniteDuration() + ? webrtc::TimeDelta::PlusInfinity() + : webrtc::TimeDelta::Millis(value_); + } }; constexpr inline DurationMs operator+(DurationMs lhs, DurationMs rhs) { diff --git a/net/dcsctp/socket/context.h b/net/dcsctp/socket/context.h index eca5b9e4fb..88233085b1 100644 --- a/net/dcsctp/socket/context.h +++ b/net/dcsctp/socket/context.h @@ -39,8 +39,8 @@ class Context { // Returns the socket callbacks. virtual DcSctpSocketCallbacks& callbacks() const = 0; - // Observes a measured RTT value, in milliseconds. - virtual void ObserveRTT(DurationMs rtt_ms) = 0; + // Observes a measured RTT value. + virtual void ObserveRTT(webrtc::TimeDelta rtt_ms) = 0; // Returns the current Retransmission Timeout (rto) value, in milliseconds. virtual DurationMs current_rto() const = 0; diff --git a/net/dcsctp/socket/heartbeat_handler.cc b/net/dcsctp/socket/heartbeat_handler.cc index 8d1d04a07e..3e0437b897 100644 --- a/net/dcsctp/socket/heartbeat_handler.cc +++ b/net/dcsctp/socket/heartbeat_handler.cc @@ -155,7 +155,7 @@ void HeartbeatHandler::HandleHeartbeatAck(HeartbeatAckChunk chunk) { TimeMs now = ctx_->callbacks().TimeMillis(); if (info->created_at() > TimeMs(0) && info->created_at() <= now) { - ctx_->ObserveRTT(now - info->created_at()); + ctx_->ObserveRTT((now - info->created_at()).ToTimeDelta()); } // https://tools.ietf.org/html/rfc4960#section-8.1 diff --git a/net/dcsctp/socket/heartbeat_handler_test.cc b/net/dcsctp/socket/heartbeat_handler_test.cc index d573192440..fc7cd52fe6 100644 --- a/net/dcsctp/socket/heartbeat_handler_test.cc +++ b/net/dcsctp/socket/heartbeat_handler_test.cc @@ -30,6 +30,7 @@ using ::testing::IsEmpty; using ::testing::NiceMock; using ::testing::Return; using ::testing::SizeIs; +using ::webrtc::TimeDelta; constexpr DurationMs kHeartbeatInterval = DurationMs(30'000); @@ -136,7 +137,7 @@ TEST_F(HeartbeatHandlerTest, SendsHeartbeatRequestsOnIdleChannel) { // Respond a while later. This RTT will be measured by the handler constexpr DurationMs rtt(313); - EXPECT_CALL(context_, ObserveRTT(rtt)).Times(1); + EXPECT_CALL(context_, ObserveRTT(rtt.ToTimeDelta())).Times(1); callbacks_.AdvanceTime(rtt); handler_.HandleHeartbeatAck(std::move(ack)); diff --git a/net/dcsctp/socket/mock_context.h b/net/dcsctp/socket/mock_context.h index 88e71d1b35..7c5cd4ec9a 100644 --- a/net/dcsctp/socket/mock_context.h +++ b/net/dcsctp/socket/mock_context.h @@ -51,7 +51,7 @@ class MockContext : public Context { MOCK_METHOD(TSN, peer_initial_tsn, (), (const, override)); MOCK_METHOD(DcSctpSocketCallbacks&, callbacks, (), (const, override)); - MOCK_METHOD(void, ObserveRTT, (DurationMs rtt_ms), (override)); + MOCK_METHOD(void, ObserveRTT, (webrtc::TimeDelta rtt), (override)); MOCK_METHOD(DurationMs, current_rto, (), (const, override)); MOCK_METHOD(bool, IncrementTxErrorCounter, diff --git a/net/dcsctp/socket/stream_reset_handler_test.cc b/net/dcsctp/socket/stream_reset_handler_test.cc index 4b129700ad..d459450ba5 100644 --- a/net/dcsctp/socket/stream_reset_handler_test.cc +++ b/net/dcsctp/socket/stream_reset_handler_test.cc @@ -48,6 +48,7 @@ using ::testing::Property; using ::testing::Return; using ::testing::SizeIs; using ::testing::UnorderedElementsAre; +using ::webrtc::TimeDelta; using ResponseResult = ReconfigurationResponseParameter::Result; using SkippedStream = AnyForwardTsnChunk::SkippedStream; @@ -115,7 +116,7 @@ class StreamResetHandlerTest : public testing::Test { kMyInitialTsn, kArwnd, producer_, - [](DurationMs rtt_ms) {}, + [](TimeDelta rtt) {}, []() {}, *t3_rtx_timer_, DcSctpOptions())), @@ -205,7 +206,7 @@ class StreamResetHandlerTest : public testing::Test { reasm_->RestoreFromState(state); retransmission_queue_ = std::make_unique( "", &callbacks_, kMyInitialTsn, kArwnd, producer_, - [](DurationMs rtt_ms) {}, []() {}, *t3_rtx_timer_, DcSctpOptions(), + [](TimeDelta rtt) {}, []() {}, *t3_rtx_timer_, DcSctpOptions(), /*supports_partial_reliability=*/true, /*use_message_interleaving=*/false); retransmission_queue_->RestoreFromState(state); diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc index c403b064de..c1d353349f 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -37,6 +37,7 @@ #include "rtc_base/strings/string_builder.h" namespace dcsctp { +using ::webrtc::TimeDelta; TransmissionControlBlock::TransmissionControlBlock( TimerManager& timer_manager, @@ -112,10 +113,10 @@ TransmissionControlBlock::TransmissionControlBlock( send_queue.EnableMessageInterleaving(capabilities.message_interleaving); } -void TransmissionControlBlock::ObserveRTT(DurationMs rtt) { +void TransmissionControlBlock::ObserveRTT(TimeDelta rtt) { DurationMs prev_rto = rto_.rto(); - rto_.ObserveRTT(rtt); - RTC_DLOG(LS_VERBOSE) << log_prefix_ << "new rtt=" << *rtt + rto_.ObserveRTT(DurationMs(rtt)); + RTC_DLOG(LS_VERBOSE) << log_prefix_ << "new rtt=" << webrtc::ToString(rtt) << ", srtt=" << *rto_.srtt() << ", rto=" << *rto_.rto() << " (" << *prev_rto << ")"; t3_rtx_->set_duration(rto_.rto()); diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h index 240454a684..8fda890169 100644 --- a/net/dcsctp/socket/transmission_control_block.h +++ b/net/dcsctp/socket/transmission_control_block.h @@ -67,7 +67,7 @@ class TransmissionControlBlock : public Context { TSN my_initial_tsn() const override { return my_initial_tsn_; } TSN peer_initial_tsn() const override { return peer_initial_tsn_; } DcSctpSocketCallbacks& callbacks() const override { return callbacks_; } - void ObserveRTT(DurationMs rtt) override; + void ObserveRTT(webrtc::TimeDelta rtt) override; DurationMs current_rto() const override { return rto_.rto(); } bool IncrementTxErrorCounter(absl::string_view reason) override { return tx_error_counter_.Increment(reason); diff --git a/net/dcsctp/tx/BUILD.gn b/net/dcsctp/tx/BUILD.gn index 5547ffa870..cd42768964 100644 --- a/net/dcsctp/tx/BUILD.gn +++ b/net/dcsctp/tx/BUILD.gn @@ -103,6 +103,7 @@ rtc_library("outstanding_data") { ":retransmission_timeout", ":send_queue", "../../../api:array_view", + "../../../api/units:time_delta", "../../../rtc_base:checks", "../../../rtc_base:logging", "../../../rtc_base/containers:flat_set", diff --git a/net/dcsctp/tx/outstanding_data.cc b/net/dcsctp/tx/outstanding_data.cc index c2706bd0d2..2a92f14218 100644 --- a/net/dcsctp/tx/outstanding_data.cc +++ b/net/dcsctp/tx/outstanding_data.cc @@ -14,6 +14,7 @@ #include #include +#include "api/units/time_delta.h" #include "net/dcsctp/common/math.h" #include "net/dcsctp/common/sequence_numbers.h" #include "net/dcsctp/public/types.h" @@ -441,8 +442,8 @@ void OutstandingData::NackAll() { RTC_DCHECK(IsConsistent()); } -absl::optional OutstandingData::MeasureRTT(TimeMs now, - UnwrappedTSN tsn) const { +webrtc::TimeDelta OutstandingData::MeasureRTT(TimeMs now, + UnwrappedTSN tsn) const { auto it = outstanding_data_.find(tsn); if (it != outstanding_data_.end() && !it->second.has_been_retransmitted()) { // https://tools.ietf.org/html/rfc4960#section-6.3.1 @@ -450,9 +451,9 @@ absl::optional OutstandingData::MeasureRTT(TimeMs now, // packets that were retransmitted (and thus for which it is ambiguous // whether the reply was for the first instance of the chunk or for a // later instance)" - return now - it->second.time_sent(); + return (now - it->second.time_sent()).ToTimeDelta(); } - return absl::nullopt; + return webrtc::TimeDelta::PlusInfinity(); } std::vector> diff --git a/net/dcsctp/tx/outstanding_data.h b/net/dcsctp/tx/outstanding_data.h index f8e939661d..323f2de349 100644 --- a/net/dcsctp/tx/outstanding_data.h +++ b/net/dcsctp/tx/outstanding_data.h @@ -147,8 +147,8 @@ class OutstandingData { // Given the current time and a TSN, it returns the measured RTT between when // the chunk was sent and now. It takes into acccount Karn's algorithm, so if - // the chunk has ever been retransmitted, it will return absl::nullopt. - absl::optional MeasureRTT(TimeMs now, UnwrappedTSN tsn) const; + // the chunk has ever been retransmitted, it will return `PlusInfinity()`. + webrtc::TimeDelta MeasureRTT(TimeMs now, UnwrappedTSN tsn) const; // Returns the internal state of all queued chunks. This is only used in // unit-tests. diff --git a/net/dcsctp/tx/outstanding_data_test.cc b/net/dcsctp/tx/outstanding_data_test.cc index b8c2e593a1..e119a56607 100644 --- a/net/dcsctp/tx/outstanding_data_test.cc +++ b/net/dcsctp/tx/outstanding_data_test.cc @@ -37,6 +37,7 @@ using ::testing::Property; using ::testing::Return; using ::testing::StrictMock; using ::testing::UnorderedElementsAre; +using ::webrtc::TimeDelta; constexpr TimeMs kNow(42); constexpr OutgoingMessageId kMessageId = OutgoingMessageId(17); @@ -365,12 +366,11 @@ TEST_F(OutstandingDataTest, MeasureRTT) { buf_.Insert(kMessageId, gen_.Ordered({1}, "BE"), kNow + DurationMs(1)); buf_.Insert(kMessageId, gen_.Ordered({1}, "BE"), kNow + DurationMs(2)); - static constexpr DurationMs kDuration(123); - ASSERT_HAS_VALUE_AND_ASSIGN( - DurationMs duration, - buf_.MeasureRTT(kNow + kDuration, unwrapper_.Unwrap(TSN(11)))); + static constexpr TimeDelta kDuration = TimeDelta::Millis(123); + TimeDelta duration = + buf_.MeasureRTT(kNow + DurationMs(kDuration), unwrapper_.Unwrap(TSN(11))); - EXPECT_EQ(duration, kDuration - DurationMs(1)); + EXPECT_EQ(duration, kDuration - TimeDelta::Millis(1)); } TEST_F(OutstandingDataTest, MustRetransmitBeforeGettingNackedAgain) { diff --git a/net/dcsctp/tx/retransmission_queue.cc b/net/dcsctp/tx/retransmission_queue.cc index 2b9843f4a7..2b166f1fde 100644 --- a/net/dcsctp/tx/retransmission_queue.cc +++ b/net/dcsctp/tx/retransmission_queue.cc @@ -44,6 +44,7 @@ namespace dcsctp { namespace { +using ::webrtc::TimeDelta; // Allow sending only slightly less than an MTU, to account for headers. constexpr float kMinBytesRequiredToSendFactor = 0.9; @@ -55,7 +56,7 @@ RetransmissionQueue::RetransmissionQueue( TSN my_initial_tsn, size_t a_rwnd, SendQueue& send_queue, - std::function on_new_rtt, + std::function on_new_rtt, std::function on_clear_retransmission_counter, Timer& t3_rtx, const DcSctpOptions& options, @@ -345,11 +346,10 @@ void RetransmissionQueue::UpdateRTT(TimeMs now, // TODO(boivie): Consider occasionally sending DATA chunks with I-bit set and // use only those packets for measurement. - absl::optional rtt = - outstanding_data_.MeasureRTT(now, cumulative_tsn_ack); + TimeDelta rtt = outstanding_data_.MeasureRTT(now, cumulative_tsn_ack); - if (rtt.has_value()) { - on_new_rtt_(*rtt); + if (rtt.IsFinite()) { + on_new_rtt_(rtt); } } diff --git a/net/dcsctp/tx/retransmission_queue.h b/net/dcsctp/tx/retransmission_queue.h index b44db2a9a0..d4c6edf7fb 100644 --- a/net/dcsctp/tx/retransmission_queue.h +++ b/net/dcsctp/tx/retransmission_queue.h @@ -60,7 +60,7 @@ class RetransmissionQueue { TSN my_initial_tsn, size_t a_rwnd, SendQueue& send_queue, - std::function on_new_rtt, + std::function on_new_rtt, std::function on_clear_retransmission_counter, Timer& t3_rtx, const DcSctpOptions& options, @@ -230,7 +230,7 @@ class RetransmissionQueue { // The size of the data chunk (DATA/I-DATA) header that is used. const size_t data_chunk_header_size_; // Called when a new RTT measurement has been done - const std::function on_new_rtt_; + const std::function on_new_rtt_; // Called when a SACK has been seen that cleared the retransmission counter. const std::function on_clear_retransmission_counter_; // The retransmission counter. diff --git a/net/dcsctp/tx/retransmission_queue_test.cc b/net/dcsctp/tx/retransmission_queue_test.cc index 096bb1262a..d7b39e502d 100644 --- a/net/dcsctp/tx/retransmission_queue_test.cc +++ b/net/dcsctp/tx/retransmission_queue_test.cc @@ -52,6 +52,7 @@ using ::testing::Pair; using ::testing::Return; using ::testing::SizeIs; using ::testing::UnorderedElementsAre; +using ::webrtc::TimeDelta; constexpr uint32_t kArwnd = 100000; constexpr uint32_t kMaxMtu = 1191; @@ -130,7 +131,7 @@ class RetransmissionQueueTest : public testing::Test { TimeMs now_ = TimeMs(0); FakeTimeoutManager timeout_manager_; TimerManager timer_manager_; - NiceMock> on_rtt_; + NiceMock> on_rtt_; NiceMock> on_clear_retransmission_counter_; NiceMock producer_; std::unique_ptr timer_; @@ -864,7 +865,7 @@ TEST_F(RetransmissionQueueTest, MeasureRTT) { now_ = now_ + DurationMs(123); - EXPECT_CALL(on_rtt_, Call(DurationMs(123))).Times(1); + EXPECT_CALL(on_rtt_, Call(TimeDelta::Millis(123))).Times(1); queue.HandleSack(now_, SackChunk(TSN(10), kArwnd, {}, {})); }