dcsctp: Report duplicate TSNs
Reporting the duplicate TSNs is a SHOULD in the RFC, and using the
duplicate TNSs is a MAY, and in reality I haven't seen an implementation
use it yet. However, it's good for debugging and for stats generation.
Bug: webrtc:12614
Change-Id: I1cc3f86961a8d289708cbf50d98dedfd25077955
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219462
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34053}
diff --git a/net/dcsctp/packet/chunk/sack_chunk.cc b/net/dcsctp/packet/chunk/sack_chunk.cc
index a9f17d7..d80e430 100644
--- a/net/dcsctp/packet/chunk/sack_chunk.cc
+++ b/net/dcsctp/packet/chunk/sack_chunk.cc
@@ -88,13 +88,12 @@
offset += kGapAckBlockSize;
}
- std::vector<TSN> duplicate_tsns;
- duplicate_tsns.reserve(nbr_of_gap_blocks);
+ std::set<TSN> duplicate_tsns;
for (int i = 0; i < nbr_of_dup_tsns; ++i) {
BoundedByteReader<kDupTsnBlockSize> sub_reader =
reader->sub_reader<kDupTsnBlockSize>(offset);
- duplicate_tsns.push_back(TSN(sub_reader.Load32<0>()));
+ duplicate_tsns.insert(TSN(sub_reader.Load32<0>()));
offset += kDupTsnBlockSize;
}
RTC_DCHECK(offset == reader->variable_data_size());
@@ -124,11 +123,11 @@
offset += kGapAckBlockSize;
}
- for (int i = 0; i < nbr_of_dup_tsns; ++i) {
+ for (TSN tsn : duplicate_tsns_) {
BoundedByteWriter<kDupTsnBlockSize> sub_writer =
writer.sub_writer<kDupTsnBlockSize>(offset);
- sub_writer.Store32<0>(*duplicate_tsns_[i]);
+ sub_writer.Store32<0>(*tsn);
offset += kDupTsnBlockSize;
}
diff --git a/net/dcsctp/packet/chunk/sack_chunk.h b/net/dcsctp/packet/chunk/sack_chunk.h
index 0b464fb..e6758fa 100644
--- a/net/dcsctp/packet/chunk/sack_chunk.h
+++ b/net/dcsctp/packet/chunk/sack_chunk.h
@@ -12,6 +12,7 @@
#include <stddef.h>
#include <cstdint>
+#include <set>
#include <string>
#include <utility>
#include <vector>
@@ -48,7 +49,7 @@
SackChunk(TSN cumulative_tsn_ack,
uint32_t a_rwnd,
std::vector<GapAckBlock> gap_ack_blocks,
- std::vector<TSN> duplicate_tsns)
+ std::set<TSN> duplicate_tsns)
: cumulative_tsn_ack_(cumulative_tsn_ack),
a_rwnd_(a_rwnd),
gap_ack_blocks_(std::move(gap_ack_blocks)),
@@ -63,7 +64,7 @@
rtc::ArrayView<const GapAckBlock> gap_ack_blocks() const {
return gap_ack_blocks_;
}
- rtc::ArrayView<const TSN> duplicate_tsns() const { return duplicate_tsns_; }
+ const std::set<TSN>& duplicate_tsns() const { return duplicate_tsns_; }
private:
static constexpr size_t kGapAckBlockSize = 4;
@@ -72,7 +73,7 @@
const TSN cumulative_tsn_ack_;
const uint32_t a_rwnd_;
std::vector<GapAckBlock> gap_ack_blocks_;
- std::vector<TSN> duplicate_tsns_;
+ std::set<TSN> duplicate_tsns_;
};
} // namespace dcsctp
diff --git a/net/dcsctp/packet/chunk_validators.cc b/net/dcsctp/packet/chunk_validators.cc
index b346703..48d3518 100644
--- a/net/dcsctp/packet/chunk_validators.cc
+++ b/net/dcsctp/packet/chunk_validators.cc
@@ -38,9 +38,7 @@
// Not more than at most one remaining? Exit early.
if (gap_ack_blocks.size() <= 1) {
return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(),
- std::move(gap_ack_blocks),
- std::vector<TSN>(sack.duplicate_tsns().begin(),
- sack.duplicate_tsns().end()));
+ std::move(gap_ack_blocks), sack.duplicate_tsns());
}
// Sort the intervals by their start value, to aid in the merging below.
@@ -63,8 +61,7 @@
}
return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(), std::move(merged),
- std::vector<TSN>(sack.duplicate_tsns().begin(),
- sack.duplicate_tsns().end()));
+ sack.duplicate_tsns());
}
bool ChunkValidators::Validate(const SackChunk& sack) {
diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc
index 3e03dfe..68a4895 100644
--- a/net/dcsctp/rx/data_tracker.cc
+++ b/net/dcsctp/rx/data_tracker.cc
@@ -52,7 +52,7 @@
// Old chunk already seen before?
if (unwrapped_tsn <= last_cumulative_acked_tsn_) {
- // TODO(boivie) Set duplicate TSN, even if it's not used in SCTP yet.
+ duplicate_tsns_.insert(unwrapped_tsn.Wrap());
return;
}
@@ -66,7 +66,11 @@
additional_tsns_.erase(additional_tsns_.begin());
}
} else {
- additional_tsns_.insert(unwrapped_tsn);
+ bool inserted = additional_tsns_.insert(unwrapped_tsn).second;
+ if (!inserted) {
+ // Already seen before.
+ duplicate_tsns_.insert(unwrapped_tsn.Wrap());
+ }
}
// https://tools.ietf.org/html/rfc4960#section-6.7
@@ -178,15 +182,11 @@
// that. So this SACK produced is more like a NR-SACK as explained in
// https://ieeexplore.ieee.org/document/4697037 and which there is an RFC
// draft at https://tools.ietf.org/html/draft-tuexen-tsvwg-sctp-multipath-17.
- std::vector<TSN> duplicate_tsns;
- duplicate_tsns.reserve(duplicates_.size());
- for (UnwrappedTSN tsn : duplicates_) {
- duplicate_tsns.push_back(tsn.Wrap());
- }
- duplicates_.clear();
+ std::set<TSN> duplicate_tsns;
+ duplicate_tsns_.swap(duplicate_tsns);
return SackChunk(last_cumulative_acked_tsn_.Wrap(), a_rwnd,
- CreateGapAckBlocks(), duplicate_tsns);
+ CreateGapAckBlocks(), std::move(duplicate_tsns));
}
std::vector<SackChunk::GapAckBlock> DataTracker::CreateGapAckBlocks() const {
diff --git a/net/dcsctp/rx/data_tracker.h b/net/dcsctp/rx/data_tracker.h
index 6146d2a..f5deaf1 100644
--- a/net/dcsctp/rx/data_tracker.h
+++ b/net/dcsctp/rx/data_tracker.h
@@ -126,7 +126,7 @@
UnwrappedTSN last_cumulative_acked_tsn_;
// Received TSNs that are not directly following `last_cumulative_acked_tsn_`.
std::set<UnwrappedTSN> additional_tsns_;
- std::set<UnwrappedTSN> duplicates_;
+ std::set<TSN> duplicate_tsns_;
};
} // namespace dcsctp
diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc
index d714b0b..7518d6d 100644
--- a/net/dcsctp/rx/data_tracker_test.cc
+++ b/net/dcsctp/rx/data_tracker_test.cc
@@ -25,6 +25,7 @@
namespace {
using ::testing::ElementsAre;
using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
constexpr size_t kArwnd = 10000;
constexpr TSN kInitialTSN(11);
@@ -230,5 +231,43 @@
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 0x8000000)));
}
+TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) {
+ Observer({11, 12, 11});
+ SackChunk sack = buf_.CreateSelectiveAck(kArwnd);
+ EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(12));
+ EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty());
+ EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(11)));
+}
+
+TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) {
+ Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16});
+ SackChunk sack = buf_.CreateSelectiveAck(kArwnd);
+ EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(16));
+ EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty());
+ EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(12), TSN(13)));
+}
+
+TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) {
+ Observer({11, /*12,*/ 13, 14, 13, 14, 15, 16});
+ SackChunk sack = buf_.CreateSelectiveAck(kArwnd);
+ EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(11));
+ EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 5)));
+ EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(13), TSN(14)));
+}
+
+TEST_F(DataTrackerTest, ClearsDuplicateTsnsAfterCreatingSack) {
+ Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16});
+ SackChunk sack1 = buf_.CreateSelectiveAck(kArwnd);
+ EXPECT_EQ(sack1.cumulative_tsn_ack(), TSN(16));
+ EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty());
+ EXPECT_THAT(sack1.duplicate_tsns(), UnorderedElementsAre(TSN(12), TSN(13)));
+
+ Observer({17});
+ SackChunk sack2 = buf_.CreateSelectiveAck(kArwnd);
+ EXPECT_EQ(sack2.cumulative_tsn_ack(), TSN(17));
+ EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty());
+ EXPECT_THAT(sack2.duplicate_tsns(), IsEmpty());
+}
+
} // namespace
} // namespace dcsctp
