De-flake VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly

De-flake VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly

BUG=webrtc:5382
R=pbos@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/c6957c7b7308a90a453f2caf1523db2cf3e8f640
Cr-Commit-Position: refs/heads/master@{#12547}

[philipel, 2016-04-25 11:42:42]

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

[pbos-webrtc, 2016-04-25 15:24:41]

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

[pbos-webrtc, 2016-04-25 15:24:41]

+R stefan@ for bitrate_controller

lgtm

[pbos-webrtc, 2016-04-25 15:25:26]

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

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:180:
start_bitrate_bps = std::max(min_bitrate_bps, start_bitrate_bps);
I think this std::max should be done inside SetMinMaxStartBitrate, WDYT?

[pbos-webrtc, 2016-04-25 15:26:29]

If the bug was inside bitrate_controller, please update the CL description to
reflect that.

[philipel, 2016-04-25 15:53:01]

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

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:180:
start_bitrate_bps = std::max(min_bitrate_bps, start_bitrate_bps);
On 2016/04/25 15:25:26, pbos-webrtc wrote:
> I think this std::max should be done inside SetMinMaxStartBitrate, WDYT?

The constraint is that min <= start <= max, so if we set start then we also make
sure that start <= max.

[philipel, 2016-04-26 08:44:20]

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

[commit-bot: I haz the power, 2016-04-26 08:44:29]

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

[commit-bot: I haz the power, 2016-04-26 10:21:22]

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

[commit-bot: I haz the power, 2016-04-26 10:21:23]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-04-26 11:10:44]

lg, but maybe we can clean up a bit too?

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:131:
bandwidth_estimation_.SetSendBitrate(start_bitrate_bps);
Replace these with a single SetMinMaxSendBitrate. Maybe we should call it
SetBitrates()?

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

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:181:
bitrate_controller_->SetMinMaxStartBitrate(start_bitrate_bps,
Allow start_bitrate_bps = -1 to mean "don't change" and remove the
SetMinMaxBitrate() method?

If you decide to do that you may have to keep the method below around because of
upstream dependencies. In that case we can deprecate it at least.

[philipel, 2016-04-28 10:42:57]

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:131:
bandwidth_estimation_.SetSendBitrate(start_bitrate_bps);
On 2016/04/26 11:10:43, stefan-webrtc (holmer) wrote:
> Replace these with a single SetMinMaxSendBitrate. Maybe we should call it
> SetBitrates()?

Done.

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

https://codereview.webrtc.org/1922483002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:181:
bitrate_controller_->SetMinMaxStartBitrate(start_bitrate_bps,
On 2016/04/26 11:10:43, stefan-webrtc (holmer) wrote:
> Allow start_bitrate_bps = -1 to mean "don't change" and remove the
> SetMinMaxBitrate() method?
> 
> If you decide to do that you may have to keep the method below around because
of
> upstream dependencies. In that case we can deprecate it at least.

Done.

[stefan-webrtc, 2016-04-28 10:49:11]

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:125: void
BitrateControllerImpl::SetMinMaxStartBitrate(int min_bitrate_bps,
Suggest naming this SetBitrates() too.

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:57: virtual void
SetStartBitrate(int start_bitrate_bps) = 0;
Deprecate these.

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h (right):

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:52: void
SetSendBitrate(int bitrate);
Name these as deprecated.

[philipel, 2016-04-28 11:17:19]

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:125: void
BitrateControllerImpl::SetMinMaxStartBitrate(int min_bitrate_bps,
On 2016/04/28 10:49:11, stefan-webrtc (holmer) wrote:
> Suggest naming this SetBitrates() too.

Done.

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:57: virtual void
SetStartBitrate(int start_bitrate_bps) = 0;
On 2016/04/28 10:49:11, stefan-webrtc (holmer) wrote:
> Deprecate these.

Done.

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h (right):

https://codereview.webrtc.org/1922483002/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:52: void
SetSendBitrate(int bitrate);
On 2016/04/28 10:49:11, stefan-webrtc (holmer) wrote:
> Name these as deprecated.

Done.

[stefan-webrtc, 2016-04-28 11:21:28]

lgtm

https://codereview.webrtc.org/1922483002/diff/40001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h (right):

https://codereview.webrtc.org/1922483002/diff/40001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:52: void
SetSendBitrate(int bitrate);
Deprecated

https://codereview.webrtc.org/1922483002/diff/40001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:53: void
SetMinMaxBitrate(int min_bitrate, int max_bitrate);
Deprecated

[philipel, 2016-04-28 11:44:33]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-04-28 11:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1922483002/#ps40001 (title:
"Feedback fixes")

[commit-bot: I haz the power, 2016-04-28 11:44:44]

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

[commit-bot: I haz the power, 2016-04-28 13:45:13]

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

[commit-bot: I haz the power, 2016-04-28 13:45:15]

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

[philipel, 2016-04-28 13:53:03]

Description was changed from

==========
De-flake VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly

BUG=webrtc:5382
==========

to

==========
De-flake VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly

BUG=webrtc:5382
R=pbos@webrtc.org, stefan@webrtc.org

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

[philipel, 2016-04-28 13:53:05]

Committed patchset #3 (id:40001) manually as
c6957c7b7308a90a453f2caf1523db2cf3e8f640 (presubmit successful).

[commit-bot: I haz the power, 2016-04-28 13:53:09]

Description was changed from

==========
De-flake VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly

BUG=webrtc:5382
R=pbos@webrtc.org, stefan@webrtc.org

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

to

==========
De-flake VideoSendStreamTest.ReconfigureBitratesSetsEncoderBitratesCorrectly

BUG=webrtc:5382
R=pbos@webrtc.org, stefan@webrtc.org

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

[commit-bot: I haz the power, 2016-04-28 13:53:11]

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