Update bitrate only when we have incoming packet.

Update bitrate only when we have incoming packet.

Also cleans up some unused code and makes sure the min bitrate of the BWE can't be set to anything lower than 10 kbps.

BUG=webrtc:5474
R=pbos@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/62a5ccdb532cfa1256c3a5860bfe087569481d24

[stefan-webrtc, 2016-02-16 10:01:01]

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

[commit-bot: I haz the power, 2016-02-16 10:01:06]

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

[commit-bot: I haz the power, 2016-02-16 10:01:52]

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

[commit-bot: I haz the power, 2016-02-16 10:01:53]

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/8764)
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5162)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5159)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7485)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7404)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12663)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11312)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12428)
  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/828)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7061)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12732)

[stefan-webrtc, 2016-02-16 12:43:34]

Rebase

[stefan-webrtc, 2016-02-16 12:44:51]

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

[commit-bot: I haz the power, 2016-02-16 12:44:58]

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

[stefan-webrtc, 2016-02-16 13:15:10]

Clean-up

[stefan-webrtc, 2016-02-16 13:15:39]

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

[stefan-webrtc, 2016-02-16 13:15:39]

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc
(left):

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc:263:
EXPECT_LE(bitrate_observer_->latest_bitrate(), bitrate_bps);
It is still possible that we have an increase before we detect the overuse.

[stefan-webrtc, 2016-02-16 13:16:14]

Description was changed from

==========
Update bitrate only when we have incoming packet.

BUG=webrtc:5474
==========

to

==========
Update bitrate only when we have incoming packet.

Also cleans up some unused code and makes sure the min bitrate of the BWE can't
be set to anything lower than 10 kbps.

BUG=webrtc:5474
==========

[pbos-webrtc, 2016-02-16 13:44:34]

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/call/congestion_co...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:193: // The congestion controller should
allow a min bitrate of 0.
Should this clamp to start and max as well? All might be <10 initially.

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

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:170:
bool RemoteBitrateEstimatorAbsSendTime::ProcessClusters(int64_t now_ms) {
Return an enum stating what it actually means.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:269:
rtc::CritScope lock(&crit_);
Take lock until line 316 to remain in a consistent state? This seems a bit scary
to me.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:310:
update_estimate |=
if/elseif and set to true

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:350:
void RemoteBitrateEstimatorAbsSendTime::UpdateEstimate(int64_t now_ms) {
Call under lock with guard and return target_bitrate that you send to observer_.

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

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h:124:
rtc::ThreadChecker network_thread_;
Can you group the members that are only accessed from the network thread under
here? Same for process_thread.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h:128:
Clock* clock_;
Clock* const

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

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:44: void
RemoveStream(uint32_t ssrc) override {}
Can this be removed?

[stefan-webrtc, 2016-02-16 14:54:27]

comments addressed.

[stefan-webrtc, 2016-02-16 14:57:16]

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/call/congestion_co...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:193: // The congestion controller should
allow a min bitrate of 0.
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Should this clamp to start and max as well? All might be <10 initially.

Done.

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

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:170:
bool RemoteBitrateEstimatorAbsSendTime::ProcessClusters(int64_t now_ms) {
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Return an enum stating what it actually means.

Done.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:269:
rtc::CritScope lock(&crit_);
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Take lock until line 316 to remain in a consistent state? This seems a bit
scary
> to me.

Done.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:310:
update_estimate |=
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> if/elseif and set to true

Done.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:350:
void RemoteBitrateEstimatorAbsSendTime::UpdateEstimate(int64_t now_ms) {
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Call under lock with guard and return target_bitrate that you send to
observer_.

Done.

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

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h:124:
rtc::ThreadChecker network_thread_;
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Can you group the members that are only accessed from the network thread under
> here? Same for process_thread.

Done.

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h:128:
Clock* clock_;
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Clock* const

Done.

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

https://codereview.webrtc.org/1699903003/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:44: void
RemoveStream(uint32_t ssrc) override {}
On 2016/02/16 13:44:34, pbos-webrtc wrote:
> Can this be removed?

Nope, implements interface.

[stefan-webrtc, 2016-02-16 14:58:55]

Remove unused Id() method

[pbos-webrtc, 2016-02-16 15:23:02]

lgtm

https://codereview.webrtc.org/1699903003/diff/80001/webrtc/call/congestion_co...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1699903003/diff/80001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:198: max_bitrate_bps =
std::max(min_bitrate_bps, max_bitrate_bps);
I think this should clamp the min bitrate instead of max.

[stefan-webrtc, 2016-02-16 15:26:17]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-02-16 15:26:22]

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

[commit-bot: I haz the power, 2016-02-16 15:28:23]

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

[commit-bot: I haz the power, 2016-02-16 15:28:24]

Try jobs failed on following builders:
  win_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...)
  win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/7094)
  win_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/7068)

[stefan-webrtc, 2016-02-16 15:52:32]

...

[stefan-webrtc, 2016-02-16 15:53:52]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-02-16 15:53:53]

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

[commit-bot: I haz the power, 2016-02-16 15:53:59]

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

[commit-bot: I haz the power, 2016-02-16 15:57:38]

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

[commit-bot: I haz the power, 2016-02-16 15:57:40]

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

[stefan-webrtc, 2016-02-16 16:06:27]

Fix lint

[commit-bot: I haz the power, 2016-02-16 16:07:35]

Description was changed from

==========
Update bitrate only when we have incoming packet.

Also cleans up some unused code and makes sure the min bitrate of the BWE can't
be set to anything lower than 10 kbps.

BUG=webrtc:5474
==========

to

==========
Update bitrate only when we have incoming packet.

Also cleans up some unused code and makes sure the min bitrate of the BWE can't
be set to anything lower than 10 kbps.

BUG=webrtc:5474
R=pbos@webrtc.org

Committed: https://crrev.com/62a5ccdb532cfa1256c3a5860bfe087569481d24
Cr-Commit-Position: refs/heads/master@{#11636}
==========

[commit-bot: I haz the power, 2016-02-16 16:07:36]

Patchset 7 (id:??) landed as
https://crrev.com/62a5ccdb532cfa1256c3a5860bfe087569481d24
Cr-Commit-Position: refs/heads/master@{#11636}

[stefan-webrtc, 2016-02-16 16:07:36]

Description was changed from

==========
Update bitrate only when we have incoming packet.

Also cleans up some unused code and makes sure the min bitrate of the BWE can't
be set to anything lower than 10 kbps.

BUG=webrtc:5474
R=pbos@webrtc.org

Committed: https://crrev.com/62a5ccdb532cfa1256c3a5860bfe087569481d24
Cr-Commit-Position: refs/heads/master@{#11636}
==========

to

==========
Update bitrate only when we have incoming packet.

Also cleans up some unused code and makes sure the min bitrate of the BWE can't
be set to anything lower than 10 kbps.

BUG=webrtc:5474
R=pbos@webrtc.org

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

[stefan-webrtc, 2016-02-16 16:07:37]

Committed patchset #7 (id:120001) manually as
62a5ccdb532cfa1256c3a5860bfe087569481d24 (presubmit successful).