Use std::unique_ptr in ForwardErrorCorrection.

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 a8f1bf8731a5697616552f6287b15817bc74b3db..e8d50fbd082b45402e49a46d52513767f3fff1fa 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -51,41 +51,20 @@ int32_t ForwardErrorCorrection::Packet::Release() {
   return ref_count;
 }
 
-// Used to link media packets to their protecting FEC packets.
-//
-// TODO(holmer): Refactor into a proper class.
-class ProtectedPacket : public ForwardErrorCorrection::SortablePacket {
- public:
-  rtc::scoped_refptr<ForwardErrorCorrection::Packet> pkt;
-};
-
-typedef std::list<ProtectedPacket*> ProtectedPacketList;
-
-//
-// Used for internal storage of FEC packets in a list.
-//
-// TODO(holmer): Refactor into a proper class.
-class FecPacket : public ForwardErrorCorrection::SortablePacket {
- public:
-  ProtectedPacketList protected_pkt_list;
-  uint32_t ssrc;  // SSRC of the current frame.
-  rtc::scoped_refptr<ForwardErrorCorrection::Packet> pkt;
-};
-
-bool ForwardErrorCorrection::SortablePacket::LessThan(
-    const SortablePacket* first,
-    const SortablePacket* second) {
+// This comparator is used to compare std::unique_ptr's pointing to
+// subclasses of SortablePackets. It needs to be parametric since
+// the std::unique_ptr's are not covariant w.r.t. the types that
+// they are pointing to.
+template <typename S, typename T>
+bool ForwardErrorCorrection::SortablePacket::LessThan::operator() (
+    const S& first,
+    const T& second) {
   return IsNewerSequenceNumber(second->seq_num, first->seq_num);
 }
 
-ForwardErrorCorrection::ReceivedPacket::ReceivedPacket() {}
-ForwardErrorCorrection::ReceivedPacket::~ReceivedPacket() {}
-
-ForwardErrorCorrection::RecoveredPacket::RecoveredPacket() {}
-ForwardErrorCorrection::RecoveredPacket::~RecoveredPacket() {}
 
 ForwardErrorCorrection::ForwardErrorCorrection()
-    : generated_fec_packets_(kMaxMediaPackets), fec_packet_list_(),
+    : generated_fec_packet_list_(kMaxMediaPackets), received_fec_packet_list_(),
       packet_mask_{0}, tmp_packet_mask_{0} {}
 ForwardErrorCorrection::~ForwardErrorCorrection() {}
 
@@ -114,7 +93,7 @@ int ForwardErrorCorrection::GenerateFec(const PacketList& media_packet_list,
                                         int num_important_packets,
                                         bool use_unequal_protection,
                                         FecMaskType fec_mask_type,
-                                        PacketList* fec_packet_list) {
+                                        std::list<Packet*>* fec_packet_list) {
   const uint16_t num_media_packets = media_packet_list.size();
   // Sanity check arguments.
   RTC_DCHECK_GT(num_media_packets, 0);
@@ -124,7 +103,8 @@ int ForwardErrorCorrection::GenerateFec(const PacketList& media_packet_list,
 
   if (num_media_packets > kMaxMediaPackets) {
     LOG(LS_WARNING) << "Can't protect " << num_media_packets
-                    << " media packets per frame. Max is " << kMaxMediaPackets;
+                    << " media packets per frame. Max is " << kMaxMediaPackets
+                    << ".";
     return -1;
   }
 
@@ -132,7 +112,7 @@ int ForwardErrorCorrection::GenerateFec(const PacketList& media_packet_list,
   int num_mask_bytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
 
   // Do some error checking on the media packets.
-  for (Packet* media_packet : media_packet_list) {
+  for (auto& media_packet : media_packet_list) {

[danilchap, 2016/06/30 19:58:37]

const auto&

[brandtr, 2016/07/01 10:45:34]

Done.

     RTC_DCHECK(media_packet);
 
     if (media_packet->length < kRtpHeaderSize) {
@@ -145,7 +125,8 @@ int ForwardErrorCorrection::GenerateFec(const PacketList& media_packet_list,
     if (media_packet->length + PacketOverhead() + kTransportOverhead >
         IP_PACKET_SIZE) {
       LOG(LS_WARNING) << "Media packet " << media_packet->length << " bytes "
-                      << "with overhead is larger than " << IP_PACKET_SIZE;
+                      << "with overhead is larger than " << IP_PACKET_SIZE
+                      << " bytes.";
     }
   }
 
@@ -155,12 +136,12 @@ int ForwardErrorCorrection::GenerateFec(const PacketList& media_packet_list,
     return 0;
   }
 
-  // Prepare FEC packets by setting them to 0.
+  // Prepare generated FEC packets by setting them to 0.
   for (int i = 0; i < num_fec_packets; ++i) {
-    memset(generated_fec_packets_[i].data, 0, IP_PACKET_SIZE);
-    generated_fec_packets_[i].length = 0;  // Use this as a marker for untouched
-                                           // packets.
-    fec_packet_list->push_back(&generated_fec_packets_[i]);
+    memset(generated_fec_packet_list_[i].data, 0, IP_PACKET_SIZE);
+    // Use this as a marker for untouched packets.
+    generated_fec_packet_list_[i].length = 0;
+    fec_packet_list->push_back(&generated_fec_packet_list_[i]);
   }
 
   const internal::PacketMaskTable mask_table(fec_mask_type, num_media_packets);
@@ -216,7 +197,7 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
       kFecHeaderSize + ulp_header_size - kRtpHeaderSize;
 
   for (int i = 0; i < num_fec_packets; ++i) {
-    Packet* const fec_packet = &generated_fec_packets_[i];
+    Packet* const fec_packet = &generated_fec_packet_list_[i];
     auto media_list_it = media_packet_list.cbegin();
     uint32_t pkt_mask_idx = i * num_mask_bytes;
     uint32_t media_pkt_idx = 0;
@@ -226,7 +207,7 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
       // Each FEC packet has a multiple byte mask. Determine if this media
       // packet should be included in FEC packet i.
       if (packet_mask[pkt_mask_idx] & (1 << (7 - media_pkt_idx))) {
-        Packet* media_packet = *media_list_it;
+        Packet* media_packet = media_list_it->get();
 
         // Assign network-ordered media payload length.
         ByteWriter<uint16_t>::WriteBigEndian(
@@ -286,36 +267,37 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
 }
 
 int ForwardErrorCorrection::InsertZerosInBitMasks(
-    const PacketList& media_packets,
+    const PacketList& media_packet_list,

[danilchap, 2016/06/30 19:58:37]

why stress packets are stored as a list?

[brandtr, 2016/07/01 10:45:34]

I don't see a point actually. I've changed the names accordingly. This will also
help if I change the backing data structures in the future. [I had a CL where I
changed to std::deque's, but terelius had a good point about my benchmarking not
being appropriate, so I scrapped that for now.]

     uint8_t* packet_mask,
     int num_mask_bytes,
     int num_fec_packets) {
-  if (media_packets.size() <= 1) {
-    return media_packets.size();
+  if (media_packet_list.size() <= 1) {
+    return media_packet_list.size();
   }
-  int last_seq_num = ParseSequenceNumber(media_packets.back()->data);
-  int first_seq_num = ParseSequenceNumber(media_packets.front()->data);
+  int last_seq_num = ParseSequenceNumber(media_packet_list.back()->data);
+  int first_seq_num = ParseSequenceNumber(media_packet_list.front()->data);
   int total_missing_seq_nums =
       static_cast<uint16_t>(last_seq_num - first_seq_num) -
-      media_packets.size() + 1;
+      media_packet_list.size() + 1;
   if (total_missing_seq_nums == 0) {
     // All sequence numbers are covered by the packet mask. No zero insertion
     // required.
-    return media_packets.size();
+    return media_packet_list.size();
   }
   // We can only protect 8 * kMaskSizeLBitSet packets.
-  if (total_missing_seq_nums + media_packets.size() > 8 * kMaskSizeLBitSet)
+  if (total_missing_seq_nums + media_packet_list.size() > 8 * kMaskSizeLBitSet)
     return -1;
   // Allocate the new mask.
   int new_mask_bytes = kMaskSizeLBitClear;
-  if (media_packets.size() + total_missing_seq_nums > 8 * kMaskSizeLBitClear) {
+  if (media_packet_list.size() +
+      total_missing_seq_nums > 8 * kMaskSizeLBitClear) {
     new_mask_bytes = kMaskSizeLBitSet;
   }
   memset(tmp_packet_mask_, 0, num_fec_packets * kMaskSizeLBitSet);
 
-  auto it = media_packets.cbegin();
+  auto media_packet_list_it = media_packet_list.cbegin();
   uint16_t prev_seq_num = first_seq_num;
-  ++it;
+  ++media_packet_list_it;
 
   // Insert the first column.
   CopyColumn(tmp_packet_mask_, new_mask_bytes, packet_mask, num_mask_bytes,
@@ -323,12 +305,12 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
   int new_bit_index = 1;
   int old_bit_index = 1;
   // Insert zeros in the bit mask for every hole in the sequence.
-  for (; it != media_packets.end(); ++it) {
+  while (media_packet_list_it != media_packet_list.end()) {
     if (new_bit_index == 8 * kMaskSizeLBitSet) {
       // We can only cover up to 48 packets.
       break;
     }
-    uint16_t seq_num = ParseSequenceNumber((*it)->data);
+    uint16_t seq_num = ParseSequenceNumber((*media_packet_list_it)->data);
     const int zeros_to_insert =
         static_cast<uint16_t>(seq_num - prev_seq_num - 1);
     if (zeros_to_insert > 0) {
@@ -341,6 +323,7 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
     ++new_bit_index;
     ++old_bit_index;
     prev_seq_num = seq_num;
+    ++media_packet_list_it;
   }
   if (new_bit_index % 8 != 0) {
     // We didn't fill the last byte. Shift bits to correct position.
@@ -412,16 +395,16 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   //   |              mask cont. (present only when L = 1)             |
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+  RTC_DCHECK(!media_packet_list.empty());
+  Packet* first_media_packet = media_packet_list.front().get();
+  RTC_DCHECK(first_media_packet);
+  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;
 
-  RTC_DCHECK(!media_packet_list.empty());
-  Packet* first_media_packet = media_packet_list.front();
-  RTC_DCHECK(first_media_packet);
-  uint16_t seq_num = ParseSequenceNumber(first_media_packet->data);
   for (int i = 0; i < num_fec_packets; ++i) {
-    Packet* const fec_packet = &generated_fec_packets_[i];
+    Packet* const fec_packet = &generated_fec_packet_list_[i];
     // -- FEC header --
     fec_packet->data[0] &= 0x7f;  // Set E to zero.
     if (l_bit == 0) {
@@ -450,65 +433,52 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
 void ForwardErrorCorrection::ResetState(
     RecoveredPacketList* recovered_packet_list) {
   // Free the memory for any existing recovered packets, if the user hasn't.
-  while (!recovered_packet_list->empty()) {
-    delete recovered_packet_list->front();
-    recovered_packet_list->pop_front();
-  }
+  recovered_packet_list->clear();
   RTC_DCHECK(recovered_packet_list->empty());

[danilchap, 2016/06/30 19:58:37]

probably no point to check std container is empty after clear

[brandtr, 2016/07/01 10:45:34]

Done.

 
-  // Free the FEC packet list.
-  while (!fec_packet_list_.empty()) {
-    FecPacket* fec_packet = fec_packet_list_.front();
-    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 =
-          fec_packet->protected_pkt_list.erase(protected_packet_list_it);
-    }
-    RTC_DCHECK(fec_packet->protected_pkt_list.empty());
-    delete fec_packet;
-    fec_packet_list_.pop_front();
-  }
-  RTC_DCHECK(fec_packet_list_.empty());
+  received_fec_packet_list_.clear();
+  RTC_DCHECK(received_fec_packet_list_.empty());
 }
 
 void ForwardErrorCorrection::InsertMediaPacket(
     ReceivedPacket* rx_packet,
     RecoveredPacketList* recovered_packet_list) {
-  auto recovered_packet_list_it = recovered_packet_list->cbegin();
 
   // Search for duplicate packets.
-  while (recovered_packet_list_it != recovered_packet_list->end()) {
-    if (rx_packet->seq_num == (*recovered_packet_list_it)->seq_num) {
+  for (const auto& recovered_packet : *recovered_packet_list) {
+    if (rx_packet->seq_num == recovered_packet->seq_num) {
       // Duplicate packet, no need to add to list.
       // Delete duplicate media packet data.
       rx_packet->pkt = nullptr;
       return;
     }
-    ++recovered_packet_list_it;
   }
-  RecoveredPacket* recovered_packet_to_insert = new RecoveredPacket();
+  std::unique_ptr<RecoveredPacket> recovered_packet_to_insert(
+      new RecoveredPacket());
+  // This "recovered packet" was not recovered using parity packets.
   recovered_packet_to_insert->was_recovered = false;
-  // Inserted Media packet is already sent to VCM.
+  // This media packet has already been passed on.
   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;
 
+  RecoveredPacket* recovered_packet_to_insert_ptr =
+      recovered_packet_to_insert.get();
   // 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(recovered_packet_to_insert);
-  recovered_packet_list->sort(SortablePacket::LessThan);
-  UpdateCoveringFecPackets(recovered_packet_to_insert);
+  recovered_packet_list->push_back(std::move(recovered_packet_to_insert));
+  recovered_packet_list->sort(SortablePacket::LessThan());
+  UpdateCoveringFecPackets(recovered_packet_to_insert_ptr);
 }
 
 void ForwardErrorCorrection::UpdateCoveringFecPackets(RecoveredPacket* packet) {
-  for (auto* fec_packet : fec_packet_list_) {
+  for (auto& fec_packet : received_fec_packet_list_) {
     // Is this FEC packet protecting the media packet |packet|?
     auto protected_it = std::lower_bound(fec_packet->protected_pkt_list.begin(),
                                          fec_packet->protected_pkt_list.end(),
                                          packet,
-                                         SortablePacket::LessThan);
+                                         SortablePacket::LessThan());
     if (protected_it != fec_packet->protected_pkt_list.end() &&
         (*protected_it)->seq_num == packet->seq_num) {
       // Found an FEC packet which is protecting |packet|.
@@ -521,14 +491,14 @@ void ForwardErrorCorrection::InsertFecPacket(
     ReceivedPacket* rx_packet,
     const RecoveredPacketList* recovered_packet_list) {
   // Check for duplicate.
-  for (auto* fec_packet : fec_packet_list_) {
-    if (rx_packet->seq_num == fec_packet->seq_num) {
+  for (auto& existing_fec_packet : received_fec_packet_list_) {

[danilchap, 2016/06/30 19:58:37]

const auto&

[brandtr, 2016/07/01 10:45:34]

Done.

+    if (rx_packet->seq_num == existing_fec_packet->seq_num) {
       // Delete duplicate FEC packet data.
       rx_packet->pkt = nullptr;
       return;
     }
   }
-  FecPacket* fec_packet = new FecPacket();
+  std::unique_ptr<ReceivedFecPacket> fec_packet(new ReceivedFecPacket());
   fec_packet->pkt = rx_packet->pkt;
   fec_packet->seq_num = rx_packet->seq_num;
   fec_packet->ssrc = rx_packet->ssrc;
@@ -543,54 +513,59 @@ void ForwardErrorCorrection::InsertFecPacket(
     uint8_t packet_mask = fec_packet->pkt->data[12 + byte_idx];
     for (uint16_t bit_idx = 0; bit_idx < 8; ++bit_idx) {
       if (packet_mask & (1 << (7 - bit_idx))) {
-        ProtectedPacket* protected_packet = new ProtectedPacket();
-        fec_packet->protected_pkt_list.push_back(protected_packet);
+        std::unique_ptr<ProtectedPacket> protected_packet(
+            new ProtectedPacket());
         // This wraps naturally with the sequence number.
         protected_packet->seq_num =
             static_cast<uint16_t>(seq_num_base + (byte_idx << 3) + bit_idx);
         protected_packet->pkt = nullptr;
+        fec_packet->protected_pkt_list.push_back(std::move(protected_packet));
       }
     }
   }
   if (fec_packet->protected_pkt_list.empty()) {
     // All-zero packet mask; we can discard this FEC packet.
     LOG(LS_WARNING) << "FEC packet has an all-zero packet mask.";
-    delete fec_packet;
   } else {
-    AssignRecoveredPackets(fec_packet, recovered_packet_list);
+    AssignRecoveredPackets(fec_packet.get(), recovered_packet_list);
     // 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.
-    fec_packet_list_.push_back(fec_packet);
-    fec_packet_list_.sort(SortablePacket::LessThan);
-    if (fec_packet_list_.size() > kMaxFecPackets) {
-      DiscardFecPacket(fec_packet_list_.front());
-      fec_packet_list_.pop_front();
+    received_fec_packet_list_.push_back(std::move(fec_packet));
+    received_fec_packet_list_.sort(SortablePacket::LessThan());
+    if (received_fec_packet_list_.size() > kMaxFecPackets) {
+      received_fec_packet_list_.pop_front();
     }
-    RTC_DCHECK_LE(fec_packet_list_.size(), kMaxFecPackets);
+    RTC_DCHECK_LE(received_fec_packet_list_.size(), kMaxFecPackets);
   }
 }
 
 void ForwardErrorCorrection::AssignRecoveredPackets(
-    FecPacket* fec_packet,
+    ReceivedFecPacket* fec_packet,
     const RecoveredPacketList* recovered_packets) {
-  // Search for missing packets which have arrived or have been recovered by
-  // another FEC packet.
-  ProtectedPacketList* not_recovered = &fec_packet->protected_pkt_list;
-  RecoveredPacketList already_recovered;
-  std::set_intersection(
-      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.
-  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)
-      ++not_recovered_it;
-    (*not_recovered_it)->pkt = (*it)->pkt;
+  ProtectedPacketList* protected_packets = &fec_packet->protected_pkt_list;
+  std::vector<RecoveredPacket*> recovered_protected_packets;
+
+  // Find intersection between the (sorted) containers |protected_packets|
+  // and |recovered_packets|, i.e. all protected packets that have already
+  // been recovered. Update the corresponding protected packets to point to
+  // the recovered packets.
+  auto it_p = protected_packets->cbegin();
+  auto it_r = recovered_packets->cbegin();
+  bool t1, t2;
+  auto cmp = SortablePacket::LessThan();

[danilchap, 2016/06/30 19:58:37]

may be
SortablePacket::LessThan cmp;

[brandtr, 2016/07/01 10:45:34]

Done.

+  while (it_p != protected_packets->end() && it_r != recovered_packets->end()) {
+    t1 = cmp(*it_p, *it_r); t2 = cmp(*it_r, *it_p);

[danilchap, 2016/06/30 19:58:37]

do not put two commands on same line
also, do not predeclare t1/t2:
bool t1 = cmp(*it_p, *it_r);
bool t2 = cmp(*it_r, *it_p);
Or, since you have to compare intersection manually anyway, you may be more
explicit:
uint16_t protected_seq_num = (*it_p)->seq_num;
uint16_t recoevered_seq_num = (*it_r)->seq_num;
if (protected_seq_num == recoevered_seq_num) {
  ...
} else if (IsNewer(one, another)) {
  ...
} else {
  ...
}

[brandtr, 2016/07/01 10:45:34]

Changed as according to the first comment. SortablePacket::LessThan is used for
sorting in other places, so I think it's nice to reuse it here as well.

+    if (!t1 && !t2) {
+      // *it_p == *it_r. (This protected packet has already been recovered.)
+      (*it_p)->pkt = (*it_r)->pkt;
+      ++it_p; ++it_r;
+    } else if (t1) {
+      // *it_p < *it_r.
+      ++it_p;
+    } else {
+      // *it_p > *it_r.
+      ++it_r;
+    }
   }
 }
 
@@ -598,7 +573,7 @@ void ForwardErrorCorrection::InsertPackets(
     ReceivedPacketList* received_packet_list,
     RecoveredPacketList* recovered_packet_list) {
   while (!received_packet_list->empty()) {
-    ReceivedPacket* rx_packet = received_packet_list->front();
+    ReceivedPacket* rx_packet = received_packet_list->front().get();
 
     // Check for discarding oldest FEC packet, to avoid wrong FEC decoding from
     // sequence number wrap-around. Detection of old FEC packet is based on
@@ -607,13 +582,12 @@ void ForwardErrorCorrection::InsertPackets(
     // TODO(marpan/holmer): We should be able to improve detection/discarding of
     // old FEC packets based on timestamp information or better sequence number
     // thresholding (e.g., to distinguish between wrap-around and reordering).
-    if (!fec_packet_list_.empty()) {
+    if (!received_fec_packet_list_.empty()) {
       uint16_t seq_num_diff =
           abs(static_cast<int>(rx_packet->seq_num) -
-              static_cast<int>(fec_packet_list_.front()->seq_num));
+              static_cast<int>(received_fec_packet_list_.front()->seq_num));
       if (seq_num_diff > 0x3fff) {
-        DiscardFecPacket(fec_packet_list_.front());
-        fec_packet_list_.pop_front();
+        received_fec_packet_list_.pop_front();
       }
     }
 
@@ -624,15 +598,16 @@ void ForwardErrorCorrection::InsertPackets(
       InsertMediaPacket(rx_packet, recovered_packet_list);
     }
     // Delete the received packet "wrapper", but not the packet data.
-    delete rx_packet;
+    // (The latter is linked to using the reference-counted |pkt| member.)

[danilchap, 2016/06/30 19:58:37]

may be just reduce comment:
// Delete the received packet "wrapper".

[brandtr, 2016/07/01 10:45:34]

Done.

     received_packet_list->pop_front();
   }
   RTC_DCHECK(received_packet_list->empty());
-  DiscardOldPackets(recovered_packet_list);
+  DiscardOldRecoveredPackets(recovered_packet_list);
 }
 
-bool ForwardErrorCorrection::StartPacketRecovery(const FecPacket* fec_packet,
-                                                 RecoveredPacket* recovered) {
+bool ForwardErrorCorrection::StartPacketRecovery(
+      const ReceivedFecPacket* 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
@@ -715,11 +690,11 @@ void ForwardErrorCorrection::XorPackets(const Packet* src_packet,
 }
 
 bool ForwardErrorCorrection::RecoverPacket(
-    const FecPacket* fec_packet,
+    const ReceivedFecPacket* fec_packet,
     RecoveredPacket* rec_packet_to_insert) {
   if (!StartPacketRecovery(fec_packet, rec_packet_to_insert))
     return false;
-  for (const auto* protected_packet : fec_packet->protected_pkt_list) {
+  for (const auto& protected_packet : fec_packet->protected_pkt_list) {
     if (protected_packet->pkt == nullptr) {
       // This is the packet we're recovering.
       rec_packet_to_insert->seq_num = protected_packet->seq_num;
@@ -734,55 +709,52 @@ bool ForwardErrorCorrection::RecoverPacket(
 
 void ForwardErrorCorrection::AttemptRecover(
     RecoveredPacketList* recovered_packet_list) {
-  auto fec_packet_list_it = fec_packet_list_.begin();
-  while (fec_packet_list_it != fec_packet_list_.end()) {
+  auto fec_packet_it = received_fec_packet_list_.begin();
+  while (fec_packet_it != received_fec_packet_list_.end()) {
     // Search for each FEC packet's protected media packets.
-    int packets_missing = NumCoveredPacketsMissing(*fec_packet_list_it);
+    int packets_missing = NumCoveredPacketsMissing(fec_packet_it->get());
 
     // We can only recover one packet with an FEC packet.
     if (packets_missing == 1) {
       // Recovery possible.
-      RecoveredPacket* packet_to_insert = new RecoveredPacket();
+      std::unique_ptr<RecoveredPacket> packet_to_insert(new RecoveredPacket());
       packet_to_insert->pkt = nullptr;
-      if (!RecoverPacket(*fec_packet_list_it, packet_to_insert)) {
+      if (!RecoverPacket(fec_packet_it->get(), packet_to_insert.get())) {
         // Can't recover using this packet, drop it.
-        DiscardFecPacket(*fec_packet_list_it);
-        fec_packet_list_it = fec_packet_list_.erase(fec_packet_list_it);
-        delete packet_to_insert;
+        fec_packet_it = received_fec_packet_list_.erase(fec_packet_it);
         continue;
       }
 
+      auto packet_to_insert_ptr = packet_to_insert.get();
       // Add recovered packet to the list of recovered packets and update any
       // FEC packets covering this packet with a pointer to the data.
       // 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(packet_to_insert);
-      recovered_packet_list->sort(SortablePacket::LessThan);
-      UpdateCoveringFecPackets(packet_to_insert);
-      DiscardOldPackets(recovered_packet_list);
-      DiscardFecPacket(*fec_packet_list_it);
-      fec_packet_list_it = fec_packet_list_.erase(fec_packet_list_it);
+      recovered_packet_list->push_back(std::move(packet_to_insert));
+      recovered_packet_list->sort(SortablePacket::LessThan());
+      UpdateCoveringFecPackets(packet_to_insert_ptr);
+      DiscardOldRecoveredPackets(recovered_packet_list);
+      fec_packet_it = received_fec_packet_list_.erase(fec_packet_it);
 
       // A packet has been recovered. We need to check the FEC list again, as
       // this may allow additional packets to be recovered.
       // Restart for first FEC packet.
-      fec_packet_list_it = fec_packet_list_.begin();
+      fec_packet_it = received_fec_packet_list_.begin();
     } else if (packets_missing == 0) {
       // Either all protected packets arrived or have been recovered. We can
       // discard this FEC packet.
-      DiscardFecPacket(*fec_packet_list_it);
-      fec_packet_list_it = fec_packet_list_.erase(fec_packet_list_it);
+      fec_packet_it = received_fec_packet_list_.erase(fec_packet_it);
     } else {
-      fec_packet_list_it++;
+      fec_packet_it++;
     }
   }
 }
 
 int ForwardErrorCorrection::NumCoveredPacketsMissing(
-    const FecPacket* fec_packet) {
+    const ReceivedFecPacket* fec_packet) {
   int packets_missing = 0;
-  for (const auto* protected_packet : fec_packet->protected_pkt_list) {
+  for (const auto& protected_packet : fec_packet->protected_pkt_list) {
     if (protected_packet->pkt == nullptr) {
       ++packets_missing;
       if (packets_missing > 1) {
@@ -793,21 +765,9 @@ int ForwardErrorCorrection::NumCoveredPacketsMissing(
   return packets_missing;
 }
 
-void ForwardErrorCorrection::DiscardFecPacket(FecPacket* fec_packet) {
-  while (!fec_packet->protected_pkt_list.empty()) {
-    delete fec_packet->protected_pkt_list.front();
-    fec_packet->protected_pkt_list.pop_front();
-  }
-  RTC_DCHECK(fec_packet->protected_pkt_list.empty());
-  delete fec_packet;
-}
-
-void ForwardErrorCorrection::DiscardOldPackets(
+void ForwardErrorCorrection::DiscardOldRecoveredPackets(
     RecoveredPacketList* recovered_packet_list) {
   while (recovered_packet_list->size() > kMaxMediaPackets) {
-    ForwardErrorCorrection::RecoveredPacket* packet =
-        recovered_packet_list->front();
-    delete packet;
     recovered_packet_list->pop_front();
   }
   RTC_DCHECK(recovered_packet_list->size() <= kMaxMediaPackets);

[danilchap, 2016/06/30 19:58:37]

this check looks unnecceray

[brandtr, 2016/07/01 10:45:34]

It is, but my feeling was that this check was there to show intent?

Btw, this doesn't compile if I change it to the corresponding RTC_DCHECK_LE, do
you have any idea why?

[danilchap, 2016/07/04 09:47:10]

Guess you right about intent: DCHECK can be used not just to tell about input
parameters, but also to quickly show result of the function.
I guess using _LE doesn't link.
Yes, I know why: DCHECK take parameters by const&, so it tries to get address of
the kMaxMediaPackets.
Use this trick:
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#the-c...
(it didn't work well with const on windows though, but works with constexpr)