Style updates to ProducerFec/FecReceiver.

webrtc/modules/rtp_rtcp/source/producer_fec.cc

Index: webrtc/modules/rtp_rtcp/source/producer_fec.cc
diff --git a/webrtc/modules/rtp_rtcp/source/producer_fec.cc b/webrtc/modules/rtp_rtcp/source/producer_fec.cc
index 2e1b220f01dcd973d90d78cb0b613bf99ef08ce7..f4a3563f308961fd71821d2ae73adc5cdd1d8e25 100644
--- a/webrtc/modules/rtp_rtcp/source/producer_fec.cc
+++ b/webrtc/modules/rtp_rtcp/source/producer_fec.cc
@@ -8,29 +8,35 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "webrtc/modules/rtp_rtcp/source/producer_fec.h"
+#include "webrtc/base/basictypes.h"

[philipel, 2016/07/04 15:35:04]

Should be with the rest of the webrtc includes, in alphabetic order.

[brandtr, 2016/07/05 11:46:05]

Done.

+#include "webrtc/base/checks.h"
 
+#include "webrtc/modules/rtp_rtcp/source/producer_fec.h"

[philipel, 2016/07/04 15:35:04]

Should be on top, before basictypes.h

[brandtr, 2016/07/05 11:46:05]

Done.

 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
 
 namespace webrtc {
 
-enum { kREDForFECHeaderLength = 1 };
+constexpr size_t kREDForFECHeaderLength = 1;

[philipel, 2016/07/04 15:35:04]

kRedForFecHeaderLength

[brandtr, 2016/07/05 11:46:05]

Done.

+
 // This controls the maximum amount of excess overhead (actual - target)
 // allowed in order to trigger GenerateFec(), before |params_.max_fec_frames|
 // is reached. Overhead here is defined as relative to number of media packets.
-enum { kMaxExcessOverhead = 50 };  // Q8.
+constexpr int kMaxExcessOverhead = 50;  // Q8.
+
 // This is the minimum number of media packets required (above some protection
 // level) in order to trigger GenerateFec(), before |params_.max_fec_frames| is
 // reached.
-enum { kMinimumMediaPackets = 4 };
+constexpr size_t kMinimumMediaPackets = 4;
+
 // Threshold on the received FEC protection level, above which we enforce at
 // least |kMinimumMediaPackets| packets for the FEC code. Below this
 // threshold |kMinimumMediaPackets| is set to default value of 1.
-enum { kHighProtectionThreshold = 80 };  // Corresponds to ~30 overhead, range
-// is 0 to 255, where 255 corresponds to 100% overhead (relative to number of
-// media packets).
+//
+// The range is between 0 and 255, where 255 corresponds to 100% overhead
+// (relative to the number of protected media packets).
+constexpr uint8_t kHighProtectionThreshold = 80;
 
 RedPacket::RedPacket(size_t length)
     : data_(new uint8_t[length]),
@@ -38,32 +44,31 @@ RedPacket::RedPacket(size_t length)
       header_length_(0) {
 }
 
-RedPacket::~RedPacket() {
-  delete [] data_;
-}
+RedPacket::~RedPacket() = default;

[philipel, 2016/07/04 15:35:04]

Remove destructor.

[brandtr, 2016/07/05 11:46:05]

Done.

 
 void RedPacket::CreateHeader(const uint8_t* rtp_header, size_t header_length,
-                             int red_pl_type, int pl_type) {
-  assert(header_length + kREDForFECHeaderLength <= length_);
-  memcpy(data_, rtp_header, header_length);
+                             int red_payload_type, int payload_type) {
+  RTC_DCHECK_LT(header_length + kREDForFECHeaderLength, length_);
+  memcpy(data_.get(), rtp_header, header_length);
   // Replace payload type.
   data_[1] &= 0x80;
-  data_[1] += red_pl_type;
+  data_[1] += red_payload_type;
   // Add RED header
   // f-bit always 0
-  data_[header_length] = static_cast<uint8_t>(pl_type);
+  data_[header_length] = static_cast<uint8_t>(payload_type);
   header_length_ = header_length + kREDForFECHeaderLength;
 }
 
 void RedPacket::SetSeqNum(int seq_num) {
-  assert(seq_num >= 0 && seq_num < (1<<16));
+  RTC_DCHECK_GE(seq_num, 0);
+  RTC_DCHECK_LT(seq_num, 1<<16);
 
   ByteWriter<uint16_t>::WriteBigEndian(&data_[2], seq_num);
 }
 
 void RedPacket::AssignPayload(const uint8_t* payload, size_t length) {
-  assert(header_length_ + length <= length_);
-  memcpy(data_ + header_length_, payload, length);
+  RTC_DCHECK_LE(header_length_ + length, length_);
+  memcpy(data_.get() + header_length_, payload, length);
 }
 
 void RedPacket::ClearMarkerBit() {
@@ -71,7 +76,7 @@ void RedPacket::ClearMarkerBit() {
 }
 
 uint8_t* RedPacket::data() const {
-  return data_;
+  return data_.get();
 }
 
 size_t RedPacket::length() const {
@@ -80,178 +85,170 @@ size_t RedPacket::length() const {
 
 ProducerFec::ProducerFec(ForwardErrorCorrection* fec)
     : fec_(fec),
-      media_packets_fec_(),
-      fec_packets_(),
-      num_frames_(0),
-      num_first_partition_(0),
-      minimum_media_packets_fec_(1),
+      media_packets_(),
+      generated_fec_packets_(),
+      num_frames_protected_(0),
+      num_important_packets_(0),
+      minimum_num_media_packets_(1),
       params_(),
       new_params_() {
   memset(&params_, 0, sizeof(params_));
   memset(&new_params_, 0, sizeof(new_params_));
 }
 
-ProducerFec::~ProducerFec() {
-  DeletePackets();
+ProducerFec::~ProducerFec() = default;

[philipel, 2016/07/04 15:35:04]

Remove destructor.

[brandtr, 2016/07/05 11:46:05]

Done.

+
+std::unique_ptr<RedPacket> ProducerFec::BuildRedPacket(
+    const uint8_t* data_buffer,
+    size_t payload_length,
+    size_t rtp_header_length,
+    int red_payload_type) {
+  std::unique_ptr<RedPacket> red_packet(
+      new RedPacket(payload_length + kREDForFECHeaderLength +
+                    rtp_header_length));
+  int payload_type = data_buffer[1] & 0x7f;
+  red_packet->CreateHeader(data_buffer, rtp_header_length,
+                           red_payload_type, payload_type);
+  red_packet->AssignPayload(data_buffer + rtp_header_length, payload_length);
+  return red_packet;
 }
 
 void ProducerFec::SetFecParameters(const FecProtectionParams* params,
-                                   int num_first_partition) {
-  // Number of first partition packets cannot exceed kMaxMediaPackets
-  assert(params->fec_rate >= 0 && params->fec_rate < 256);
-  if (num_first_partition >
+                                   int num_important_packets) {
+  // Number of important packets (in unequal protection parlance) cannot

[philipel, 2016/07/04 15:35:04]

Wut? Is parlence some established term when it comes to FEC?

[brandtr, 2016/07/05 11:46:05]

Rephrased :)

+  // exceed kMaxMediaPackets.
+  RTC_DCHECK_GE(params->fec_rate, 0);
+  RTC_DCHECK_LE(params->fec_rate, 255);
+  if (num_important_packets >
       static_cast<int>(ForwardErrorCorrection::kMaxMediaPackets)) {
-      num_first_partition =
+      num_important_packets =
           ForwardErrorCorrection::kMaxMediaPackets;
   }
   // Store the new params and apply them for the next set of FEC packets being
   // produced.
   new_params_ = *params;
-  num_first_partition_ = num_first_partition;
+  num_important_packets_ = num_important_packets;
   if (params->fec_rate > kHighProtectionThreshold) {
-    minimum_media_packets_fec_ = kMinimumMediaPackets;
+    minimum_num_media_packets_ = kMinimumMediaPackets;
   } else {
-    minimum_media_packets_fec_ = 1;
+    minimum_num_media_packets_ = 1;
   }
 }
 
-RedPacket* ProducerFec::BuildRedPacket(const uint8_t* data_buffer,
-                                       size_t payload_length,
-                                       size_t rtp_header_length,
-                                       int red_pl_type) {
-  RedPacket* red_packet = new RedPacket(
-      payload_length + kREDForFECHeaderLength + rtp_header_length);
-  int pl_type = data_buffer[1] & 0x7f;
-  red_packet->CreateHeader(data_buffer, rtp_header_length,
-                           red_pl_type, pl_type);
-  red_packet->AssignPayload(data_buffer + rtp_header_length, payload_length);
-  return red_packet;
-}
-
 int ProducerFec::AddRtpPacketAndGenerateFec(const uint8_t* data_buffer,
                                             size_t payload_length,
                                             size_t rtp_header_length) {
-  assert(fec_packets_.empty());
-  if (media_packets_fec_.empty()) {
+  RTC_DCHECK(generated_fec_packets_.empty());
+  if (media_packets_.empty()) {
     params_ = new_params_;
   }
   bool complete_frame = false;
   const bool marker_bit = (data_buffer[1] & kRtpMarkerBitMask) ? true : false;
-  if (media_packets_fec_.size() < ForwardErrorCorrection::kMaxMediaPackets) {
-    // Generic FEC can only protect up to kMaxMediaPackets packets.
+  if (media_packets_.size() < ForwardErrorCorrection::kMaxMediaPackets) {
+    // Generic FEC can only protect up to |kMaxMediaPackets| packets.
     std::unique_ptr<ForwardErrorCorrection::Packet> packet(
         new ForwardErrorCorrection::Packet());
     packet->length = payload_length + rtp_header_length;
     memcpy(packet->data, data_buffer, packet->length);
-    media_packets_fec_.push_back(std::move(packet));
+    media_packets_.push_back(std::move(packet));
   }
   if (marker_bit) {
-    ++num_frames_;
+    ++num_frames_protected_;
     complete_frame = true;
   }
   // Produce FEC over at most |params_.max_fec_frames| frames, or as soon as:
   // (1) the excess overhead (actual overhead - requested/target overhead) is
   // less than |kMaxExcessOverhead|, and
-  // (2) at least |minimum_media_packets_fec_| media packets is reached.
+  // (2) at least |minimum_num_media_packets_| media packets is reached.
   if (complete_frame &&
-      (num_frames_ == params_.max_fec_frames ||
+      (num_frames_protected_ == params_.max_fec_frames ||
           (ExcessOverheadBelowMax() && MinimumMediaPacketsReached()))) {
-    assert(num_first_partition_ <=
-           static_cast<int>(ForwardErrorCorrection::kMaxMediaPackets));
+    RTC_DCHECK_LE(num_important_packets_,
+                  static_cast<int>(ForwardErrorCorrection::kMaxMediaPackets));
     // TODO(pbos): Consider whether unequal protection should be enabled or not,
     // it is currently always disabled.
-    int ret = fec_->GenerateFec(media_packets_fec_, params_.fec_rate,
-                                num_first_partition_, false,
-                                params_.fec_mask_type, &fec_packets_);
-    if (fec_packets_.empty()) {
-      num_frames_ = 0;
-      DeletePackets();
+    const bool kUseUnequalProtection = false;
+    // TODO(brandtr): The fact above means that the value of
+    // |num_important_packets_| has no importance when calling GenerateFec().
+    int ret = fec_->GenerateFec(media_packets_, params_.fec_rate,
+                                num_important_packets_, kUseUnequalProtection,
+                                params_.fec_mask_type, &generated_fec_packets_);
+    if (generated_fec_packets_.empty()) {
+      num_frames_protected_ = 0;
+      media_packets_.clear();
     }
     return ret;
   }
   return 0;
 }
 
-// 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 by
-// |params_.fec_rate|, and is only achievable in the limit of large number of
-// media packets.
-bool ProducerFec::ExcessOverheadBelowMax() {
+bool ProducerFec::ExcessOverheadBelowMax() const {
   return ((Overhead() - params_.fec_rate) < kMaxExcessOverhead);
 }
 
-// Returns true if the media packet list for the FEC is at least
-// |minimum_media_packets_fec_|. This condition tries to capture the effect
-// that, for the same amount of protection/overhead, longer codes
-// (e.g. (2k,2m) vs (k,m)) are generally more effective at recovering losses.
 bool ProducerFec::MinimumMediaPacketsReached() {
-  float avg_num_packets_frame = static_cast<float>(media_packets_fec_.size()) /
-                                num_frames_;
-  if (avg_num_packets_frame < 2.0f) {
-  return (static_cast<int>(media_packets_fec_.size()) >=
-      minimum_media_packets_fec_);
+  float num_packets = static_cast<float>(media_packets_.size());
+  float average_num_packets_frame_per_frame = num_packets/num_frames_protected_;
+  if (average_num_packets_frame_per_frame < 2.0f) {
+    return (num_packets >= minimum_num_media_packets_);
   } else {
     // For larger rates (more packets/frame), increase the threshold.
-    return (static_cast<int>(media_packets_fec_.size()) >=
-        minimum_media_packets_fec_ + 1);
+    return (num_packets >= minimum_num_media_packets_ + 1);
   }
 }
 
 bool ProducerFec::FecAvailable() const {
-  return !fec_packets_.empty();
+  return !generated_fec_packets_.empty();
 }
 
 size_t ProducerFec::NumAvailableFecPackets() const {
-  return fec_packets_.size();
-}
-
-std::vector<RedPacket*> ProducerFec::GetFecPackets(int red_pl_type,
-                                                   int fec_pl_type,
-                                                   uint16_t first_seq_num,
-                                                   size_t rtp_header_length) {
-  std::vector<RedPacket*> fec_packets;
-  fec_packets.reserve(fec_packets_.size());
-  uint16_t sequence_number = first_seq_num;
-  while (!fec_packets_.empty()) {
-    // Build FEC packet. The FEC packets in |fec_packets_| doesn't
-    // have RTP headers, so we're reusing the header from the last
-    // media packet.
-    ForwardErrorCorrection::Packet* packet_to_send = fec_packets_.front();
-    ForwardErrorCorrection::Packet* last_media_packet =
-        media_packets_fec_.back().get();
-
-    RedPacket* red_packet = new RedPacket(
-        packet_to_send->length + kREDForFECHeaderLength + rtp_header_length);
+  return generated_fec_packets_.size();
+}
+
+std::vector<std::unique_ptr<RedPacket>> ProducerFec::GetFecPacketsAsRed(
+    int red_payload_type,
+    int ulpfec_payload_type,
+    uint16_t first_seq_num,
+    size_t rtp_header_length) {
+  std::vector<std::unique_ptr<RedPacket>> red_packets;
+  red_packets.reserve(generated_fec_packets_.size());
+  RTC_DCHECK_GT(media_packets_.size(), static_cast<size_t>(0));

[philipel, 2016/07/04 15:35:04]

RTC_DCHECK(!media_packets_.empty())

[brandtr, 2016/07/05 11:46:05]

Ugh, this unsightly thing was my bad.

+  ForwardErrorCorrection::Packet* last_media_packet =
+      media_packets_.back().get();
+  uint16_t seq_num = first_seq_num;
+  for (const auto& fec_packet : generated_fec_packets_) {
+    // 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<RedPacket> red_packet(new RedPacket(
+        fec_packet->length + kREDForFECHeaderLength + rtp_header_length));
     red_packet->CreateHeader(last_media_packet->data, rtp_header_length,
-                             red_pl_type, fec_pl_type);
-    red_packet->SetSeqNum(sequence_number++);
+                             red_payload_type, ulpfec_payload_type);
+    red_packet->SetSeqNum(seq_num++);
     red_packet->ClearMarkerBit();
-    red_packet->AssignPayload(packet_to_send->data, packet_to_send->length);
+    red_packet->AssignPayload(fec_packet->data, fec_packet->length);
 
-    fec_packets.push_back(red_packet);
-
-    fec_packets_.pop_front();
+    red_packets.push_back(std::move(red_packet));
   }
-  DeletePackets();
-  num_frames_ = 0;
-  return fec_packets;
+
+  // Reset state.
+  media_packets_.clear();
+  generated_fec_packets_.clear();
+  num_frames_protected_ = 0;
+
+  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 inhereted from the
+  // 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.
-  assert(!media_packets_fec_.empty());
-  int num_fec_packets = fec_->GetNumberOfFecPackets(media_packets_fec_.size(),
+  RTC_DCHECK(!media_packets_.empty());
+  int num_fec_packets = fec_->GetNumberOfFecPackets(media_packets_.size(),
                                                     params_.fec_rate);
   // Return the overhead in Q8.
-  return (num_fec_packets << 8) / media_packets_fec_.size();
-}
-
-void ProducerFec::DeletePackets() {
-  media_packets_fec_.clear();
+  return (num_fec_packets << 8) / media_packets_.size();
 }
 
 }  // namespace webrtc