Propagate packet pacing information to SenTimeHistory

Propagate packet pacing information to SenTimeHistory.

In order to not make this CL too large I have broken it down into at least two steps. In this CL we only propagate the pacing information part of the way:

webrtc::PacedSender::Process                        <--- propagate from here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- to here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- goal is to propagte it here


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

[philipel, 2017-01-10 15:32:49]

Description was changed from

==========
Propagate created probe clusters to ProbeBitrateEstimator.

A new interface ProbeClusterCreatedObserver is now implemented by the
CongestionController. When a probing cluster is created by BitrateProber
it will call this callback. From the CongestionController the information
is propagated down like this:

  BitrateProber::CreateProbeCluster ->
  CongestionController::OnProbingClusterCreated ->
  TransportFeedbackAdapter::OnProbingClusterCreated ->
  DelayBasedBwe::OnProbingClusterCreated ->
  ProbeBitrateEstimator::OnProbingClusterCreated

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

to

==========
Propagate created probe clusters to ProbeBitrateEstimator.

A new interface ProbeClusterCreatedObserver is now implemented by the
CongestionController. When a probing cluster is created by the BitrateProber
it will call this callback. From the CongestionController the information
is propagated down to the ProbeBitrateEstimator like this:

  BitrateProber::CreateProbeCluster ->
  CongestionController::OnProbingClusterCreated ->
  TransportFeedbackAdapter::OnProbingClusterCreated ->
  DelayBasedBwe::OnProbingClusterCreated ->
  ProbeBitrateEstimator::OnProbingClusterCreated

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

[philipel, 2017-01-10 15:41:56]

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

[nisse-webrtc, 2017-01-10 16:04:41]

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:24: virtual void
OnProbingClusterCreated(int cluster_id,
Please document what the parameters mean.

Might also make sense to collect them into a struct and pass a const&.

It's unclear to me what use the CongestionController can make of the fact that a
probe was created. I would have expected the measurement *results*, once the
probe is complete and ACKs are seen, to be signalled to the
CongestionController.

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:262: prober_(new
BitrateProber(cluster_created_observer)),
If this thing is owned by the PacedSender, which in turn is owned by the
CongestionController, maybe there's some way to short circuit the signalling so
we don't need the ProbeClusterCreatedObserver interface.

[philipel, 2017-01-11 09:03:33]

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:24: virtual void
OnProbingClusterCreated(int cluster_id,
On 2017/01/10 16:04:41, nisse-webrtc wrote:
> Please document what the parameters mean.
> 

Done

> It's unclear to me what use the CongestionController can make of the fact that
a
> probe was created. I would have expected the measurement *results*, once the
> probe is complete and ACKs are seen, to be signalled to the
> CongestionController.

The CongestionController can't do anything with this data other than pass it on.
The reason the CongestionController is the class that implements this interface
is because it is the first class that connects the BitrateProber and the
ProbeBitrateEstimator.

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:262: prober_(new
BitrateProber(cluster_created_observer)),
On 2017/01/10 16:04:41, nisse-webrtc wrote:
> If this thing is owned by the PacedSender, which in turn is owned by the
> CongestionController, maybe there's some way to short circuit the signalling
so
> we don't need the ProbeClusterCreatedObserver interface.

I can't see how we could avoid using a callback in this case.

[nisse-webrtc, 2017-01-11 09:10:32]

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:24: virtual void
OnProbingClusterCreated(int cluster_id,
On 2017/01/11 09:03:33, philipel wrote:
> The CongestionController can't do anything with this data other than pass it
on.
> The reason the CongestionController is the class that implements this
interface
> is because it is the first class that connects the BitrateProber and the
> ProbeBitrateEstimator.

Even if it delegates the processing, I still don't see how it's useful. The
result of the probe is needed before it can be useful for estimating stuff.

Or is this rather a *request* to start sending a new probe? If so, I think it
needs a different name. 

I feel I don't grasp the big picture here.

[nisse-webrtc, 2017-01-11 10:47:40]

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:25: // |cluster_id| = the id of the
cluster.
I think I understand the purpose of this now. The thing is, that the cluster id
is sent over the wire and included in the feedback messages. And this interface
is used to provide information about the probe associated with the cluster id
from the code generating the probe to the code processing the feedback.

I think a comment briefly explaining the signalling flow and the purpose of this
interface would be helpful.

Also consider if the values really are the ones needed by the callee. There are
two very related decisions: How much data to send, and how much data to receive
feedback for before using the probe as a valid measurement. It would make sense
to have the same component be responsible for both decisions.

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:30: int min_probes) = 0;
I think min_packets is a better name than min_probes.

[philipel, 2017-01-11 12:08:04]

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:25: // |cluster_id| = the id of the
cluster.
On 2017/01/11 10:47:40, nisse-webrtc wrote:
> I think I understand the purpose of this now. The thing is, that the cluster
id
> is sent over the wire and included in the feedback messages. And this
interface
> is used to provide information about the probe associated with the cluster id
> from the code generating the probe to the code processing the feedback.

Actually, the cluster id is not sent over the wire, it is saved on the sender
side in the SendTimeHistory. When we receive feedback we use the sequence number
to match the saved history with the incoming feedback.

> I think a comment briefly explaining the signalling flow and the purpose of
this
> interface would be helpful.

Adding comments where we make assumptions about how the rest of the code work is
very hard to maintain.

> Also consider if the values really are the ones needed by the callee. There
are
> two very related decisions: How much data to send, and how much data to
receive
> feedback for before using the probe as a valid measurement. It would make
sense
> to have the same component be responsible for both decisions.

I agree that the two decisions are very related, but I think adding another
class that will be used by both the BitrateProber and the ProbeBitrateEstimator
will just add more complexity to a rather straight forward design. I don't think
it's worth it.

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:30: int min_probes) = 0;
On 2017/01/11 10:47:40, nisse-webrtc wrote:
> I think min_packets is a better name than min_probes.

We have used "probes" to refer to packets being parts of a probe cluster
throughout the rest of the code, so if we want to change that then I guess that
should be done in a refactoring CL.

[nisse-webrtc, 2017-01-11 12:33:39]

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:25: // |cluster_id| = the id of the
cluster.
On 2017/01/11 12:08:04, philipel wrote:
> Adding comments where we make assumptions about how the rest of the code work
is
> very hard to maintain.

The class still needs a comment explaining its purpose. If you feel this isn't
the right place for that explanation, consider moving the declaration of the
interface to some other place where the explanation fits better.

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:25: // |cluster_id| = the id of the
cluster.
On 2017/01/11 12:08:04, philipel wrote:
> Actually, the cluster id is not sent over the wire, it is saved on the sender
> side in the SendTimeHistory. When we receive feedback we use the sequence
number
> to match the saved history with the incoming feedback.

In that case, I think information about the probe really should be added to
SendTimeHistory.

So you look up the sequence number in the SendTimeHistory, and that should
return *all* information needed to act on the feedback. Is this interface
required to get there? 

I would prefer if you extended this cl to actually store the information in the
right place; without that, the interface looks a bit premature to me and hard to
review properly.

[philipel, 2017-01-12 10:39:29]

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:25: // |cluster_id| = the id of the
cluster.
On 2017/01/11 12:33:39, nisse-webrtc wrote:
> On 2017/01/11 12:08:04, philipel wrote:
> > Actually, the cluster id is not sent over the wire, it is saved on the
sender
> > side in the SendTimeHistory. When we receive feedback we use the sequence
> number
> > to match the saved history with the incoming feedback.
> 
> In that case, I think information about the probe really should be added to
> SendTimeHistory.
> 
> So you look up the sequence number in the SendTimeHistory, and that should
> return *all* information needed to act on the feedback. Is this interface
> required to get there? 
> 
> I would prefer if you extended this cl to actually store the information in
the
> right place; without that, the interface looks a bit premature to me and hard
to
> review properly.

I think it makes more sense to store this information in the
ProbeBitrateEstimator rather than the SendTimeHistory.

[nisse-webrtc, 2017-01-12 11:25:27]

On 2017/01/12 10:39:29, philipel wrote:
>
https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
> File webrtc/modules/pacing/bitrate_prober.h (right):
> 
>
https://codereview.webrtc.org/2628563003/diff/40001/webrtc/modules/pacing/bit...
> webrtc/modules/pacing/bitrate_prober.h:25: // |cluster_id| = the id of the
> cluster.
> On 2017/01/11 12:33:39, nisse-webrtc wrote:
> > On 2017/01/11 12:08:04, philipel wrote:
> > > Actually, the cluster id is not sent over the wire, it is saved on the
> sender
> > > side in the SendTimeHistory. When we receive feedback we use the sequence
> > number
> > > to match the saved history with the incoming feedback.
> > 
> > In that case, I think information about the probe really should be added to
> > SendTimeHistory.
> > 
> > So you look up the sequence number in the SendTimeHistory, and that should
> > return *all* information needed to act on the feedback. Is this interface
> > required to get there? 
> > 
> > I would prefer if you extended this cl to actually store the information in
> the
> > right place; without that, the interface looks a bit premature to me and
hard
> to
> > review properly.
> 
> I think it makes more sense to store this information in the
> ProbeBitrateEstimator rather than the SendTimeHistory.

I'm not convinced. I guess this is partly a question of style. I would expect it
to make the code better to have a single class that is responsible for
book-keeping of all packet-send details which are needed by the time we process
the feedback. Before this cl, as far as I understand, that single class is the
SendTimeHistory, and when you need additional book-keeping, you want to split
the responsibility over several classes.

I can understand the modularization argument, that each little piece of the
feedback processing should keep track of the information it needs, and then has
its own signalling path from the sending code. But I would expect the pieces of
the feedback processing to be quite tightly coupled, and than this type of
modularization makes the code more complex, not less.

Maybe I'm not familiar enough with this code to make the right judgment. In that
case, I'm afraid you need to get a second opinion from someone who is.

And really, this would be a lot easier to discuss if you extended the cl to
actually *use* the information end-to-end, i.e., make the simplest possibly
change which replaces one of the hard-coded assumptions in the estimator with
the correct value which is signalled over the new interface.

[philipel, 2017-01-26 09:54:43]

Description was changed from

==========
Propagate created probe clusters to ProbeBitrateEstimator.

A new interface ProbeClusterCreatedObserver is now implemented by the
CongestionController. When a probing cluster is created by the BitrateProber
it will call this callback. From the CongestionController the information
is propagated down to the ProbeBitrateEstimator like this:

  BitrateProber::CreateProbeCluster ->
  CongestionController::OnProbingClusterCreated ->
  TransportFeedbackAdapter::OnProbingClusterCreated ->
  DelayBasedBwe::OnProbingClusterCreated ->
  ProbeBitrateEstimator::OnProbingClusterCreated

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

to

==========
Propagate packet pacing information to SenTimeHistory.

In order to not make this CL to large I have broken it down into at least two
steps. In this CL we only propagate the pacing information part of the way:

webrtc::PacedSender::Process                        <--- propagate from here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- to here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- goal is to propagte it
here


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

[philipel, 2017-01-26 09:55:03]

Description was changed from

==========
Propagate packet pacing information to SenTimeHistory.

In order to not make this CL to large I have broken it down into at least two
steps. In this CL we only propagate the pacing information part of the way:

webrtc::PacedSender::Process                        <--- propagate from here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- to here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- goal is to propagte it
here


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

to

==========
Propagate packet pacing information to SenTimeHistory.

In order to not make this CL too large I have broken it down into at least two
steps. In this CL we only propagate the pacing information part of the way:

webrtc::PacedSender::Process                        <--- propagate from here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- to here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- goal is to propagte it
here


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

[nisse-webrtc, 2017-01-30 09:01:51]

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

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:542: int send_bitrate_bps = -1;
You could consider using rtc::Optional rather than special values. (Not that I
see any problem with using -1 here).

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
Do you really need both this struct and PacedPacketInfo?

If you need additional fields in this struct, you could include 
PacedPacketInfo as a member. You might then be able to eliminate or simplify the
code copying the values around.

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:34: static const int kDontCare =
-1;
This is used to initialize PacedPacketInfo? I think it would be clearer with
kProbeMinBytes, kProbeMinProbes and some arbitrary but valid values.

Or if that's less writing, define a const PacedPacketInfo once and use the same
instance everywhere.

[philipel, 2017-01-30 09:31:29]

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

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/include/mo...
webrtc/modules/include/module_common_types.h:542: int send_bitrate_bps = -1;
On 2017/01/30 09:01:50, nisse-webrtc wrote:
> You could consider using rtc::Optional rather than special values. (Not that I
> see any problem with using -1 here).

Acknowledged.

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
On 2017/01/30 09:01:51, nisse-webrtc wrote:
> Do you really need both this struct and PacedPacketInfo?
> 
> If you need additional fields in this struct, you could include 
> PacedPacketInfo as a member. You might then be able to eliminate or simplify
the
> code copying the values around.

This struct contains state specific to a probe cluster, so I don't think we
should pass this type of struct around too much.

The reason I return it to the PacedSender is because they are so tightly
coupled. Maybe I should make PacedSender and BitrateProber friends?

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router_unittest.cc:34: static const int kDontCare =
-1;
On 2017/01/30 09:01:51, nisse-webrtc wrote:
> This is used to initialize PacedPacketInfo? I think it would be clearer with
> kProbeMinBytes, kProbeMinProbes and some arbitrary but valid values.
> 
> Or if that's less writing, define a const PacedPacketInfo once and use the
same
> instance everywhere.

Added kProbeMinBytes and kProbeMinProbes.

[philipel, 2017-01-30 09:31:40]

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

[commit-bot: I haz the power, 2017-01-30 09:31:51]

Dry run: 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-01-30 09:33:00]

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

[commit-bot: I haz the power, 2017-01-30 09:33:01]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16770)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22196)

[philipel, 2017-01-30 09:47:29]

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

[commit-bot: I haz the power, 2017-01-30 09:47:40]

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

[philipel, 2017-01-30 09:48:20]

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

[nisse-webrtc, 2017-01-30 10:02:48]

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
On 2017/01/30 09:31:28, philipel wrote:
> On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > Do you really need both this struct and PacedPacketInfo?
> > 
> > If you need additional fields in this struct, you could include 
> > PacedPacketInfo as a member. You might then be able to eliminate or simplify
> the
> > code copying the values around.
> 
> This struct contains state specific to a probe cluster, so I don't think we
> should pass this type of struct around too much.
> 
> The reason I return it to the PacedSender is because they are so tightly
> coupled. Maybe I should make PacedSender and BitrateProber friends?

I don't think friend declarations is an appropropriate tool here. To me, it
seems fine to declare it as a public type on either class. What I was suggesting
was to define it as

struct ProbeCluster {
  PacedPacketInfo probe_info;
  // Additional fields
  int64_t time_started_ms;
  etc.
};

The main (but small) benefit is that you can then copy or pass the embedded
probe_info without copying fields individually.

[commit-bot: I haz the power, 2017-01-30 10:26:20]

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

[commit-bot: I haz the power, 2017-01-30 10:26:21]

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

[philipel, 2017-01-30 11:58:05]

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
On 2017/01/30 10:02:48, nisse-webrtc wrote:
> On 2017/01/30 09:31:28, philipel wrote:
> > On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > > Do you really need both this struct and PacedPacketInfo?
> > > 
> > > If you need additional fields in this struct, you could include 
> > > PacedPacketInfo as a member. You might then be able to eliminate or
simplify
> > the
> > > code copying the values around.
> > 
> > This struct contains state specific to a probe cluster, so I don't think we
> > should pass this type of struct around too much.
> > 
> > The reason I return it to the PacedSender is because they are so tightly
> > coupled. Maybe I should make PacedSender and BitrateProber friends?
> 
> I don't think friend declarations is an appropropriate tool here. To me, it
> seems fine to declare it as a public type on either class. What I was
suggesting
> was to define it as
> 
> struct ProbeCluster {
>   PacedPacketInfo probe_info;
>   // Additional fields
>   int64_t time_started_ms;
>   etc.
> };
> 
> The main (but small) benefit is that you can then copy or pass the embedded
> probe_info without copying fields individually.

I didn't change the ProbeCluster struct but now CurrentCluster returns a
PacedPacketInfo instead of a ProbeCluster, so ProbeCluster can be private again.

[nisse-webrtc, 2017-01-30 13:29:47]

On 2017/01/30 11:58:05, philipel wrote:
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> File webrtc/modules/pacing/bitrate_prober.h (right):
> 
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
> On 2017/01/30 10:02:48, nisse-webrtc wrote:
> > On 2017/01/30 09:31:28, philipel wrote:
> > > On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > > > Do you really need both this struct and PacedPacketInfo?
> > > > 
> > > > If you need additional fields in this struct, you could include 
> > > > PacedPacketInfo as a member. You might then be able to eliminate or
> simplify
> > > the
> > > > code copying the values around.
> > > 
> > > This struct contains state specific to a probe cluster, so I don't think
we
> > > should pass this type of struct around too much.
> > > 
> > > The reason I return it to the PacedSender is because they are so tightly
> > > coupled. Maybe I should make PacedSender and BitrateProber friends?
> > 
> > I don't think friend declarations is an appropropriate tool here. To me, it
> > seems fine to declare it as a public type on either class. What I was
> suggesting
> > was to define it as
> > 
> > struct ProbeCluster {
> >   PacedPacketInfo probe_info;
> >   // Additional fields
> >   int64_t time_started_ms;
> >   etc.
> > };
> > 
> > The main (but small) benefit is that you can then copy or pass the embedded
> > probe_info without copying fields individually.
> 
> I didn't change the ProbeCluster struct but now CurrentCluster returns a
> PacedPacketInfo instead of a ProbeCluster, so ProbeCluster can be private
again.

That's nice. And with the change I suggest, you could simplify CurrentCluster to

  return clusters_.front().packet_info;

Wouldn't that be neater?

[philipel, 2017-01-30 14:04:08]

On 2017/01/30 13:29:47, nisse-webrtc wrote:
> On 2017/01/30 11:58:05, philipel wrote:
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > File webrtc/modules/pacing/bitrate_prober.h (right):
> > 
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
> > On 2017/01/30 10:02:48, nisse-webrtc wrote:
> > > On 2017/01/30 09:31:28, philipel wrote:
> > > > On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > > > > Do you really need both this struct and PacedPacketInfo?
> > > > > 
> > > > > If you need additional fields in this struct, you could include 
> > > > > PacedPacketInfo as a member. You might then be able to eliminate or
> > simplify
> > > > the
> > > > > code copying the values around.
> > > > 
> > > > This struct contains state specific to a probe cluster, so I don't think
> we
> > > > should pass this type of struct around too much.
> > > > 
> > > > The reason I return it to the PacedSender is because they are so tightly
> > > > coupled. Maybe I should make PacedSender and BitrateProber friends?
> > > 
> > > I don't think friend declarations is an appropropriate tool here. To me,
it
> > > seems fine to declare it as a public type on either class. What I was
> > suggesting
> > > was to define it as
> > > 
> > > struct ProbeCluster {
> > >   PacedPacketInfo probe_info;
> > >   // Additional fields
> > >   int64_t time_started_ms;
> > >   etc.
> > > };
> > > 
> > > The main (but small) benefit is that you can then copy or pass the
embedded
> > > probe_info without copying fields individually.
> > 
> > I didn't change the ProbeCluster struct but now CurrentCluster returns a
> > PacedPacketInfo instead of a ProbeCluster, so ProbeCluster can be private
> again.
> 
> That's nice. And with the change I suggest, you could simplify CurrentCluster
to
> 
>   return clusters_.front().packet_info;
> 
> Wouldn't that be neater?

That is true, but then I would have to access some members like
"cluster.pacing_info.min_probes" and some others like "cluster.sent_probes",
which I don't think is cleaner.

[nisse-webrtc, 2017-01-30 14:24:39]

On 2017/01/30 14:04:08, philipel wrote:
> On 2017/01/30 13:29:47, nisse-webrtc wrote:
> > On 2017/01/30 11:58:05, philipel wrote:
> > >
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > > File webrtc/modules/pacing/bitrate_prober.h (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > > webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
> > > On 2017/01/30 10:02:48, nisse-webrtc wrote:
> > > > On 2017/01/30 09:31:28, philipel wrote:
> > > > > On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > > > > > Do you really need both this struct and PacedPacketInfo?
> > > > > > 
> > > > > > If you need additional fields in this struct, you could include 
> > > > > > PacedPacketInfo as a member. You might then be able to eliminate or
> > > simplify
> > > > > the
> > > > > > code copying the values around.
> > > > > 
> > > > > This struct contains state specific to a probe cluster, so I don't
think
> > we
> > > > > should pass this type of struct around too much.
> > > > > 
> > > > > The reason I return it to the PacedSender is because they are so
tightly
> > > > > coupled. Maybe I should make PacedSender and BitrateProber friends?
> > > > 
> > > > I don't think friend declarations is an appropropriate tool here. To me,
> it
> > > > seems fine to declare it as a public type on either class. What I was
> > > suggesting
> > > > was to define it as
> > > > 
> > > > struct ProbeCluster {
> > > >   PacedPacketInfo probe_info;
> > > >   // Additional fields
> > > >   int64_t time_started_ms;
> > > >   etc.
> > > > };
> > > > 
> > > > The main (but small) benefit is that you can then copy or pass the
> embedded
> > > > probe_info without copying fields individually.
> > > 
> > > I didn't change the ProbeCluster struct but now CurrentCluster returns a
> > > PacedPacketInfo instead of a ProbeCluster, so ProbeCluster can be private
> > again.
> > 
> > That's nice. And with the change I suggest, you could simplify
CurrentCluster
> to
> > 
> >   return clusters_.front().packet_info;
> > 
> > Wouldn't that be neater?
> 
> That is true, but then I would have to access some members like
> "cluster.pacing_info.min_probes" and some others like "cluster.sent_probes",
> which I don't think is cleaner.

If that bothers you, you could consider using inheritance, or accessor methods.
Personally, I don't mind if access to internal fields looks a bit different from
the exported fields (and coding style seems to prefer members over inheritance
when inheritance isn't needed for interface reasons), but it's a question of
taste, I guess.

I think it's poor design to have the same collection of three values in two
different structs, which then are used in closely related code.

In addition, it's a bit confusing to have default values defined in two places,
and they're different... If you really don't want to have one shared struct,
could you at least delete one set of default values, and rename fields so that
corresponding fields in the two structs have the same name?

[philipel, 2017-01-30 15:51:46]

On 2017/01/30 14:24:39, nisse-webrtc wrote:
> On 2017/01/30 14:04:08, philipel wrote:
> > On 2017/01/30 13:29:47, nisse-webrtc wrote:
> > > On 2017/01/30 11:58:05, philipel wrote:
> > > >
> > >
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > > > File webrtc/modules/pacing/bitrate_prober.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > > > webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
> > > > On 2017/01/30 10:02:48, nisse-webrtc wrote:
> > > > > On 2017/01/30 09:31:28, philipel wrote:
> > > > > > On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > > > > > > Do you really need both this struct and PacedPacketInfo?
> > > > > > > 
> > > > > > > If you need additional fields in this struct, you could include 
> > > > > > > PacedPacketInfo as a member. You might then be able to eliminate
or
> > > > simplify
> > > > > > the
> > > > > > > code copying the values around.
> > > > > > 
> > > > > > This struct contains state specific to a probe cluster, so I don't
> think
> > > we
> > > > > > should pass this type of struct around too much.
> > > > > > 
> > > > > > The reason I return it to the PacedSender is because they are so
> tightly
> > > > > > coupled. Maybe I should make PacedSender and BitrateProber friends?
> > > > > 
> > > > > I don't think friend declarations is an appropropriate tool here. To
me,
> > it
> > > > > seems fine to declare it as a public type on either class. What I was
> > > > suggesting
> > > > > was to define it as
> > > > > 
> > > > > struct ProbeCluster {
> > > > >   PacedPacketInfo probe_info;
> > > > >   // Additional fields
> > > > >   int64_t time_started_ms;
> > > > >   etc.
> > > > > };
> > > > > 
> > > > > The main (but small) benefit is that you can then copy or pass the
> > embedded
> > > > > probe_info without copying fields individually.
> > > > 
> > > > I didn't change the ProbeCluster struct but now CurrentCluster returns a
> > > > PacedPacketInfo instead of a ProbeCluster, so ProbeCluster can be
private
> > > again.
> > > 
> > > That's nice. And with the change I suggest, you could simplify
> CurrentCluster
> > to
> > > 
> > >   return clusters_.front().packet_info;
> > > 
> > > Wouldn't that be neater?
> > 
> > That is true, but then I would have to access some members like
> > "cluster.pacing_info.min_probes" and some others like "cluster.sent_probes",
> > which I don't think is cleaner.
> 
> If that bothers you, you could consider using inheritance, or accessor
methods.
> Personally, I don't mind if access to internal fields looks a bit different
from
> the exported fields (and coding style seems to prefer members over inheritance
> when inheritance isn't needed for interface reasons), but it's a question of
> taste, I guess.
> 
> I think it's poor design to have the same collection of three values in two
> different structs, which then are used in closely related code.

Acknowledged.

> In addition, it's a bit confusing to have default values defined in two
places,
> and they're different... If you really don't want to have one shared struct,
> could you at least delete one set of default values,

I didn't remove PacketInfo::kNotAProbe to avoid making the CL too large, removed
it now.

> and rename fields so that corresponding fields in the two structs have the
same name?

The thing is that BitrateProber has the struct called ProbeCluster, so naming
ProbeCluster::id to ProbeCluster::probe_cluster_id just seems redundant and
naming PacedPacketInfo::probe_cluster_id to PacedPacketInfo::id is ambiguous.

[nisse-webrtc, 2017-01-31 09:06:26]

On 2017/01/30 15:51:46, philipel wrote:
> On 2017/01/30 14:24:39, nisse-webrtc wrote:
> > On 2017/01/30 14:04:08, philipel wrote:
> > > On 2017/01/30 13:29:47, nisse-webrtc wrote:
> > > > On 2017/01/30 11:58:05, philipel wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > > > > File webrtc/modules/pacing/bitrate_prober.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.webrtc.org/2628563003/diff/60001/webrtc/modules/pacing/bit...
> > > > > webrtc/modules/pacing/bitrate_prober.h:28: struct ProbeCluster {
> > > > > On 2017/01/30 10:02:48, nisse-webrtc wrote:
> > > > > > On 2017/01/30 09:31:28, philipel wrote:
> > > > > > > On 2017/01/30 09:01:51, nisse-webrtc wrote:
> > > > > > > > Do you really need both this struct and PacedPacketInfo?
> > > > > > > > 
> > > > > > > > If you need additional fields in this struct, you could include 
> > > > > > > > PacedPacketInfo as a member. You might then be able to eliminate
> or
> > > > > simplify
> > > > > > > the
> > > > > > > > code copying the values around.
> > > > > > > 
> > > > > > > This struct contains state specific to a probe cluster, so I don't
> > think
> > > > we
> > > > > > > should pass this type of struct around too much.
> > > > > > > 
> > > > > > > The reason I return it to the PacedSender is because they are so
> > tightly
> > > > > > > coupled. Maybe I should make PacedSender and BitrateProber
friends?
> > > > > > 
> > > > > > I don't think friend declarations is an appropropriate tool here. To
> me,
> > > it
> > > > > > seems fine to declare it as a public type on either class. What I
was
> > > > > suggesting
> > > > > > was to define it as
> > > > > > 
> > > > > > struct ProbeCluster {
> > > > > >   PacedPacketInfo probe_info;
> > > > > >   // Additional fields
> > > > > >   int64_t time_started_ms;
> > > > > >   etc.
> > > > > > };
> > > > > > 
> > > > > > The main (but small) benefit is that you can then copy or pass the
> > > embedded
> > > > > > probe_info without copying fields individually.
> > > > > 
> > > > > I didn't change the ProbeCluster struct but now CurrentCluster returns
a
> > > > > PacedPacketInfo instead of a ProbeCluster, so ProbeCluster can be
> private
> > > > again.
> > > > 
> > > > That's nice. And with the change I suggest, you could simplify
> > CurrentCluster
> > > to
> > > > 
> > > >   return clusters_.front().packet_info;
> > > > 
> > > > Wouldn't that be neater?
> > > 
> > > That is true, but then I would have to access some members like
> > > "cluster.pacing_info.min_probes" and some others like
"cluster.sent_probes",
> > > which I don't think is cleaner.
> > 
> > If that bothers you, you could consider using inheritance, or accessor
> methods.
> > Personally, I don't mind if access to internal fields looks a bit different
> from
> > the exported fields (and coding style seems to prefer members over
inheritance
> > when inheritance isn't needed for interface reasons), but it's a question of
> > taste, I guess.
> > 
> > I think it's poor design to have the same collection of three values in two
> > different structs, which then are used in closely related code.
> 
> Acknowledged.
> 
> > In addition, it's a bit confusing to have default values defined in two
> places,
> > and they're different... If you really don't want to have one shared struct,
> > could you at least delete one set of default values,
> 
> I didn't remove PacketInfo::kNotAProbe to avoid making the CL too large,
removed
> it now.
> 
> > and rename fields so that corresponding fields in the two structs have the
> same name?
> 
> The thing is that BitrateProber has the struct called ProbeCluster, so naming
> ProbeCluster::id to ProbeCluster::probe_cluster_id just seems redundant and
> naming PacedPacketInfo::probe_cluster_id to PacedPacketInfo::id is ambiguous.

Well, I'm really not comfortable with grouping the same set of numbers into two
neighboring but different structs. It's not an unusual pattern, with one struct
type 
for fields which are more public, which in one way or the other is extended with
additional
fields for internal use. I'm sure things can be arranged and named in a way to
make the
code more consistent. But we're not making much progress.

[philipel, 2017-01-31 10:47:05]

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

[commit-bot: I haz the power, 2017-01-31 10:47:10]

Dry run: 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-01-31 11:54:16]

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

[commit-bot: I haz the power, 2017-01-31 11:54:17]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-02-02 13:25:15]

I think part of the confusion is that there are actually *three* related
structs:

1. webrtc::PacketInfo, defined in rtp_rtcp_defines.h.

2. webrtc::PacedPacketInfo, new, defined in module_common_types.h.

3. webrtc::BitrateProber::ProbeCluster, defined in bitrate_prober.h (private,
looks like an extended version of 2).

I think it's mainly the overlap and inconsistency between (2) and (3) I find
disturbing. I haven't paid much attention to (1).

Can you have another look if there's some easy way you can improve consistency
(naming, default values? (And it would still make sense to me to making the
smaller struct a member of the larger one))? Feel free to add a TODO item
assigned to either of us if you think there's reasonable improvement that should
be postponed.

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/include/m...
File webrtc/modules/include/module_common_types.h (right):

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/include/m...
webrtc/modules/include/module_common_types.h:542: int send_bitrate_bps = -1;
These are the -1 default values.

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/pacing/bi...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/pacing/bi...
webrtc/modules/pacing/bitrate_prober.h:78: int min_probes = 0;
And here we have 0 default values.

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:247: struct PacketInfo {
And this struct is mostly unchanged, and a bit different from the other two, it
doesn't have the info on probe size.

[philipel, 2017-02-02 15:38:15]

On 2017/02/02 13:25:15, nisse-webrtc wrote:
> I think part of the confusion is that there are actually *three* related
> structs:
> 
> 1. webrtc::PacketInfo, defined in rtp_rtcp_defines.h.
> 
> 2. webrtc::PacedPacketInfo, new, defined in module_common_types.h.
> 
> 3. webrtc::BitrateProber::ProbeCluster, defined in bitrate_prober.h (private,
> looks like an extended version of 2).
> 
> I think it's mainly the overlap and inconsistency between (2) and (3) I find
> disturbing. I haven't paid much attention to (1).
> 
> Can you have another look if there's some easy way you can improve consistency
> (naming, default values? (And it would still make sense to me to making the
> smaller struct a member of the larger one))? Feel free to add a TODO item
> assigned to either of us if you think there's reasonable improvement that
should
> be postponed.

I have looked at it again, but simply put I can't find a way to make both of us
happy :). I have changed so that the ProbeCluster struct now has a
PacedPacketInfo. I don't feel particularly strong about it but I don't agree
with this change for two reasons:

 1. The implementation of BitrateProber now depends on a "public" struct, so the
risk is that you can't change on without having to change the other.

 2. It makes the code less clean. Having a private struct with a format closer
related to the problem this class is suppose to solve makes the code easier to
read.

Anyhow, BitrateProber is a small class and a refactoring like this is very
quick, so worst case scenario is that we have to do a quick re-refactoring if
things change to much.

[philipel, 2017-02-02 15:38:38]

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/pacing/bi...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/pacing/bi...
webrtc/modules/pacing/bitrate_prober.h:78: int min_probes = 0;
On 2017/02/02 13:25:15, nisse-webrtc wrote:
> And here we have 0 default values.

Sorry, I didn't realize that the first time you mentioned it. I have now
replaced these values with a PacedPacketInfo.

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2628563003/diff/160001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:247: struct PacketInfo {
On 2017/02/02 13:25:15, nisse-webrtc wrote:
> And this struct is mostly unchanged, and a bit different from the other two,
it
> doesn't have the info on probe size.

In order to not make this CL too large I'm waiting to change this struct until
the PacedPacketInfo actually reach SendTimeHistory.

Then the idea is to replace |probe_cluste_id| with |pacing_info|, added a TODO
about it.

[nisse-webrtc, 2017-02-02 15:47:27]

lgtm.

Thanks for the consistency improvements.

[philipel, 2017-02-02 15:49:12]

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

[commit-bot: I haz the power, 2017-02-02 15:49:25]

Dry run: 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-02 15:50:53]

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

[commit-bot: I haz the power, 2017-02-02 15:50:54]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17066)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22328)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21069)

[philipel, 2017-02-02 16:25:10]

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

[commit-bot: I haz the power, 2017-02-02 16:25:14]

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-02 17:00:15]

stefan, PTAL

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

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

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

Dry run: This issue passed the CQ dry run.

[philipel, 2017-02-16 12:27:05]

ping

[stefan-webrtc, 2017-02-16 12:35:42]

lgtm

[philipel, 2017-02-16 12:56:04]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-16 12:56:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2628563003/#ps200001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-02-16 12:56:14]

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-16 13:00:06]

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

[commit-bot: I haz the power, 2017-02-16 13:00:08]

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

[philipel, 2017-02-17 10:21:44]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-02-17 10:21:45]

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/2628563003/#ps220001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-02-17 10:21:53]

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-17 11:59:44]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487326904486380,
"parent_rev": "0baf55d23b6986c210006393e0fb1f8564204291", "commit_rev":
"c7bf32a110af7f427e14c0dc32e3f3789e51c096"}

[commit-bot: I haz the power, 2017-02-17 11:59:47]

Description was changed from

==========
Propagate packet pacing information to SenTimeHistory.

In order to not make this CL too large I have broken it down into at least two
steps. In this CL we only propagate the pacing information part of the way:

webrtc::PacedSender::Process                        <--- propagate from here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- to here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- goal is to propagte it
here


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

to

==========
Propagate packet pacing information to SenTimeHistory.

In order to not make this CL too large I have broken it down into at least two
steps. In this CL we only propagate the pacing information part of the way:

webrtc::PacedSender::Process                        <--- propagate from here
webrtc::PacedSender::SendPacket
webrtc::PacketRouter::TimeToSendPacket
webrtc::ModuleRtpRtcpImpl::TimeToSendPacket         <--- to here
webrtc::RTPSender::TimeToSendPacket
webrtc::RTPSender::PrepareAndSendPacket
webrtc::RTPSender::AddPacketToTransportFeedback
webrtc::TransportFeedbackAdapter::AddPacket
webrtc::SendTimeHistory::AddAndRemoveOld            <--- goal is to propagte it
here


BUG=webrtc:6822

Review-Url: https://codereview.webrtc.org/2628563003
Cr-Commit-Position: refs/heads/master@{#16664}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c7bf32a110af7f427e14c0dc3...
==========

[commit-bot: I haz the power, 2017-02-17 11:59:52]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/c7bf32a110af7f427e14c0dc3...