Update SendTimeHistory to store complete PacketInfo, not just send time

This will be used for the send side bitrate estimation. Storing various
meta-data about packets that can be retreived when arrival time feeback
arrives.

BUG=webrtc:4173

Review URL: https://codereview.webrtc.org/1288033008

Cr-Commit-Position: refs/heads/master@{#9859}

webrtc/modules/remote_bitrate_estimator/BUILD.gn

@@ -10,10 +10,8 @@ source_set("remote_bitrate_estimator") {
   sources = [
     "include/bwe_defines.h",
     "include/remote_bitrate_estimator.h",
-    "include/send_time_history.h",
     "rate_statistics.cc",
     "rate_statistics.h",
-    "send_time_history.cc",
   ]
 
   configs += [ "../../:common_inherited_config" ]
@@ -29,6 +27,7 @@ source_set("rbe_components") {
   sources = [
     "aimd_rate_control.cc",
     "aimd_rate_control.h",
+    "include/send_time_history.h",
     "inter_arrival.cc",
     "inter_arrival.h",
     "overuse_detector.cc",
@@ -37,6 +36,7 @@ source_set("rbe_components") {
     "overuse_estimator.h",
     "remote_bitrate_estimator_abs_send_time.cc",
     "remote_bitrate_estimator_single_stream.cc",
+    "send_time_history.cc",
   ]
 
   configs += [ "../..:common_config" ]

webrtc/modules/remote_bitrate_estimator/include/send_time_history.h

@@ -15,6 +15,7 @@
 
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/basictypes.h"
+#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
 
 namespace webrtc {
 
@@ -23,8 +24,12 @@ class SendTimeHistory {
   explicit SendTimeHistory(int64_t packet_age_limit);
   virtual ~SendTimeHistory();
 
-  void AddAndRemoveOldSendTimes(uint16_t sequence_number, int64_t timestamp);
-  bool GetSendTime(uint16_t sequence_number, int64_t* timestamp, bool remove);
+  void AddAndRemoveOld(const PacketInfo& packet);
+  bool UpdateSendTime(uint16_t sequence_number, int64_t timestamp);
+  // Look up PacketInfo for a sent packet, based on the sequence number, and
+  // populate all fields except for receive_time. The packet parameter must
+  // thus be non-null and have the sequence_number field set.
+  bool GetInfo(PacketInfo* packet, bool remove);
   void Clear();
 
  private:
@@ -33,7 +38,7 @@ class SendTimeHistory {
 
   const int64_t packet_age_limit_;
   uint16_t oldest_sequence_number_;  // Oldest may not be lowest.
-  std::map<uint16_t, int64_t> history_;
+  std::map<uint16_t, PacketInfo> history_;
 
   DISALLOW_COPY_AND_ASSIGN(SendTimeHistory);
 };

webrtc/modules/remote_bitrate_estimator/send_time_history.cc

@@ -25,14 +25,23 @@ void SendTimeHistory::Clear() {
   history_.clear();
 }
 
-void SendTimeHistory::AddAndRemoveOldSendTimes(uint16_t sequence_number,
-                                               int64_t timestamp) {
-  EraseOld(timestamp - packet_age_limit_);
+void SendTimeHistory::AddAndRemoveOld(const PacketInfo& packet) {
+  EraseOld(packet.send_time_ms - packet_age_limit_);
 
   if (history_.empty())
-    oldest_sequence_number_ = sequence_number;
+    oldest_sequence_number_ = packet.sequence_number;
 
-  history_[sequence_number] = timestamp;
+  history_.insert(
+      std::pair<uint16_t, PacketInfo>(packet.sequence_number, packet));
+}
+
+bool SendTimeHistory::UpdateSendTime(uint16_t sequence_number,
+                                     int64_t send_time_ms) {
+  auto it = history_.find(sequence_number);
+  if (it == history_.end())
+    return false;
+  it->second.send_time_ms = send_time_ms;
+  return true;
 }
 
 void SendTimeHistory::EraseOld(int64_t limit) {
@@ -40,7 +49,7 @@ void SendTimeHistory::EraseOld(int64_t limit) {
     auto it = history_.find(oldest_sequence_number_);
     assert(it != history_.end());
 
-    if (it->second > limit)
+    if (it->second.send_time_ms > limit)
       return;  // Oldest packet within age limit, return.
 
     // TODO(sprang): Warn if erasing (too many) old items?
@@ -68,16 +77,16 @@ void SendTimeHistory::UpdateOldestSequenceNumber() {
   oldest_sequence_number_ = it->first;
 }
 
-bool SendTimeHistory::GetSendTime(uint16_t sequence_number,
-                                  int64_t* timestamp,
-                                  bool remove) {
-  auto it = history_.find(sequence_number);
+bool SendTimeHistory::GetInfo(PacketInfo* packet, bool remove) {
+  auto it = history_.find(packet->sequence_number);
   if (it == history_.end())
     return false;
-  *timestamp = it->second;
+  int64_t receive_time = packet->arrival_time_ms;
+  *packet = it->second;
+  packet->arrival_time_ms = receive_time;
   if (remove) {
     history_.erase(it);
-    if (sequence_number == oldest_sequence_number_)
+    if (packet->sequence_number == oldest_sequence_number_)
       UpdateOldestSequenceNumber();
   }
   return true;

webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc

@@ -17,6 +17,7 @@
 #include "webrtc/system_wrappers/interface/clock.h"
 
 namespace webrtc {
+namespace test {
 
 static const int kDefaultHistoryLengthMs = 1000;
 
@@ -33,117 +34,191 @@ class SendTimeHistoryTest : public ::testing::Test {
   webrtc::SimulatedClock clock_;
 };
 
+// Help class extended so we can do EXPECT_EQ and collections.
+class PacketInfo : public webrtc::PacketInfo {
+ public:
+  PacketInfo() : webrtc::PacketInfo(0, 0, 0, 0, false) {}
+  PacketInfo(int64_t arrival_time_ms,
+             int64_t send_time_ms,
+             uint16_t sequence_number,
+             size_t payload_size,
+             bool was_paced)
+      : webrtc::PacketInfo(arrival_time_ms,
+                           send_time_ms,
+                           sequence_number,
+                           payload_size,
+                           was_paced) {}
+  bool operator==(const PacketInfo& other) const {
+    return arrival_time_ms == other.arrival_time_ms &&
+           send_time_ms == other.send_time_ms &&
+           sequence_number == other.sequence_number &&
+           payload_size == other.payload_size && was_paced == other.was_paced;
+  }
+};
+
 TEST_F(SendTimeHistoryTest, AddRemoveOne) {
-  const uint16_t kSeqNo = 1;
-  const int64_t kTimestamp = 2;
-  history_.AddAndRemoveOldSendTimes(kSeqNo, kTimestamp);
+  const uint16_t kSeqNo = 10;
+  const PacketInfo kSentPacket = {0, 1, kSeqNo, 1, true};
+  history_.AddAndRemoveOld(kSentPacket);
 
-  int64_t time = 0;
-  EXPECT_TRUE(history_.GetSendTime(kSeqNo, &time, false));
-  EXPECT_EQ(kTimestamp, time);
+  PacketInfo received_packet = {0, 0, kSeqNo, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&received_packet, false));
+  EXPECT_EQ(kSentPacket, received_packet);
 
-  time = 0;
-  EXPECT_TRUE(history_.GetSendTime(kSeqNo, &time, true));
-  EXPECT_EQ(kTimestamp, time);
+  received_packet = {0, 0, kSeqNo, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&received_packet, true));
+  EXPECT_EQ(kSentPacket, received_packet);
 
-  time = 0;
-  EXPECT_FALSE(history_.GetSendTime(kSeqNo, &time, true));
+  received_packet = {0, 0, kSeqNo, 0, false};
+  EXPECT_FALSE(history_.GetInfo(&received_packet, true));
+}
+
+TEST_F(SendTimeHistoryTest, UpdateSendTime) {
+  const uint16_t kSeqNo = 10;
+  const int64_t kSendTime = 1000;
+  const int64_t kSendTimeUpdated = 2000;
+  const PacketInfo kSentPacket = {0, kSendTime, kSeqNo, 1, true};
+  const PacketInfo kUpdatedPacket = {0, kSendTimeUpdated, kSeqNo, 1, true};
+
+  history_.AddAndRemoveOld(kSentPacket);
+  PacketInfo info = {0, 0, kSeqNo, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, false));
+  EXPECT_EQ(kSentPacket, info);
+
+  EXPECT_TRUE(history_.UpdateSendTime(kSeqNo, kSendTimeUpdated));
+
+  info = {0, 0, kSeqNo, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, true));
+  EXPECT_EQ(kUpdatedPacket, info);
+
+  EXPECT_FALSE(history_.UpdateSendTime(kSeqNo, kSendTimeUpdated));
+}
+
+TEST_F(SendTimeHistoryTest, PopulatesExpectedFields) {
+  const uint16_t kSeqNo = 10;
+  const int64_t kSendTime = 1000;
+  const int64_t kReceiveTime = 2000;
+  const size_t kPayloadSize = 42;
+  const bool kPaced = true;
+  const PacketInfo kSentPacket = {0, kSendTime, kSeqNo, kPayloadSize, kPaced};
+
+  history_.AddAndRemoveOld(kSentPacket);
+
+  PacketInfo info = {kReceiveTime, 0, kSeqNo, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, true));
+  EXPECT_EQ(kReceiveTime, info.arrival_time_ms);
+  EXPECT_EQ(kSendTime, info.send_time_ms);
+  EXPECT_EQ(kSeqNo, info.sequence_number);
+  EXPECT_EQ(kPayloadSize, info.payload_size);
+  EXPECT_EQ(kPaced, info.was_paced);
 }
 
 TEST_F(SendTimeHistoryTest, AddThenRemoveOutOfOrder) {
-  struct Timestamp {
-    Timestamp(uint16_t sequence_number, int64_t timestamp)
-        : sequence_number(sequence_number), timestamp(timestamp) {}
-    uint16_t sequence_number;
-    int64_t timestamp;
-  };
-  std::vector<Timestamp> timestamps;
+  std::vector<PacketInfo> sent_packets;
+  std::vector<PacketInfo> received_packets;
   const size_t num_items = 100;
+  const size_t kPacketSize = 400;
+  const size_t kTransmissionTime = 1234;
+  const bool kPaced = true;
   for (size_t i = 0; i < num_items; ++i) {
-    timestamps.push_back(
-        Timestamp(static_cast<uint16_t>(i), static_cast<int64_t>(i)));
+    sent_packets.push_back(PacketInfo(0, static_cast<int64_t>(i),
+                                      static_cast<uint16_t>(i), kPacketSize,
+                                      kPaced));
+    received_packets.push_back(
+        PacketInfo(static_cast<int64_t>(i) + kTransmissionTime, 0,
+                   static_cast<uint16_t>(i), kPacketSize, false));
   }
-  std::vector<Timestamp> randomized_timestamps = timestamps;
-  std::random_shuffle(randomized_timestamps.begin(),
-                      randomized_timestamps.end());
+  for (size_t i = 0; i < num_items; ++i)
+    history_.AddAndRemoveOld(sent_packets[i]);
+  std::random_shuffle(received_packets.begin(), received_packets.end());
   for (size_t i = 0; i < num_items; ++i) {
-    history_.AddAndRemoveOldSendTimes(timestamps[i].sequence_number,
-                                      timestamps[i].timestamp);
-  }
-  for (size_t i = 0; i < num_items; ++i) {
-    int64_t timestamp;
-    EXPECT_TRUE(history_.GetSendTime(randomized_timestamps[i].sequence_number,
-                                     &timestamp, false));
-    EXPECT_EQ(randomized_timestamps[i].timestamp, timestamp);
-    EXPECT_TRUE(history_.GetSendTime(randomized_timestamps[i].sequence_number,
-                                     &timestamp, true));
-  }
-  for (size_t i = 0; i < num_items; ++i) {
-    int64_t timestamp;
-    EXPECT_FALSE(
-        history_.GetSendTime(timestamps[i].sequence_number, &timestamp, false));
+    PacketInfo packet = received_packets[i];
+    EXPECT_TRUE(history_.GetInfo(&packet, false));
+    PacketInfo sent_packet = sent_packets[packet.sequence_number];
+    sent_packet.arrival_time_ms = packet.arrival_time_ms;
+    EXPECT_EQ(sent_packet, packet);
+    EXPECT_TRUE(history_.GetInfo(&packet, true));
   }
+  for (PacketInfo packet : sent_packets)
+    EXPECT_FALSE(history_.GetInfo(&packet, false));
 }
 
 TEST_F(SendTimeHistoryTest, HistorySize) {
   const int kItems = kDefaultHistoryLengthMs / 100;
+  for (int i = 0; i < kItems; ++i)
+    history_.AddAndRemoveOld(PacketInfo(0, i * 100, i, 0, false));
   for (int i = 0; i < kItems; ++i) {
-    history_.AddAndRemoveOldSendTimes(i, i * 100);
+    PacketInfo info = {0, 0, static_cast<uint16_t>(i), 0, false};
+    EXPECT_TRUE(history_.GetInfo(&info, false));
+    EXPECT_EQ(i * 100, info.send_time_ms);
   }
-  int64_t timestamp;
-  for (int i = 0; i < kItems; ++i) {
-    EXPECT_TRUE(history_.GetSendTime(i, &timestamp, false));
-    EXPECT_EQ(i * 100, timestamp);
-  }
-  history_.AddAndRemoveOldSendTimes(kItems, kItems * 100);
-  EXPECT_FALSE(history_.GetSendTime(0, &timestamp, false));
+  history_.AddAndRemoveOld(PacketInfo(0, kItems * 100, kItems, 0, false));
+  PacketInfo info = {0, 0, 0, 0, false};
+  EXPECT_FALSE(history_.GetInfo(&info, false));
   for (int i = 1; i < (kItems + 1); ++i) {
-    EXPECT_TRUE(history_.GetSendTime(i, &timestamp, false));
-    EXPECT_EQ(i * 100, timestamp);
+    info = {0, 0, static_cast<uint16_t>(i), 0, false};
+    EXPECT_TRUE(history_.GetInfo(&info, false));
+    EXPECT_EQ(i * 100, info.send_time_ms);
   }
 }
 
 TEST_F(SendTimeHistoryTest, HistorySizeWithWraparound) {
-  const int kMaxSeqNo = std::numeric_limits<uint16_t>::max();
-  history_.AddAndRemoveOldSendTimes(kMaxSeqNo - 2, 0);
-  history_.AddAndRemoveOldSendTimes(kMaxSeqNo - 1, 100);
-  history_.AddAndRemoveOldSendTimes(kMaxSeqNo, 200);
-  history_.AddAndRemoveOldSendTimes(0, 1000);
-  int64_t timestamp;
-  EXPECT_FALSE(history_.GetSendTime(kMaxSeqNo - 2, &timestamp, false));
-  EXPECT_TRUE(history_.GetSendTime(kMaxSeqNo - 1, &timestamp, false));
-  EXPECT_TRUE(history_.GetSendTime(kMaxSeqNo, &timestamp, false));
-  EXPECT_TRUE(history_.GetSendTime(0, &timestamp, false));
+  const uint16_t kMaxSeqNo = std::numeric_limits<uint16_t>::max();
+  history_.AddAndRemoveOld(PacketInfo(0, 0, kMaxSeqNo - 2, 0, false));
+  history_.AddAndRemoveOld(PacketInfo(0, 100, kMaxSeqNo - 1, 0, false));
+  history_.AddAndRemoveOld(PacketInfo(0, 200, kMaxSeqNo, 0, false));
+  history_.AddAndRemoveOld(PacketInfo(0, kDefaultHistoryLengthMs, 0, 0, false));
+  PacketInfo info = {0, 0, static_cast<uint16_t>(kMaxSeqNo - 2), 0, false};
+  EXPECT_FALSE(history_.GetInfo(&info, false));
+  info = {0, 0, static_cast<uint16_t>(kMaxSeqNo - 1), 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, false));
+  info = {0, 0, static_cast<uint16_t>(kMaxSeqNo), 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, false));
+  info = {0, 0, 0, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, false));
 
   // Create a gap (kMaxSeqNo - 1) -> 0.
-  EXPECT_TRUE(history_.GetSendTime(kMaxSeqNo, &timestamp, true));
+  info = {0, 0, kMaxSeqNo, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, true));
 
-  history_.AddAndRemoveOldSendTimes(1, 1100);
+  history_.AddAndRemoveOld(PacketInfo(0, 1100, 1, 0, false));
 
-  EXPECT_FALSE(history_.GetSendTime(kMaxSeqNo - 2, &timestamp, false));
-  EXPECT_FALSE(history_.GetSendTime(kMaxSeqNo - 1, &timestamp, false));
-  EXPECT_FALSE(history_.GetSendTime(kMaxSeqNo, &timestamp, false));
-  EXPECT_TRUE(history_.GetSendTime(0, &timestamp, false));
-  EXPECT_TRUE(history_.GetSendTime(1, &timestamp, false));
+  info = {0, 0, static_cast<uint16_t>(kMaxSeqNo - 2), 0, false};
+  EXPECT_FALSE(history_.GetInfo(&info, false));
+  info = {0, 0, static_cast<uint16_t>(kMaxSeqNo - 1), 0, false};
+  EXPECT_FALSE(history_.GetInfo(&info, false));
+  info = {0, 0, kMaxSeqNo, 0, false};
+  EXPECT_FALSE(history_.GetInfo(&info, false));
+  info = {0, 0, 0, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, false));
+  info = {0, 0, 1, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, false));
 }
 
 TEST_F(SendTimeHistoryTest, InterlievedGetAndRemove) {
   const uint16_t kSeqNo = 1;
   const int64_t kTimestamp = 2;
+  PacketInfo packets[3] = {{0, kTimestamp, kSeqNo, 0, false},
+                           {0, kTimestamp + 1, kSeqNo + 1, 0, false},
+                           {0, kTimestamp + 2, kSeqNo + 2, 0, false}};
 
-  history_.AddAndRemoveOldSendTimes(kSeqNo, kTimestamp);
-  history_.AddAndRemoveOldSendTimes(kSeqNo + 1, kTimestamp + 1);
+  history_.AddAndRemoveOld(packets[0]);
+  history_.AddAndRemoveOld(packets[1]);
 
-  int64_t time = 0;
-  EXPECT_TRUE(history_.GetSendTime(kSeqNo, &time, true));
-  EXPECT_EQ(kTimestamp, time);
+  PacketInfo info = {0, 0, packets[0].sequence_number, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, true));
+  EXPECT_EQ(packets[0], info);
 
-  history_.AddAndRemoveOldSendTimes(kSeqNo + 2, kTimestamp + 2);
+  history_.AddAndRemoveOld(packets[2]);
 
-  EXPECT_TRUE(history_.GetSendTime(kSeqNo + 1, &time, true));
-  EXPECT_EQ(kTimestamp + 1, time);
-  EXPECT_TRUE(history_.GetSendTime(kSeqNo + 2, &time, true));
-  EXPECT_EQ(kTimestamp + 2, time);
+  info = {0, 0, packets[1].sequence_number, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, true));
+  EXPECT_EQ(packets[1], info);
+
+  info = {0, 0, packets[2].sequence_number, 0, false};
+  EXPECT_TRUE(history_.GetInfo(&info, true));
+  EXPECT_EQ(packets[2], info);
 }
 
+}  // namespace test
 }  // namespace webrtc

webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc

@@ -132,7 +132,8 @@ Packet::Packet()
       creation_time_us_(-1),
       send_time_us_(-1),
       sender_timestamp_us_(-1),
-      payload_size_(0) {
+      payload_size_(0),
+      paced_(false) {
 }
 
 Packet::Packet(int flow_id, int64_t send_time_us, size_t payload_size)
@@ -140,7 +141,8 @@ Packet::Packet(int flow_id, int64_t send_time_us, size_t payload_size)
       creation_time_us_(send_time_us),
       send_time_us_(send_time_us),
       sender_timestamp_us_(send_time_us),
-      payload_size_(payload_size) {
+      payload_size_(payload_size),
+      paced_(false) {
 }
 
 Packet::~Packet() {

webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc

@@ -50,11 +50,9 @@ void FullBweSender::GiveFeedback(const FeedbackPacket& feedback) {
       static_cast<const SendSideBweFeedback&>(feedback);
   if (fb.packet_feedback_vector().empty())
     return;
-  // TODO(sprang): Unconstify PacketInfo so we don't need temp copy?
   std::vector<PacketInfo> packet_feedback_vector(fb.packet_feedback_vector());
-  for (PacketInfo& packet : packet_feedback_vector) {
-    if (!send_time_history_.GetSendTime(packet.sequence_number,
-                                        &packet.send_time_ms, true)) {
+  for (PacketInfo& packet_info : packet_feedback_vector) {
+    if (!send_time_history_.GetInfo(&packet_info, true)) {
       LOG(LS_WARNING) << "Ack arrived too late.";
     }
   }
@@ -95,9 +93,10 @@ void FullBweSender::OnPacketsSent(const Packets& packets) {
   for (Packet* packet : packets) {
     if (packet->GetPacketType() == Packet::kMedia) {
       MediaPacket* media_packet = static_cast<MediaPacket*>(packet);
-      send_time_history_.AddAndRemoveOldSendTimes(
-          media_packet->header().sequenceNumber,
-          media_packet->GetAbsSendTimeInMs());
+      PacketInfo info(0, media_packet->sender_timestamp_ms(),
+                      media_packet->header().sequenceNumber,
+                      media_packet->payload_size(), packet->paced());
+      send_time_history_.AddAndRemoveOld(info);
     }
   }
 }

webrtc/modules/remote_bitrate_estimator/test/packet.h

@@ -42,6 +42,8 @@ class Packet {
   virtual void set_sender_timestamp_us(int64_t sender_timestamp_us) {
     sender_timestamp_us_ = sender_timestamp_us;
   }
+  virtual void set_paced(bool paced) { paced_ = paced; }
+  virtual bool paced() const { return paced_; }
   virtual int64_t creation_time_ms() const {
     return (creation_time_us_ + 500) / 1000;
   }
@@ -56,6 +58,7 @@ class Packet {
   int64_t send_time_us_;  // Time the packet left last processor touching it.
   int64_t sender_timestamp_us_;  // Time the packet left the Sender.
   size_t payload_size_;  // Size of the (non-existent, simulated) payload.
+  bool paced_;           // True if sent through paced sender.
 };
 
 class MediaPacket : public Packet {

webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc

@@ -271,6 +271,8 @@ void PacedVideoSender::QueuePackets(Packets* batch,
   }
   Packets to_transfer;
   to_transfer.splice(to_transfer.begin(), queue_, queue_.begin(), it);
+  for (Packet* packet : to_transfer)
+    packet->set_paced(true);
   bwe_->OnPacketsSent(to_transfer);
   batch->merge(to_transfer, DereferencingComparator<Packet>);
 }

webrtc/video_engine/vie_channel_group.cc

@@ -149,7 +149,8 @@ class AdaptedSendTimeHistory : public SendTimeHistory, public SendTimeObserver {
   virtual ~AdaptedSendTimeHistory() {}
 
   void OnPacketSent(uint16_t sequence_number, int64_t send_time) override {
-    SendTimeHistory::AddAndRemoveOldSendTimes(sequence_number, send_time);
+    PacketInfo info(0, send_time, sequence_number, 0, false);
+    SendTimeHistory::AddAndRemoveOld(info);
   }
 };