From 393e2664703351833948a30802e50d4ce101662a Mon Sep 17 00:00:00 2001 From: Rasmus Brandt Date: Mon, 22 Jan 2018 12:52:36 +0100 Subject: [PATCH] Use correct RTP header length in RED generation for ULPFEC packets. Prior to this change, in certain circumstances the RTP header length used when creating a RedPacket was incorrect. This was due to an assumption that a new media packet would _always_ be added to the UlpfecGenerator's internal media packet buffer. This is not correct, and the fix is to keep track of whatever RTP header length that is currently correct. Bug: webrtc:8767 Change-Id: I6d61429a19d4693dde9330f0469d13c5dfbeac52 Reviewed-on: https://webrtc-review.googlesource.com/40600 Reviewed-by: Stefan Holmer Reviewed-by: Philip Eliasson Reviewed-by: Danil Chapovalov Commit-Queue: Rasmus Brandt Cr-Commit-Position: refs/heads/master@{#21720} --- modules/rtp_rtcp/source/rtp_sender_video.cc | 3 +- modules/rtp_rtcp/source/ulpfec_generator.cc | 19 +++++-- modules/rtp_rtcp/source/ulpfec_generator.h | 4 +- .../source/ulpfec_generator_unittest.cc | 49 ++++++++++++++++--- test/fuzzers/ulpfec_generator_fuzzer.cc | 3 +- 5 files changed, 61 insertions(+), 17 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index ad618a5bb3..da756889c0 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -145,8 +145,7 @@ void RTPSenderVideo::SendVideoPacketAsRedMaybeWithUlpfec( uint16_t first_fec_sequence_number = rtp_sender_->AllocateSequenceNumber(num_fec_packets); fec_packets = ulpfec_generator_.GetUlpfecPacketsAsRed( - red_payload_type_, ulpfec_payload_type_, first_fec_sequence_number, - media_packet->headers_size()); + red_payload_type_, ulpfec_payload_type_, first_fec_sequence_number); RTC_DCHECK_EQ(num_fec_packets, fec_packets.size()); if (retransmission_settings_ & kRetransmitFECPackets) fec_storage = kAllowRetransmission; diff --git a/modules/rtp_rtcp/source/ulpfec_generator.cc b/modules/rtp_rtcp/source/ulpfec_generator.cc index 8dc9752863..3432bdd11d 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator.cc +++ b/modules/rtp_rtcp/source/ulpfec_generator.cc @@ -13,6 +13,7 @@ #include #include +#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/byte_io.h" #include "modules/rtp_rtcp/source/forward_error_correction.h" #include "modules/rtp_rtcp/source/rtp_utility.h" @@ -105,6 +106,7 @@ UlpfecGenerator::UlpfecGenerator() UlpfecGenerator::UlpfecGenerator(std::unique_ptr fec) : fec_(std::move(fec)), + last_media_packet_rtp_header_length_(0), num_protected_frames_(0), min_num_media_packets_(1) { memset(¶ms_, 0, sizeof(params_)); @@ -142,6 +144,10 @@ int UlpfecGenerator::AddRtpPacketAndGenerateFec(const uint8_t* data_buffer, packet->length = payload_length + rtp_header_length; memcpy(packet->data, data_buffer, packet->length); media_packets_.push_back(std::move(packet)); + // Keep track of the RTP header length, so we can copy the RTP header + // from |packet| to newly generated ULPFEC+RED packets. + RTC_DCHECK_GE(rtp_header_length, kRtpHeaderSize); + last_media_packet_rtp_header_length_ = rtp_header_length; } if (marker_bit) { ++num_protected_frames_; @@ -200,8 +206,7 @@ size_t UlpfecGenerator::MaxPacketOverhead() const { std::vector> UlpfecGenerator::GetUlpfecPacketsAsRed( int red_payload_type, int ulpfec_payload_type, - uint16_t first_seq_num, - size_t rtp_header_length) { + uint16_t first_seq_num) { std::vector> red_packets; red_packets.reserve(generated_fec_packets_.size()); RTC_DCHECK(!media_packets_.empty()); @@ -212,9 +217,12 @@ std::vector> UlpfecGenerator::GetUlpfecPacketsAsRed( // Wrap FEC packet (including FEC headers) in a RED packet. Since the // FEC packets in |generated_fec_packets_| don't have RTP headers, we // reuse the header from the last media packet. - std::unique_ptr red_packet(new RedPacket( - fec_packet->length + kRedForFecHeaderLength + rtp_header_length)); - red_packet->CreateHeader(last_media_packet->data, rtp_header_length, + RTC_DCHECK_GT(last_media_packet_rtp_header_length_, 0); + std::unique_ptr red_packet( + new RedPacket(last_media_packet_rtp_header_length_ + + kRedForFecHeaderLength + fec_packet->length)); + red_packet->CreateHeader(last_media_packet->data, + last_media_packet_rtp_header_length_, red_payload_type, ulpfec_payload_type); red_packet->SetSeqNum(seq_num++); red_packet->ClearMarkerBit(); @@ -237,6 +245,7 @@ int UlpfecGenerator::Overhead() const { void UlpfecGenerator::ResetState() { media_packets_.clear(); + last_media_packet_rtp_header_length_ = 0; generated_fec_packets_.clear(); num_protected_frames_ = 0; } diff --git a/modules/rtp_rtcp/source/ulpfec_generator.h b/modules/rtp_rtcp/source/ulpfec_generator.h index 3de7ae2986..639c15be25 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator.h +++ b/modules/rtp_rtcp/source/ulpfec_generator.h @@ -69,8 +69,7 @@ class UlpfecGenerator { std::vector> GetUlpfecPacketsAsRed( int red_payload_type, int ulpfec_payload_type, - uint16_t first_seq_num, - size_t rtp_header_length); + uint16_t first_seq_num); private: explicit UlpfecGenerator(std::unique_ptr fec); @@ -98,6 +97,7 @@ class UlpfecGenerator { std::unique_ptr fec_; ForwardErrorCorrection::PacketList media_packets_; + size_t last_media_packet_rtp_header_length_; std::list generated_fec_packets_; int num_protected_frames_; int min_num_media_packets_; diff --git a/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc b/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc index 6586d0dc88..b05775443c 100644 --- a/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc +++ b/modules/rtp_rtcp/source/ulpfec_generator_unittest.cc @@ -95,8 +95,8 @@ TEST_F(UlpfecGeneratorTest, NoEmptyFecWithSeqNumGaps) { size_t num_fec_packets = ulpfec_generator_.NumAvailableFecPackets(); if (num_fec_packets > 0) { std::vector> fec_packets = - ulpfec_generator_.GetUlpfecPacketsAsRed( - kRedPayloadType, kFecPayloadType, 100, p.header_size); + ulpfec_generator_.GetUlpfecPacketsAsRed(kRedPayloadType, + kFecPayloadType, 100); EXPECT_EQ(num_fec_packets, fec_packets.size()); } } @@ -122,10 +122,10 @@ TEST_F(UlpfecGeneratorTest, OneFrameFec) { last_timestamp = packet->header.header.timestamp; } EXPECT_TRUE(ulpfec_generator_.FecAvailable()); - uint16_t seq_num = packet_generator_.NextPacketSeqNum(); + const uint16_t seq_num = packet_generator_.NextPacketSeqNum(); std::vector> red_packets = ulpfec_generator_.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, - seq_num, kRtpHeaderSize); + seq_num); EXPECT_FALSE(ulpfec_generator_.FecAvailable()); ASSERT_EQ(1u, red_packets.size()); VerifyHeader(seq_num, last_timestamp, kRedPayloadType, kFecPayloadType, @@ -157,14 +157,51 @@ TEST_F(UlpfecGeneratorTest, TwoFrameFec) { } } EXPECT_TRUE(ulpfec_generator_.FecAvailable()); - uint16_t seq_num = packet_generator_.NextPacketSeqNum(); + const uint16_t seq_num = packet_generator_.NextPacketSeqNum(); std::vector> red_packets = ulpfec_generator_.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, - seq_num, kRtpHeaderSize); + seq_num); EXPECT_FALSE(ulpfec_generator_.FecAvailable()); ASSERT_EQ(1u, red_packets.size()); VerifyHeader(seq_num, last_timestamp, kRedPayloadType, kFecPayloadType, red_packets.front().get(), false); } +TEST_F(UlpfecGeneratorTest, MixedMediaRtpHeaderLengths) { + constexpr size_t kShortRtpHeaderLength = 12; + constexpr size_t kLongRtpHeaderLength = 16; + + // Only one frame required to generate FEC. + FecProtectionParams params = {127, 1, kFecMaskRandom}; + ulpfec_generator_.SetFecParameters(params); + + // Fill up internal buffer with media packets with short RTP header length. + packet_generator_.NewFrame(kUlpfecMaxMediaPackets + 1); + for (size_t i = 0; i < kUlpfecMaxMediaPackets; ++i) { + std::unique_ptr packet = + packet_generator_.NextPacket(i, 10); + EXPECT_EQ(0, ulpfec_generator_.AddRtpPacketAndGenerateFec( + packet->data, packet->length, kShortRtpHeaderLength)); + EXPECT_FALSE(ulpfec_generator_.FecAvailable()); + } + + // Kick off FEC generation with media packet with long RTP header length. + // Since the internal buffer is full, this packet will not be protected. + std::unique_ptr packet = + packet_generator_.NextPacket(kUlpfecMaxMediaPackets, 10); + EXPECT_EQ(0, ulpfec_generator_.AddRtpPacketAndGenerateFec( + packet->data, packet->length, kLongRtpHeaderLength)); + EXPECT_TRUE(ulpfec_generator_.FecAvailable()); + + // Ensure that the RED header is placed correctly, i.e. the correct + // RTP header length was used in the RED packet creation. + const uint16_t seq_num = packet_generator_.NextPacketSeqNum(); + std::vector> red_packets = + ulpfec_generator_.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, + seq_num); + for (const auto& red_packet : red_packets) { + EXPECT_EQ(kFecPayloadType, red_packet->data()[kShortRtpHeaderLength]); + } +} + } // namespace webrtc diff --git a/test/fuzzers/ulpfec_generator_fuzzer.cc b/test/fuzzers/ulpfec_generator_fuzzer.cc index 03728f6bac..995bf290b3 100644 --- a/test/fuzzers/ulpfec_generator_fuzzer.cc +++ b/test/fuzzers/ulpfec_generator_fuzzer.cc @@ -51,8 +51,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { const size_t num_fec_packets = generator.NumAvailableFecPackets(); if (num_fec_packets > 0) { std::vector> fec_packets = - generator.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, 100, - rtp_header_length); + generator.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, 100); RTC_CHECK_EQ(num_fec_packets, fec_packets.size()); } }