From c07ebb30c530888dc56a6d70cd5537336fdf12c2 Mon Sep 17 00:00:00 2001 From: Rasmus Brandt Date: Tue, 4 Oct 2016 10:57:36 +0200 Subject: [PATCH] Simplify public interface of ProducerFec. - Change some member functions to be private. These were only called by other private member functions. - Replace DeleteMediaPackets() with direct calls to media_packets_.clear() - Rename GetFecPacketsAsRed to GetUlpfecPacketsAsRed. No functional changes are intended by this CL. BUG=webrtc:5654 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/2305793003 . Cr-Commit-Position: refs/heads/master@{#14491} --- .../modules/rtp_rtcp/source/producer_fec.cc | 28 +++++++------ webrtc/modules/rtp_rtcp/source/producer_fec.h | 39 +++++++++++-------- .../rtp_rtcp/source/producer_fec_unittest.cc | 12 +++--- .../rtp_rtcp/source/rtp_sender_video.cc | 2 +- webrtc/test/fuzzers/producer_fec_fuzzer.cc | 4 +- 5 files changed, 45 insertions(+), 40 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/producer_fec.cc b/webrtc/modules/rtp_rtcp/source/producer_fec.cc index cdd898d733..2a2327cd94 100644 --- a/webrtc/modules/rtp_rtcp/source/producer_fec.cc +++ b/webrtc/modules/rtp_rtcp/source/producer_fec.cc @@ -21,6 +21,8 @@ namespace webrtc { +namespace { + constexpr size_t kRedForFecHeaderLength = 1; // This controls the maximum amount of excess overhead (actual - target) @@ -47,6 +49,8 @@ constexpr uint8_t kHighProtectionThreshold = 80; // |kMinMediaPackets| + 1 packets are sent to the FEC code. constexpr float kMinMediaPacketsAdaptationThreshold = 2.0f; +} // namespace + RedPacket::RedPacket(size_t length) : data_(new uint8_t[length]), length_(length), @@ -101,9 +105,7 @@ ProducerFec::ProducerFec() memset(&new_params_, 0, sizeof(new_params_)); } -ProducerFec::~ProducerFec() { - DeleteMediaPackets(); -} +ProducerFec::~ProducerFec() = default; std::unique_ptr ProducerFec::BuildRedPacket( const uint8_t* data_buffer, @@ -179,8 +181,7 @@ int ProducerFec::AddRtpPacketAndGenerateFec(const uint8_t* data_buffer, num_important_packets_, kUseUnequalProtection, params_.fec_mask_type, &generated_fec_packets_); if (generated_fec_packets_.empty()) { - num_protected_frames_ = 0; - DeleteMediaPackets(); + ResetState(); } return ret; } @@ -216,7 +217,7 @@ size_t ProducerFec::MaxPacketOverhead() const { return fec_->MaxPacketOverhead(); } -std::vector> ProducerFec::GetFecPacketsAsRed( +std::vector> ProducerFec::GetUlpfecPacketsAsRed( int red_payload_type, int ulpfec_payload_type, uint16_t first_seq_num, @@ -240,18 +241,13 @@ std::vector> ProducerFec::GetFecPacketsAsRed( red_packet->AssignPayload(fec_packet->data, fec_packet->length); red_packets.push_back(std::move(red_packet)); } - // Reset state. - DeleteMediaPackets(); - generated_fec_packets_.clear(); - num_protected_frames_ = 0; + + ResetState(); + return red_packets; } int ProducerFec::Overhead() const { - // Overhead is defined as relative to the number of media packets, and not - // relative to total number of packets. This definition is inherited from the - // protection factor produced by video_coding module and how the FEC - // generation is implemented. RTC_DCHECK(!media_packets_.empty()); int num_fec_packets = fec_->NumFecPackets(media_packets_.size(), params_.fec_rate); @@ -259,8 +255,10 @@ int ProducerFec::Overhead() const { return (num_fec_packets << 8) / media_packets_.size(); } -void ProducerFec::DeleteMediaPackets() { +void ProducerFec::ResetState() { media_packets_.clear(); + generated_fec_packets_.clear(); + num_protected_frames_ = 0; } } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/producer_fec.h b/webrtc/modules/rtp_rtcp/source/producer_fec.h index 7ea07403a6..d52b4ed8f8 100644 --- a/webrtc/modules/rtp_rtcp/source/producer_fec.h +++ b/webrtc/modules/rtp_rtcp/source/producer_fec.h @@ -59,6 +59,28 @@ class ProducerFec { size_t payload_length, size_t rtp_header_length); + // Returns true if there are generated FEC packets available. + bool FecAvailable() const; + + size_t NumAvailableFecPackets() const; + + // Returns the overhead, per packet, for FEC (and possibly RED). + size_t MaxPacketOverhead() const; + + // Returns generated FEC packets with RED headers added. + std::vector> GetUlpfecPacketsAsRed( + int red_payload_type, + int ulpfec_payload_type, + uint16_t first_seq_num, + size_t rtp_header_length); + + private: + // Overhead is defined as relative to the number of media packets, and not + // relative to total number of packets. This definition is inherited from the + // protection factor produced by video_coding module and how the FEC + // generation is implemented. + int Overhead() const; + // Returns true if the excess overhead (actual - target) for the FEC is below // the amount |kMaxExcessOverhead|. This effects the lower protection level // cases and low number of media packets/frame. The target overhead is given @@ -72,23 +94,8 @@ class ProducerFec { // (e.g. (2k,2m) vs (k,m)) are generally more effective at recovering losses. bool MinimumMediaPacketsReached() const; - // Returns true if there are generated FEC packets available. - bool FecAvailable() const; + void ResetState(); - size_t NumAvailableFecPackets() const; - - size_t MaxPacketOverhead() const; - - // Returns generated FEC packets with RED headers added. - std::vector> GetFecPacketsAsRed( - int red_payload_type, - int ulpfec_payload_type, - uint16_t first_seq_num, - size_t rtp_header_length); - - private: - void DeleteMediaPackets(); - int Overhead() const; std::unique_ptr fec_; ForwardErrorCorrection::PacketList media_packets_; std::list generated_fec_packets_; diff --git a/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc b/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc index 31304b4213..d705bd0475 100644 --- a/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc @@ -94,8 +94,8 @@ TEST_F(ProducerFecTest, NoEmptyFecWithSeqNumGaps) { size_t num_fec_packets = producer_.NumAvailableFecPackets(); if (num_fec_packets > 0) { std::vector> fec_packets = - producer_.GetFecPacketsAsRed(kRedPayloadType, kFecPayloadType, 100, - p.header_size); + producer_.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, 100, + p.header_size); EXPECT_EQ(num_fec_packets, fec_packets.size()); } } @@ -123,8 +123,8 @@ TEST_F(ProducerFecTest, OneFrameFec) { EXPECT_TRUE(producer_.FecAvailable()); uint16_t seq_num = packet_generator_.NextPacketSeqNum(); std::vector> red_packets = - producer_.GetFecPacketsAsRed(kRedPayloadType, kFecPayloadType, seq_num, - kRtpHeaderSize); + producer_.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, seq_num, + kRtpHeaderSize); EXPECT_FALSE(producer_.FecAvailable()); ASSERT_EQ(1u, red_packets.size()); VerifyHeader(seq_num, last_timestamp, kRedPayloadType, kFecPayloadType, @@ -158,8 +158,8 @@ TEST_F(ProducerFecTest, TwoFrameFec) { EXPECT_TRUE(producer_.FecAvailable()); uint16_t seq_num = packet_generator_.NextPacketSeqNum(); std::vector> red_packets = - producer_.GetFecPacketsAsRed(kRedPayloadType, kFecPayloadType, seq_num, - kRtpHeaderSize); + producer_.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, seq_num, + kRtpHeaderSize); EXPECT_FALSE(producer_.FecAvailable()); ASSERT_EQ(1u, red_packets.size()); VerifyHeader(seq_num, last_timestamp, kRedPayloadType, kFecPayloadType, diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc index 9020f6a0fd..722a484a87 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -129,7 +129,7 @@ void RTPSenderVideo::SendVideoPacketAsRed( if (num_fec_packets > 0) { uint16_t first_fec_sequence_number = rtp_sender_->AllocateSequenceNumber(num_fec_packets); - fec_packets = producer_fec_.GetFecPacketsAsRed( + fec_packets = producer_fec_.GetUlpfecPacketsAsRed( red_payload_type_, fec_payload_type_, first_fec_sequence_number, media_packet->headers_size()); RTC_DCHECK_EQ(num_fec_packets, fec_packets.size()); diff --git a/webrtc/test/fuzzers/producer_fec_fuzzer.cc b/webrtc/test/fuzzers/producer_fec_fuzzer.cc index 3319138748..269b4a1cd6 100644 --- a/webrtc/test/fuzzers/producer_fec_fuzzer.cc +++ b/webrtc/test/fuzzers/producer_fec_fuzzer.cc @@ -53,8 +53,8 @@ void FuzzOneInput(const uint8_t* data, size_t size) { const size_t num_fec_packets = producer.NumAvailableFecPackets(); if (num_fec_packets > 0) { std::vector> fec_packets = - producer.GetFecPacketsAsRed(kRedPayloadType, kFecPayloadType, 100, - rtp_header_length); + producer.GetUlpfecPacketsAsRed(kRedPayloadType, kFecPayloadType, 100, + rtp_header_length); RTC_CHECK_EQ(num_fec_packets, fec_packets.size()); } }