Change PacketKey to be unique for each frame packet by including

frame_ID. PacketKey was originally generated by combining capture_time, ssrc, and packet_id. This key may not necessarily be unique. Two or more frames with the same cpature_time had the same key and may cause one to be evicted from the list. This, in turn, will cause a receiver to become blocked waiting on a frame that the PacedSender will never attempt to send. BUG=373560 Review URL: https://codereview.chromium.org/1487223002 Cr-Commit-Position: refs/heads/master@{#363035}
marcosholgado · Dec 3, 2015 · 12ede5b · 12ede5b
1 parent 7643332
commit 12ede5b
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 31 deletions.
diff --git a/media/cast/net/pacing/paced_sender.cc b/media/cast/net/pacing/paced_sender.cc
@@ -31,10 +31,12 @@ static const size_t kRidiculousNumberOfPackets =
 DedupInfo::DedupInfo() : last_byte_acked_for_audio(0) {}
 
 // static
-PacketKey PacedPacketSender::MakePacketKey(const base::TimeTicks& ticks,
+PacketKey PacedPacketSender::MakePacketKey(PacketKey::PacketType packet_type,
+                                           uint32 frame_id,
                                            uint32 ssrc,
                                            uint16 packet_id) {
-  return std::make_pair(ticks, std::make_pair(ssrc, packet_id));
+  PacketKey key{packet_type, frame_id, ssrc, packet_id};
+  return key;
 }
 
 PacedSender::PacketSendRecord::PacketSendRecord()
@@ -125,7 +127,7 @@ bool PacedSender::ShouldResend(const PacketKey& packet_key,
   // packet Y sent just before X. Reject retransmission of X if ACK for
   // Y has not been received.
   // Only do this for video packets.
-  if (packet_key.second.first == video_ssrc_) {
+  if (packet_key.ssrc == video_ssrc_) {
     if (dedup_info.last_byte_acked_for_audio &&
         it->second.last_byte_sent_for_audio &&
         dedup_info.last_byte_acked_for_audio <
@@ -169,9 +171,9 @@ bool PacedSender::ResendPackets(const SendPacketVector& packets,
 
 bool PacedSender::SendRtcpPacket(uint32 ssrc, PacketRef packet) {
   if (state_ == State_TransportBlocked) {
-    priority_packet_list_[
-        PacedPacketSender::MakePacketKey(base::TimeTicks(), ssrc, 0)] =
-        make_pair(PacketType_RTCP, packet);
+    PacketKey key =
+        PacedPacketSender::MakePacketKey(PacketKey::RTCP, 0, ssrc, 0);
+    priority_packet_list_[key] = make_pair(PacketType_RTCP, packet);
   } else {
     // We pass the RTCP packets straight through.
     if (!transport_->SendPacket(
@@ -204,7 +206,7 @@ PacketRef PacedSender::PopNextPacket(PacketType* packet_type,
 
 bool PacedSender::IsHighPriority(const PacketKey& packet_key) const {
   return std::find(priority_ssrcs_.begin(), priority_ssrcs_.end(),
-                   packet_key.second.first) != priority_ssrcs_.end();
+                   packet_key.ssrc) != priority_ssrcs_.end();
 }
 
 bool PacedSender::empty() const {
@@ -298,7 +300,7 @@ void PacedSender::SendStoredPackets() {
     send_record.last_byte_sent_for_audio = GetLastByteSentForSsrc(audio_ssrc_);
     send_history_[packet_key] = send_record;
     send_history_buffer_[packet_key] = send_record;
-    last_byte_sent_[packet_key.second.first] = send_record.last_byte_sent;
+    last_byte_sent_[packet_key.ssrc] = send_record.last_byte_sent;
 
     if (socket_blocked) {
       state_ = State_TransportBlocked;

diff --git a/media/cast/net/pacing/paced_sender.h b/media/cast/net/pacing/paced_sender.h
@@ -6,6 +6,7 @@
 #define MEDIA_CAST_NET_PACING_PACED_SENDER_H_
 
 #include <map>
+#include <tuple>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -26,15 +27,26 @@ namespace cast {
 static const size_t kTargetBurstSize = 10;
 static const size_t kMaxBurstSize = 20;
 
-// Use std::pair for free comparison operators.
-// { capture_time, ssrc, packet_id }
 // The PacketKey is designed to meet two criteria:
 // 1. When we re-send the same packet again, we can use the packet key
 //    to identify it so that we can de-duplicate packets in the queue.
 // 2. The sort order of the PacketKey determines the order that packets
-//    are sent out. Using the capture_time as the first member basically
-//    means that older packets are sent first.
-typedef std::pair<base::TimeTicks, std::pair<uint32, uint16> > PacketKey;
+//    are sent out.
+// 3. The PacketKey is unique for each RTP (frame) packet.
+struct PacketKey {
+  enum PacketType { RTCP = 0, RTP = 1 };
+
+  PacketType packet_type;
+  uint32 frame_id;
+  uint32 ssrc;
+  uint16 packet_id;
+
+  bool operator<(const PacketKey& key) const {
+    return std::tie(packet_type, frame_id, ssrc, packet_id) <
+           std::tie(key.packet_type, key.frame_id, key.ssrc, key.packet_id);
+  }
+};
+
 typedef std::vector<std::pair<PacketKey, PacketRef> > SendPacketVector;
 
 // Information used to deduplicate retransmission packets.
@@ -68,7 +80,8 @@ class PacedPacketSender {
 
   virtual ~PacedPacketSender() {}
 
-  static PacketKey MakePacketKey(const base::TimeTicks& ticks,
+  static PacketKey MakePacketKey(PacketKey::PacketType,
+                                 uint32 frame_id,
                                  uint32 ssrc,
                                  uint16 packet_id);
 };

diff --git a/media/cast/net/pacing/paced_sender_unittest.cc b/media/cast/net/pacing/paced_sender_unittest.cc
@@ -57,7 +57,7 @@ class TestPacketSender : public PacketSender {
 
 class PacedSenderTest : public ::testing::Test {
  protected:
-  PacedSenderTest() {
+  PacedSenderTest() : frame_id_(0) {
     testing_clock_.Advance(
         base::TimeDelta::FromMilliseconds(kStartMillisecond));
     task_runner_ = new test::FakeSingleThreadTaskRunner(&testing_clock_);
@@ -77,14 +77,10 @@ class PacedSenderTest : public ::testing::Test {
                                           bool audio) {
     DCHECK_GE(packet_size, 12u);
     SendPacketVector packets;
-    base::TimeTicks frame_tick = testing_clock_.NowTicks();
-    // Advance the clock so that we don't get the same frame_tick
-    // next time this function is called.
-    testing_clock_.Advance(base::TimeDelta::FromMilliseconds(1));
     for (int i = 0; i < num_of_packets_in_frame; ++i) {
       PacketKey key = PacedPacketSender::MakePacketKey(
-          frame_tick,
-          audio ? kAudioSsrc : kVideoSsrc, // ssrc
+          PacketKey::RTP, frame_id_,
+          audio ? kAudioSsrc : kVideoSsrc,  // ssrc
           i);
 
       PacketRef packet(new base::RefCountedData<Packet>);
@@ -102,6 +98,9 @@ class PacedSenderTest : public ::testing::Test {
       CHECK(success);
       packets.push_back(std::make_pair(key, packet));
     }
+    // Increase |frame_id_| so that we don't get the same key next time this
+    // function is called.
+    ++frame_id_;
     return packets;
   }
 
@@ -124,6 +123,7 @@ class PacedSenderTest : public ::testing::Test {
   TestPacketSender mock_transport_;
   scoped_refptr<test::FakeSingleThreadTaskRunner> task_runner_;
   scoped_ptr<PacedSender> paced_sender_;
+  uint32 frame_id_;
 
   DISALLOW_COPY_AND_ASSIGN(PacedSenderTest);
 };

diff --git a/media/cast/net/rtp/packet_storage_unittest.cc b/media/cast/net/rtp/packet_storage_unittest.cc
@@ -24,18 +24,17 @@ static const size_t kStoredFrames = 10;
 static void StoreFrames(size_t number_of_frames,
                         uint32 first_frame_id,
                         PacketStorage* storage) {
-  const base::TimeTicks kTicks;
   const int kSsrc = 1;
   for (size_t i = 0; i < number_of_frames; ++i) {
     SendPacketVector packets;
     // First frame has 1 packet, second frame has 2 packets, etc.
     const size_t kNumberOfPackets = i + 1;
     for (size_t j = 0; j < kNumberOfPackets; ++j) {
       Packet test_packet(1, 0);
-      packets.push_back(
-          std::make_pair(PacedPacketSender::MakePacketKey(
-                             kTicks, kSsrc, base::checked_cast<uint16>(j)),
-                         new base::RefCountedData<Packet>(test_packet)));
+      packets.push_back(std::make_pair(
+          PacedPacketSender::MakePacketKey(PacketKey::RTP, i, kSsrc,
+                                           base::checked_cast<uint16>(j)),
+          new base::RefCountedData<Packet>(test_packet)));
     }
     storage->StoreFrame(first_frame_id, packets);
     ++first_frame_id;

diff --git a/media/cast/net/rtp/rtp_packetizer.cc b/media/cast/net/rtp/rtp_packetizer.cc
@@ -107,10 +107,8 @@ void RtpPacketizer::SendFrameAsPackets(const EncodedFrame& frame) {
                         data_iter + payload_length);
     data_iter += payload_length;
 
-    const PacketKey key =
-        PacedPacketSender::MakePacketKey(frame.reference_time,
-                                         config_.ssrc,
-                                         packet_id_++);
+    const PacketKey key = PacedPacketSender::MakePacketKey(
+        PacketKey::RTP, frame.frame_id, config_.ssrc, packet_id_++);
     packets.push_back(make_pair(key, packet));
 
     // Update stats.

diff --git a/media/cast/net/rtp/rtp_sender.cc b/media/cast/net/rtp/rtp_sender.cc
@@ -77,7 +77,7 @@ void RtpSender::ResendPackets(
     for (SendPacketVector::const_iterator it = stored_packets->begin();
          it != stored_packets->end(); ++it) {
       const PacketKey& packet_key = it->first;
-      const uint16 packet_id = packet_key.second.second;
+      const uint16 packet_id = packet_key.packet_id;
 
       // Should we resend the packet?
       bool resend = resend_all;