From ab9535c0989b79b74b241dee88c505d79fa474f1 Mon Sep 17 00:00:00 2001 From: Joachim Reiersen Date: Mon, 11 Sep 2023 17:30:07 -0700 Subject: [PATCH] Use single packet limit when all fragments end up in one H.264 packet Update RtpPacketizerH264::PacketizeStapA to use single_packet_reduction_len when all fragments end up in one H.264 packet. Previous code was using first_packet_reduction_len + last_packet_reduction_len for this case, which can cause an occasional RTC_CHECK crash in RtpPacketizerH264::NextAggregatePacket due to exceeding the available payload capacity of an RTP packet. Bug: webrtc:15477 Change-Id: Iba1564a6a29618bef22f19d82aba938420994b23 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/319645 Reviewed-by: Philip Eliasson Reviewed-by: Danil Chapovalov Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40737} --- modules/rtp_rtcp/source/rtp_format_h264.cc | 21 +++---- .../source/rtp_format_h264_unittest.cc | 57 +++++++++++++++---- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_format_h264.cc b/modules/rtp_rtcp/source/rtp_format_h264.cc index cc8d1bff34..d066bafb90 100644 --- a/modules/rtp_rtcp/source/rtp_format_h264.cc +++ b/modules/rtp_rtcp/source/rtp_format_h264.cc @@ -153,28 +153,25 @@ bool RtpPacketizerH264::PacketizeFuA(size_t fragment_index) { size_t RtpPacketizerH264::PacketizeStapA(size_t fragment_index) { // Aggregate fragments into one packet (STAP-A). size_t payload_size_left = limits_.max_payload_len; - if (input_fragments_.size() == 1) - payload_size_left -= limits_.single_packet_reduction_len; - else if (fragment_index == 0) - payload_size_left -= limits_.first_packet_reduction_len; int aggregated_fragments = 0; size_t fragment_headers_length = 0; rtc::ArrayView fragment = input_fragments_[fragment_index]; RTC_CHECK_GE(payload_size_left, fragment.size()); ++num_packets_left_; + const bool has_first_fragment = fragment_index == 0; auto payload_size_needed = [&] { size_t fragment_size = fragment.size() + fragment_headers_length; - if (input_fragments_.size() == 1) { - // Single fragment, single packet, payload_size_left already adjusted - // with limits_.single_packet_reduction_len. + bool has_last_fragment = fragment_index == input_fragments_.size() - 1; + if (has_first_fragment && has_last_fragment) { + return fragment_size + limits_.single_packet_reduction_len; + } else if (has_first_fragment) { + return fragment_size + limits_.first_packet_reduction_len; + } else if (has_last_fragment) { + return fragment_size + limits_.last_packet_reduction_len; + } else { return fragment_size; } - if (fragment_index == input_fragments_.size() - 1) { - // Last fragment, so STAP-A might be the last packet. - return fragment_size + limits_.last_packet_reduction_len; - } - return fragment_size; }; while (payload_size_left >= payload_size_needed()) { diff --git a/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc b/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc index 80d8801437..18311c6e8c 100644 --- a/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc @@ -285,14 +285,50 @@ TEST(RtpPacketizerH264Test, StapARespectsFirstPacketReduction) { 0, 2, nalus[2][0], nalus[2][1])); } +TEST(RtpPacketizerH264Test, StapARespectsSinglePacketReduction) { + RtpPacketizer::PayloadSizeLimits limits; + limits.max_payload_len = 1000; + // It is possible for single_packet_reduction_len to be greater than + // first_packet_reduction_len + last_packet_reduction_len. Check that the + // right limit is used when first and last fragment go to one packet. + limits.first_packet_reduction_len = 4; + limits.last_packet_reduction_len = 0; + limits.single_packet_reduction_len = 8; + // 3 fragments of sizes 2 + 2 + 981, plus 7 bytes of headers, is expected to + // be packetized to single packet of size 992. + rtc::Buffer first_nalus[] = {GenerateNalUnit(/*size=*/2), + GenerateNalUnit(/*size=*/2), + GenerateNalUnit(/*size=*/981)}; + rtc::Buffer first_frame = CreateFrame(first_nalus); + + RtpPacketizerH264 first_packetizer(first_frame, limits, + H264PacketizationMode::NonInterleaved); + std::vector packets = FetchAllPackets(&first_packetizer); + + // Expect that everything fits in a single packet. + ASSERT_THAT(packets, SizeIs(1)); + EXPECT_EQ(packets[0].payload_size(), 992u); + + // Increasing the last fragment size by one exceeds + // single_packet_reduction_len and produces two packets. + rtc::Buffer second_nalus[] = {GenerateNalUnit(/*size=*/2), + GenerateNalUnit(/*size=*/2), + GenerateNalUnit(/*size=*/982)}; + rtc::Buffer second_frame = CreateFrame(second_nalus); + RtpPacketizerH264 second_packetizer(second_frame, limits, + H264PacketizationMode::NonInterleaved); + packets = FetchAllPackets(&second_packetizer); + ASSERT_THAT(packets, SizeIs(2)); +} + TEST(RtpPacketizerH264Test, StapARespectsLastPacketReduction) { RtpPacketizer::PayloadSizeLimits limits; limits.max_payload_len = 1000; limits.last_packet_reduction_len = 100; + const size_t kFirstFragmentSize = 1000; const size_t kLastFragmentSize = - limits.max_payload_len - limits.last_packet_reduction_len; - rtc::Buffer nalus[] = {GenerateNalUnit(/*size=*/2), - GenerateNalUnit(/*size=*/2), + limits.max_payload_len - limits.last_packet_reduction_len + 1; + rtc::Buffer nalus[] = {GenerateNalUnit(/*size=*/kFirstFragmentSize), GenerateNalUnit(/*size=*/kLastFragmentSize)}; rtc::Buffer frame = CreateFrame(nalus); @@ -300,14 +336,13 @@ TEST(RtpPacketizerH264Test, StapARespectsLastPacketReduction) { H264PacketizationMode::NonInterleaved); std::vector packets = FetchAllPackets(&packetizer); - ASSERT_THAT(packets, SizeIs(2)); - // Expect 1st packet is aggregate of 1st two fragments. - EXPECT_THAT(packets[0].payload(), - ElementsAre(kStapA, // - 0, 2, nalus[0][0], nalus[0][1], // - 0, 2, nalus[1][0], nalus[1][1])); - // Expect 2nd packet is single nalu. - EXPECT_THAT(packets[1].payload(), ElementsAreArray(nalus[2])); + ASSERT_THAT(packets, SizeIs(3)); + // Expect 1st packet contains first fragment. + EXPECT_THAT(packets[0].payload()[0], kSlice); + // Expect 2nd and 3rd packets to be FU-A since last_packet_reduction_len + // was exceeded by one byte. + EXPECT_THAT(packets[1].payload()[0], kFuA); + EXPECT_THAT(packets[2].payload()[0], kFuA); } TEST(RtpPacketizerH264Test, TooSmallForStapAHeaders) {