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 d61087a4a0282da84bf19d467c8e54ed786e34d6..34d2d41c31a79c25dc3d2d285f6666274c4715ea 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -49,41 +49,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) {}
+    : generated_fec_packet_list_(kMaxMediaPackets) {}
+
 ForwardErrorCorrection::~ForwardErrorCorrection() {}
 
 // Input packet
@@ -106,12 +85,13 @@ ForwardErrorCorrection::~ForwardErrorCorrection() {}
 //
 // Note that any potential RED headers are added/removed before calling
 // GenerateFEC or DecodeFEC.
-int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
-                                            uint8_t protection_factor,
-                                            int num_important_packets,
-                                            bool use_unequal_protection,
-                                            FecMaskType fec_mask_type,
-                                            PacketList* fec_packet_list) {
+int32_t ForwardErrorCorrection::GenerateFEC(
+    const PacketList& media_packet_list,
+    uint8_t protection_factor,
+    int num_important_packets,
+    bool use_unequal_protection,
+    FecMaskType fec_mask_type,
+    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);
@@ -121,7 +101,8 @@ int32_t 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;
   }
 
@@ -129,7 +110,7 @@ int32_t 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) {
     RTC_DCHECK(media_packet);
 
     if (media_packet->length < kRtpHeaderSize) {
@@ -142,7 +123,8 @@ int32_t 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.";
     }
   }
 
@@ -152,12 +134,12 @@ int32_t 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 +198,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 +208,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,38 +268,39 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
 }
 
 int ForwardErrorCorrection::InsertZerosInBitMasks(
-    const PacketList& media_packets,
+    const PacketList& media_packet_list,
     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;
   }
   std::unique_ptr<uint8_t[]> new_mask(
       new uint8_t[num_fec_packets * kMaskSizeLBitSet]);
   memset(new_mask.get(), 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(new_mask.get(), new_mask_bytes, packet_mask, num_mask_bytes,
@@ -325,12 +308,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) {
@@ -343,6 +326,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.
@@ -415,7 +399,7 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
   //   |              mask cont. (present only when L = 1)             |
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   RTC_DCHECK(!media_packet_list.empty());
-  Packet* first_media_packet = media_packet_list.front();
+  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;
@@ -423,7 +407,7 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
       l_bit ? kUlpHeaderSizeLBitSet : kUlpHeaderSizeLBitClear;
 
   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) {
@@ -452,44 +436,31 @@ 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());
 
-  // 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) {
+  auto it = recovered_packet_list->cbegin();
+  while (it != recovered_packet_list->end()) {

[stefan-webrtc, 2016/06/29 19:36:18]

Perhaps use range-based loop instead?

[brandtr, 2016/06/30 14:05:54]

Done.

+    if (rx_packet->seq_num == (*it)->seq_num) {
       // Duplicate packet, no need to add to list.
       // Delete duplicate media packet data.
       rx_packet->pkt = nullptr;
       return;
     }
-    ++recovered_packet_list_it;
+    ++it;
   }
-  RecoveredPacket* recovered_packet_to_insert = new RecoveredPacket;
+  std::unique_ptr<RecoveredPacket> recovered_packet_to_insert(
+      new RecoveredPacket);

[stefan-webrtc, 2016/06/29 19:36:18]

new RecoveredPacket()

[brandtr, 2016/06/30 14:05:55]

Done.

+  // This "recovered packet" was not recovered using parity packets.
   recovered_packet_to_insert->was_recovered = false;
   // Inserted Media packet is already sent to VCM.

[stefan-webrtc, 2016/06/29 19:36:18]

I think you can replace VCM with jitter buffer, or simply write "passed on"

[brandtr, 2016/06/30 14:05:55]

Done.

   recovered_packet_to_insert->returned = true;
@@ -497,20 +468,22 @@ void ForwardErrorCorrection::InsertMediaPacket(
   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|.
@@ -523,14 +496,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_) {
+    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);

[stefan-webrtc, 2016/06/29 19:36:18]

new ReceivedFecPacket()

[brandtr, 2016/06/30 14:05:55]

Done.

   fec_packet->pkt = rx_packet->pkt;
   fec_packet->seq_num = rx_packet->seq_num;
   fec_packet->ssrc = rx_packet->ssrc;
@@ -545,54 +518,58 @@ 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);

[stefan-webrtc, 2016/06/29 19:36:18]

()

[brandtr, 2016/06/30 14:05:55]

Done.

         // 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) {

[stefan-webrtc, 2016/06/29 19:36:18]

Feel free to remove {} since this is a two line if statement.

[brandtr, 2016/06/30 14:05:54]

I'm happy to keep the {}, as it conforms with the rest of the code in the FEC
subsystem. (Discussed this during today's standup.)

+      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();
+  while (it_p != protected_packets->end() && it_r != recovered_packets->end()) {

[stefan-webrtc, 2016/06/29 19:36:18]

Does this implement intersect? Is it better than calling std::set_intersection
as before? Should it be broken out to a separate function?

[brandtr, 2016/06/30 14:05:54]

Yes, these lines correspond to the previous call to std::set_intersection and
the following updating of the recovered packet's pkt member. There should be no
difference in functionality.

I would've like to keep the previous code, but was unable to figure out how to
do it; the problem is that the types, when wrapped by std::unique_ptr, are not
natively covariant, i.e. the fact that a RecoveredPacket is a SortablePacket
does not mean that a std::unique_ptr<RecoveredPacket> is a
std::unique_ptr<SortablePacket>. This breaks the call to the comparator in
std::set_intersection (see e.g.
http://en.cppreference.com/w/cpp/algorithm/set_intersection). My solution was to
reimplement the set intersection manually and use a parametric comparator (see
row 56).

I discussed the issue of the invariant std::unique_ptr's with some team members,
and their advice was to reimplement the set intersection manually. I'm happy to
change back to the previous solution, if you have an idea on how to get it to
work :)

I don't believe that this needs to be it's own function, as this code is only
used in this function, whose sole purpose is to do these operations.

[stefan-webrtc, 2016/06/30 14:18:35]

I see, didn't think of that issue. I still think a separate function would be
good to make the code more readable.

[brandtr, 2016/07/01 06:49:07]

This would entail moving the whole body of this function to a new function
though, since (in the current implementation) the set intersection and the
updating of the packet pointers (see row 564) are intertwined.

I could decouple the set intersection and pointer updating, thus being able to
split out the set intersection into its own function. That would require
iterating through the packets in the intersection twice, however.

+    t1 = cmp(*it_p, *it_r); t2 = cmp(*it_r, *it_p);
+    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;
+    }
   }
 }
 
@@ -600,7 +577,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
@@ -609,13 +586,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();
       }
     }
 
@@ -626,15 +602,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.)
     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
@@ -717,11 +694,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 (auto* protected_packet : fec_packet->protected_pkt_list) {
+  for (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;
@@ -736,55 +713,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 (auto* protected_packet : fec_packet->protected_pkt_list) {
+  for (auto& protected_packet : fec_packet->protected_pkt_list) {
     if (protected_packet->pkt == nullptr) {
       ++packets_missing;
       if (packets_missing > 1) {
@@ -795,21 +769,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);