From e62a588314f15c926b5732df0fabfd55b10548f2 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Wed, 9 Oct 2019 20:43:32 +0200 Subject: [PATCH] Merging TransportFeedbackAdapter and SendTimeHistory. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They were already tightly coupled, merging them makes the relations clearer. We also remove the kill switch for removing duplicate feedback events since there has been no need to use it. The potential to account for bytes sent in AddNewPacket was also removed since it is not used by TransportFeedbackAdapter. Bug: webrtc:9883 Change-Id: I51823e0ce838c22158637954749310e0d0eeff27 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/156140 Commit-Queue: Sebastian Jansson Reviewed-by: Björn Terelius Cr-Commit-Position: refs/heads/master@{#29449} --- modules/congestion_controller/rtp/BUILD.gn | 3 - .../rtp/send_time_history.cc | 169 ------------ .../rtp/send_time_history.h | 75 ----- .../rtp/send_time_history_unittest.cc | 260 ------------------ .../rtp/transport_feedback_adapter.cc | 161 ++++++++--- .../rtp/transport_feedback_adapter.h | 33 ++- .../transport_feedback_adapter_unittest.cc | 30 -- 7 files changed, 154 insertions(+), 577 deletions(-) delete mode 100644 modules/congestion_controller/rtp/send_time_history.cc delete mode 100644 modules/congestion_controller/rtp/send_time_history.h delete mode 100644 modules/congestion_controller/rtp/send_time_history_unittest.cc diff --git a/modules/congestion_controller/rtp/BUILD.gn b/modules/congestion_controller/rtp/BUILD.gn index 2c7377990e..d3e995e8de 100644 --- a/modules/congestion_controller/rtp/BUILD.gn +++ b/modules/congestion_controller/rtp/BUILD.gn @@ -43,8 +43,6 @@ rtc_source_set("control_handler") { rtc_static_library("transport_feedback") { visibility = [ "*" ] sources = [ - "send_time_history.cc", - "send_time_history.h", "transport_feedback_adapter.cc", "transport_feedback_adapter.h", ] @@ -71,7 +69,6 @@ if (rtc_include_tests) { sources = [ "congestion_controller_unittests_helper.cc", "congestion_controller_unittests_helper.h", - "send_time_history_unittest.cc", "transport_feedback_adapter_unittest.cc", ] deps = [ diff --git a/modules/congestion_controller/rtp/send_time_history.cc b/modules/congestion_controller/rtp/send_time_history.cc deleted file mode 100644 index 2d0356b996..0000000000 --- a/modules/congestion_controller/rtp/send_time_history.cc +++ /dev/null @@ -1,169 +0,0 @@ -/* - * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/congestion_controller/rtp/send_time_history.h" - -#include -#include - -#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "rtc_base/checks.h" -#include "rtc_base/logging.h" - -namespace webrtc { - -SendTimeHistory::SendTimeHistory(int64_t packet_age_limit_ms) - : packet_age_limit_ms_(packet_age_limit_ms) {} - -SendTimeHistory::~SendTimeHistory() {} - -void SendTimeHistory::RemoveOld(int64_t at_time_ms) { - while (!history_.empty() && - at_time_ms - history_.begin()->second.creation_time_ms > - packet_age_limit_ms_) { - // TODO(sprang): Warn if erasing (too many) old items? - RemovePacketBytes(history_.begin()->second); - history_.erase(history_.begin()); - } -} - -void SendTimeHistory::AddNewPacket(PacketFeedback packet) { - packet.long_sequence_number = - seq_num_unwrapper_.Unwrap(packet.sequence_number); - history_.insert(std::make_pair(packet.long_sequence_number, packet)); - if (packet.send_time_ms >= 0) { - AddPacketBytes(packet); - last_send_time_ms_ = std::max(last_send_time_ms_, packet.send_time_ms); - } -} - -void SendTimeHistory::AddUntracked(size_t packet_size, int64_t send_time_ms) { - if (send_time_ms < last_send_time_ms_) { - RTC_LOG(LS_WARNING) << "ignoring untracked data for out of order packet."; - } - pending_untracked_size_ += packet_size; - last_untracked_send_time_ms_ = - std::max(last_untracked_send_time_ms_, send_time_ms); -} - -SendTimeHistory::Status SendTimeHistory::OnSentPacket(uint16_t sequence_number, - int64_t send_time_ms) { - int64_t unwrapped_seq_num = seq_num_unwrapper_.Unwrap(sequence_number); - auto it = history_.find(unwrapped_seq_num); - if (it == history_.end()) - return Status::kNotAdded; - bool packet_retransmit = it->second.send_time_ms >= 0; - it->second.send_time_ms = send_time_ms; - last_send_time_ms_ = std::max(last_send_time_ms_, send_time_ms); - if (!packet_retransmit) - AddPacketBytes(it->second); - if (pending_untracked_size_ > 0) { - if (send_time_ms < last_untracked_send_time_ms_) - RTC_LOG(LS_WARNING) - << "appending acknowledged data for out of order packet. (Diff: " - << last_untracked_send_time_ms_ - send_time_ms << " ms.)"; - it->second.unacknowledged_data += pending_untracked_size_; - pending_untracked_size_ = 0; - } - return packet_retransmit ? Status::kDuplicate : Status::kOk; -} - -absl::optional SendTimeHistory::GetPacket( - uint16_t sequence_number) const { - int64_t unwrapped_seq_num = - seq_num_unwrapper_.UnwrapWithoutUpdate(sequence_number); - absl::optional optional_feedback; - auto it = history_.find(unwrapped_seq_num); - if (it != history_.end()) - optional_feedback.emplace(it->second); - return optional_feedback; -} - -bool SendTimeHistory::GetFeedback(PacketFeedback* packet_feedback, - bool remove) { - RTC_DCHECK(packet_feedback); - int64_t unwrapped_seq_num = - seq_num_unwrapper_.Unwrap(packet_feedback->sequence_number); - UpdateAckedSeqNum(unwrapped_seq_num); - RTC_DCHECK_GE(*last_ack_seq_num_, 0); - auto it = history_.find(unwrapped_seq_num); - if (it == history_.end()) - return false; - - // Save arrival_time not to overwrite it. - int64_t arrival_time_ms = packet_feedback->arrival_time_ms; - *packet_feedback = it->second; - packet_feedback->arrival_time_ms = arrival_time_ms; - - if (remove) - history_.erase(it); - return true; -} - -DataSize SendTimeHistory::GetOutstandingData(uint16_t local_net_id, - uint16_t remote_net_id) const { - auto it = in_flight_bytes_.find({local_net_id, remote_net_id}); - if (it != in_flight_bytes_.end()) { - return DataSize::bytes(it->second); - } else { - return DataSize::Zero(); - } -} - -absl::optional SendTimeHistory::GetFirstUnackedSendTime() const { - if (!last_ack_seq_num_) - return absl::nullopt; - auto it = history_.find(*last_ack_seq_num_); - if (it == history_.end() || - it->second.send_time_ms == PacketFeedback::kNoSendTime) - return absl::nullopt; - return it->second.send_time_ms; -} - -void SendTimeHistory::AddPacketBytes(const PacketFeedback& packet) { - if (packet.send_time_ms < 0 || packet.payload_size == 0 || - (last_ack_seq_num_ && *last_ack_seq_num_ >= packet.long_sequence_number)) - return; - auto it = in_flight_bytes_.find({packet.local_net_id, packet.remote_net_id}); - if (it != in_flight_bytes_.end()) { - it->second += packet.payload_size; - } else { - in_flight_bytes_[{packet.local_net_id, packet.remote_net_id}] = - packet.payload_size; - } -} - -void SendTimeHistory::RemovePacketBytes(const PacketFeedback& packet) { - if (packet.send_time_ms < 0 || packet.payload_size == 0 || - (last_ack_seq_num_ && *last_ack_seq_num_ >= packet.long_sequence_number)) - return; - auto it = in_flight_bytes_.find({packet.local_net_id, packet.remote_net_id}); - if (it != in_flight_bytes_.end()) { - it->second -= packet.payload_size; - if (it->second == 0) - in_flight_bytes_.erase(it); - } -} - -void SendTimeHistory::UpdateAckedSeqNum(int64_t acked_seq_num) { - if (last_ack_seq_num_ && *last_ack_seq_num_ >= acked_seq_num) - return; - - auto unacked_it = history_.begin(); - if (last_ack_seq_num_) - unacked_it = history_.lower_bound(*last_ack_seq_num_); - - auto newly_acked_end = history_.upper_bound(acked_seq_num); - for (; unacked_it != newly_acked_end; ++unacked_it) { - RemovePacketBytes(unacked_it->second); - } - last_ack_seq_num_.emplace(acked_seq_num); -} -} // namespace webrtc diff --git a/modules/congestion_controller/rtp/send_time_history.h b/modules/congestion_controller/rtp/send_time_history.h deleted file mode 100644 index 9563fb8681..0000000000 --- a/modules/congestion_controller/rtp/send_time_history.h +++ /dev/null @@ -1,75 +0,0 @@ -/* - * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MODULES_CONGESTION_CONTROLLER_RTP_SEND_TIME_HISTORY_H_ -#define MODULES_CONGESTION_CONTROLLER_RTP_SEND_TIME_HISTORY_H_ - -#include -#include - -#include "absl/types/optional.h" -#include "api/units/data_size.h" -#include "modules/include/module_common_types_public.h" -#include "rtc_base/constructor_magic.h" - -namespace webrtc { -struct PacketFeedback; - -class SendTimeHistory { - public: - enum class Status { kNotAdded, kOk, kDuplicate }; - - explicit SendTimeHistory(int64_t packet_age_limit_ms); - ~SendTimeHistory(); - - // Cleanup old entries, then add new packet info with provided parameters. - void RemoveOld(int64_t at_time_ms); - void AddNewPacket(PacketFeedback packet); - - void AddUntracked(size_t packet_size, int64_t send_time_ms); - - // Updates packet info identified by |sequence_number| with |send_time_ms|. - // Returns a PacketSendState indicating if the packet was not found, sent, - // or if it was previously already marked as sent. - Status OnSentPacket(uint16_t sequence_number, int64_t send_time_ms); - - // Retrieves packet info identified by |sequence_number|. - absl::optional GetPacket(uint16_t sequence_number) const; - - // Look up PacketFeedback for a sent packet, based on the sequence number, and - // populate all fields except for arrival_time. The packet parameter must - // thus be non-null and have the sequence_number field set. - bool GetFeedback(PacketFeedback* packet_feedback, bool remove); - - DataSize GetOutstandingData(uint16_t local_net_id, - uint16_t remote_net_id) const; - - absl::optional GetFirstUnackedSendTime() const; - - private: - using RemoteAndLocalNetworkId = std::pair; - - void AddPacketBytes(const PacketFeedback& packet); - void RemovePacketBytes(const PacketFeedback& packet); - void UpdateAckedSeqNum(int64_t acked_seq_num); - const int64_t packet_age_limit_ms_; - size_t pending_untracked_size_ = 0; - int64_t last_send_time_ms_ = -1; - int64_t last_untracked_send_time_ms_ = -1; - SequenceNumberUnwrapper seq_num_unwrapper_; - std::map history_; - absl::optional last_ack_seq_num_; - std::map in_flight_bytes_; - - RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(SendTimeHistory); -}; - -} // namespace webrtc -#endif // MODULES_CONGESTION_CONTROLLER_RTP_SEND_TIME_HISTORY_H_ diff --git a/modules/congestion_controller/rtp/send_time_history_unittest.cc b/modules/congestion_controller/rtp/send_time_history_unittest.cc deleted file mode 100644 index 604685cee8..0000000000 --- a/modules/congestion_controller/rtp/send_time_history_unittest.cc +++ /dev/null @@ -1,260 +0,0 @@ -/* - * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "modules/congestion_controller/rtp/send_time_history.h" - -#include -#include -#include -#include - -#include "api/transport/network_types.h" -#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "system_wrappers/include/clock.h" -#include "test/gtest.h" - -namespace webrtc { -namespace test { - -static const int kDefaultHistoryLengthMs = 1000; - -class SendTimeHistoryTest : public ::testing::Test { - protected: - SendTimeHistoryTest() : clock_(0), history_(kDefaultHistoryLengthMs) {} - ~SendTimeHistoryTest() {} - - virtual void SetUp() {} - - virtual void TearDown() {} - - void AddPacketWithSendTime(uint16_t sequence_number, - size_t length, - int64_t send_time_ms, - const PacedPacketInfo& pacing_info) { - PacketFeedback packet(clock_.TimeInMilliseconds(), sequence_number, length, - 0, 0, pacing_info); - history_.RemoveOld(clock_.TimeInMilliseconds()); - history_.AddNewPacket(packet); - history_.OnSentPacket(sequence_number, send_time_ms); - } - - webrtc::SimulatedClock clock_; - SendTimeHistory history_; -}; - -TEST_F(SendTimeHistoryTest, SaveAndRestoreNetworkId) { - const PacedPacketInfo kPacingInfo(0, 5, 1200); - uint16_t sequence_number = 0; - int64_t now_ms = clock_.TimeInMilliseconds(); - for (int i = 1; i < 5; ++i) { - PacketFeedback packet(now_ms, sequence_number, 1000, i, i - 1, kPacingInfo); - history_.RemoveOld(clock_.TimeInMilliseconds()); - history_.AddNewPacket(packet); - history_.OnSentPacket(sequence_number, now_ms); - PacketFeedback restored(now_ms, sequence_number); - EXPECT_TRUE(history_.GetFeedback(&restored, sequence_number++)); - EXPECT_EQ(packet.local_net_id, restored.local_net_id); - EXPECT_EQ(packet.remote_net_id, restored.remote_net_id); - } -} - -TEST_F(SendTimeHistoryTest, AddRemoveOne) { - const uint16_t kSeqNo = 10; - // TODO(philipel): Fix PacedPacketInfo constructor? - const PacedPacketInfo kPacingInfo(0, 5, 1200); - const PacketFeedback kSentPacket(0, 1, kSeqNo, 1, kPacingInfo); - AddPacketWithSendTime(kSeqNo, 1, 1, kPacingInfo); - - PacketFeedback received_packet(0, 0, kSeqNo, 0, kPacingInfo); - EXPECT_TRUE(history_.GetFeedback(&received_packet, false)); - EXPECT_EQ(kSentPacket, received_packet); - - PacketFeedback received_packet2(0, 0, kSeqNo, 0, kPacingInfo); - EXPECT_TRUE(history_.GetFeedback(&received_packet2, true)); - EXPECT_EQ(kSentPacket, received_packet2); - - PacketFeedback received_packet3(0, 0, kSeqNo, 0, kPacingInfo); - EXPECT_FALSE(history_.GetFeedback(&received_packet3, true)); -} - -TEST_F(SendTimeHistoryTest, GetPacketReturnsSentPacket) { - const uint16_t kSeqNo = 10; - const PacedPacketInfo kPacingInfo(0, 5, 1200); - const PacketFeedback kSentPacket(0, -1, 1, kSeqNo, 123, 0, 0, kPacingInfo); - AddPacketWithSendTime(kSeqNo, 123, 1, kPacingInfo); - auto sent_packet = history_.GetPacket(kSeqNo); - EXPECT_EQ(kSentPacket, *sent_packet); -} - -TEST_F(SendTimeHistoryTest, GetPacketEmptyForRemovedPacket) { - const uint16_t kSeqNo = 10; - const PacedPacketInfo kPacingInfo(0, 5, 1200); - AddPacketWithSendTime(kSeqNo, 123, 1, kPacingInfo); - auto sent_packet = history_.GetPacket(kSeqNo); - PacketFeedback received_packet(0, 0, kSeqNo, 0, kPacingInfo); - EXPECT_TRUE(history_.GetFeedback(&received_packet, true)); - sent_packet = history_.GetPacket(kSeqNo); - EXPECT_FALSE(sent_packet.has_value()); -} - -TEST_F(SendTimeHistoryTest, PopulatesExpectedFields) { - const uint16_t kSeqNo = 10; - const int64_t kSendTime = 1000; - const int64_t kReceiveTime = 2000; - const size_t kPayloadSize = 42; - const PacedPacketInfo kPacingInfo(3, 10, 1212); - - AddPacketWithSendTime(kSeqNo, kPayloadSize, kSendTime, kPacingInfo); - - PacketFeedback packet_feedback(kReceiveTime, kSeqNo); - EXPECT_TRUE(history_.GetFeedback(&packet_feedback, true)); - EXPECT_EQ(kReceiveTime, packet_feedback.arrival_time_ms); - EXPECT_EQ(kSendTime, packet_feedback.send_time_ms); - EXPECT_EQ(kSeqNo, packet_feedback.sequence_number); - EXPECT_EQ(kPayloadSize, packet_feedback.payload_size); - EXPECT_EQ(kPacingInfo, packet_feedback.pacing_info); -} - -TEST_F(SendTimeHistoryTest, AddThenRemoveOutOfOrder) { - std::vector sent_packets; - std::vector received_packets; - const size_t num_items = 100; - const size_t kPacketSize = 400; - const size_t kTransmissionTime = 1234; - const PacedPacketInfo kPacingInfo(1, 2, 200); - for (size_t i = 0; i < num_items; ++i) { - sent_packets.push_back(PacketFeedback(0, static_cast(i), - static_cast(i), kPacketSize, - kPacingInfo)); - received_packets.push_back(PacketFeedback( - static_cast(i) + kTransmissionTime, 0, - static_cast(i), kPacketSize, PacedPacketInfo())); - } - for (size_t i = 0; i < num_items; ++i) { - PacketFeedback packet = sent_packets[i]; - packet.arrival_time_ms = PacketFeedback::kNotReceived; - packet.send_time_ms = PacketFeedback::kNoSendTime; - history_.RemoveOld(clock_.TimeInMilliseconds()); - history_.AddNewPacket(packet); - } - for (size_t i = 0; i < num_items; ++i) - history_.OnSentPacket(sent_packets[i].sequence_number, - sent_packets[i].send_time_ms); - std::shuffle(received_packets.begin(), received_packets.end(), - std::mt19937(std::random_device()())); - for (size_t i = 0; i < num_items; ++i) { - PacketFeedback packet = received_packets[i]; - EXPECT_TRUE(history_.GetFeedback(&packet, false)); - PacketFeedback sent_packet = sent_packets[packet.sequence_number]; - sent_packet.arrival_time_ms = packet.arrival_time_ms; - EXPECT_EQ(sent_packet, packet); - EXPECT_TRUE(history_.GetFeedback(&packet, true)); - } - for (PacketFeedback packet : sent_packets) - EXPECT_FALSE(history_.GetFeedback(&packet, false)); -} - -TEST_F(SendTimeHistoryTest, HistorySize) { - const int kItems = kDefaultHistoryLengthMs / 100; - for (int i = 0; i < kItems; ++i) { - clock_.AdvanceTimeMilliseconds(100); - AddPacketWithSendTime(i, 0, i * 100, PacedPacketInfo()); - } - for (int i = 0; i < kItems; ++i) { - PacketFeedback packet(0, 0, static_cast(i), 0, PacedPacketInfo()); - EXPECT_TRUE(history_.GetFeedback(&packet, false)); - EXPECT_EQ(i * 100, packet.send_time_ms); - } - clock_.AdvanceTimeMilliseconds(101); - AddPacketWithSendTime(kItems, 0, kItems * 101, PacedPacketInfo()); - PacketFeedback packet(0, 0, 0, 0, PacedPacketInfo()); - EXPECT_FALSE(history_.GetFeedback(&packet, false)); - for (int i = 1; i < (kItems + 1); ++i) { - PacketFeedback packet2(0, 0, static_cast(i), 0, - PacedPacketInfo()); - EXPECT_TRUE(history_.GetFeedback(&packet2, false)); - int64_t expected_time_ms = (i == kItems) ? i * 101 : i * 100; - EXPECT_EQ(expected_time_ms, packet2.send_time_ms); - } -} - -TEST_F(SendTimeHistoryTest, HistorySizeWithWraparound) { - const uint16_t kMaxSeqNo = std::numeric_limits::max(); - AddPacketWithSendTime(kMaxSeqNo - 2, 0, 0, PacedPacketInfo()); - - clock_.AdvanceTimeMilliseconds(100); - AddPacketWithSendTime(kMaxSeqNo - 1, 1, 100, PacedPacketInfo()); - - clock_.AdvanceTimeMilliseconds(100); - AddPacketWithSendTime(kMaxSeqNo, 0, 200, PacedPacketInfo()); - - clock_.AdvanceTimeMilliseconds(kDefaultHistoryLengthMs - 200 + 1); - AddPacketWithSendTime(0, 0, kDefaultHistoryLengthMs, PacedPacketInfo()); - - PacketFeedback packet(0, static_cast(kMaxSeqNo - 2)); - EXPECT_FALSE(history_.GetFeedback(&packet, false)); - PacketFeedback packet2(0, static_cast(kMaxSeqNo - 1)); - EXPECT_TRUE(history_.GetFeedback(&packet2, false)); - PacketFeedback packet3(0, static_cast(kMaxSeqNo)); - EXPECT_TRUE(history_.GetFeedback(&packet3, false)); - PacketFeedback packet4(0, 0); - EXPECT_TRUE(history_.GetFeedback(&packet4, false)); - - // Create a gap (kMaxSeqNo - 1) -> 0. - PacketFeedback packet5(0, kMaxSeqNo); - EXPECT_TRUE(history_.GetFeedback(&packet5, true)); - - clock_.AdvanceTimeMilliseconds(100); - AddPacketWithSendTime(1, 0, 1100, PacedPacketInfo()); - - PacketFeedback packet6(0, static_cast(kMaxSeqNo - 2)); - EXPECT_FALSE(history_.GetFeedback(&packet6, false)); - PacketFeedback packet7(0, static_cast(kMaxSeqNo - 1)); - EXPECT_FALSE(history_.GetFeedback(&packet7, false)); - PacketFeedback packet8(0, kMaxSeqNo); - EXPECT_FALSE(history_.GetFeedback(&packet8, false)); - PacketFeedback packet9(0, 0); - EXPECT_TRUE(history_.GetFeedback(&packet9, false)); - PacketFeedback packet10(0, 1); - EXPECT_TRUE(history_.GetFeedback(&packet10, false)); -} - -TEST_F(SendTimeHistoryTest, InterlievedGetAndRemove) { - const uint16_t kSeqNo = 1; - const int64_t kTimestamp = 2; - const PacedPacketInfo kPacingInfo1(1, 1, 100); - const PacedPacketInfo kPacingInfo2(2, 2, 200); - const PacedPacketInfo kPacingInfo3(3, 3, 300); - PacketFeedback packets[3] = { - {0, kTimestamp, kSeqNo, 0, kPacingInfo1}, - {0, kTimestamp + 1, kSeqNo + 1, 0, kPacingInfo2}, - {0, kTimestamp + 2, kSeqNo + 2, 0, kPacingInfo3}}; - - AddPacketWithSendTime(packets[0].sequence_number, packets[0].payload_size, - packets[0].send_time_ms, packets[0].pacing_info); - AddPacketWithSendTime(packets[1].sequence_number, packets[1].payload_size, - packets[1].send_time_ms, packets[1].pacing_info); - PacketFeedback packet(0, 0, packets[0].sequence_number, 0, PacedPacketInfo()); - EXPECT_TRUE(history_.GetFeedback(&packet, true)); - EXPECT_EQ(packets[0], packet); - - AddPacketWithSendTime(packets[2].sequence_number, packets[2].payload_size, - packets[2].send_time_ms, packets[2].pacing_info); - - PacketFeedback packet2(0, 0, packets[1].sequence_number, 0, kPacingInfo1); - EXPECT_TRUE(history_.GetFeedback(&packet2, true)); - EXPECT_EQ(packets[1], packet2); - - PacketFeedback packet3(0, 0, packets[2].sequence_number, 0, kPacingInfo2); - EXPECT_TRUE(history_.GetFeedback(&packet3, true)); - EXPECT_EQ(packets[2], packet3); -} -} // namespace test -} // namespace webrtc diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index e77e0e7c2f..1b667aef14 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -47,9 +47,7 @@ const int64_t kNoTimestamp = -1; const int64_t kSendTimeHistoryWindowMs = 60000; TransportFeedbackAdapter::TransportFeedbackAdapter() - : allow_duplicates_(field_trial::IsEnabled( - "WebRTC-TransportFeedbackAdapter-AllowDuplicates")), - send_time_history_(kSendTimeHistoryWindowMs), + : packet_age_limit_ms_(kSendTimeHistoryWindowMs), current_offset_ms_(kNoTimestamp), last_timestamp_us_(kNoTimestamp), local_net_id_(0), @@ -82,16 +80,25 @@ void TransportFeedbackAdapter::AddPacket(const RtpPacketSendInfo& packet_info, Timestamp creation_time) { { rtc::CritScope cs(&lock_); - PacketFeedback packet_feedback( - creation_time.ms(), packet_info.transport_sequence_number, - packet_info.length + overhead_bytes, local_net_id_, remote_net_id_, - packet_info.pacing_info); + PacketFeedback packet(creation_time.ms(), + packet_info.transport_sequence_number, + packet_info.length + overhead_bytes, local_net_id_, + remote_net_id_, packet_info.pacing_info); if (packet_info.has_rtp_sequence_number) { - packet_feedback.ssrc = packet_info.ssrc; - packet_feedback.rtp_sequence_number = packet_info.rtp_sequence_number; + packet.ssrc = packet_info.ssrc; + packet.rtp_sequence_number = packet_info.rtp_sequence_number; } - send_time_history_.RemoveOld(creation_time.ms()); - send_time_history_.AddNewPacket(std::move(packet_feedback)); + packet.long_sequence_number = + seq_num_unwrapper_.Unwrap(packet.sequence_number); + + while (!history_.empty() && + creation_time.ms() - history_.begin()->second.creation_time_ms > + packet_age_limit_ms_) { + // TODO(sprang): Warn if erasing (too many) old items? + RemoveInFlightPacketBytes(history_.begin()->second); + history_.erase(history_.begin()); + } + history_.insert(std::make_pair(packet.long_sequence_number, packet)); } { @@ -107,27 +114,43 @@ absl::optional TransportFeedbackAdapter::ProcessSentPacket( rtc::CritScope cs(&lock_); // TODO(srte): Only use one way to indicate that packet feedback is used. if (sent_packet.info.included_in_feedback || sent_packet.packet_id != -1) { - SendTimeHistory::Status send_status = send_time_history_.OnSentPacket( - sent_packet.packet_id, sent_packet.send_time_ms); - absl::optional packet; - if (allow_duplicates_ || - send_status != SendTimeHistory::Status::kDuplicate) { - packet = send_time_history_.GetPacket(sent_packet.packet_id); - } - - if (packet) { - SentPacket msg; - msg.size = DataSize::bytes(packet->payload_size); - msg.send_time = Timestamp::ms(packet->send_time_ms); - msg.sequence_number = packet->long_sequence_number; - msg.prior_unacked_data = DataSize::bytes(packet->unacknowledged_data); - msg.data_in_flight = - send_time_history_.GetOutstandingData(local_net_id_, remote_net_id_); - return msg; + int64_t unwrapped_seq_num = + seq_num_unwrapper_.Unwrap(sent_packet.packet_id); + auto it = history_.find(unwrapped_seq_num); + if (it != history_.end()) { + bool packet_retransmit = it->second.send_time_ms >= 0; + it->second.send_time_ms = sent_packet.send_time_ms; + last_send_time_ms_ = + std::max(last_send_time_ms_, sent_packet.send_time_ms); + // TODO(srte): Don't do this on retransmit. + if (pending_untracked_size_ > 0) { + if (sent_packet.send_time_ms < last_untracked_send_time_ms_) + RTC_LOG(LS_WARNING) + << "appending acknowledged data for out of order packet. (Diff: " + << last_untracked_send_time_ms_ - sent_packet.send_time_ms + << " ms.)"; + it->second.unacknowledged_data += pending_untracked_size_; + pending_untracked_size_ = 0; + } + if (!packet_retransmit) { + AddInFlightPacketBytes(it->second); + auto packet = it->second; + SentPacket msg; + msg.size = DataSize::bytes(packet.payload_size); + msg.send_time = Timestamp::ms(packet.send_time_ms); + msg.sequence_number = packet.long_sequence_number; + msg.prior_unacked_data = DataSize::bytes(packet.unacknowledged_data); + msg.data_in_flight = GetOutstandingData(); + return msg; + } } } else if (sent_packet.info.included_in_allocation) { - send_time_history_.AddUntracked(sent_packet.info.packet_size_bytes, - sent_packet.send_time_ms); + if (sent_packet.send_time_ms < last_send_time_ms_) { + RTC_LOG(LS_WARNING) << "ignoring untracked data for out of order packet."; + } + pending_untracked_size_ += sent_packet.info.packet_size_bytes; + last_untracked_send_time_ms_ = + std::max(last_untracked_send_time_ms_, sent_packet.send_time_ms); } return absl::nullopt; } @@ -165,10 +188,11 @@ TransportFeedbackAdapter::ProcessTransportFeedback( } { rtc::CritScope cs(&lock_); - absl::optional first_unacked_send_time_ms = - send_time_history_.GetFirstUnackedSendTime(); - if (first_unacked_send_time_ms) - msg.first_unacked_send_time = Timestamp::ms(*first_unacked_send_time_ms); + auto it = history_.find(last_ack_seq_num_); + if (it != history_.end() && + it->second.send_time_ms != PacketFeedback::kNoSendTime) { + msg.first_unacked_send_time = Timestamp::ms(it->second.send_time_ms); + } } msg.feedback_time = feedback_receive_time; msg.prior_in_flight = prior_in_flight; @@ -185,7 +209,12 @@ void TransportFeedbackAdapter::SetNetworkIds(uint16_t local_id, DataSize TransportFeedbackAdapter::GetOutstandingData() const { rtc::CritScope cs(&lock_); - return send_time_history_.GetOutstandingData(local_net_id_, remote_net_id_); + auto it = in_flight_bytes_.find({local_net_id_, remote_net_id_}); + if (it != in_flight_bytes_.end()) { + return DataSize::bytes(it->second); + } else { + return DataSize::Zero(); + } } std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( @@ -220,7 +249,7 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( PacketFeedback packet_feedback(PacketFeedback::kNotReceived, seq_num); // Note: Element not removed from history because it might be reported // as received by another feedback. - if (!send_time_history_.GetFeedback(&packet_feedback, false)) + if (!GetFeedback(&packet_feedback, false)) ++failed_lookups; if (packet_feedback.local_net_id == local_net_id_ && packet_feedback.remote_net_id == remote_net_id_) { @@ -232,7 +261,7 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( offset_us += packet.delta_us(); timestamp_ms = current_offset_ms_ + (offset_us / 1000); PacketFeedback packet_feedback(timestamp_ms, packet.sequence_number()); - if (!send_time_history_.GetFeedback(&packet_feedback, true)) + if (!GetFeedback(&packet_feedback, true)) ++failed_lookups; if (packet_feedback.local_net_id == local_net_id_ && packet_feedback.remote_net_id == remote_net_id_) { @@ -256,4 +285,62 @@ TransportFeedbackAdapter::GetTransportFeedbackVector() const { return last_packet_feedback_vector_; } +bool TransportFeedbackAdapter::GetFeedback(PacketFeedback* packet_feedback, + bool remove) { + RTC_DCHECK(packet_feedback); + int64_t acked_seq_num = + seq_num_unwrapper_.Unwrap(packet_feedback->sequence_number); + + if (acked_seq_num > last_ack_seq_num_) { + // Returns history_.begin() if last_ack_seq_num_ < 0, since any valid + // sequence number is >= 0. + auto unacked_it = history_.lower_bound(last_ack_seq_num_); + auto newly_acked_end = history_.upper_bound(acked_seq_num); + for (; unacked_it != newly_acked_end; ++unacked_it) { + RemoveInFlightPacketBytes(unacked_it->second); + } + last_ack_seq_num_ = acked_seq_num; + } + + auto it = history_.find(acked_seq_num); + if (it == history_.end()) + return false; + + // Save arrival_time not to overwrite it. + int64_t arrival_time_ms = packet_feedback->arrival_time_ms; + *packet_feedback = it->second; + packet_feedback->arrival_time_ms = arrival_time_ms; + + if (remove) + history_.erase(it); + return true; +} + +void TransportFeedbackAdapter::AddInFlightPacketBytes( + const PacketFeedback& packet) { + RTC_DCHECK_NE(packet.send_time_ms, -1); + if (last_ack_seq_num_ >= packet.long_sequence_number) + return; + auto it = in_flight_bytes_.find({packet.local_net_id, packet.remote_net_id}); + if (it != in_flight_bytes_.end()) { + it->second += packet.payload_size; + } else { + in_flight_bytes_[{packet.local_net_id, packet.remote_net_id}] = + packet.payload_size; + } +} + +void TransportFeedbackAdapter::RemoveInFlightPacketBytes( + const PacketFeedback& packet) { + if (packet.send_time_ms < 0 || + last_ack_seq_num_ >= packet.long_sequence_number) + return; + auto it = in_flight_bytes_.find({packet.local_net_id, packet.remote_net_id}); + if (it != in_flight_bytes_.end()) { + it->second -= packet.payload_size; + if (it->second == 0) + in_flight_bytes_.erase(it); + } +} + } // namespace webrtc diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.h b/modules/congestion_controller/rtp/transport_feedback_adapter.h index d347f2dfae..edd3fb86c3 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.h +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.h @@ -12,10 +12,13 @@ #define MODULES_CONGESTION_CONTROLLER_RTP_TRANSPORT_FEEDBACK_ADAPTER_H_ #include +#include +#include #include #include "api/transport/network_types.h" -#include "modules/congestion_controller/rtp/send_time_history.h" +#include "modules/include/module_common_types_public.h" +#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "rtc_base/critical_section.h" #include "rtc_base/network/sent_packet.h" #include "rtc_base/thread_annotations.h" @@ -55,16 +58,40 @@ class TransportFeedbackAdapter { DataSize GetOutstandingData() const; private: + using RemoteAndLocalNetworkId = std::pair; + + enum class SendTimeHistoryStatus { kNotAdded, kOk, kDuplicate }; + void OnTransportFeedback(const rtcp::TransportFeedback& feedback); std::vector GetPacketFeedbackVector( const rtcp::TransportFeedback& feedback, Timestamp feedback_time); - const bool allow_duplicates_; + // Look up PacketFeedback for a sent packet, based on the sequence number, and + // populate all fields except for arrival_time. The packet parameter must + // thus be non-null and have the sequence_number field set. + bool GetFeedback(PacketFeedback* packet_feedback, bool remove) + RTC_RUN_ON(&lock_); + void AddInFlightPacketBytes(const PacketFeedback& packet) RTC_RUN_ON(&lock_); + void RemoveInFlightPacketBytes(const PacketFeedback& packet) + RTC_RUN_ON(&lock_); rtc::CriticalSection lock_; - SendTimeHistory send_time_history_ RTC_GUARDED_BY(&lock_); + + const int64_t packet_age_limit_ms_; + size_t pending_untracked_size_ RTC_GUARDED_BY(&lock_) = 0; + int64_t last_send_time_ms_ RTC_GUARDED_BY(&lock_) = -1; + int64_t last_untracked_send_time_ms_ RTC_GUARDED_BY(&lock_) = -1; + SequenceNumberUnwrapper seq_num_unwrapper_ RTC_GUARDED_BY(&lock_); + std::map history_ RTC_GUARDED_BY(&lock_); + + // Sequence numbers are never negative, using -1 as it always < a real + // sequence number. + int64_t last_ack_seq_num_ RTC_GUARDED_BY(&lock_) = -1; + std::map in_flight_bytes_ + RTC_GUARDED_BY(&lock_); + int64_t current_offset_ms_; int64_t last_timestamp_us_; std::vector last_packet_feedback_vector_; diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc index 3fdc21fa8f..593c9940a8 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter_unittest.cc @@ -425,36 +425,6 @@ TEST_F(TransportFeedbackAdapterTest, IgnoreDuplicatePacketSentCalls) { EXPECT_FALSE(duplicate_packet.has_value()); } -TEST_F(TransportFeedbackAdapterTest, AllowDuplicatePacketSentCallsWithTrial) { - // Allow duplicates if this field trial kill-switch is enabled. - webrtc::test::ScopedFieldTrials field_trial( - "WebRTC-TransportFeedbackAdapter-AllowDuplicates/Enabled/"); - // Re-run setup so the flags goes into effect. - SetUp(); - - const PacketFeedback packet(100, 200, 0, 1500, kPacingInfo0); - - // Add a packet and then mark it as sent. - RtpPacketSendInfo packet_info; - packet_info.ssrc = kSsrc; - packet_info.transport_sequence_number = packet.sequence_number; - packet_info.length = packet.payload_size; - packet_info.pacing_info = packet.pacing_info; - adapter_->AddPacket(packet_info, 0u, - Timestamp::ms(clock_.TimeInMilliseconds())); - absl::optional sent_packet = - adapter_->ProcessSentPacket(rtc::SentPacket( - packet.sequence_number, packet.send_time_ms, rtc::PacketInfo())); - EXPECT_TRUE(sent_packet.has_value()); - - // Call ProcessSentPacket() again with the same sequence number. This packet - // should still be allowed due to the field trial/ - absl::optional duplicate_packet = - adapter_->ProcessSentPacket(rtc::SentPacket( - packet.sequence_number, packet.send_time_ms, rtc::PacketInfo())); - EXPECT_TRUE(duplicate_packet.has_value()); -} - } // namespace test } // namespace webrtc_cc } // namespace webrtc