zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dcsctp: Fix post-review comments for DataTracker

Browse Source 
These are some fixes that were added after submission of
https://webrtc-review.googlesource.com/c/src/+/213664

Mainly:

 * Don't accept TSNs that have a too large difference from expected
 * Renaming of member variable (to confirm to style guidelines)

Bug: webrtc:12614
Change-Id: I06e11ab2acf5d307b68c3cbc135fde2c038ee690
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215070
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33721}
This commit is contained in:
Victor Boivie
2021-04-13 11:23:16 +02:00
committed by Commit Bot
parent 0498519844
commit c54f6722ce
3 changed files with 76 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@ -26,9 +26,30 @@
namespace dcsctp {
bool DataTracker::IsTSNValid(TSN tsn) const {
UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.PeekUnwrap(tsn);
// Note that this method doesn't return `false` for old DATA chunks, as those
// are actually valid, and receiving those may affect the generated SACK
// response (by setting "duplicate TSNs").
uint32_t difference =
UnwrappedTSN::Difference(unwrapped_tsn, last_cumulative_acked_tsn_);
if (difference > kMaxAcceptedOutstandingFragments) {
return false;
}
return true;
}
void DataTracker::Observe(TSN tsn,
AnyDataChunk::ImmediateAckFlag immediate_ack) {
UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.Unwrap(tsn);
// IsTSNValid must be called prior to calling this method.
RTC_DCHECK(
UnwrappedTSN::Difference(unwrapped_tsn, last_cumulative_acked_tsn_) <=
kMaxAcceptedOutstandingFragments);
// 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.
@ -38,14 +59,14 @@ void DataTracker::Observe(TSN tsn,
if (unwrapped_tsn == last_cumulative_acked_tsn_.next_value()) {
last_cumulative_acked_tsn_ = unwrapped_tsn;
// The cumulative acked tsn may be moved even further, if a gap was filled.
while (!additional_tsns.empty() &&
*additional_tsns.begin() ==
while (!additional_tsns_.empty() &&
*additional_tsns_.begin() ==
last_cumulative_acked_tsn_.next_value()) {
last_cumulative_acked_tsn_.Increment();
additional_tsns.erase(additional_tsns.begin());
additional_tsns_.erase(additional_tsns_.begin());
}
} else {
additional_tsns.insert(unwrapped_tsn);
additional_tsns_.insert(unwrapped_tsn);
}
// https://tools.ietf.org/html/rfc4960#section-6.7
@ -54,7 +75,7 @@ void DataTracker::Observe(TSN tsn,
// the received DATA chunk sequence, it SHOULD send a SACK with Gap Ack
// Blocks immediately. The data receiver continues sending a SACK after
// receipt of each SCTP packet that doesn't fill the gap."
if (!additional_tsns.empty()) {
if (!additional_tsns_.empty()) {
UpdateAckState(AckState::kImmediate, "packet loss");
}
@ -119,15 +140,15 @@ void DataTracker::HandleForwardTsn(TSN new_cumulative_ack) {
// now overlapping with the new value, remove them.
last_cumulative_acked_tsn_ = unwrapped_tsn;
int erased_additional_tsns = std::distance(
additional_tsns.begin(), additional_tsns.upper_bound(unwrapped_tsn));
additional_tsns.erase(additional_tsns.begin(),
additional_tsns.upper_bound(unwrapped_tsn));
additional_tsns_.begin(), additional_tsns_.upper_bound(unwrapped_tsn));
additional_tsns_.erase(additional_tsns_.begin(),
additional_tsns_.upper_bound(unwrapped_tsn));
// See if the `last_cumulative_acked_tsn_` can be moved even further:
while (!additional_tsns.empty() &&
*additional_tsns.begin() == last_cumulative_acked_tsn_.next_value()) {
while (!additional_tsns_.empty() &&
*additional_tsns_.begin() == last_cumulative_acked_tsn_.next_value()) {
last_cumulative_acked_tsn_.Increment();
additional_tsns.erase(additional_tsns.begin());
additional_tsns_.erase(additional_tsns_.begin());
++erased_additional_tsns;
}
@ -179,17 +200,17 @@ std::vector<SackChunk::GapAckBlock> DataTracker::CreateGapAckBlocks() const {
auto flush = [&]() {
if (first_tsn_in_block.has_value()) {
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_);
auto start_diff = UnwrappedTSN::Difference(*first_tsn_in_block,
last_cumulative_acked_tsn_);
auto 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;
last_tsn_in_block = absl::nullopt;
}
};
for (UnwrappedTSN tsn : additional_tsns) {
for (UnwrappedTSN tsn : additional_tsns_) {
if (last_tsn_in_block.has_value() &&
last_tsn_in_block->next_value() == tsn) {
// Continuing the same block.

14
net/dcsctp/rx/data_tracker.h
View File

@ -38,6 +38,13 @@ namespace dcsctp {
// 200ms, whatever is smallest).
class DataTracker {
public:
// The maximum number of accepted in-flight DATA chunks. This indicates the
// maximum difference from this buffer's last cumulative ack TSN, and any
// received data. Data received beyond this limit will be dropped, which will
// force the transmitter to send data that actually increases the last
// cumulative acked TSN.
static constexpr uint32_t kMaxAcceptedOutstandingFragments = 256;
explicit DataTracker(absl::string_view log_prefix,
Timer* delayed_ack_timer,
TSN peer_initial_tsn)
@ -46,6 +53,11 @@ class DataTracker {
last_cumulative_acked_tsn_(
tsn_unwrapper_.Unwrap(TSN(*peer_initial_tsn - 1))) {}
// Indicates if the provided TSN is valid. If this return false, the data
// should be dropped and not added to any other buffers, which essentially
// means that there is intentional packet loss.
bool IsTSNValid(TSN tsn) const;
// Call for every incoming data chunk.
void Observe(TSN tsn,
AnyDataChunk::ImmediateAckFlag immediate_ack =
@ -113,7 +125,7 @@ class DataTracker {
// All TSNs up until (and including) this value have been seen.
UnwrappedTSN last_cumulative_acked_tsn_;
// Received TSNs that are not directly following `last_cumulative_acked_tsn_`.
std::set<UnwrappedTSN> additional_tsns;
std::set<UnwrappedTSN> additional_tsns_;
std::set<UnwrappedTSN> duplicates_;
};
} // namespace dcsctp

27
net/dcsctp/rx/data_tracker_test.cc
View File

@ -27,6 +27,7 @@ using ::testing::ElementsAre;
using ::testing::IsEmpty;
constexpr size_t kArwnd = 10000;
constexpr TSN kInitialTSN(11);
class DataTrackerTest : public testing::Test {
protected:
@ -37,7 +38,7 @@ class DataTrackerTest : public testing::Test {
"test/delayed_ack",
[]() { return absl::nullopt; },
TimerOptions(DurationMs(0)))),
buf_("log: ", timer_.get(), /*peer_initial_tsn=*/TSN(11)) {}
buf_("log: ", timer_.get(), kInitialTSN) {}
void Observer(std::initializer_list<uint32_t> tsns) {
for (const uint32_t tsn : tsns) {
@ -205,5 +206,29 @@ TEST_F(DataTrackerTest, ForceShouldSendSackImmediately) {
EXPECT_TRUE(buf_.ShouldSendAck());
}
TEST_F(DataTrackerTest, WillAcceptValidTSNs) {
// The initial TSN is always one more than the last, which is our base.
TSN last_tsn = TSN(*kInitialTSN - 1);
int limit = static_cast<int>(DataTracker::kMaxAcceptedOutstandingFragments);
for (int i = -limit; i <= limit; ++i) {
EXPECT_TRUE(buf_.IsTSNValid(TSN(*last_tsn + i)));
}
}
TEST_F(DataTrackerTest, WillNotAcceptInvalidTSNs) {
// The initial TSN is always one more than the last, which is our base.
TSN last_tsn = TSN(*kInitialTSN - 1);
size_t limit = DataTracker::kMaxAcceptedOutstandingFragments;
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn + limit + 1)));
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - (limit + 1))));
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn + 65536)));
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 65536)));
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn + 0x8000000)));
EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 0x8000000)));
}
} // namespace
} // namespace dcsctp