Cleanup BWE SendTimeHistory class

webrtc/modules/remote_bitrate_estimator/include/send_time_history.h

Index: webrtc/modules/remote_bitrate_estimator/include/send_time_history.h
diff --git a/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h b/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h
index f59790b4a544de02ee6dfc5c8f7d2174e56008c8..26451ea8eed28fc747cdebd25ef253550e92f146 100644
--- a/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h
+++ b/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h
@@ -13,37 +13,44 @@
 
 #include <map>
 
-#include "webrtc/base/constructormagic.h"
 #include "webrtc/base/basictypes.h"
-#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h"
+#include "webrtc/base/constructormagic.h"
 
 namespace webrtc {
+class Clock;
+struct PacketInfo;
 
 class SendTimeHistory {
  public:
-  SendTimeHistory(Clock* clock, int64_t packet_age_limit);
-  virtual ~SendTimeHistory();
+  SendTimeHistory(Clock* clock, int64_t packet_age_limit_ms);
+  ~SendTimeHistory();
+
+  void Clear();
 
+  // Cleanup overaged entries, then add new packet info with given parameters.

[philipel, 2016/06/21 09:48:29]

Cleanup old entries...

[danilchap, 2016/06/21 11:45:30]

Done.

   void AddAndRemoveOld(uint16_t sequence_number,
-                       size_t length,
+                       size_t payload_size,
                        int probe_cluster_id);
-  bool OnSentPacket(uint16_t sequence_number, int64_t timestamp);
+
+  // Updates send_time_ms field of packet info identified by |sequence_number|.

[philipel, 2016/06/21 09:48:29]

Use |send_time_ms| or remove || from |sequence_number|, prefer to use ||.

[danilchap, 2016/06/21 11:45:30]

|| refer to function parameter. In current phrase send_time_ms doesn't (it refer
to internal field instead).
Rephrased so that send_time_ms refers to parameter now which makes more sense
for an interface description.

[philipel, 2016/06/21 12:07:14]

Acknowledged.

+  // Return false if not found.
+  bool OnSentPacket(uint16_t sequence_number, int64_t send_time_ms);
+
   // Look up PacketInfo for a sent packet, based on the sequence number, and
-  // populate all fields except for receive_time. The packet parameter must
+  // populate all fields except for arrival_time. The packet parameter must
   // thus be non-null and have the sequence_number field set.
-  bool GetInfo(PacketInfo* packet, bool remove);
-  void Clear();
+  bool GetInfo(PacketInfo* packet_info, bool remove);
 
  private:
-  void EraseOld();
-  void UpdateOldestSequenceNumber();
+  using PacketInfoHistory = std::map<uint16_t, PacketInfo>;

[philipel, 2016/06/21 09:48:29]

Include "webrtc/modules/video_coding/sequence_number_util.h" and define 
PacketInfoHistory as std::map<uint16_t, PacketInfo,
DescendingSeqNumComp<uint16_t>>.

As long as you only have at most half the interval of packets in the
PacketInfoHistory they will be sorted correctly even if the sequence numbers
wrap.

Also, you don't need to define PacketInfoHistory if you do this, just let it be
a map.

[danilchap, 2016/06/21 11:45:30]

This is good idea, but unfortunately it would create rbe -> video_coding
dependency that generally doesn't make sense.
(including header without linking dependency is still a dependency)
reimplementing comparer structure instead.
(btw, you mean AscendingSeqNumComp, not DescendingSeqNumComp?)

[philipel, 2016/06/21 12:07:14]

I guess that makes sense. I think the right thing to do is to move
"sequence_number_util.h" to some better place where it can be used for cases
such as this, but that is for another CL of course.

+  void EraseOldestPacketInfo();
 
   Clock* const clock_;
-  const int64_t packet_age_limit_;
-  uint16_t oldest_sequence_number_;  // Oldest may not be lowest.
-  std::map<uint16_t, PacketInfo> history_;
+  const int64_t packet_age_limit_ms_;
+  PacketInfoHistory history_;
+  PacketInfoHistory::iterator oldest_packet_info_;

[philipel, 2016/06/21 09:48:29]

Use custom comparator instead and remove oldest_packet_info_.

[danilchap, 2016/06/21 11:45:30]

Done.

 
-  RTC_DISALLOW_COPY_AND_ASSIGN(SendTimeHistory);
+  RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(SendTimeHistory);
 };
 
 }  // namespace webrtc