Cast: reduce the amount of retransmission packets

media/cast/transport/rtp_sender/packet_storage/packet_storage.h

Index: media/cast/transport/rtp_sender/packet_storage/packet_storage.h
diff --git a/media/cast/transport/rtp_sender/packet_storage/packet_storage.h b/media/cast/transport/rtp_sender/packet_storage/packet_storage.h
index 85efc664c48b96f7574d22c456a9c8397651df78..3883e1b868a9e6231a9bd0ba1d6e5444cf492067 100644
--- a/media/cast/transport/rtp_sender/packet_storage/packet_storage.h
+++ b/media/cast/transport/rtp_sender/packet_storage/packet_storage.h
@@ -5,6 +5,7 @@
 #ifndef MEDIA_CAST_TRANSPORT_RTP_SENDER_PACKET_STORAGE_PACKET_STORAGE_H_
 #define MEDIA_CAST_TRANSPORT_RTP_SENDER_PACKET_STORAGE_PACKET_STORAGE_H_
 
+#include <deque>
 #include <list>
 #include <map>
 #include <vector>
@@ -28,45 +29,45 @@ class StoredPacket;
 typedef std::pair<uint32, uint16> StorageIndex;
 typedef std::map<StorageIndex, std::pair<PacketKey, PacketRef> > PacketMap;
 
+// Stores a list of frames. Each frame consists a list of packets.
+typedef std::deque<SendPacketVector> FrameList;

[miu, 2014/06/07 00:59:42]

naming nit: FrameQueue or FrameDeque?

[Alpha Left Google, 2014/06/07 01:15:09]

Done.

+
 // Frame IDs are generally stored as 8-bit values when sent over the
 // wire. This means that having a history longer than 255 frames makes
 // no sense.
-const int kMaxStoredFrames = 255;
+const size_t kMaxStoredFrames = 255;

[miu, 2014/06/07 00:59:41]

You can kill this.  This constant is defined in cast_defines.h
(kMaxUnackedFrames):
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/cast_de...

[Alpha Left Google, 2014/06/07 01:15:09]

Done.

 
 class PacketStorage {
  public:
-  PacketStorage(int stored_frames);
+  PacketStorage(size_t stored_frames);

[miu, 2014/06/07 00:59:41]

Need 'explicit' keyword.

[miu, 2014/06/07 00:59:41]

This pruning/upper-limit would not be necessary if AudioSender and VideoSender
could call a ReleaseFrame() method once it gets an ACK for a frame.

[Alpha Left Google, 2014/06/07 01:15:09]

I think I'll keep the current design. There is extra plumbing to do what you
suggested and the benefit is unclear to me.

[miu, 2014/06/07 02:00:09]

SGTM for this change.  The benefits I see are:

1. Reduced memory usage: PacketStorage would only have to keep those frames in
memory that the receiver does not yet have.
2. PacketStorage could store any number of frames, without a need to configure a
"max limit" and also not need any pruning logic.

   virtual ~PacketStorage();
 
   // Returns true if this class is configured correctly.
   // (stored frames > 0 && stored_frames < kMaxStoredFrames)
   bool IsValid() const;
 
-  void StorePacket(uint32 frame_id,
-                   uint16 packet_id,
-                   const PacketKey& key,
-                   PacketRef packet);
+  // Store a list of packets for a frame.

[miu, 2014/06/07 00:59:41]

nit: s/a list of/all of the/
 

[Alpha Left Google, 2014/06/07 01:15:09]

Done.

+  void StoreFrame(uint32 frame_id, const SendPacketVector& packets);
 
-  // Copies all missing packets into the packet list.
-  void GetPackets(
-      const MissingFramesAndPacketsMap& missing_frames_and_packets,
-      SendPacketVector* packets_to_resend);
+  // Returns a list of packets for a frame indexed by a 8-bits ID.

[miu, 2014/06/07 00:59:41]

s/by a 8-bits ID/by the lowest 8 bits of the ID/

BTW--I understand why you have to use a uint8 here, but it really sucks.  In
future code changes, we need to stop propagating the data types that were used
to optimize the wire format, and start using the appropriate full-width data
types throughout the cast library.

[Alpha Left Google, 2014/06/07 01:15:09]

This one unfortunately needs to be 8 bits because it comes from NACK. :(

The method is specifically called GetFrame8 to clarify so we know what happens
when passing in int32 or something else.

+  // Returns NULL if the frame cannot be found.
+  const SendPacketVector* GetFrame8(uint8 frame_id_8bits) const;
 
-  // Copies packet into the packet list.
-  bool GetPacket(uint8 frame_id_8bit,
-                 uint16 packet_id,
-                 SendPacketVector* packets);
- private:
-  FRIEND_TEST_ALL_PREFIXES(PacketStorageTest, PacketContent);
+  // Get the number of stored frames.
+  size_t GetNumberOfStoredFrames() const;
 
-  // Same as GetPacket, but takes a 32-bit frame id.
-  bool GetPacket32(uint32 frame_id,
-                   uint16 packet_id,
-                   SendPacketVector* packets);
-  void CleanupOldPackets(uint32 current_frame_id);
+  // If there is only one referecne to the packet then copy the
+  // reference and return.
+  // Otherwise return a deep copy of the packet.
+  static PacketRef FastCopyPacket(const PacketRef& packet);

[miu, 2014/06/07 00:59:41]

There's no need for this to be a member of this class.  The method is only
called in one place, from RtpSender, and the logic is fairly trivial.  It's just
an extra layer of indirection.

[Alpha Left Google, 2014/06/07 01:15:09]

There might be other place that needs this. And it was originally in
PacketStorage. In this particular case it is because PacketStoarge keeps one ref
that this fast copy can be done.

[miu, 2014/06/07 02:00:09]

However, the function is static.  It has no way of knowing what object holds the
"one ref."  The holder might not be the PacketStorage instance the caller is
using.  On the other hand, RtpSender *does* know this the case.

Thinking about this "encapsulation of functionality problem" further, I think
you should just get rid of the PacketStorage class, and move all its code into
RtpSender.  WDYT?

 
+ private:
   PacketMap stored_packets_;

[miu, 2014/06/07 00:59:41]

naming nit: Can we call this max_frames_to_keep_ instead?  At least,
s/packet/frame/.

[miu, 2014/06/07 00:59:41]

This member is no longer used.

[Alpha Left Google, 2014/06/07 01:15:09]

Done.

[Alpha Left Google, 2014/06/07 01:15:09]

Changed it to max_stored_frames_.

-  int stored_frames_;
+  size_t stored_frames_;

[miu, 2014/06/07 00:59:42]

nit: const

[Alpha Left Google, 2014/06/07 01:15:09]

Done.

+
+  FrameList frames_;
+  uint32 first_frame_id_in_list_;
+  uint32 last_frame_id_in_list_;
 
   DISALLOW_COPY_AND_ASSIGN(PacketStorage);
 };