Change ForwardErrorCorrection class to accept one received packet at a time.

webrtc/modules/rtp_rtcp/source/forward_error_correction.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 324 matching lines @@
 
 void ForwardErrorCorrection::ResetState(
     RecoveredPacketList* recovered_packets) {
   // Free the memory for any existing recovered packets, if the caller hasn't.
   recovered_packets->clear();
   received_fec_packets_.clear();
 }
 
 void ForwardErrorCorrection::InsertMediaPacket(
     RecoveredPacketList* recovered_packets,
-    ReceivedPacket* received_packet) {
-  RTC_DCHECK_EQ(received_packet->ssrc, protected_media_ssrc_);
+    const ReceivedPacket& received_packet) {
+  RTC_DCHECK_EQ(received_packet.ssrc, protected_media_ssrc_);
 
   // Search for duplicate packets.
   for (const auto& recovered_packet : *recovered_packets) {
-    RTC_DCHECK_EQ(recovered_packet->ssrc, received_packet->ssrc);
-    if (recovered_packet->seq_num == received_packet->seq_num) {
+    RTC_DCHECK_EQ(recovered_packet->ssrc, received_packet.ssrc);
+    if (recovered_packet->seq_num == received_packet.seq_num) {
       // Duplicate packet, no need to add to list.
-      // Delete duplicate media packet data.
-      received_packet->pkt = nullptr;

[brandtr, 2017/09/14 11:49:45]

Just to verify: the refcount of |received_packet->pkt| will be decremented when
the unique_ptr owning |received_packet| goes out of scope?

[nisse-webrtc, 2017/09/14 12:55:51]

That's my understanding. And it would be more natural to accept a
scoped_refptr<Packet> as input.

       return;
     }
   }
 
   std::unique_ptr<RecoveredPacket> recovered_packet(new RecoveredPacket());
   // This "recovered packet" was not recovered using parity packets.
   recovered_packet->was_recovered = false;
   // This media packet has already been passed on.
   recovered_packet->returned = true;
-  recovered_packet->ssrc = received_packet->ssrc;
-  recovered_packet->seq_num = received_packet->seq_num;
-  recovered_packet->pkt = received_packet->pkt;
-  recovered_packet->pkt->length = received_packet->pkt->length;
+  recovered_packet->ssrc = received_packet.ssrc;
+  recovered_packet->seq_num = received_packet.seq_num;
+  recovered_packet->pkt = received_packet.pkt;
+  recovered_packet->pkt->length = received_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.
   RecoveredPacket* recovered_packet_ptr = recovered_packet.get();
   recovered_packets->push_back(std::move(recovered_packet));
   recovered_packets->sort(SortablePacket::LessThan());
   UpdateCoveringFecPackets(*recovered_packet_ptr);
 }
 
 void ForwardErrorCorrection::UpdateCoveringFecPackets(
     const RecoveredPacket& packet) {
   for (auto& fec_packet : received_fec_packets_) {
     // Is this FEC packet protecting the media packet |packet|?
     auto protected_it = std::lower_bound(fec_packet->protected_packets.begin(),
                                          fec_packet->protected_packets.end(),
                                          &packet, SortablePacket::LessThan());
     if (protected_it != fec_packet->protected_packets.end() &&
         (*protected_it)->seq_num == packet.seq_num) {
       // Found an FEC packet which is protecting |packet|.
       (*protected_it)->pkt = packet.pkt;
     }
   }
 }
 
 void ForwardErrorCorrection::InsertFecPacket(
     const RecoveredPacketList& recovered_packets,
-    ReceivedPacket* received_packet) {
-  RTC_DCHECK_EQ(received_packet->ssrc, ssrc_);
+    const ReceivedPacket& received_packet) {
+  RTC_DCHECK_EQ(received_packet.ssrc, ssrc_);
 
   // Check for duplicate.
   for (const auto& existing_fec_packet : received_fec_packets_) {
-    RTC_DCHECK_EQ(existing_fec_packet->ssrc, received_packet->ssrc);
-    if (existing_fec_packet->seq_num == received_packet->seq_num) {
-      // Delete duplicate FEC packet data.
-      received_packet->pkt = nullptr;

[brandtr, 2017/09/14 11:49:45]

And here.

+    RTC_DCHECK_EQ(existing_fec_packet->ssrc, received_packet.ssrc);
+    if (existing_fec_packet->seq_num == received_packet.seq_num) {
+      // Drop duplicate FEC packet data.
       return;
     }
   }
 
   std::unique_ptr<ReceivedFecPacket> fec_packet(new ReceivedFecPacket());
-  fec_packet->pkt = received_packet->pkt;
-  fec_packet->ssrc = received_packet->ssrc;
-  fec_packet->seq_num = received_packet->seq_num;
+  fec_packet->pkt = received_packet.pkt;
+  fec_packet->ssrc = received_packet.ssrc;
+  fec_packet->seq_num = received_packet.seq_num;
   // Parse ULPFEC/FlexFEC header specific info.
   bool ret = fec_header_reader_->ReadFecHeader(fec_packet.get());
   if (!ret) {
     return;
   }
 
   // TODO(brandtr): Update here when we support multistream protection.
   if (fec_packet->protected_ssrc != protected_media_ssrc_) {
     LOG(LS_INFO)
         << "Received FEC packet is protecting an unknown media SSRC; dropping.";
@@ skipping 56 matching lines @@
       ++it_r;
     } else {  // *it_p == *it_r.
       // This protected packet has already been recovered.
       (*it_p)->pkt = (*it_r)->pkt;
       ++it_p;
       ++it_r;
     }
   }
 }
 
-void ForwardErrorCorrection::InsertPackets(
-    ReceivedPacketList* received_packets,
+void ForwardErrorCorrection::InsertPacket(
+    const ReceivedPacket& received_packet,
     RecoveredPacketList* recovered_packets) {
-  while (!received_packets->empty()) {
-    ReceivedPacket* received_packet = received_packets->front().get();
-
-    // Discard old FEC packets such that the sequence numbers in
-    // |received_fec_packets_| span at most 1/2 of the sequence number space.
-    // This is important for keeping |received_fec_packets_| sorted, and may
-    // also reduce the possibility of incorrect decoding due to sequence number
-    // wrap-around.
-    // 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 (!received_fec_packets_.empty() &&
-        received_packet->ssrc == received_fec_packets_.front()->ssrc) {
-      // It only makes sense to detect wrap-around when |received_packet|
-      // and |front_received_fec_packet| belong to the same sequence number
-      // space, i.e., the same SSRC. This happens when |received_packet|
-      // is a FEC packet, or if |received_packet| is a media packet and
-      // RED+ULPFEC is used.
-      auto it = received_fec_packets_.begin();
-      while (it != received_fec_packets_.end()) {
-        uint16_t seq_num_diff = abs(static_cast<int>(received_packet->seq_num) -
-                                    static_cast<int>((*it)->seq_num));
-        if (seq_num_diff > 0x3fff) {
-          it = received_fec_packets_.erase(it);
-        } else {
-          // No need to keep iterating, since |received_fec_packets_| is sorted.
-          break;
-        }
+  // Discard old FEC packets such that the sequence numbers in
+  // |received_fec_packets_| span at most 1/2 of the sequence number space.
+  // This is important for keeping |received_fec_packets_| sorted, and may
+  // also reduce the possibility of incorrect decoding due to sequence number
+  // wrap-around.
+  // 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 (!received_fec_packets_.empty() &&
+      received_packet.ssrc == received_fec_packets_.front()->ssrc) {
+    // It only makes sense to detect wrap-around when |received_packet|
+    // and |front_received_fec_packet| belong to the same sequence number
+    // space, i.e., the same SSRC. This happens when |received_packet|
+    // is a FEC packet, or if |received_packet| is a media packet and
+    // RED+ULPFEC is used.
+    auto it = received_fec_packets_.begin();
+    while (it != received_fec_packets_.end()) {
+      // TODO(nisse): This handling of wraparound appears broken, should be
+      // static_cast<int16_t>(
+      //      received_packet.seq_num - back_recovered_packet->seq_num)
+      uint16_t seq_num_diff = abs(static_cast<int>(received_packet.seq_num) -
+                                  static_cast<int>((*it)->seq_num));
+      if (seq_num_diff > 0x3fff) {
+        it = received_fec_packets_.erase(it);
+      } else {
+        // No need to keep iterating, since |received_fec_packets_| is sorted.
+        break;
       }
     }
+  }
 
-    if (received_packet->is_fec) {
-      InsertFecPacket(*recovered_packets, received_packet);
-    } else {
-      InsertMediaPacket(recovered_packets, received_packet);
-    }
-    // Delete the received packet "wrapper".
-    received_packets->pop_front();
+  if (received_packet.is_fec) {
+    InsertFecPacket(*recovered_packets, received_packet);
+  } else {
+    InsertMediaPacket(recovered_packets, received_packet);
   }
-  RTC_DCHECK(received_packets->empty());
+
   DiscardOldRecoveredPackets(recovered_packets);
 }
 
 bool ForwardErrorCorrection::StartPacketRecovery(
     const ReceivedFecPacket& fec_packet,
     RecoveredPacket* recovered_packet) {
   // Sanity check packet length.
   if (fec_packet.pkt->length < fec_packet.fec_header_size) {
     LOG(LS_WARNING)
         << "The FEC packet is truncated: it does not contain enough room "
@@ skipping 170 matching lines @@
 }
 
 uint16_t ForwardErrorCorrection::ParseSequenceNumber(uint8_t* packet) {
   return (packet[2] << 8) + packet[3];
 }
 
 uint32_t ForwardErrorCorrection::ParseSsrc(uint8_t* packet) {
   return (packet[8] << 24) + (packet[9] << 16) + (packet[10] << 8) + packet[11];
 }
 
-int ForwardErrorCorrection::DecodeFec(
-    ReceivedPacketList* received_packets,
-    RecoveredPacketList* recovered_packets) {
-  RTC_DCHECK(received_packets);
+void ForwardErrorCorrection::DecodeFec(const ReceivedPacket& received_packet,
+                                       RecoveredPacketList* recovered_packets) {
   RTC_DCHECK(recovered_packets);
 
   const size_t max_media_packets = fec_header_reader_->MaxMediaPackets();
   if (recovered_packets->size() == max_media_packets) {
     const RecoveredPacket* back_recovered_packet =
         recovered_packets->back().get();
-    for (const auto& received_packet : *received_packets) {
-      if (received_packet->ssrc == back_recovered_packet->ssrc) {
-        const unsigned int seq_num_diff =
-            abs(static_cast<int>(received_packet->seq_num) -
-                static_cast<int>(back_recovered_packet->seq_num));
-        if (seq_num_diff > max_media_packets) {
-          // A big gap in sequence numbers. The old recovered packets
-          // are now useless, so it's safe to do a reset.
-          LOG(LS_INFO) << "Big gap in media/ULPFEC sequence numbers. No need "
-                          "to keep the old packets in the FEC buffers, thus "
-                          "resetting them.";
-          ResetState(recovered_packets);
-          break;
-        }
+
+    if (received_packet.ssrc == back_recovered_packet->ssrc) {
+      // TODO(nisse): This handling of wraparound appears broken, should be
+      // static_cast<int16_t>(
+      //      received_packet.seq_num - back_recovered_packet->seq_num)
+      const unsigned int seq_num_diff =
+          abs(static_cast<int>(received_packet.seq_num) -
+              static_cast<int>(back_recovered_packet->seq_num));
+      if (seq_num_diff > max_media_packets) {
+        // A big gap in sequence numbers. The old recovered packets
+        // are now useless, so it's safe to do a reset.
+        LOG(LS_INFO) << "Big gap in media/ULPFEC sequence numbers. No need "
+                        "to keep the old packets in the FEC buffers, thus "
+                        "resetting them.";
+        ResetState(recovered_packets);
       }
     }
   }
 
-  InsertPackets(received_packets, recovered_packets);
+  InsertPacket(received_packet, recovered_packets);
   AttemptRecovery(recovered_packets);
-
-  return 0;
 }
 
 size_t ForwardErrorCorrection::MaxPacketOverhead() const {
   return fec_header_writer_->MaxPacketOverhead();
 }
 
 FecHeaderReader::FecHeaderReader(size_t max_media_packets,
                                  size_t max_fec_packets)
     : max_media_packets_(max_media_packets),
       max_fec_packets_(max_fec_packets) {}
@@ skipping 23 matching lines @@
 
 size_t FecHeaderWriter::MaxFecPackets() const {
   return max_fec_packets_;
 }
 
 size_t FecHeaderWriter::MaxPacketOverhead() const {
   return max_packet_overhead_;
 }
 
 }  // namespace webrtc