Use signed integers for limiting packet size in video packetizers
Using signed integers allows to centralize checking of edge cases
in RtpPacketizer::SplitAboutEqually and
reduce chance of hitting issues with size_t underflow
Bug: webrtc:9680
Change-Id: Ic05bf0a9565a277c4608f43061ca46cf44e82d08
Reviewed-on: https://webrtc-review.googlesource.com/98602
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24618}diff --git a/modules/rtp_rtcp/source/rtp_format.cc b/modules/rtp_rtcp/source/rtp_format.cc
index 8a6e8f1..e966833 100644
--- a/modules/rtp_rtcp/source/rtp_format.cc
+++ b/modules/rtp_rtcp/source/rtp_format.cc
@@ -59,26 +59,42 @@
}
}
-std::vector<size_t> RtpPacketizer::SplitAboutEqually(
- size_t payload_len,
+std::vector<int> RtpPacketizer::SplitAboutEqually(
+ int payload_len,
const PayloadSizeLimits& limits) {
- RTC_CHECK_GT(limits.max_payload_len, limits.first_packet_reduction_len);
- RTC_CHECK_GT(limits.max_payload_len, limits.last_packet_reduction_len);
+ RTC_DCHECK_GT(payload_len, 0);
+ // First or last packet larger than normal are unsupported.
+ RTC_DCHECK_GE(limits.first_packet_reduction_len, 0);
+ RTC_DCHECK_GE(limits.last_packet_reduction_len, 0);
+ std::vector<int> result;
+ if (limits.max_payload_len - limits.first_packet_reduction_len < 1 ||
+ limits.max_payload_len - limits.last_packet_reduction_len < 1) {
+ // Capacity is not enough to put a single byte into one of the packets.
+ return result;
+ }
// First and last packet of the frame can be smaller. Pretend that it's
// the same size, but we must write more payload to it.
// Assume frame fits in single packet if packet has extra space for sum
// of first and last packets reductions.
- size_t total_bytes = payload_len + limits.first_packet_reduction_len +
- limits.last_packet_reduction_len;
+ int total_bytes = payload_len + limits.first_packet_reduction_len +
+ limits.last_packet_reduction_len;
// Integer divisions with rounding up.
- size_t num_packets_left =
+ int num_packets_left =
(total_bytes + limits.max_payload_len - 1) / limits.max_payload_len;
- size_t bytes_per_packet = total_bytes / num_packets_left;
- size_t num_larger_packets = total_bytes % num_packets_left;
- size_t remaining_data = payload_len;
- std::vector<size_t> result;
+ if (payload_len < num_packets_left) {
+ // Edge case where limits force to have more packets than there are payload
+ // bytes. This may happen when there is single byte of payload that can't be
+ // put into single packet if
+ // first_packet_reduction + last_packet_reduction >= max_payload_len.
+ return result;
+ }
+
+ int bytes_per_packet = total_bytes / num_packets_left;
+ int num_larger_packets = total_bytes % num_packets_left;
+ int remaining_data = payload_len;
+
result.reserve(num_packets_left);
bool first_packet = true;
while (remaining_data > 0) {
@@ -86,7 +102,7 @@
// per-packet payload size when needed.
if (num_packets_left == num_larger_packets)
++bytes_per_packet;
- size_t current_packet_bytes = bytes_per_packet;
+ int current_packet_bytes = bytes_per_packet;
if (first_packet) {
if (current_packet_bytes > limits.first_packet_reduction_len + 1)
current_packet_bytes -= limits.first_packet_reduction_len;
diff --git a/modules/rtp_rtcp/source/rtp_format.h b/modules/rtp_rtcp/source/rtp_format.h
index 7a1f494..9fad4cf 100644
--- a/modules/rtp_rtcp/source/rtp_format.h
+++ b/modules/rtp_rtcp/source/rtp_format.h
@@ -27,9 +27,9 @@
class RtpPacketizer {
public:
struct PayloadSizeLimits {
- size_t max_payload_len = 1200;
- size_t first_packet_reduction_len = 0;
- size_t last_packet_reduction_len = 0;
+ int max_payload_len = 1200;
+ int first_packet_reduction_len = 0;
+ int last_packet_reduction_len = 0;
};
static std::unique_ptr<RtpPacketizer> Create(
VideoCodecType type,
@@ -51,8 +51,9 @@
virtual bool NextPacket(RtpPacketToSend* packet) = 0;
// Split payload_len into sum of integers with respect to |limits|.
- static std::vector<size_t> SplitAboutEqually(size_t payload_len,
- const PayloadSizeLimits& limits);
+ // Returns empty vector on failure.
+ static std::vector<int> SplitAboutEqually(int payload_len,
+ const PayloadSizeLimits& limits);
};
// TODO(sprang): Update the depacketizer to return a std::unqie_ptr with a copy
diff --git a/modules/rtp_rtcp/source/rtp_format_unittest.cc b/modules/rtp_rtcp/source/rtp_format_unittest.cc
index 8fb780e..a79c434 100644
--- a/modules/rtp_rtcp/source/rtp_format_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_format_unittest.cc
@@ -31,7 +31,7 @@
// adjustement provided by limits,
// i.e. last packet expected to be smaller than 'average' by reduction_len.
int EffectivePacketsSizeDifference(
- std::vector<size_t> sizes,
+ std::vector<int> sizes,
const RtpPacketizer::PayloadSizeLimits& limits) {
// Account for larger last packet header.
sizes.back() += limits.last_packet_reduction_len;
@@ -41,7 +41,7 @@
return *minmax.second - *minmax.first;
}
-size_t Sum(const std::vector<size_t>& sizes) {
+int Sum(const std::vector<int>& sizes) {
return std::accumulate(sizes.begin(), sizes.end(), 0);
}
@@ -50,8 +50,7 @@
limits.max_payload_len = 5;
limits.last_packet_reduction_len = 2;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(13, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(13, limits);
EXPECT_THAT(Sum(payload_sizes), 13);
}
@@ -61,8 +60,7 @@
limits.max_payload_len = 5;
limits.last_packet_reduction_len = 2;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(13, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(13, limits);
EXPECT_THAT(payload_sizes, Each(Le(limits.max_payload_len)));
}
@@ -73,8 +71,7 @@
limits.max_payload_len = 5;
limits.first_packet_reduction_len = 2;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(13, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(13, limits);
ASSERT_THAT(payload_sizes, Not(IsEmpty()));
EXPECT_EQ(payload_sizes.front() + limits.first_packet_reduction_len,
@@ -87,8 +84,7 @@
limits.max_payload_len = 5;
limits.last_packet_reduction_len = 2;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(13, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(13, limits);
ASSERT_THAT(payload_sizes, Not(IsEmpty()));
EXPECT_LE(payload_sizes.back() + limits.last_packet_reduction_len,
@@ -100,8 +96,7 @@
limits.max_payload_len = 5;
limits.last_packet_reduction_len = 2;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(13, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(13, limits);
EXPECT_EQ(EffectivePacketsSizeDifference(payload_sizes, limits), 0);
}
@@ -112,8 +107,7 @@
limits.max_payload_len = 5;
limits.last_packet_reduction_len = 2;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(13, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(13, limits);
// Computed by hand. 3 packets would have exactly capacity 3*5-2=13
// (max length - for each packet minus last packet reduction).
EXPECT_THAT(payload_sizes, SizeIs(3));
@@ -124,8 +118,7 @@
limits.max_payload_len = 7;
limits.last_packet_reduction_len = 5;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(28, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(28, limits);
EXPECT_THAT(Sum(payload_sizes), 28);
}
@@ -136,8 +129,7 @@
limits.max_payload_len = 7;
limits.last_packet_reduction_len = 5;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(28, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(28, limits);
EXPECT_THAT(payload_sizes, Each(Le(limits.max_payload_len)));
}
@@ -148,8 +140,7 @@
limits.max_payload_len = 7;
limits.first_packet_reduction_len = 5;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(28, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(28, limits);
EXPECT_LE(payload_sizes.front() + limits.first_packet_reduction_len,
limits.max_payload_len);
@@ -161,8 +152,7 @@
limits.max_payload_len = 7;
limits.last_packet_reduction_len = 5;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(28, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(28, limits);
EXPECT_LE(payload_sizes.back(),
limits.max_payload_len - limits.last_packet_reduction_len);
@@ -174,8 +164,7 @@
limits.max_payload_len = 7;
limits.last_packet_reduction_len = 5;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(28, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(28, limits);
EXPECT_LE(EffectivePacketsSizeDifference(payload_sizes, limits), 1);
}
@@ -186,8 +175,7 @@
limits.max_payload_len = 7;
limits.last_packet_reduction_len = 5;
- std::vector<size_t> payload_sizes =
- RtpPacketizer::SplitAboutEqually(24, limits);
+ std::vector<int> payload_sizes = RtpPacketizer::SplitAboutEqually(24, limits);
// Computed by hand. 4 packets would have capacity 4*7-5=23 (max length -
// for each packet minus last packet reduction).
// 5 packets is enough for kPayloadSize.
@@ -203,11 +191,11 @@
// Naive implementation would split 1450 payload + 1050 reduction bytes into 5
// packets 500 bytes each, thus leaving first packet zero bytes and even less
// to last packet.
- std::vector<size_t> payload_sizes =
+ std::vector<int> payload_sizes =
RtpPacketizer::SplitAboutEqually(1450, limits);
- EXPECT_EQ(Sum(payload_sizes), 1450u);
- EXPECT_THAT(payload_sizes, Each(Gt(0u)));
+ EXPECT_EQ(Sum(payload_sizes), 1450);
+ EXPECT_THAT(payload_sizes, Each(Gt(0)));
}
TEST(RtpPacketizerSplitAboutEqually,
@@ -232,5 +220,55 @@
EXPECT_THAT(RtpPacketizer::SplitAboutEqually(20, limits), ElementsAre(9, 11));
}
+TEST(RtpPacketizerSplitAboutEqually, RejectsZeroMaxPayloadLen) {
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = 0;
+
+ EXPECT_THAT(RtpPacketizer::SplitAboutEqually(20, limits), IsEmpty());
+}
+
+TEST(RtpPacketizerSplitAboutEqually, RejectsZeroFirstPacketLen) {
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = 5;
+ limits.first_packet_reduction_len = 5;
+
+ EXPECT_THAT(RtpPacketizer::SplitAboutEqually(20, limits), IsEmpty());
+}
+
+TEST(RtpPacketizerSplitAboutEqually, RejectsZeroLastPacketLen) {
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = 5;
+ limits.last_packet_reduction_len = 5;
+
+ EXPECT_THAT(RtpPacketizer::SplitAboutEqually(20, limits), IsEmpty());
+}
+
+TEST(RtpPacketizerSplitAboutEqually, CantPutSinglePayloadByteInTwoPackets) {
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = 10;
+ limits.first_packet_reduction_len = 6;
+ limits.last_packet_reduction_len = 4;
+
+ EXPECT_THAT(RtpPacketizer::SplitAboutEqually(1, limits), IsEmpty());
+}
+
+TEST(RtpPacketizerSplitAboutEqually, CanPutTwoPayloadBytesInTwoPackets) {
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = 10;
+ limits.first_packet_reduction_len = 6;
+ limits.last_packet_reduction_len = 4;
+
+ EXPECT_THAT(RtpPacketizer::SplitAboutEqually(2, limits), ElementsAre(1, 1));
+}
+
+TEST(RtpPacketizerSplitAboutEqually, CanPutSinglePayloadByteInOnePacket) {
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = 11;
+ limits.first_packet_reduction_len = 6;
+ limits.last_packet_reduction_len = 4;
+
+ EXPECT_THAT(RtpPacketizer::SplitAboutEqually(1, limits), ElementsAre(1));
+}
+
} // namespace
} // namespace webrtc
diff --git a/modules/rtp_rtcp/source/rtp_format_video_generic.h b/modules/rtp_rtcp/source/rtp_format_video_generic.h
index 7d14886..03509f5 100644
--- a/modules/rtp_rtcp/source/rtp_format_video_generic.h
+++ b/modules/rtp_rtcp/source/rtp_format_video_generic.h
@@ -52,8 +52,8 @@
uint8_t header_[3];
size_t header_size_;
rtc::ArrayView<const uint8_t> remaining_payload_;
- std::vector<size_t> payload_sizes_;
- std::vector<size_t>::const_iterator current_packet_;
+ std::vector<int> payload_sizes_;
+ std::vector<int>::const_iterator current_packet_;
RTC_DISALLOW_COPY_AND_ASSIGN(RtpPacketizerGeneric);
};
diff --git a/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc b/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc
index 8cf1ffa..aa2829b 100644
--- a/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_format_video_generic_unittest.cc
@@ -33,10 +33,9 @@
constexpr RtpPacketizer::PayloadSizeLimits kNoSizeLimits;
-std::vector<size_t> NextPacketFillPayloadSizes(
- RtpPacketizerGeneric* packetizer) {
+std::vector<int> NextPacketFillPayloadSizes(RtpPacketizerGeneric* packetizer) {
RtpPacketToSend packet(nullptr);
- std::vector<size_t> result;
+ std::vector<int> result;
while (packetizer->NextPacket(&packet)) {
result.push_back(packet.payload_size());
}
@@ -52,7 +51,7 @@
RtpPacketizerGeneric packetizer(kPayload, limits, RTPVideoHeader(),
kVideoFrameKey);
- std::vector<size_t> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+ std::vector<int> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
EXPECT_THAT(payload_sizes, Each(Le(limits.max_payload_len)));
}
@@ -66,7 +65,7 @@
RtpPacketizerGeneric packetizer(kPayload, limits, RTPVideoHeader(),
kVideoFrameKey);
- std::vector<size_t> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+ std::vector<int> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
// With kPayloadSize > max_payload_len^2, there should be packets that use
// all the payload, otherwise it is possible to use less packets.
@@ -94,7 +93,7 @@
}
TEST(RtpPacketizerVideoGeneric, RespectsMaxPayloadSizeWithExtendedHeader) {
- const size_t kPayloadSize = 50;
+ const int kPayloadSize = 50;
const uint8_t kPayload[kPayloadSize] = {};
RtpPacketizer::PayloadSizeLimits limits;
@@ -104,13 +103,13 @@
RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header,
kVideoFrameKey);
- std::vector<size_t> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+ std::vector<int> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
EXPECT_THAT(payload_sizes, Each(Le(limits.max_payload_len)));
}
TEST(RtpPacketizerVideoGeneric, UsesMaxPayloadSizeWithExtendedHeader) {
- const size_t kPayloadSize = 50;
+ const int kPayloadSize = 50;
const uint8_t kPayload[kPayloadSize] = {};
RtpPacketizer::PayloadSizeLimits limits;
@@ -119,7 +118,7 @@
rtp_video_header.generic.emplace().frame_id = 37;
RtpPacketizerGeneric packetizer(kPayload, limits, rtp_video_header,
kVideoFrameKey);
- std::vector<size_t> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
+ std::vector<int> payload_sizes = NextPacketFillPayloadSizes(&packetizer);
// With kPayloadSize > max_payload_len^2, there should be packets that use
// all the payload, otherwise it is possible to use less packets.
@@ -127,7 +126,7 @@
}
TEST(RtpPacketizerVideoGeneric, FrameIdOver15bitsWrapsAround) {
- const size_t kPayloadSize = 13;
+ const int kPayloadSize = 13;
const uint8_t kPayload[kPayloadSize] = {};
RTPVideoHeader rtp_video_header;
@@ -146,7 +145,7 @@
}
TEST(RtpPacketizerVideoGeneric, NoFrameIdDoesNotWriteExtendedHeader) {
- const size_t kPayloadSize = 13;
+ const int kPayloadSize = 13;
const uint8_t kPayload[kPayloadSize] = {};
RtpPacketizerGeneric packetizer(kPayload, kNoSizeLimits, RTPVideoHeader(),
diff --git a/modules/rtp_rtcp/source/rtp_format_vp8.cc b/modules/rtp_rtcp/source/rtp_format_vp8.cc
index b219dac..40bc291 100644
--- a/modules/rtp_rtcp/source/rtp_format_vp8.cc
+++ b/modules/rtp_rtcp/source/rtp_format_vp8.cc
@@ -173,13 +173,6 @@
PayloadSizeLimits limits,
const RTPVideoHeaderVP8& hdr_info)
: hdr_(BuildHeader(hdr_info)), remaining_payload_(payload) {
- if (limits.max_payload_len - limits.last_packet_reduction_len <
- hdr_.size() + 1) {
- // The provided payload length is not long enough for the payload
- // descriptor and one payload byte in the last packet.
- current_packet_ = payload_sizes_.begin();
- return;
- }
limits.max_payload_len -= hdr_.size();
payload_sizes_ = SplitAboutEqually(payload.size(), limits);
current_packet_ = payload_sizes_.begin();
diff --git a/modules/rtp_rtcp/source/rtp_format_vp8.h b/modules/rtp_rtcp/source/rtp_format_vp8.h
index 2fdd6c5..e4bc36e 100644
--- a/modules/rtp_rtcp/source/rtp_format_vp8.h
+++ b/modules/rtp_rtcp/source/rtp_format_vp8.h
@@ -61,8 +61,8 @@
RawHeader hdr_;
rtc::ArrayView<const uint8_t> remaining_payload_;
- std::vector<size_t> payload_sizes_;
- std::vector<size_t>::const_iterator current_packet_;
+ std::vector<int> payload_sizes_;
+ std::vector<int>::const_iterator current_packet_;
RTC_DISALLOW_COPY_AND_ASSIGN(RtpPacketizerVp8);
};
diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc
index bf8150d..f72740e9 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -335,20 +335,18 @@
retransmission_settings = retransmission_settings_;
}
- size_t packet_capacity = rtp_sender_->MaxRtpPacketSize() -
- fec_packet_overhead -
- (rtp_sender_->RtxStatus() ? kRtxHeaderSize : 0);
+ int packet_capacity = rtp_sender_->MaxRtpPacketSize() - fec_packet_overhead -
+ (rtp_sender_->RtxStatus() ? kRtxHeaderSize : 0);
RTC_DCHECK_LE(packet_capacity, rtp_header->capacity());
RTC_DCHECK_GT(packet_capacity, rtp_header->headers_size());
RTC_DCHECK_GT(packet_capacity, last_packet->headers_size());
- size_t max_data_payload_length = packet_capacity - rtp_header->headers_size();
+ RtpPacketizer::PayloadSizeLimits limits;
+ limits.max_payload_len = packet_capacity - rtp_header->headers_size();
+
RTC_DCHECK_GE(last_packet->headers_size(), rtp_header->headers_size());
- size_t last_packet_reduction_len =
+ limits.last_packet_reduction_len =
last_packet->headers_size() - rtp_header->headers_size();
- RtpPacketizer::PayloadSizeLimits limits;
- limits.max_payload_len = max_data_payload_length;
- limits.last_packet_reduction_len = last_packet_reduction_len;
std::unique_ptr<RtpPacketizer> packetizer = RtpPacketizer::Create(
video_type, rtc::MakeArrayView(payload_data, payload_size), limits,
*video_header, frame_type, fragmentation);
@@ -368,9 +366,10 @@
: absl::make_unique<RtpPacketToSend>(*rtp_header);
if (!packetizer->NextPacket(packet.get()))
return false;
- RTC_DCHECK_LE(packet->payload_size(),
- last ? max_data_payload_length - last_packet_reduction_len
- : max_data_payload_length);
+ RTC_DCHECK_LE(
+ packet->payload_size(),
+ last ? limits.max_payload_len - limits.last_packet_reduction_len
+ : limits.max_payload_len);
if (!rtp_sender_->AssignSequenceNumber(packet.get()))
return false;
