Use std::unique_ptr in ForwardErrorCorrection.

webrtc/modules/rtp_rtcp/source/forward_error_correction.h

Index: webrtc/modules/rtp_rtcp/source/forward_error_correction.h
diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h
index fb35374ac693d11cec0412ad36401b555c12b0f0..98e2868556fa8a3e33421f3df042eeb965bd2a07 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h
@@ -24,9 +24,6 @@
 
 namespace webrtc {
 
-// Forward declaration.
-class FecPacket;
-
 // Performs codec-independent forward error correction (FEC), based on RFC 5109.
 // Option exists to enable unequal protection (UEP) across packets.
 // This is not to be confused with protection within packets
@@ -62,9 +59,13 @@ class ForwardErrorCorrection {
   // TODO(holmer): Refactor into a proper class.
   class SortablePacket {
    public:
-    // True if first is <= than second.
-    static bool LessThan(const SortablePacket* first,
-                         const SortablePacket* second);
+    // Functor which returns true if the sequence number of |first|
+    // is < the sequence number of |second|.
+    class LessThan {
+     public:
+      template <typename S, typename T>
+      bool operator() (const S& first, const T& second);
+    };
 
     uint16_t seq_num;
   };
@@ -88,8 +89,8 @@ class ForwardErrorCorrection {
   // TODO(holmer): Refactor into a proper class.
   class ReceivedPacket : public SortablePacket {
    public:
-    ReceivedPacket();
-    ~ReceivedPacket();
+    ReceivedPacket() = default;
+    ~ReceivedPacket() = default;
 
     uint32_t ssrc;  // SSRC of the current frame. Must be set for FEC
                     // packets, but not required for media packets.
@@ -103,8 +104,8 @@ class ForwardErrorCorrection {
   // TODO(holmer): Refactor into a proper class.
   class RecoveredPacket : public SortablePacket {
    public:
-    RecoveredPacket();
-    ~RecoveredPacket();
+    RecoveredPacket() = default;
+    ~RecoveredPacket() = default;
 
     bool was_recovered;  // Will be true if this packet was recovered by
                          // the FEC. Otherwise it was a media packet passed in
@@ -116,9 +117,9 @@ class ForwardErrorCorrection {
     rtc::scoped_refptr<Packet> pkt;  // Pointer to the packet storage.
   };
 
-  typedef std::list<Packet*> PacketList;
-  typedef std::list<ReceivedPacket*> ReceivedPacketList;
-  typedef std::list<RecoveredPacket*> RecoveredPacketList;
+  typedef std::list<std::unique_ptr<Packet>> PacketList;
+  typedef std::list<std::unique_ptr<ReceivedPacket>> ReceivedPacketList;
+  typedef std::list<std::unique_ptr<RecoveredPacket>> RecoveredPacketList;
 
   ForwardErrorCorrection();
   virtual ~ForwardErrorCorrection();
@@ -162,7 +163,7 @@ class ForwardErrorCorrection {
   int 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);
+                  std::list<Packet*>* fec_packet_list);
 
   /**
    *  Decodes a list of media and FEC packets. It will parse the input received
@@ -209,7 +210,30 @@ class ForwardErrorCorrection {
   void ResetState(RecoveredPacketList* recovered_packet_list);
 
  private:
-  typedef std::list<FecPacket*> FecPacketList;
+  // Forward declarations.
+  class ProtectedPacket;

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

do you need to forward declare them? May be just their declaration here.
Though you might move declaration into .cc file. Then this forward declaration
would be neccecary.

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

Not strictly necessary, but I think I did it to try to follow the declaration
order specified in the style guide. I have now changed order of the
declarations, but that means splitting up the two 'using' (former 'typedef')
lines into two parts.

I moved the declarations of these internal classes to the header file because
the compiler was complaining about ReceivedFecPacketList being an incomplete
type at row 334, when the declaration was in the .cc file. (I think this might
have to do with the std::unique_ptr's, whereas a normal pointer to an incomplete
type was OK?)

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

Style guide
(https://google.github.io/styleguide/cppguide.html#Declaration_Order) tells that
using, typedef and enums should go first.
member class look like typedef to me (can't find any better category for it), so
having member class, then using seems legal. Intermixing them seems legal too,
but may be slightly less readable.

+  class ReceivedFecPacket;
+
+  typedef std::list<std::unique_ptr<ProtectedPacket>> ProtectedPacketList;

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

prefer
using ProtectedPacketList = std::list<std::unique_ptr<ProtectedPacket>>;

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

Done.

+  typedef std::list<std::unique_ptr<ReceivedFecPacket>> ReceivedFecPacketList;
+
+  // 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;
+  };
+
+  // Used for internal storage of received FEC packets in a list.
+  //
+  // TODO(holmer): Refactor into a proper class.
+  class ReceivedFecPacket : public ForwardErrorCorrection::SortablePacket {
+   public:
+    ProtectedPacketList protected_pkt_list;
+    uint32_t ssrc;  // SSRC of the current frame.
+    rtc::scoped_refptr<ForwardErrorCorrection::Packet> pkt;
+  };
 
   // Analyzes |media_packets| for holes in the sequence and inserts zero columns
   // into the |packet_mask| where those holes are found. Zero columns means that
@@ -271,7 +295,8 @@ class ForwardErrorCorrection {
 
   // Assigns pointers to already recovered packets covered by this FEC packet.
   static void AssignRecoveredPackets(
-      FecPacket* fec_packet, const RecoveredPacketList* recovered_packets);
+      ReceivedFecPacket* fec_packet,
+      const RecoveredPacketList* recovered_packets);
 
   // Insert into recovered list in correct position.
   void InsertRecoveredPacket(RecoveredPacket* rec_packet_to_insert,
@@ -281,7 +306,7 @@ class ForwardErrorCorrection {
   void AttemptRecover(RecoveredPacketList* recovered_packet_list);
 
   // Initializes the packet recovery using the FEC packet.
-  static bool StartPacketRecovery(const FecPacket* fec_packet,
+  static bool StartPacketRecovery(const ReceivedFecPacket* fec_packet,
                                   RecoveredPacket* recovered);
 
   // Performs XOR between |src_packet| and |dst_packet| and stores the result
@@ -292,21 +317,21 @@ class ForwardErrorCorrection {
   static bool FinishPacketRecovery(RecoveredPacket* recovered);
 
   // Recover a missing packet.
-  bool RecoverPacket(const FecPacket* fec_packet,
+  bool RecoverPacket(const ReceivedFecPacket* fec_packet,
                      RecoveredPacket* rec_packet_to_insert);
 
   // Get the number of missing media packets which are covered by this
   // FEC packet. An FEC packet can recover at most one packet, and if zero
   // packets are missing the FEC packet can be discarded.
   // This function returns 2 when two or more packets are missing.
-  static int NumCoveredPacketsMissing(const FecPacket* fec_packet);
+  static int NumCoveredPacketsMissing(const ReceivedFecPacket* fec_packet);
 
-  static void DiscardFecPacket(FecPacket* fec_packet);
-  static void DiscardOldPackets(RecoveredPacketList* recovered_packet_list);
+  static void DiscardOldRecoveredPackets(
+      RecoveredPacketList* recovered_packet_list);
   static uint16_t ParseSequenceNumber(uint8_t* packet);
 
-  std::vector<Packet> generated_fec_packets_;
-  FecPacketList fec_packet_list_;
+  std::vector<Packet> generated_fec_packet_list_;

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

what is motivation for this variable rename?

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

For symmetry with the name below. I agree with your other comment though, that
there is no point in explicitly naming all the variables "xxx_list".

+  ReceivedFecPacketList received_fec_packet_list_;
 
   // Arrays used to avoid dynamically allocating memory when generating
   // the packet masks in the ULPFEC headers.