From d912446f529ae8ac780288a3f44760a83ea44863 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Wed, 11 Aug 2021 23:02:23 +0200 Subject: [PATCH] dcsctp: Refactor chunk acking The same code was done for both acking chunks due to moving the cum-ack-tsn and when acking gap-ack-blocks. Unify them completely to have a single code path. Bug: webrtc:12943 Change-Id: I3b864e41cc2ec674460517660c23b72a70edf720 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228521 Reviewed-by: Florent Castelli Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/master@{#34773} --- net/dcsctp/tx/retransmission_queue.cc | 54 +++++++++++++-------------- net/dcsctp/tx/retransmission_queue.h | 12 +++--- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/net/dcsctp/tx/retransmission_queue.cc b/net/dcsctp/tx/retransmission_queue.cc index c1334ecf0f..c3d567d42e 100644 --- a/net/dcsctp/tx/retransmission_queue.cc +++ b/net/dcsctp/tx/retransmission_queue.cc @@ -113,15 +113,8 @@ void RetransmissionQueue::RemoveAcked(UnwrappedTSN cumulative_tsn_ack, AckInfo& ack_info) { auto first_unacked = outstanding_data_.upper_bound(cumulative_tsn_ack); - for (auto it = outstanding_data_.begin(); it != first_unacked; ++it) { - ack_info.bytes_acked_by_cumulative_tsn_ack += it->second.data().size(); - ack_info.acked_tsns.push_back(it->first.Wrap()); - if (it->second.is_outstanding()) { - outstanding_bytes_ -= GetSerializedChunkSize(it->second.data()); - --outstanding_items_; - } else if (it->second.should_be_retransmitted()) { - to_be_retransmitted_.erase(it->first); - } + for (auto iter = outstanding_data_.begin(); iter != first_unacked; ++iter) { + AckChunk(ack_info, iter); } outstanding_data_.erase(outstanding_data_.begin(), first_unacked); @@ -142,25 +135,30 @@ void RetransmissionQueue::AckGapBlocks( auto end = outstanding_data_.upper_bound( UnwrappedTSN::AddTo(cumulative_tsn_ack, block.end)); for (auto iter = start; iter != end; ++iter) { - if (!iter->second.is_acked()) { - ack_info.bytes_acked_by_new_gap_ack_blocks += - iter->second.data().size(); - if (iter->second.is_outstanding()) { - outstanding_bytes_ -= GetSerializedChunkSize(iter->second.data()); - --outstanding_items_; - } - if (iter->second.should_be_retransmitted()) { - to_be_retransmitted_.erase(iter->first); - } - iter->second.Ack(); - ack_info.highest_tsn_acked = - std::max(ack_info.highest_tsn_acked, iter->first); - ack_info.acked_tsns.push_back(iter->first.Wrap()); - } + AckChunk(ack_info, iter); } } } +void RetransmissionQueue::AckChunk( + AckInfo& ack_info, + std::map::iterator iter) { + if (!iter->second.is_acked()) { + ack_info.bytes_acked += iter->second.data().size(); + ack_info.acked_tsns.push_back(iter->first.Wrap()); + if (iter->second.is_outstanding()) { + outstanding_bytes_ -= GetSerializedChunkSize(iter->second.data()); + --outstanding_items_; + } + if (iter->second.should_be_retransmitted()) { + to_be_retransmitted_.erase(iter->first); + } + iter->second.Ack(); + ack_info.highest_tsn_acked = + std::max(ack_info.highest_tsn_acked, iter->first); + } +} + void RetransmissionQueue::NackBetweenAckBlocks( UnwrappedTSN cumulative_tsn_ack, rtc::ArrayView gap_ack_blocks, @@ -421,9 +419,8 @@ bool RetransmissionQueue::HandleSack(TimeMs now, const SackChunk& sack) { // Note: It may be started again in a bit further down. t3_rtx_.Stop(); - HandleIncreasedCumulativeTsnAck( - old_outstanding_bytes, ack_info.bytes_acked_by_cumulative_tsn_ack + - ack_info.bytes_acked_by_new_gap_ack_blocks); + HandleIncreasedCumulativeTsnAck(old_outstanding_bytes, + ack_info.bytes_acked); } if (ack_info.has_packet_loss) { @@ -434,8 +431,7 @@ bool RetransmissionQueue::HandleSack(TimeMs now, const SackChunk& sack) { // https://tools.ietf.org/html/rfc4960#section-8.2 // "When an outstanding TSN is acknowledged [...] the endpoint shall clear // the error counter ..." - if (ack_info.bytes_acked_by_cumulative_tsn_ack > 0 || - ack_info.bytes_acked_by_new_gap_ack_blocks > 0) { + if (ack_info.bytes_acked > 0) { on_clear_retransmission_counter_(); } diff --git a/net/dcsctp/tx/retransmission_queue.h b/net/dcsctp/tx/retransmission_queue.h index ceb4426606..f81c6042ad 100644 --- a/net/dcsctp/tx/retransmission_queue.h +++ b/net/dcsctp/tx/retransmission_queue.h @@ -234,11 +234,8 @@ class RetransmissionQueue { // All TSNs that have been acked (for the first time) in this SACK. std::vector acked_tsns; - // Bytes acked by increasing cumulative_tsn_ack in this SACK - size_t bytes_acked_by_cumulative_tsn_ack = 0; - - // Bytes acked by gap blocks in this SACK. - size_t bytes_acked_by_new_gap_ack_blocks = 0; + // Bytes acked by increasing cumulative_tsn_ack and gap_ack_blocks. + size_t bytes_acked = 0; // Indicates if this SACK indicates that packet loss has occurred. Just // because a packet is missing in the SACK doesn't necessarily mean that @@ -289,6 +286,11 @@ class RetransmissionQueue { rtc::ArrayView gap_ack_blocks, AckInfo& ack_info); + // Acks the chunk referenced by `iter` and updates state in `ack_info` and the + // object's state. + void AckChunk(AckInfo& ack_info, + std::map::iterator iter); + // Mark chunks reported as "missing", as "nacked" or "to be retransmitted" // depending how many times this has happened. Only packets up until // `ack_info.highest_tsn_acked` (highest TSN newly acknowledged) are