admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dcsctp: Make Sequence Number API more consistent

Browse Source 
* `AddTo` and `Difference` are made into static methods, as one may have
  believed that these modified the current object previously. The
  `Increment` method is kept, as it's obvious that it modifies the
  current object as it doesn't have a return value, and `next_value` is
  kept, as its naming (lower-case, snake) indicates that it's a simple
  accessor.
* Difference will return the absolute difference. This is actually the
  only reasonable choice, as the return value was unsigned and any
  negative value would just wrap.

Bug: webrtc:12614
Change-Id: If14a71636e67fc612d12759dc80a9c2518c85281
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215069
Reviewed-by: Tommi <tommi@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33714}
This commit is contained in:
Victor Boivie 2021-04-13 11:11:04 +02:00 committed by Commit Bot
parent ce423ce12d
commit 250fbb3c48
4 changed files with 47 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
net/dcsctp/common/sequence_numbers.h
View File

@ -121,19 +121,26 @@ class UnwrappedSequenceNumber {
// Increments the value.
void Increment() { ++value_; }
// Returns the next value relative to this sequence number.
UnwrappedSequenceNumber<WrappedType> next_value() const {
return UnwrappedSequenceNumber<WrappedType>(value_ + 1);
}
// Adds a delta to the current value.
UnwrappedSequenceNumber<WrappedType> AddTo(int delta) const {
return UnwrappedSequenceNumber<WrappedType>(value_ + delta);
// Returns a new sequence number based on `value`, and adding `delta` (which
// may be negative).
static UnwrappedSequenceNumber<WrappedType> AddTo(
UnwrappedSequenceNumber<WrappedType> value,
int delta) {
return UnwrappedSequenceNumber<WrappedType>(value.value_ + delta);
}
// Compares the difference between two sequence numbers.
typename WrappedType::UnderlyingType Difference(
UnwrappedSequenceNumber<WrappedType> other) const {
return value_ - other.value_;
// Returns the absolute difference between `lhs` and `rhs`.
static typename WrappedType::UnderlyingType Difference(
UnwrappedSequenceNumber<WrappedType> lhs,
UnwrappedSequenceNumber<WrappedType> rhs) {
return (lhs.value_ > rhs.value_) ? (lhs.value_ - rhs.value_)
: (rhs.value_ - lhs.value_);
}
private:

39
net/dcsctp/common/sequence_numbers_test.cc
View File

@ -32,9 +32,9 @@ TEST(SequenceNumbersTest, SimpleUnwrapping) {
EXPECT_LT(s1, s3);
EXPECT_LT(s2, s3);
EXPECT_EQ(s1.Difference(s0), 1);
EXPECT_EQ(s2.Difference(s0), 2);
EXPECT_EQ(s3.Difference(s0), 3);
EXPECT_EQ(TestSequence::Difference(s1, s0), 1);
EXPECT_EQ(TestSequence::Difference(s2, s0), 2);
EXPECT_EQ(TestSequence::Difference(s3, s0), 3);
EXPECT_GT(s1, s0);
EXPECT_GT(s2, s0);
@ -50,7 +50,7 @@ TEST(SequenceNumbersTest, SimpleUnwrapping) {
s2.Increment();
EXPECT_EQ(s2, s3);
EXPECT_EQ(s0.AddTo(2), s3);
EXPECT_EQ(TestSequence::AddTo(s0, 2), s3);
}
TEST(SequenceNumbersTest, MidValueUnwrapping) {
@ -68,9 +68,9 @@ TEST(SequenceNumbersTest, MidValueUnwrapping) {
EXPECT_LT(s1, s3);
EXPECT_LT(s2, s3);
EXPECT_EQ(s1.Difference(s0), 1);
EXPECT_EQ(s2.Difference(s0), 2);
EXPECT_EQ(s3.Difference(s0), 3);
EXPECT_EQ(TestSequence::Difference(s1, s0), 1);
EXPECT_EQ(TestSequence::Difference(s2, s0), 2);
EXPECT_EQ(TestSequence::Difference(s3, s0), 3);
EXPECT_GT(s1, s0);
EXPECT_GT(s2, s0);
@ -86,7 +86,7 @@ TEST(SequenceNumbersTest, MidValueUnwrapping) {
s2.Increment();
EXPECT_EQ(s2, s3);
EXPECT_EQ(s0.AddTo(2), s3);
EXPECT_EQ(TestSequence::AddTo(s0, 2), s3);
}
TEST(SequenceNumbersTest, WrappedUnwrapping) {
@ -104,9 +104,9 @@ TEST(SequenceNumbersTest, WrappedUnwrapping) {
EXPECT_LT(s1, s3);
EXPECT_LT(s2, s3);
EXPECT_EQ(s1.Difference(s0), 1);
EXPECT_EQ(s2.Difference(s0), 2);
EXPECT_EQ(s3.Difference(s0), 3);
EXPECT_EQ(TestSequence::Difference(s1, s0), 1);
EXPECT_EQ(TestSequence::Difference(s2, s0), 2);
EXPECT_EQ(TestSequence::Difference(s3, s0), 3);
EXPECT_GT(s1, s0);
EXPECT_GT(s2, s0);
@ -122,7 +122,7 @@ TEST(SequenceNumbersTest, WrappedUnwrapping) {
s2.Increment();
EXPECT_EQ(s2, s3);
EXPECT_EQ(s0.AddTo(2), s3);
EXPECT_EQ(TestSequence::AddTo(s0, 2), s3);
}
TEST(SequenceNumbersTest, WrapAroundAFewTimes) {
@ -183,5 +183,20 @@ TEST(SequenceNumbersTest, UnwrappingSmallerNumberIsAlwaysSmaller) {
}
}
TEST(SequenceNumbersTest, DifferenceIsAbsolute) {
TestSequence::Unwrapper unwrapper;
TestSequence this_value = unwrapper.Unwrap(Wrapped(10));
TestSequence other_value = TestSequence::AddTo(this_value, 100);
EXPECT_EQ(TestSequence::Difference(this_value, other_value), 100);
EXPECT_EQ(TestSequence::Difference(other_value, this_value), 100);
TestSequence minus_value = TestSequence::AddTo(this_value, -100);
EXPECT_EQ(TestSequence::Difference(this_value, minus_value), 100);
EXPECT_EQ(TestSequence::Difference(minus_value, this_value), 100);
}
} // namespace
} // namespace dcsctp

7
net/dcsctp/rx/data_tracker.cc
View File

@ -179,9 +179,10 @@ std::vector<SackChunk::GapAckBlock> DataTracker::CreateGapAckBlocks() const {
auto flush = [&]() {
if (first_tsn_in_block.has_value()) {
int start_diff =
first_tsn_in_block->Difference(last_cumulative_acked_tsn_);
int end_diff = last_tsn_in_block->Difference(last_cumulative_acked_tsn_);
int start_diff = UnwrappedTSN::Difference(*first_tsn_in_block,
last_cumulative_acked_tsn_);
int end_diff = UnwrappedTSN::Difference(*last_tsn_in_block,
last_cumulative_acked_tsn_);
gap_ack_blocks.emplace_back(static_cast<uint16_t>(start_diff),
static_cast<uint16_t>(end_diff));
first_tsn_in_block = absl::nullopt;

3
net/dcsctp/rx/traditional_reassembly_streams.cc
View File

@ -172,7 +172,8 @@ size_t TraditionalReassemblyStreams::OrderedStream::TryToAssembleMessage() {
return 0;
}
uint32_t tsn_diff = chunks.rbegin()->first.Difference(chunks.begin()->first);
uint32_t tsn_diff =
UnwrappedTSN::Difference(chunks.rbegin()->first, chunks.begin()->first);
if (tsn_diff != chunks.size() - 1) {
return 0;
}