Added cluster id to PacedSender::Callback::TimeToSendPacket.

Added cluster id to PacedSender::Callback::TimeToSendPacket.
Also added cluster id to paced_sender::Packet and set the cluster id of
the probing packet that is about to be sent.

BUG=webrtc:5859
R=danilchap@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/29dca2ce9599f16cd7931a90cdfabb4a5a6092f1
Cr-Commit-Position: refs/heads/master@{#12718}

[philipel, 2016-05-10 14:38:39]

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

[danilchap, 2016-05-10 15:31:39]

What is the plan for cluster_id? where do you plan to use it?

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/bitrate...
File webrtc/modules/pacing/bitrate_prober.cc (left):

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:70: bitrate_log << "Start probing for
bandwidth, bitrates:";
is log message change related to cluster_id?

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:51: int cluster_id = -1)
try to avoid default arguments. Here it is easy: constructor is called just once
and the -1 value could be added there, into InsertPacket function.
Or, if there is no plan to know cluster_id when you create Packet object, just
do not have this parameter and initialized cluster_id member with a constant.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:71: int cluster_id;
may be instead of modifying Packet structure, using non-const Pop, unrecommended
default argument, etc. it would be easier to add an argument to SendPacket
function: cluster_id is not really property of a packet but property of the
current state when packet is sent.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:119: Packet& BeginPop() {
If you want return a non-const object, return a pointer instead of reference.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:397: packet.cluster_id =
prober_->CurrentClusterId();
should you clear cluster_id if not probing? Either here or before calling
CancelPop.

[philipel, 2016-05-11 09:39:16]

The cluster id is suppose to be saved in the SendTimeHistory class so when then
sender gets receiver reports it can easily match which packet that was used for
which probing attempt. Today there is logic which guess if a packet was used for
probing or not but it's going to be harder to do that if we probe mid-call.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/bitrate...
File webrtc/modules/pacing/bitrate_prober.cc (left):

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:70: bitrate_log << "Start probing for
bandwidth, bitrates:";
On 2016/05/10 15:31:39, danilchap wrote:
> is log message change related to cluster_id?

No, not really. It was formatted a bit strangely so I just made it a bit more
readable.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:51: int cluster_id = -1)
On 2016/05/10 15:31:39, danilchap wrote:
> try to avoid default arguments. Here it is easy: constructor is called just
once
> and the -1 value could be added there, into InsertPacket function.
> Or, if there is no plan to know cluster_id when you create Packet object, just
> do not have this parameter and initialized cluster_id member with a constant.

Removed default argument, |cluster_id| is not initialized to -1.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:71: int cluster_id;
On 2016/05/10 15:31:39, danilchap wrote:
> may be instead of modifying Packet structure, using non-const Pop,
unrecommended
> default argument, etc. it would be easier to add an argument to SendPacket
> function: cluster_id is not really property of a packet but property of the
> current state when packet is sent.

I agree, removed |cluster_id| From Packet class.

https://codereview.webrtc.org/1962303002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:119: Packet& BeginPop() {
On 2016/05/10 15:31:39, danilchap wrote:
> If you want return a non-const object, return a pointer instead of reference.

True, but now it's const again :)

[danilchap, 2016-05-11 09:58:52]

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

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:117: const Packet& packet =
*prio_queue_.top();
Restore const&

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

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:393: int cluster_id = prober_->IsProbing()
? prober_->CurrentClusterId() : -1;
may be rename variable to prober_cluster_id everywhere except here.
Here it is easy to see this variable is about prober.
In send functions it will not be obvious outside this CL.

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.h:47: int cluster_id = -1) override;
Default arguments are banned on virtual functions.

[philipel, 2016-05-11 14:19:05]

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

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:117: const Packet& packet =
*prio_queue_.top();
On 2016/05/11 09:58:52, danilchap wrote:
> Restore const&

Done.

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

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:393: int cluster_id = prober_->IsProbing()
? prober_->CurrentClusterId() : -1;
On 2016/05/11 09:58:52, danilchap wrote:
> may be rename variable to prober_cluster_id everywhere except here.
> Here it is easy to see this variable is about prober.
> In send functions it will not be obvious outside this CL.

Done.

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/1962303002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.h:47: int cluster_id = -1) override;
On 2016/05/11 09:58:52, danilchap wrote:
> Default arguments are banned on virtual functions.

Done.

[danilchap, 2016-05-11 14:27:10]

lgtm

[philipel, 2016-05-12 11:25:11]

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

[commit-bot: I haz the power, 2016-05-12 11:25:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962303002/60001

[commit-bot: I haz the power, 2016-05-12 11:26:31]

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

[commit-bot: I haz the power, 2016-05-12 11:26:32]

Dry run: Try jobs failed on following builders:
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...)
  linux_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/12096)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11285)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3084)

[philipel, 2016-05-12 11:37:36]

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

[commit-bot: I haz the power, 2016-05-12 11:37:40]

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

[stefan-webrtc, 2016-05-12 11:43:46]

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:79: bitrate_log <<
cluster.max_probe_packets << ") ";
Let's merge these two lines:
bitrate_log << "(" << cluster.probe_bitrate_bps << ":" <<
cluster.max_probe_packets << ") ";

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:34: int probe_cluster_id));
Add a test which actually verifies the cluster id

[commit-bot: I haz the power, 2016-05-12 11:52:16]

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

[commit-bot: I haz the power, 2016-05-12 11:52:16]

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

[philipel, 2016-05-12 12:43:42]

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:79: bitrate_log <<
cluster.max_probe_packets << ") ";
On 2016/05/12 11:43:46, stefan-webrtc (holmer) wrote:
> Let's merge these two lines:
> bitrate_log << "(" << cluster.probe_bitrate_bps << ":" <<
> cluster.max_probe_packets << ") ";

Done.

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/1962303002/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:34: int probe_cluster_id));
On 2016/05/12 11:43:46, stefan-webrtc (holmer) wrote:
> Add a test which actually verifies the cluster id

Done.

[stefan-webrtc, 2016-05-12 13:39:46]

lgtm

[philipel, 2016-05-12 13:52:54]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-05-12 13:52:55]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1962303002/#ps100001 (title:
"Addded probing cluster unittest.")

[commit-bot: I haz the power, 2016-05-12 13:52:58]

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

[commit-bot: I haz the power, 2016-05-12 15:53:44]

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

[commit-bot: I haz the power, 2016-05-12 15:53:46]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[commit-bot: I haz the power, 2016-05-13 09:13:18]

Description was changed from

==========
Added cluster id to PacedSender::Callback::TimeToSendPacket.
Also added cluster id to paced_sender::Packet and set the cluster id of
the probing packet that is about to be sent.

BUG=webrtc:5859
==========

to

==========
Added cluster id to PacedSender::Callback::TimeToSendPacket.
Also added cluster id to paced_sender::Packet and set the cluster id of
the probing packet that is about to be sent.

BUG=webrtc:5859
R=danilchap@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/29dca2ce9599f16cd7931a90cdfabb4a5a6092f1
Cr-Commit-Position: refs/heads/master@{#12718}
==========

[philipel, 2016-05-13 09:13:19]

Description was changed from

==========
Added cluster id to PacedSender::Callback::TimeToSendPacket.
Also added cluster id to paced_sender::Packet and set the cluster id of
the probing packet that is about to be sent.

BUG=webrtc:5859
R=danilchap@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/29dca2ce9599f16cd7931a90cdfabb4a5a6092f1
Cr-Commit-Position: refs/heads/master@{#12718}
==========

to

==========
Added cluster id to PacedSender::Callback::TimeToSendPacket.
Also added cluster id to paced_sender::Packet and set the cluster id of
the probing packet that is about to be sent.

BUG=webrtc:5859
R=danilchap@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/29dca2ce9599f16cd7931a90c...
==========

[commit-bot: I haz the power, 2016-05-13 09:13:20]

Patchset 6 (id:??) landed as
https://crrev.com/29dca2ce9599f16cd7931a90cdfabb4a5a6092f1
Cr-Commit-Position: refs/heads/master@{#12718}

[philipel, 2016-05-13 09:13:21]

Committed patchset #6 (id:100001) manually as
29dca2ce9599f16cd7931a90cdfabb4a5a6092f1 (presubmit successful).