Cleanup BWE SendTimeHistory class

webrtc/modules/remote_bitrate_estimator/include/send_time_history.h

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_INCLUDE_SEND_TIME_HISTORY_H_
 #define WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_INCLUDE_SEND_TIME_HISTORY_H_
 
 #include <map>
 
+#include "webrtc/base/basictypes.h"
 #include "webrtc/base/constructormagic.h"
-#include "webrtc/base/basictypes.h"
-#include "webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.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 AddAndRemoveOld(uint16_t sequence_number,
-                       size_t length,
-                       int probe_cluster_id);
-  bool OnSentPacket(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();
 
+  // 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 payload_size,
+                       int probe_cluster_id);
+
+  // 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 arrival_time. The packet parameter must
+  // thus be non-null and have the sequence_number field set.
+  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
 #endif  // WEBRTC_MODULES_REMOTE_BITRATE_ESTIMATOR_INCLUDE_SEND_TIME_HISTORY_H_