Fix suspend below min bitrate in new API by making it possible to set min bitrate at the receive-si…

Fix suspend below min bitrate in new API by making it possible to set min bitrate at the receive-side.

In addition to this the ramp-up tests are refactored to use a receive call instead of only a remote bitrate estimator, and to make use of BaseTest.

BUG=webrtc:4836
Committed: https://crrev.com/4fbd145dcefd23169a9b1612d5ca92dace8196d6
Cr-Commit-Position: refs/heads/master@{#10087}

[stefan-webrtc, 2015-09-25 09:45:51]

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

[stefan-webrtc, 2015-09-25 09:46:34]

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

[commit-bot: I haz the power, 2015-09-25 09:46:47]

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

[commit-bot: I haz the power, 2015-09-25 09:47:36]

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

[commit-bot: I haz the power, 2015-09-25 09:47:37]

Dry run: Try jobs failed on following builders:
  android_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/5874)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4576)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/8322)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/98)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/4241)

[stefan-webrtc, 2015-09-25 09:51:08]

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

[commit-bot: I haz the power, 2015-09-25 09:51:16]

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

[commit-bot: I haz the power, 2015-09-25 09:53:26]

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

[commit-bot: I haz the power, 2015-09-25 09:53:27]

Dry run: Try jobs failed on following builders:
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/9660)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9607)

[stefan-webrtc, 2015-09-25 10:04:57]

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

[commit-bot: I haz the power, 2015-09-25 10:05:05]

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

[commit-bot: I haz the power, 2015-09-25 10:53:03]

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

[commit-bot: I haz the power, 2015-09-25 10:53:04]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2015-09-25 12:41:01]

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h
(right):

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h:69:
Clock* clock);
git cl format, this might fit on one line

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:104: encoder_config->streams[0].target_bitrate_bps
= 2000000;
target = max = 20...

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:108: if (num_streams_ == 1) {
Consolidate with previous num_streams_ == 1 check

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:138: send_config->rtp.nack.rtp_history_ms =
test::CallTest::kNackRtpHistoryMs;
Move these constants under test::BaseTest or something shared?

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:151: for (VideoReceiveStream::Config& recv_config :
*receive_configs) {
regular for-loop (size_t i = 0...), VideoReceiveStream::Config* recv_config =
&(*receive_configs)[i];

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:224:
send_stats.substreams[ssrc].rtp_stats.transmitted.packets;
Should this be all packets (incl. retransmissions)?

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:225: total_sent +=
Should this account for retransmitted.TotalBytes + fec.TotalBytes etc? Should
rtp_stats have a TotalBytes + we use that?

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:238: for (uint32_t ssrc : rtx_ssrcs_) {
Same here.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests.h
File webrtc/video/rampup_tests.h (right):

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:60: size_t num_streams_;
const?

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:61: bool rtx_;
const?

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:62: bool red_;
const?

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:78: static bool SendBitrateAdapterThread(void* obj);
BitrateStatsPollingThread

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:80: int start_bitrate_bps_;
const?

[stefan-webrtc, 2015-09-28 09:28:32]

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h
(right):

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h:69:
Clock* clock);
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> git cl format, this might fit on one line

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:104: encoder_config->streams[0].target_bitrate_bps
= 2000000;
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> target = max = 20...

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:108: if (num_streams_ == 1) {
On 2015/09/25 12:41:01, pbos-webrtc wrote:
> Consolidate with previous num_streams_ == 1 check

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:138: send_config->rtp.nack.rtp_history_ms =
test::CallTest::kNackRtpHistoryMs;
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> Move these constants under test::BaseTest or something shared?

I didn't find any good shared place. These tests are actually built upon
CallTest, it's just that BaseTest doesn't have any base class common with
CallTest. Some other tests using these constants are using CallTest more
directly, and would have to depend on BaseTest in a less obvious way (e.g.,
VideoQualityTest).

Keeping as is as I don't think it's too weird.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:151: for (VideoReceiveStream::Config& recv_config :
*receive_configs) {
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> regular for-loop (size_t i = 0...), VideoReceiveStream::Config* recv_config =
> &(*receive_configs)[i];

Basically no difference? And the index "i" can also be seen as being tied to
ssrcs_ and rtx_ssrcs_. I'd prefer to keep as is :)

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:224:
send_stats.substreams[ssrc].rtp_stats.transmitted.packets;
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> Should this be all packets (incl. retransmissions)?

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:225: total_sent +=
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> Should this account for retransmitted.TotalBytes + fec.TotalBytes etc? Should
> rtp_stats have a TotalBytes + we use that?

Added fec here, but not a method for computing the sum. I think it's better that
people actually know what they get when they call a method. Then you have to
decide if you want retransmissions and/or fec.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:238: for (uint32_t ssrc : rtx_ssrcs_) {
On 2015/09/25 12:41:00, pbos-webrtc wrote:
> Same here.

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests.h
File webrtc/video/rampup_tests.h (right):

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:60: size_t num_streams_;
On 2015/09/25 12:41:01, pbos-webrtc wrote:
> const?

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:61: bool rtx_;
On 2015/09/25 12:41:01, pbos-webrtc wrote:
> const?

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:62: bool red_;
On 2015/09/25 12:41:01, pbos-webrtc wrote:
> const?

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:78: static bool SendBitrateAdapterThread(void* obj);
On 2015/09/25 12:41:01, pbos-webrtc wrote:
> BitrateStatsPollingThread

Done.

https://codereview.webrtc.org/1368943002/diff/40001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.h:80: int start_bitrate_bps_;
On 2015/09/25 12:41:01, pbos-webrtc wrote:
> const?

Done.

[pbos-webrtc, 2015-09-28 09:31:29]

sweet, lgtm

[stefan-webrtc, 2015-09-28 09:32:09]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2015-09-28 09:32:30]

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

[commit-bot: I haz the power, 2015-09-28 09:33:09]

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

[commit-bot: I haz the power, 2015-09-28 09:33:10]

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

[stefan-webrtc, 2015-09-28 10:13:15]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-09-28 10:13:15]

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/1368943002/#ps100001 (title:
"Merge.")

[commit-bot: I haz the power, 2015-09-28 10:13:31]

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

[commit-bot: I haz the power, 2015-09-28 10:57:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-28 10:57:34]

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