Propagate packet pacing information to SendTimeHistory.

Propagate packet pacing information to SendTimeHistory.

In order to not make this CL too large I have broken it down into at least two
steps. Previous CL: https://codereview.chromium.org/2628563003/

webrtc::PacedSender::Process                        <--- previous CL start here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- previous CL end here, this Cl start here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- this CL end here

BUG=webrtc:6822
Review-Url: https://codereview.webrtc.org/2708873003
Cr-Commit-Position: refs/heads/master@{#16796}
Committed: https://chromium.googlesource.com/external/webrtc/+/8aadd50b961e7f984020b7540f5385da62403567

[philipel, 2017-02-21 16:05:30]

philipel@webrtc.org changed reviewers:
+ nisse@webrtc.org

[nisse-webrtc, 2017-02-22 12:37:48]

Looks pretty good. I've added some comments mainly to check if I understand
what's really going on.

It seems the bulk of the changes are  replacing a cluster_id int with
PacedPacketInfo it lots of methods.

It would be nice to see the additional information taken into use, but I take it
you think that's a large enough change to be postponed to the next cl?

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:35:
PacedPacketInfo(probe_cluster_id, -1, -1));
This implies that there's no change in behavior, right?

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:541: bool operator==(const
PacedPacketInfo& rhs) const {
Is this for tests only?

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

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:48:
sequence_number, payload_size, pacing_info)));
And this is where pacing_info stops?

Where is the PacketInfo retrieved?

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:397: retransmission,
pacing_info);
And this is where pacing_info gets passed further on.

Is it populated at this point? Are values used yet at the end of the line?

[philipel, 2017-02-22 13:11:26]

> It would be nice to see the additional information taken into use, but I take
it
> you think that's a large enough change to be postponed to the next cl?

Yes, I will do that in a followup CL for two reasons:
to not make this CL larger than it already is, and
to not mix this "boilerplate-code" with the code that actually change the
probing behavior.

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:35:
PacedPacketInfo(probe_cluster_id, -1, -1));
On 2017/02/22 12:37:47, nisse-webrtc wrote:
> This implies that there's no change in behavior, right?

Yes, that's right.

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/include/mo...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:541: bool operator==(const
PacedPacketInfo& rhs) const {
On 2017/02/22 12:37:47, nisse-webrtc wrote:
> Is this for tests only?

Yes, I implemented it for convenience sake to make tests easier to write.

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

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:48:
sequence_number, payload_size, pacing_info)));
On 2017/02/22 12:37:47, nisse-webrtc wrote:
> And this is where pacing_info stops?
> 
> Where is the PacketInfo retrieved?

When we receive feedback the TransportFeedbackAdapter (which SendTimeHistory
lives in) extract the PacketInfo and combines that information with the incoming
feedback before giving it over to the DelayBasedBwe.

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2708873003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:397: retransmission,
pacing_info);
On 2017/02/22 12:37:47, nisse-webrtc wrote:
> And this is where pacing_info gets passed further on.
> 
> Is it populated at this point? Are values used yet at the end of the line?

Yes, |pacing_info| is populated by the pacer and is not used until we actually
receive feedback. The |pacing_info| is just passed around everywhere just to end
up in SendTimeHistory.

[nisse-webrtc, 2017-02-22 13:18:02]

lgtm

[philipel, 2017-02-22 13:19:39]

The CQ bit was checked by philipel@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 13:19:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[philipel, 2017-02-22 13:20:04]

philipel@webrtc.org changed reviewers:
+ stefan@webrtc.org

[philipel, 2017-02-22 13:20:05]

Stefan, PTAL

[stefan-webrtc, 2017-02-22 13:29:07]

lgtm

[commit-bot: I haz the power, 2017-02-22 13:39:23]

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

[commit-bot: I haz the power, 2017-02-22 13:39:24]

Dry run: Try jobs failed on following builders:
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/23534)

[philipel, 2017-02-22 14:13:31]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-22 14:13:32]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2708873003/#ps40001 (title:
"TransportFeedbackAdapterTest fix.")

[commit-bot: I haz the power, 2017-02-22 14:13:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 15:40:02]

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

[commit-bot: I haz the power, 2017-02-22 15:40:03]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[philipel, 2017-02-23 10:43:27]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2017-02-23 10:43:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-23 10:56:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487846607659740,
"parent_rev": "7adadb1fdfbd6c9ff38162d9ecae7d9213d99653", "commit_rev":
"8aadd50b961e7f984020b7540f5385da62403567"}

[commit-bot: I haz the power, 2017-02-23 10:56:16]

Description was changed from

==========
Propagate packet pacing information to SendTimeHistory.

In order to not make this CL too large I have broken it down into at least two
steps. Previous CL: https://codereview.chromium.org/2628563003/

webrtc::PacedSender::Process                        <--- previous CL start here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- previous CL end here,
this Cl start here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- this CL end here

BUG=webrtc:6822
==========

to

==========
Propagate packet pacing information to SendTimeHistory.

In order to not make this CL too large I have broken it down into at least two
steps. Previous CL: https://codereview.chromium.org/2628563003/

webrtc::PacedSender::Process                        <--- previous CL start here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- previous CL end here,
this Cl start here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- this CL end here

BUG=webrtc:6822

Review-Url: https://codereview.webrtc.org/2708873003
Cr-Commit-Position: refs/heads/master@{#16796}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8aadd50b961e7f984020b7540...
==========

[commit-bot: I haz the power, 2017-02-23 10:56:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/8aadd50b961e7f984020b7540...