Update SendTimeHistory to store complete PacketInfo, not just send time

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
Committed: https://crrev.com/318673cf5a1b230d445a50fdc4869f4b8f99c85d
Cr-Commit-Position: refs/heads/master@{#9859}

[sprang_webrtc, 2015-08-19 11:10:45]

sprang@webrtc.org changed reviewers:
+ asapersson@webrtc.org

[sprang_webrtc, 2015-08-31 13:18:38]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org
- asapersson@webrtc.org

[sprang_webrtc, 2015-08-31 13:18:39]

Reassigning to stefan@

[stefan-webrtc, 2015-09-03 11:30:33]

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:29: bool
Populate(PacketInfo* packet, bool remove);
GetPacketInfo would be better I think. Populate could mean that we want to
insert packet into the history.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:35:
PacketInfo(packet)));
Do you really need PacketInfo()?

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:39: int64_t
timestamp) {
send_time_ms

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:86:
packet->was_paced = it->second.was_paced;
*packet = it->second?

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:37: //
Help struct extended so we can do EXPECT_EQ and collections.
"Help class"? Or should it be a struct?

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:55:
payload_size == other.payload_size && was_paced == other.was_paced;
put was_paced... on its own line.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:53: //
TODO(sprang): Unconstify PacketInfo so we don't need temp copy?
Has this been done already? I don't see any consts.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet.h (right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet.h:46: virtual bool
was_paced() const { return paced_; }
paced()

[sprang_webrtc, 2015-09-03 13:21:03]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2015-09-03 13:21:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288033008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288033008/40001

[commit-bot: I haz the power, 2015-09-03 13:21:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 13:21:13]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sprang_webrtc, 2015-09-03 13:24:09]

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:29: bool
Populate(PacketInfo* packet, bool remove);
On 2015/09/03 11:30:32, stefan-webrtc (holmer) wrote:
> GetPacketInfo would be better I think. Populate could mean that we want to
> insert packet into the history.

Updated, and added a comment about expected input/output.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:35:
PacketInfo(packet)));
On 2015/09/03 11:30:32, stefan-webrtc (holmer) wrote:
> Do you really need PacketInfo()?

Done.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:39: int64_t
timestamp) {
On 2015/09/03 11:30:32, stefan-webrtc (holmer) wrote:
> send_time_ms

Done.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:86:
packet->was_paced = it->second.was_paced;
On 2015/09/03 11:30:32, stefan-webrtc (holmer) wrote:
> *packet = it->second?

Done-ish. See comment i .h-file.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:37: //
Help struct extended so we can do EXPECT_EQ and collections.
On 2015/09/03 11:30:33, stefan-webrtc (holmer) wrote:
> "Help class"? Or should it be a struct?

Class. It was a struct..

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:55:
payload_size == other.payload_size && was_paced == other.was_paced;
On 2015/09/03 11:30:33, stefan-webrtc (holmer) wrote:
> put was_paced... on its own line.

clang-format says no!

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc:53: //
TODO(sprang): Unconstify PacketInfo so we don't need temp copy?
On 2015/09/03 11:30:33, stefan-webrtc (holmer) wrote:
> Has this been done already? I don't see any consts.

This comment doesn't make sense any more. Removing it.

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/packet.h (right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/packet.h:46: virtual bool
was_paced() const { return paced_; }
On 2015/09/03 11:30:33, stefan-webrtc (holmer) wrote:
> paced()

Done.

[stefan-webrtc, 2015-09-04 06:42:35]

lgtm

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc
(right):

https://codereview.webrtc.org/1288033008/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc:55:
payload_size == other.payload_size && was_paced == other.was_paced;
On 2015/09/03 13:24:09, språng wrote:
> On 2015/09/03 11:30:33, stefan-webrtc (holmer) wrote:
> > put was_paced... on its own line.
> 
> clang-format says no!

Bad clang format! But ok!

[sprang_webrtc, 2015-09-04 11:42:21]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2015-09-04 11:42:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288033008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288033008/80001

[commit-bot: I haz the power, 2015-09-04 11:43:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-04 11:43:31]

Patchset 5 (id:??) landed as
https://crrev.com/318673cf5a1b230d445a50fdc4869f4b8f99c85d
Cr-Commit-Position: refs/heads/master@{#9859}