From ea7e7a97537d83ffc3abd06c401f5d5389f04458 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Fri, 21 Dec 2018 13:16:16 +0100 Subject: [PATCH] Fix incorrect behavior in H264 packetizer in some cases Just ignoring single_packet_reduction_len is wrong, because if the fragment is put in a single packet it might still be the first or the last packet in the whole sequence. Bug: none Change-Id: I4a2fbebe1d49cbef9298bb32d9cecaa617e4dfc3 Reviewed-on: https://webrtc-review.googlesource.com/c/115403 Commit-Queue: Ilya Nikolaevskiy Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#26084} --- modules/rtp_rtcp/source/rtp_format_h264.cc | 15 ++++++++++--- .../source/rtp_format_h264_unittest.cc | 22 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_format_h264.cc b/modules/rtp_rtcp/source/rtp_format_h264.cc index 7a7bcdfbb8..b702a09902 100644 --- a/modules/rtp_rtcp/source/rtp_format_h264.cc +++ b/modules/rtp_rtcp/source/rtp_format_h264.cc @@ -219,10 +219,19 @@ bool RtpPacketizerH264::PacketizeFuA(size_t fragment_index) { PayloadSizeLimits limits = limits_; // Leave room for the FU-A header. limits.max_payload_len -= kFuAHeaderSize; - // Ignore single/first/last packet reductions unless it is single/first/last + // Update single/first/last packet reductions unless it is single/first/last // fragment. - if (input_fragments_.size() != 1) - limits.single_packet_reduction_len = 0; + if (input_fragments_.size() != 1) { + // if this fragment is put into a single packet, it might still be the + // first or the last packet in the whole sequence of packets. + if (fragment_index == input_fragments_.size() - 1) { + limits.single_packet_reduction_len = limits_.last_packet_reduction_len; + } else if (fragment_index == 0) { + limits.single_packet_reduction_len = limits_.first_packet_reduction_len; + } else { + limits.single_packet_reduction_len = 0; + } + } if (fragment_index != 0) limits.first_packet_reduction_len = 0; if (fragment_index != input_fragments_.size() - 1) diff --git a/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc b/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc index aeab813b53..0419159dd8 100644 --- a/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc @@ -379,6 +379,28 @@ TEST(RtpPacketizerH264Test, MixedStapAFUA) { ElementsAreArray(next_fragment, kStapANaluSize)); } +TEST(RtpPacketizerH264Test, LastFragmentFitsInSingleButNotLastPacket) { + RtpPacketizer::PayloadSizeLimits limits; + limits.max_payload_len = 1178; + limits.first_packet_reduction_len = 0; + limits.last_packet_reduction_len = 20; + limits.single_packet_reduction_len = 20; + // Actual sizes, which triggered this bug. + size_t fragments[] = {20, 8, 18, 1161}; + RTPFragmentationHeader fragmentation = CreateFragmentation(fragments); + rtc::Buffer frame = CreateFrame(fragmentation); + + RtpPacketizerH264 packetizer( + frame, limits, H264PacketizationMode::NonInterleaved, fragmentation); + std::vector packets = FetchAllPackets(&packetizer); + + // Last packet has to be of correct size. + // Incorrect implementation might miss this constraint and not split the last + // fragment in two packets. + EXPECT_LE(static_cast(packets.back().payload_size()), + limits.max_payload_len - limits.last_packet_reduction_len); +} + // Splits frame with payload size |frame_payload_size| without fragmentation, // Returns sizes of the payloads excluding fua headers. std::vector TestFua(size_t frame_payload_size,