Move congestion controller processing to the pacer thread.

Move congestion controller processing to the pacer thread.

Also rename it from pacer_thread_ to congestion_controller_thread_.

BUG=webrtc:6847
Review-Url: https://codereview.webrtc.org/2637783003
Cr-Commit-Position: refs/heads/master@{#16134}
Committed: https://chromium.googlesource.com/external/webrtc/+/b3dc2b7b1e02970c49064a61a4bd0fc941249c93

[nisse-webrtc, 2017-01-16 15:15:06]

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

[stefan-webrtc, 2017-01-16 15:42:46]

lgtm

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:337: //
TODO(nisse): I think this is C++11. Ok?
I think this should be fine

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/pacing/alr...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/pacing/alr...
webrtc/modules/pacing/alr_detector.cc:40: // function isn't called before
SetEstimatedBitrate.
I guess the guarantee is here in form of a DCHECK... :)

[nisse-webrtc, 2017-01-16 16:14:03]

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:337: //
TODO(nisse): I think this is C++11. Ok?
On 2017/01/16 15:42:46, stefan-webrtc wrote:
> I think this should be fine

Nice, doing it this way then.

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/pacing/alr...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2637783003/diff/20001/webrtc/modules/pacing/alr...
webrtc/modules/pacing/alr_detector.cc:40: // function isn't called before
SetEstimatedBitrate.
On 2017/01/16 15:42:46, stefan-webrtc wrote:
> I guess the guarantee is here in form of a DCHECK... :)

Problem was, this thread reorg made this check crash. And then I fixed it by
adding an if (bytes_sent > 0) around the call.

Looks a bit brittle... So I guess the right thing is to tolerate zero
estimated_bitrate_bps_.

[danilchap, 2017-01-16 17:09:39]

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:342:
MaybeTriggerOnNetworkChanged();
look like it is this function that SetEstimatedBitrate.
Does it make sense to put it before pacer_->Process(); // With a comment why.

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/pacing/alr...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/pacing/alr...
webrtc/modules/pacing/alr_detector.cc:39: // TODO(nisse): It's unclear what
guarantees there are that this
might be better to write TODO by specifying what should be done and, if
applicable, when it should be done.
If comment can't be phrased in such a way, may it is not a TODO, but just a
comment.

[nisse-webrtc, 2017-01-17 07:42:39]

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:342:
MaybeTriggerOnNetworkChanged();
On 2017/01/16 17:09:39, danilchap wrote:
> look like it is this function that SetEstimatedBitrate.
> Does it make sense to put it before pacer_->Process(); // With a comment why.

Reorder isn't enough, first call to MaybeTriggerOnNetworkChanged gets
|estimate_changed| == false, and hence doesn't call SetEstimatedBitrate.

So PacedSender::Process still needs the change to make calling
alr_detector_->OnBytesSent conditional on bytes_sent > 0, otherwise it crashes
in the AlrDetector DCHECK.

I'd be happy to reorder the calls anyway you like (I really don't know  if some
order makes more sense than any  other order), but it seems it doesn't by itself
solve the AlrDetector call order requirements.

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/pacing/alr...
File webrtc/modules/pacing/alr_detector.cc (right):

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/pacing/alr...
webrtc/modules/pacing/alr_detector.cc:39: // TODO(nisse): It's unclear what
guarantees there are that this
On 2017/01/16 17:09:39, danilchap wrote:
> might be better to write TODO by specifying what should be done and, if
> applicable, when it should be done.
> If comment can't be phrased in such a way, may it is not a TODO, but just a
> comment.

I'll try to rephrase it. The need action is to document the guarantee, if one
exists, or else remove the DCHECK.

[stefan-webrtc, 2017-01-17 07:59:54]

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:342:
MaybeTriggerOnNetworkChanged();
On 2017/01/17 07:42:39, nisse-webrtc wrote:
> On 2017/01/16 17:09:39, danilchap wrote:
> > look like it is this function that SetEstimatedBitrate.
> > Does it make sense to put it before pacer_->Process(); // With a comment
why.
> 
> Reorder isn't enough, first call to MaybeTriggerOnNetworkChanged gets
> |estimate_changed| == false, and hence doesn't call SetEstimatedBitrate.
> 
> So PacedSender::Process still needs the change to make calling
> alr_detector_->OnBytesSent conditional on bytes_sent > 0, otherwise it crashes
> in the AlrDetector DCHECK.
> 
> I'd be happy to reorder the calls anyway you like (I really don't know  if
some
> order makes more sense than any  other order), but it seems it doesn't by
itself
> solve the AlrDetector call order requirements.

It might be a good idea to call MaybeTriggerOnNetworkChanged() before
probe_controller_->Process(), pacer_->Process() and
remote_estimator_proxy_.Process(), simply because they use the result of
MaybeTriggerOnNetworkChanged().

[nisse-webrtc, 2017-01-17 08:39:35]

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller.cc (right):

https://codereview.webrtc.org/2637783003/diff/40001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller.cc:342:
MaybeTriggerOnNetworkChanged();
On 2017/01/17 07:59:54, stefan-webrtc wrote:
> On 2017/01/17 07:42:39, nisse-webrtc wrote:
> > On 2017/01/16 17:09:39, danilchap wrote:
> > > look like it is this function that SetEstimatedBitrate.
> > > Does it make sense to put it before pacer_->Process(); // With a comment
> why.
> > 
> > Reorder isn't enough, first call to MaybeTriggerOnNetworkChanged gets
> > |estimate_changed| == false, and hence doesn't call SetEstimatedBitrate.
> > 
> > So PacedSender::Process still needs the change to make calling
> > alr_detector_->OnBytesSent conditional on bytes_sent > 0, otherwise it
crashes
> > in the AlrDetector DCHECK.
> > 
> > I'd be happy to reorder the calls anyway you like (I really don't know  if
> some
> > order makes more sense than any  other order), but it seems it doesn't by
> itself
> > solve the AlrDetector call order requirements.
> 
> It might be a good idea to call MaybeTriggerOnNetworkChanged() before
> probe_controller_->Process(), pacer_->Process() and
> remote_estimator_proxy_.Process(), simply because they use the result of
> MaybeTriggerOnNetworkChanged().

Done.

[danilchap, 2017-01-17 08:57:22]

lgtm

[nisse-webrtc, 2017-01-17 08:58:29]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-17 08:58:29]

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/2637783003/#ps80001 (title:
"Reorder calls in CongestionController::Process.")

[commit-bot: I haz the power, 2017-01-17 08:58:37]

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, 2017-01-17 10:44:00]

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

[commit-bot: I haz the power, 2017-01-17 10:44:01]

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

[nisse-webrtc, 2017-01-18 08:08:24]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-01-18 08:08:35]

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, 2017-01-18 08:10:32]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484726904674860,
"parent_rev": "c0370ef12643d873dfbc2cb488b2dc217ff318e2", "commit_rev":
"b3dc2b7b1e02970c49064a61a4bd0fc941249c93"}

[commit-bot: I haz the power, 2017-01-18 08:10:34]

Description was changed from

==========
Move congestion controller processing to the pacer thread.

Also rename it from pacer_thread_ to congestion_controller_thread_.

BUG=webrtc:6847
==========

to

==========
Move congestion controller processing to the pacer thread.

Also rename it from pacer_thread_ to congestion_controller_thread_.

BUG=webrtc:6847

Review-Url: https://codereview.webrtc.org/2637783003
Cr-Commit-Position: refs/heads/master@{#16134}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b3dc2b7b1e02970c49064a61a...
==========

[commit-bot: I haz the power, 2017-01-18 08:10:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/b3dc2b7b1e02970c49064a61a...

[nisse-webrtc, 2017-01-19 12:25:12]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2644603003/ by nisse@webrtc.org.

The reason for reverting is: Speculative revert for perf regression related to
ramp-up on android. See
https://bugs.chromium.org/p/chromium/issues/detail?id=682611.