Style updates for ForwardErrorCorrection and related classes

webrtc/modules/rtp_rtcp/source/forward_error_correction.cc

Index: webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
index 623c658a17401b877bd6748471f1e7ed8b9f3552..7b10bc0a345239022cb5c26e8cf5258899525a78 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -37,7 +37,8 @@ const uint8_t kUlpHeaderSizeLBitClear = (2 + kMaskSizeLBitClear);
 // Transport header size in bytes. Assume UDP/IPv4 as a reasonable minimum.
 const uint8_t kTransportOverhead = 28;
 
-enum { kMaxFecPackets = ForwardErrorCorrection::kMaxMediaPackets };
+// Maximum number of FEC packets stored internally.
+const uint8_t kMaxFecPackets = ForwardErrorCorrection::kMaxMediaPackets;

[danilchap, 2016/06/29 10:31:52]

prefer size_t for this variable.

[brandtr, 2016/06/29 14:24:10]

Done.

Would it make sense to change all of these constants to constexpr as well?

[danilchap, 2016/06/29 15:18:24]

For new code should use constexpr, for old code like this one - up to you.
Same for moving this constants into unnamed namespace.

 
 int32_t ForwardErrorCorrection::Packet::AddRef() {
   return ++ref_count_;
@@ -85,8 +86,7 @@ ForwardErrorCorrection::RecoveredPacket::RecoveredPacket() {}
 ForwardErrorCorrection::RecoveredPacket::~RecoveredPacket() {}
 
 ForwardErrorCorrection::ForwardErrorCorrection()
-    : generated_fec_packets_(kMaxMediaPackets), fec_packet_received_(false) {}
-
+    : generated_fec_packets_(kMaxMediaPackets) {}
 ForwardErrorCorrection::~ForwardErrorCorrection() {}
 
 // Input packet
@@ -106,6 +106,9 @@ ForwardErrorCorrection::~ForwardErrorCorrection() {}
 //   |                     FEC Level 0 Payload                       |
 //   |                                                               |
 //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//
+// N.B. that any potential RED headers are added/removed before calling

[danilchap, 2016/06/29 10:31:53]

what is N.B. ?

[brandtr, 2016/06/29 14:24:10]

"Nota Bene" == "note well" in latin. It's a bit academese, so I changed it to
"Note".

+// GenerateFEC or DecodeFEC.
 int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
                                             uint8_t protection_factor,
                                             int num_important_packets,
@@ -182,10 +185,10 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
     num_mask_bytes = kMaskSizeLBitSet;
   }
 
-  GenerateFecBitStrings(media_packet_list, packet_mask.get(), num_fec_packets,
-                        l_bit);
-  GenerateFecUlpHeaders(media_packet_list, packet_mask.get(), l_bit,
-                        num_fec_packets);
+  GenerateFecBitStrings(media_packet_list, packet_mask.get(),
+                        num_fec_packets, l_bit);
+  GenerateFecUlpHeaders(media_packet_list, packet_mask.get(),
+                        num_fec_packets, l_bit);
 
   return 0;
 }
@@ -207,9 +210,7 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
     uint8_t* packet_mask,
     int num_fec_packets,
     bool l_bit) {
-  if (media_packet_list.empty()) {
-    return;
-  }
+  assert(!media_packet_list.empty());

[danilchap, 2016/06/29 10:31:52]

use RTC_DCHECK instead of assert

[brandtr, 2016/06/29 14:24:10]

Done.

   uint8_t media_payload_length[2];
   const int num_mask_bytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
   const uint16_t ulp_header_size =
@@ -219,7 +220,7 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
 
   for (int i = 0; i < num_fec_packets; ++i) {
     Packet* const fec_packet = &generated_fec_packets_[i];
-    PacketList::const_iterator media_list_it = media_packet_list.begin();
+    auto media_list_it = media_packet_list.cbegin();
     uint32_t pkt_mask_idx = i * num_mask_bytes;
     uint32_t media_pkt_idx = 0;
     uint16_t fec_packet_length = 0;
@@ -237,9 +238,10 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
         fec_packet_length = media_packet->length + fec_rtp_offset;
         // On the first protected packet, we don't need to XOR.
         if (fec_packet->length == 0) {
-          // Copy the first 2 bytes of the RTP header.
-          memcpy(fec_packet->data, media_packet->data, 2);
-          // Copy the 5th to 8th bytes of the RTP header.
+          // Copy the first 2 bytes of the RTP header. Note that the E and L
+          // bits are overwritten in GenerateFecUlpHeaders.
+          memcpy(&fec_packet->data[0], &media_packet->data[0], 2);
+          // Copy the 5th to 8th bytes of the RTP header (timestamp).
           memcpy(&fec_packet->data[4], &media_packet->data[4], 4);
           // Copy network-ordered payload size.
           memcpy(&fec_packet->data[8], media_payload_length, 2);
@@ -291,7 +293,6 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
     uint8_t* packet_mask,
     int num_mask_bytes,
     int num_fec_packets) {
-  uint8_t* new_mask = NULL;
   if (media_packets.size() <= 1) {
     return media_packets.size();
   }
@@ -313,15 +314,16 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
   if (media_packets.size() + total_missing_seq_nums > 8 * kMaskSizeLBitClear) {
     new_mask_bytes = kMaskSizeLBitSet;
   }
-  new_mask = new uint8_t[num_fec_packets * kMaskSizeLBitSet];
-  memset(new_mask, 0, num_fec_packets * kMaskSizeLBitSet);
+  std::unique_ptr<uint8_t[]> new_mask(
+      new uint8_t[num_fec_packets * kMaskSizeLBitSet]);
+  memset(new_mask.get(), 0, num_fec_packets * kMaskSizeLBitSet);
 
-  PacketList::const_iterator it = media_packets.begin();
+  auto it = media_packets.cbegin();
   uint16_t prev_seq_num = first_seq_num;
   ++it;
 
   // Insert the first column.
-  CopyColumn(new_mask, new_mask_bytes, packet_mask, num_mask_bytes,
+  CopyColumn(new_mask.get(), new_mask_bytes, packet_mask, num_mask_bytes,
              num_fec_packets, 0, 0);
   int new_bit_index = 1;
   int old_bit_index = 1;
@@ -335,11 +337,11 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
     const int zeros_to_insert =
         static_cast<uint16_t>(seq_num - prev_seq_num - 1);
     if (zeros_to_insert > 0) {
-      InsertZeroColumns(zeros_to_insert, new_mask, new_mask_bytes,
+      InsertZeroColumns(zeros_to_insert, new_mask.get(), new_mask_bytes,
                         num_fec_packets, new_bit_index);
     }
     new_bit_index += zeros_to_insert;
-    CopyColumn(new_mask, new_mask_bytes, packet_mask, num_mask_bytes,
+    CopyColumn(new_mask.get(), new_mask_bytes, packet_mask, num_mask_bytes,
                num_fec_packets, new_bit_index, old_bit_index);
     ++new_bit_index;
     ++old_bit_index;
@@ -353,8 +355,7 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
     }
   }
   // Replace the old mask with the new.
-  memcpy(packet_mask, new_mask, kMaskSizeLBitSet * num_fec_packets);
-  delete[] new_mask;
+  memcpy(packet_mask, new_mask.get(), kMaskSizeLBitSet * num_fec_packets);
   return new_bit_index;
 }
 
@@ -393,8 +394,8 @@ void ForwardErrorCorrection::CopyColumn(uint8_t* new_mask,
 void ForwardErrorCorrection::GenerateFecUlpHeaders(
     const PacketList& media_packet_list,
     uint8_t* packet_mask,
-    bool l_bit,
-    int num_fec_packets) {
+    int num_fec_packets,
+    bool l_bit) {
   // -- Generate FEC and ULP headers --
   //
   // FEC Header, 10 bytes
@@ -416,9 +417,9 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   //   |              mask cont. (present only when L = 1)             |
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-  PacketList::const_iterator media_list_it = media_packet_list.begin();
-  Packet* media_packet = *media_list_it;
-  assert(media_packet != NULL);
+  Packet* first_media_packet = media_packet_list.front();

[danilchap, 2016/06/29 10:31:53]

may be add RTC_DCHECK(!media_packet_list.empty()) before this line.

[brandtr, 2016/06/29 14:24:10]

Done.

+  assert(first_media_packet != NULL);

[danilchap, 2016/06/29 10:31:52]

RTC_DCHECK(first_media_packet);

[brandtr, 2016/06/29 14:24:10]

Done.

+  uint16_t seq_num = ParseSequenceNumber(first_media_packet->data);
   int num_mask_bytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
   const uint16_t ulp_header_size =
       l_bit ? kUlpHeaderSizeLBitSet : kUlpHeaderSizeLBitClear;
@@ -432,10 +433,10 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
     } else {
       fec_packet->data[0] |= 0x40;  // Set the L bit.
     }
-    // Two byte sequence number from first RTP packet to SN base.
+    // Sequence number from first media packet used as SN base.
     // We use the same sequence number base for every FEC packet,
     // but that's not required in general.
-    memcpy(&fec_packet->data[2], &media_packet->data[2], 2);
+    ByteWriter<uint16_t>::WriteBigEndian(&fec_packet->data[2], seq_num);
 
     // -- ULP header --
     // Copy the payload size to the protection length field.
@@ -452,8 +453,6 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
 
 void ForwardErrorCorrection::ResetState(
     RecoveredPacketList* recovered_packet_list) {
-  fec_packet_received_ = false;
-
   // Free the memory for any existing recovered packets, if the user hasn't.
   while (!recovered_packet_list->empty()) {
     delete recovered_packet_list->front();
@@ -463,10 +462,9 @@ void ForwardErrorCorrection::ResetState(
 
   // Free the FEC packet list.
   while (!fec_packet_list_.empty()) {
-    FecPacketList::iterator fec_packet_list_it = fec_packet_list_.begin();
+    auto fec_packet_list_it = fec_packet_list_.cbegin();
     FecPacket* fec_packet = *fec_packet_list_it;

[danilchap, 2016/06/29 10:31:52]

may be reduce a bit futher:
FecPacket* fec_packet = fec_packet_list_.front();

[brandtr, 2016/06/29 14:24:10]

Done.

-    ProtectedPacketList::iterator protected_packet_list_it;
-    protected_packet_list_it = fec_packet->protected_pkt_list.begin();
+    auto protected_packet_list_it = fec_packet->protected_pkt_list.begin();
     while (protected_packet_list_it != fec_packet->protected_pkt_list.end()) {
       delete *protected_packet_list_it;
       protected_packet_list_it =
@@ -482,8 +480,7 @@ void ForwardErrorCorrection::ResetState(
 void ForwardErrorCorrection::InsertMediaPacket(
     ReceivedPacket* rx_packet,
     RecoveredPacketList* recovered_packet_list) {
-  RecoveredPacketList::iterator recovered_packet_list_it =
-      recovered_packet_list->begin();
+  auto recovered_packet_list_it = recovered_packet_list->cbegin();
 
   // Search for duplicate packets.
   while (recovered_packet_list_it != recovered_packet_list->end()) {
@@ -493,28 +490,28 @@ void ForwardErrorCorrection::InsertMediaPacket(
       rx_packet->pkt = NULL;
       return;
     }
-    recovered_packet_list_it++;
+    ++recovered_packet_list_it;
   }
-  RecoveredPacket* recoverd_packet_to_insert = new RecoveredPacket;
-  recoverd_packet_to_insert->was_recovered = false;
+  RecoveredPacket* recovered_packet_to_insert = new RecoveredPacket;
+  recovered_packet_to_insert->was_recovered = false;
   // Inserted Media packet is already sent to VCM.
-  recoverd_packet_to_insert->returned = true;
-  recoverd_packet_to_insert->seq_num = rx_packet->seq_num;
-  recoverd_packet_to_insert->pkt = rx_packet->pkt;
-  recoverd_packet_to_insert->pkt->length = rx_packet->pkt->length;
+  recovered_packet_to_insert->returned = true;
+  recovered_packet_to_insert->seq_num = rx_packet->seq_num;
+  recovered_packet_to_insert->pkt = rx_packet->pkt;
+  recovered_packet_to_insert->pkt->length = rx_packet->pkt->length;
 
   // TODO(holmer): Consider replacing this with a binary search for the right
   // position, and then just insert the new packet. Would get rid of the sort.
-  recovered_packet_list->push_back(recoverd_packet_to_insert);
+  recovered_packet_list->push_back(recovered_packet_to_insert);
   recovered_packet_list->sort(SortablePacket::LessThan);
-  UpdateCoveringFECPackets(recoverd_packet_to_insert);
+  UpdateCoveringFECPackets(recovered_packet_to_insert);
 }
 
 void ForwardErrorCorrection::UpdateCoveringFECPackets(RecoveredPacket* packet) {
-  for (FecPacketList::iterator it = fec_packet_list_.begin();
-       it != fec_packet_list_.end(); ++it) {
+  for (auto it = fec_packet_list_.cbegin();

[danilchap, 2016/06/29 10:31:52]

this loop seems better to reduce to c++11 loop:
for (auto* fec_packet : fec_packet_list_) {
 s/(*it)/fec_packet/
}

[brandtr, 2016/06/29 14:24:10]

Done.

+        it != fec_packet_list_.end(); ++it) {
     // Is this FEC packet protecting the media packet |packet|?
-    ProtectedPacketList::iterator protected_it = std::lower_bound(
+    auto protected_it = std::lower_bound(
         (*it)->protected_pkt_list.begin(), (*it)->protected_pkt_list.end(),
         packet, SortablePacket::LessThan);
     if (protected_it != (*it)->protected_pkt_list.end() &&
@@ -528,17 +525,15 @@ void ForwardErrorCorrection::UpdateCoveringFECPackets(RecoveredPacket* packet) {
 void ForwardErrorCorrection::InsertFECPacket(
     ReceivedPacket* rx_packet,
     const RecoveredPacketList* recovered_packet_list) {
-  fec_packet_received_ = true;
-
   // Check for duplicate.
-  FecPacketList::iterator fec_packet_list_it = fec_packet_list_.begin();
+  auto fec_packet_list_it = fec_packet_list_.begin();
   while (fec_packet_list_it != fec_packet_list_.end()) {

[danilchap, 2016/06/29 10:31:52]

reduce this one to c++11 loop too.

[brandtr, 2016/06/29 14:24:10]

Done.

     if (rx_packet->seq_num == (*fec_packet_list_it)->seq_num) {
       // Delete duplicate FEC packet data.
       rx_packet->pkt = NULL;
       return;
     }
-    fec_packet_list_it++;
+    ++fec_packet_list_it;
   }
   FecPacket* fec_packet = new FecPacket;
   fec_packet->pkt = rx_packet->pkt;
@@ -590,14 +585,14 @@ void ForwardErrorCorrection::AssignRecoveredPackets(
   ProtectedPacketList* not_recovered = &fec_packet->protected_pkt_list;
   RecoveredPacketList already_recovered;
   std::set_intersection(
-      recovered_packets->begin(), recovered_packets->end(),
-      not_recovered->begin(), not_recovered->end(),
+      recovered_packets->cbegin(), recovered_packets->cend(),
+      not_recovered->cbegin(), not_recovered->cend(),
       std::inserter(already_recovered, already_recovered.end()),
       SortablePacket::LessThan);
   // Set the FEC pointers to all recovered packets so that we don't have to
   // search for them when we are doing recovery.
-  ProtectedPacketList::iterator not_recovered_it = not_recovered->begin();
-  for (RecoveredPacketList::iterator it = already_recovered.begin();
+  auto not_recovered_it = not_recovered->cbegin();
+  for (auto it = already_recovered.cbegin();
        it != already_recovered.end(); ++it) {
     // Search for the next recovered packet in |not_recovered|.
     while ((*not_recovered_it)->seq_num != (*it)->seq_num)
@@ -643,8 +638,8 @@ void ForwardErrorCorrection::InsertPackets(
   DiscardOldPackets(recovered_packet_list);
 }
 
-bool ForwardErrorCorrection::InitRecovery(const FecPacket* fec_packet,
-                                          RecoveredPacket* recovered) {
+bool ForwardErrorCorrection::InitRecoveryOfPacket(const FecPacket* fec_packet,
+                                                  RecoveredPacket* recovered) {
   // This is the first packet which we try to recover with.
   const uint16_t ulp_header_size = fec_packet->pkt->data[0] & 0x40
                                        ? kUlpHeaderSizeLBitSet
@@ -684,7 +679,8 @@ bool ForwardErrorCorrection::InitRecovery(const FecPacket* fec_packet,
   return true;
 }
 
-bool ForwardErrorCorrection::FinishRecovery(RecoveredPacket* recovered) {
+bool ForwardErrorCorrection::FinishRecoveryOfPacket(
+    RecoveredPacket* recovered) {
   // Set the RTP version to 2.
   recovered->pkt->data[0] |= 0x80;  // Set the 1st bit.
   recovered->pkt->data[0] &= 0xbf;  // Clear the 2nd bit.
@@ -729,10 +725,9 @@ void ForwardErrorCorrection::XorPackets(const Packet* src_packet,
 bool ForwardErrorCorrection::RecoverPacket(
     const FecPacket* fec_packet,
     RecoveredPacket* rec_packet_to_insert) {
-  if (!InitRecovery(fec_packet, rec_packet_to_insert))
+  if (!InitRecoveryOfPacket(fec_packet, rec_packet_to_insert))
     return false;
-  ProtectedPacketList::const_iterator protected_it =
-      fec_packet->protected_pkt_list.begin();
+  auto protected_it = fec_packet->protected_pkt_list.cbegin();

[danilchap, 2016/06/29 10:31:52]

ditto

[brandtr, 2016/06/29 14:24:10]

Done.

   while (protected_it != fec_packet->protected_pkt_list.end()) {
     if ((*protected_it)->pkt == NULL) {
       // This is the packet we're recovering.
@@ -742,14 +737,14 @@ bool ForwardErrorCorrection::RecoverPacket(
     }
     ++protected_it;
   }
-  if (!FinishRecovery(rec_packet_to_insert))
+  if (!FinishRecoveryOfPacket(rec_packet_to_insert))
     return false;
   return true;
 }
 
 void ForwardErrorCorrection::AttemptRecover(
     RecoveredPacketList* recovered_packet_list) {
-  FecPacketList::iterator fec_packet_list_it = fec_packet_list_.begin();
+  auto fec_packet_list_it = fec_packet_list_.begin();
   while (fec_packet_list_it != fec_packet_list_.end()) {
     // Search for each FEC packet's protected media packets.
     int packets_missing = NumCoveredPacketsMissing(*fec_packet_list_it);
@@ -797,8 +792,7 @@ void ForwardErrorCorrection::AttemptRecover(
 int ForwardErrorCorrection::NumCoveredPacketsMissing(
     const FecPacket* fec_packet) {
   int packets_missing = 0;
-  ProtectedPacketList::const_iterator it =
-      fec_packet->protected_pkt_list.begin();
+  auto it = fec_packet->protected_pkt_list.cbegin();

[danilchap, 2016/06/29 10:31:52]

ditto

[brandtr, 2016/06/29 14:24:10]

Done.

   for (; it != fec_packet->protected_pkt_list.end(); ++it) {
     if ((*it)->pkt == NULL) {
       ++packets_missing;