Cast: Fix two video freezing problems

media/cast/transport/rtp_sender/packet_storage/packet_storage.cc

Index: media/cast/transport/rtp_sender/packet_storage/packet_storage.cc
diff --git a/media/cast/transport/rtp_sender/packet_storage/packet_storage.cc b/media/cast/transport/rtp_sender/packet_storage/packet_storage.cc
index ea1eb0b7c41f2111d66901f73ebb26d3c8c50677..ad4ae80f47cccc63e049cefda44e8d6d9e8af0d9 100644
--- a/media/cast/transport/rtp_sender/packet_storage/packet_storage.cc
+++ b/media/cast/transport/rtp_sender/packet_storage/packet_storage.cc
@@ -12,49 +12,26 @@ namespace media {
 namespace cast {
 namespace transport {
 
-// Limit the max time delay to avoid frame id wrap around; 256 / 60 fps.
-const int kMaxAllowedTimeStoredMs = 4000;
-
 typedef PacketMap::iterator PacketMapIterator;
-typedef TimeToPacketMap::iterator TimeToPacketIterator;
-
 
-PacketStorage::PacketStorage(base::TickClock* clock, int max_time_stored_ms)
-    : clock_(clock) {
-  max_time_stored_ = base::TimeDelta::FromMilliseconds(max_time_stored_ms);
-  DCHECK_LE(max_time_stored_ms, kMaxAllowedTimeStoredMs) << "Invalid argument";
+PacketStorage::PacketStorage(int stored_frames)
+    : stored_frames_(stored_frames) {
+  DCHECK_GT(stored_frames, 0);
+  DCHECK_LT(stored_frames, 250);

[Alpha Left Google, 2014/04/29 00:55:05]

Please add comments to explain why. Particularly with the algorithm in
GetPacket().

[hubbe, 2014/04/29 17:19:58]

Done.

 }
 
 PacketStorage::~PacketStorage() {
 }
 
-void PacketStorage::CleanupOldPackets(base::TimeTicks now) {
-  TimeToPacketIterator time_it = time_to_packet_map_.begin();
-
-  // Check max size.
-  while (time_to_packet_map_.size() >= kMaxStoredPackets) {
-    PacketMapIterator store_it = stored_packets_.find(time_it->second);
-
-    // We should always find the packet.
-    DCHECK(store_it != stored_packets_.end()) << "Invalid state";
-    time_to_packet_map_.erase(time_it);
-    stored_packets_.erase(store_it);
-    time_it = time_to_packet_map_.begin();
-  }
-
-  // Time out old packets.
-  while (time_it != time_to_packet_map_.end()) {
-    if (now < time_it->first + max_time_stored_) {
+void PacketStorage::CleanupOldPackets(uint32 current_frame_id) {
+  uint32 frame_to_remove = current_frame_id - stored_frames_;

[Alpha Left Google, 2014/04/29 00:55:05]

Do we care about wrap around? For audio maybe?

[hubbe, 2014/04/29 17:19:58]

We do. That's why I use IsOlderFrameId()

+  while (!stored_packets_.empty()) {
+    if (IsOlderFrameId(stored_packets_.begin()->first.first,
+                       frame_to_remove)) {
+      stored_packets_.erase(stored_packets_.begin());
+    } else {
       break;
     }
-    // Packet too old.
-    PacketMapIterator store_it = stored_packets_.find(time_it->second);
-
-    // We should always find the packet.
-    DCHECK(store_it != stored_packets_.end()) << "Invalid state";
-    time_to_packet_map_.erase(time_it);
-    stored_packets_.erase(store_it);
-    time_it = time_to_packet_map_.begin();
   }
 }
 
@@ -62,11 +39,8 @@ void PacketStorage::StorePacket(uint32 frame_id,
                                 uint16 packet_id,
                                 const PacketKey& key,
                                 PacketRef packet) {
-  base::TimeTicks now = clock_->NowTicks();
-  CleanupOldPackets(now);
-
-  // Internally we only use the 8 LSB of the frame id.
-  uint32 index = ((0xff & frame_id) << 16) + packet_id;
+  CleanupOldPackets(frame_id);
+  StorageIndex index(frame_id, packet_id);
   PacketMapIterator it = stored_packets_.find(index);
   if (it != stored_packets_.end()) {
     // We have already saved this.
@@ -74,7 +48,6 @@ void PacketStorage::StorePacket(uint32 frame_id,
     return;
   }
   stored_packets_[index] = std::make_pair(key, packet);
-  time_to_packet_map_.insert(std::make_pair(now, index));
 }
 
 void PacketStorage::GetPackets(
@@ -109,11 +82,10 @@ void PacketStorage::GetPackets(
   }
 }
 
-bool PacketStorage::GetPacket(uint8 frame_id,
-                              uint16 packet_id,
-                              SendPacketVector* packets) {
-  // Internally we only use the 8 LSB of the frame id.
-  uint32 index = (static_cast<uint32>(frame_id) << 16) + packet_id;
+bool PacketStorage::GetPacket32(uint32 frame_id,
+                                uint16 packet_id,
+                                SendPacketVector* packets) {
+  StorageIndex index(frame_id, packet_id);
   PacketMapIterator it = stored_packets_.find(index);
   if (it == stored_packets_.end()) {
     return false;
@@ -137,6 +109,26 @@ bool PacketStorage::GetPacket(uint8 frame_id,
   return true;
 }
 
+bool PacketStorage::GetPacket(uint8 frame_id_8bit,
+                              uint16 packet_id,
+                              SendPacketVector* packets) {
+  if (stored_packets_.empty()) {
+    return false;
+  }
+  uint32 last_stored = stored_packets_.rbegin()->first.first;
+  uint32 frame_id_32bit = (last_stored & ~0xFF) | frame_id_8bit;

[Alpha Left Google, 2014/04/29 00:55:05]

Do you know if we have a utility method somewhere to do this conversion?

[hubbe, 2014/04/29 17:19:58]

No idea.
The obvious place for it (cast_defines.h) didn't contain one.

+  if (IsNewerFrameId(frame_id_32bit, last_stored)) {
+    frame_id_32bit -= 0x100;
+  }
+  DCHECK_EQ(frame_id_8bit, frame_id_32bit & 0xff);
+  DCHECK(IsOlderFrameId(frame_id_32bit, last_stored) &&
+         IsNewerFrameId(frame_id_32bit + stored_frames_ + 1, last_stored))
+      << " 32bit: " << frame_id_32bit
+      << " 8bit: " << static_cast<int>(frame_id_8bit)
+      << " last_stored: " << last_stored;
+  return GetPacket32(frame_id_32bit, packet_id, packets);
+}
+
 }  // namespace transport
 }  // namespace cast
 }  // namespace media