CongestionController::SetBweBitrates may now trigger probing.

CongestionController::SetBweBitrates may now trigger probing.

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

Committed: https://chromium.googlesource.com/external/webrtc/+/eb680eac5dad102a4df698dff9e7aec974a97d98

[philipel, 2016-08-16 11:38:50]

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

[philipel, 2016-08-16 11:38:57]

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

[commit-bot: I haz the power, 2016-08-16 11:39:07]

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

[stefan-webrtc, 2016-08-16 11:48:39]

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:231: if
(last_reported_bitrate_bps_ != 0 &&
Add comment to explain why we don't do this if no bitrate has been reported and
why the max bitrate must have been increased.

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:233:
max_bitrate_bps_ < max_bitrate_bps) {
I think it's easier to read if we do max_bitrate_bps > max_bitrate_bps_, but
maybe I'm just odd. :)

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:129:
uint32_t last_reported_bitrate_bps_ GUARDED_BY(critsect_);
Make this int to avoid cast?

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

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:270: void PacedSender::ProbeAtBitrate(int
bitrate_bps, int num_packets) {
I still think "CreateProbeAtBitrate" would be better? :)

[philipel, 2016-08-16 13:04:32]

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:231: if
(last_reported_bitrate_bps_ != 0 &&
On 2016/08/16 11:48:39, stefan-webrtc (holmer) wrote:
> Add comment to explain why we don't do this if no bitrate has been reported
and
> why the max bitrate must have been increased.

Done.

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:233:
max_bitrate_bps_ < max_bitrate_bps) {
On 2016/08/16 11:48:39, stefan-webrtc (holmer) wrote:
> I think it's easier to read if we do max_bitrate_bps > max_bitrate_bps_, but
> maybe I'm just odd. :)

Done.

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:129:
uint32_t last_reported_bitrate_bps_ GUARDED_BY(critsect_);
On 2016/08/16 11:48:39, stefan-webrtc (holmer) wrote:
> Make this int to avoid cast?

If I change it I have to cast at line 367 in the .cc file, and since this is the
value from the interface BitrateController::GetNetworkParameters its easier to
cast as I do.

But I guess it would be nicer to change the interface to work with ints instead
of uin32_t, uint8_t and int64_t, but that's for another CL.

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

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:270: void PacedSender::ProbeAtBitrate(int
bitrate_bps, int num_packets) {
On 2016/08/16 11:48:39, stefan-webrtc (holmer) wrote:
> I still think "CreateProbeAtBitrate" would be better? :)

Changed to CreateProbeCluster

[stefan-webrtc, 2016-08-16 13:28:29]

lgtm % nits

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2246403002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:129:
uint32_t last_reported_bitrate_bps_ GUARDED_BY(critsect_);
On 2016/08/16 13:04:31, philipel wrote:
> On 2016/08/16 11:48:39, stefan-webrtc (holmer) wrote:
> > Make this int to avoid cast?
> 
> If I change it I have to cast at line 367 in the .cc file, and since this is
the
> value from the interface BitrateController::GetNetworkParameters its easier to
> cast as I do.
> 
> But I guess it would be nicer to change the interface to work with ints
instead
> of uin32_t, uint8_t and int64_t, but that's for another CL.

Acknowledged.

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

https://codereview.webrtc.org/2246403002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:89: clusters.front().max_probe_packets);
git cl format

[philipel, 2016-08-16 13:34:06]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-08-16 13:34:07]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2246403002/#ps40001 (title:
"git cl format")

[commit-bot: I haz the power, 2016-08-16 13:34: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, 2016-08-16 13:46:21]

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

[commit-bot: I haz the power, 2016-08-16 13:46:22]

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

[philipel, 2016-08-16 14:12:40]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-08-16 14:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2246403002/#ps60001 (title:
"Don't set probing state to active in the case of no clusters.")

[commit-bot: I haz the power, 2016-08-16 14:12:46]

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, 2016-08-16 16:13:24]

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

[commit-bot: I haz the power, 2016-08-16 16:13:25]

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

[commit-bot: I haz the power, 2016-08-17 09:12:19]

Description was changed from

==========
CongestionController::SetBweBitrates may now trigger probing.

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

to

==========
CongestionController::SetBweBitrates may now trigger probing.

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

Committed: https://crrev.com/eb680eac5dad102a4df698dff9e7aec974a97d98
Cr-Commit-Position: refs/heads/master@{#13791}
==========

[philipel, 2016-08-17 09:12:19]

Description was changed from

==========
CongestionController::SetBweBitrates may now trigger probing.

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

Committed: https://crrev.com/eb680eac5dad102a4df698dff9e7aec974a97d98
Cr-Commit-Position: refs/heads/master@{#13791}
==========

to

==========
CongestionController::SetBweBitrates may now trigger probing.

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

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

[commit-bot: I haz the power, 2016-08-17 09:12:20]

Patchset 4 (id:??) landed as
https://crrev.com/eb680eac5dad102a4df698dff9e7aec974a97d98
Cr-Commit-Position: refs/heads/master@{#13791}

[philipel, 2016-08-17 09:12:21]

Committed patchset #4 (id:60001) manually as
eb680eac5dad102a4df698dff9e7aec974a97d98 (presubmit successful).