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

dcsctp: Remove limit of message fragmentation

Browse Source 
This was available in the beginning, as a way to increase the chance of
a message sent with partial reliability to be delivered, by avoiding it
to be fragmented in too small fragments.

This however added a few downsides:
 * Packet efficiency goes down, as the entire MTU isn't always used
 * Complexity increases when adding message interleaving, since if one
   stream refuses to produce messages, but there is another stream with
   a very small message that could fit in its place, it should be used
   instead.

Removing this feature altogether is much easier. It's hard to defend.

Bug: webrtc:5696
Change-Id: Ie2f296e052f4a32a281497d379c0d528a2df3308
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/257168
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36396}
This commit is contained in:
Victor Boivie 2022-03-18 16:40:12 +01:00 committed by WebRTC LUCI CQ
parent fbf0ac0ecd
commit 8beccd5c47
3 changed files with 18 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
net/dcsctp/tx/rr_send_queue.cc
View File

@ -136,19 +136,13 @@ void RRSendQueue::OutgoingStream::Add(DcSctpMessage message,
RTC_DCHECK(IsConsistent());
}
absl::optional<SendQueue::DataToSend> RRSendQueue::OutgoingStream::Produce(
TimeMs now,
size_t max_size) {
SendQueue::DataToSend RRSendQueue::OutgoingStream::Produce(TimeMs now,
size_t max_size) {
RTC_DCHECK(!items_.empty());
Item* item = &items_.front();
DcSctpMessage& message = item->message;
if (item->remaining_size > max_size && max_size < kMinimumFragmentedPayload) {
RTC_DCHECK(IsConsistent());
return absl::nullopt;
}
// Allocate Message ID and SSN when the first fragment is sent.
if (!item->message_id.has_value()) {
MID& mid =
@ -354,22 +348,20 @@ absl::optional<SendQueue::DataToSend> RRSendQueue::Produce(TimeMs now,
RTC_DCHECK(stream_it != streams_.end());
}
absl::optional<DataToSend> data = stream_it->second.Produce(now, max_size);
if (data.has_value()) {
RTC_DLOG(LS_VERBOSE) << log_prefix_ << "Producing DATA, type="
<< (data->data.is_unordered ? "unordered" : "ordered")
<< "::"
<< (*data->data.is_beginning && *data->data.is_end
? "complete"
: *data->data.is_beginning
? "first"
: *data->data.is_end ? "last" : "middle")
<< ", stream_id=" << *stream_it->first
<< ", ppid=" << *data->data.ppid
<< ", length=" << data->data.payload.size();
DataToSend data = stream_it->second.Produce(now, max_size);
RTC_DLOG(LS_VERBOSE) << log_prefix_ << "Producing DATA, type="
<< (data.data.is_unordered ? "unordered" : "ordered")
<< "::"
<< (*data.data.is_beginning && *data.data.is_end
? "complete"
: *data.data.is_beginning
? "first"
: *data.data.is_end ? "last" : "middle")
<< ", stream_id=" << *stream_it->first
<< ", ppid=" << *data.data.ppid
<< ", length=" << data.data.payload.size();
previous_message_has_ended_ = *data->data.is_end;
}
previous_message_has_ended_ = *data.data.is_end;
RTC_DCHECK(IsConsistent());
return data;

8
net/dcsctp/tx/rr_send_queue.h
View File

@ -40,9 +40,6 @@ namespace dcsctp {
// established, this send queue is always present - even for closed connections.
class RRSendQueue : public SendQueue {
public:
// How small a data chunk's payload may be, if having to fragment a message.
static constexpr size_t kMinimumFragmentedPayload = 10;
RRSendQueue(absl::string_view log_prefix,
size_t buffer_size,
std::function<void(StreamID)> on_buffered_amount_low,
@ -126,8 +123,9 @@ class RRSendQueue : public SendQueue {
TimeMs expires_at,
const SendOptions& send_options);
// Possibly produces a data chunk to send.
absl::optional<DataToSend> Produce(TimeMs now, size_t max_size);
// Produces a data chunk to send. This is only called on streams that have
// data available.
DataToSend Produce(TimeMs now, size_t max_size);
const ThresholdWatcher& buffered_amount() const { return buffered_amount_; }
ThresholdWatcher& buffered_amount() { return buffered_amount_; }

64
net/dcsctp/tx/rr_send_queue_test.cc
View File

@ -154,35 +154,6 @@ TEST_F(RRSendQueueTest, BufferBecomesFullAndEmptied) {
EXPECT_TRUE(buf_.IsEmpty());
}
TEST_F(RRSendQueueTest, WillNotSendTooSmallPacket) {
std::vector<uint8_t> payload(RRSendQueue::kMinimumFragmentedPayload + 1);
buf_.Add(kNow, DcSctpMessage(kStreamID, kPPID, payload));
// Wouldn't fit enough payload (wouldn't want to fragment)
EXPECT_FALSE(
buf_.Produce(kNow,
/*max_size=*/RRSendQueue::kMinimumFragmentedPayload - 1)
.has_value());
// Minimum fragment
absl::optional<SendQueue::DataToSend> chunk_one =
buf_.Produce(kNow,
/*max_size=*/RRSendQueue::kMinimumFragmentedPayload);
ASSERT_TRUE(chunk_one.has_value());
EXPECT_EQ(chunk_one->data.stream_id, kStreamID);
EXPECT_EQ(chunk_one->data.ppid, kPPID);
// There is only one byte remaining - it can be fetched as it doesn't require
// additional fragmentation.
absl::optional<SendQueue::DataToSend> chunk_two =
buf_.Produce(kNow, /*max_size=*/1);
ASSERT_TRUE(chunk_two.has_value());
EXPECT_EQ(chunk_two->data.stream_id, kStreamID);
EXPECT_EQ(chunk_two->data.ppid, kPPID);
EXPECT_TRUE(buf_.IsEmpty());
}
TEST_F(RRSendQueueTest, DefaultsToOrderedSend) {
std::vector<uint8_t> payload(20);
@ -774,40 +745,5 @@ TEST_F(RRSendQueueTest, WillStayInAStreamAsLongAsThatMessageIsSending) {
EXPECT_FALSE(buf_.Produce(kNow, kOneFragmentPacketSize).has_value());
}
TEST_F(RRSendQueueTest, WillStayInStreamWhenOnlySmallFragmentRemaining) {
buf_.Add(kNow,
DcSctpMessage(StreamID(5), kPPID,
std::vector<uint8_t>(kOneFragmentPacketSize * 2)));
buf_.Add(kNow, DcSctpMessage(StreamID(6), kPPID, std::vector<uint8_t>(1)));
ASSERT_HAS_VALUE_AND_ASSIGN(SendQueue::DataToSend chunk1,
buf_.Produce(kNow, kOneFragmentPacketSize));
EXPECT_EQ(chunk1.data.stream_id, StreamID(5));
EXPECT_THAT(chunk1.data.payload, SizeIs(kOneFragmentPacketSize));
// Now assume that there will be a lot of previous chunks that need to be
// retransmitted, which fills up the next packet and there is little space
// left in the packet for new chunks. What it should NOT do right now is to
// try to send a message from StreamID 6. And it should not try to send a very
// small fragment from StreamID 5 either. So just skip this one.
EXPECT_FALSE(buf_.Produce(kNow, 8).has_value());
// When the next produce request comes with a large buffer to fill, continue
// sending from StreamID 5.
ASSERT_HAS_VALUE_AND_ASSIGN(SendQueue::DataToSend chunk2,
buf_.Produce(kNow, kOneFragmentPacketSize));
EXPECT_EQ(chunk2.data.stream_id, StreamID(5));
EXPECT_THAT(chunk2.data.payload, SizeIs(kOneFragmentPacketSize));
// Lastly, produce a message on StreamID 6.
ASSERT_HAS_VALUE_AND_ASSIGN(SendQueue::DataToSend chunk3,
buf_.Produce(kNow, kOneFragmentPacketSize));
EXPECT_EQ(chunk3.data.stream_id, StreamID(6));
EXPECT_THAT(chunk3.data.payload, SizeIs(1));
EXPECT_FALSE(buf_.Produce(kNow, 8).has_value());
}
} // namespace
} // namespace dcsctp