Simplify CongestionController.

Simplify CongestionController.

- Removes the dependency on CallStats.
- Implements Module interface so that we don't have to register
  each internal component to the process thread separately.

R=pbos@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/789ba92e1462c4ad45dc13bec6616658df30c663
Cr-Commit-Position: refs/heads/master@{#11655}

[stefan-webrtc, 2016-02-17 12:10:34]

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

[pbos-webrtc, 2016-02-17 12:17:12]

lgtm

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
webrtc/call/congestion_controller.cc:217: if (transport_feedback_adapter_.get()
== nullptr) {
!transport_feedback_adapter_

https://codereview.webrtc.org/1704983002/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc
(right):

https://codereview.webrtc.org/1704983002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:38:
TransportFeedbackAdapter::~TransportFeedbackAdapter() {
Remove empty dtor?

[the sun, 2016-02-17 12:47:27]

Very nice!

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
webrtc/call/congestion_controller.cc:213: TransportFeedbackObserver*
So, you may return a pointer to the TFA on one thread here, while the CC dtor
just started executing on a different thread.

Can you instead always set up the TFA in the ctor? Does it matter a lot that the
object is unused by receive streams? Then you could get rid of the lock.

https://codereview.webrtc.org/1704983002/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc
(right):

https://codereview.webrtc.org/1704983002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:38:
TransportFeedbackAdapter::~TransportFeedbackAdapter() {
On 2016/02/17 12:17:12, pbos-webrtc wrote:
> Remove empty dtor?

or even better: keep the dtor impl here, but forward-declare
RemoteBitrateEstimator in the .h (remove include).

[stefan-webrtc, 2016-02-17 13:32:01]

.

[stefan-webrtc, 2016-02-17 13:32:48]

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
File webrtc/call/congestion_controller.cc (right):

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
webrtc/call/congestion_controller.cc:213: TransportFeedbackObserver*
On 2016/02/17 12:47:26, the sun wrote:
> So, you may return a pointer to the TFA on one thread here, while the CC dtor
> just started executing on a different thread.
> 
> Can you instead always set up the TFA in the ctor? Does it matter a lot that
the
> object is unused by receive streams? Then you could get rid of the lock.

Yes, it turned out to be fairly easy to make that change.

Done.

https://codereview.webrtc.org/1704983002/diff/1/webrtc/call/congestion_contro...
webrtc/call/congestion_controller.cc:217: if (transport_feedback_adapter_.get()
== nullptr) {
On 2016/02/17 12:17:12, pbos-webrtc wrote:
> !transport_feedback_adapter_

Done.

https://codereview.webrtc.org/1704983002/diff/1/webrtc/modules/remote_bitrate...
File webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc
(right):

https://codereview.webrtc.org/1704983002/diff/1/webrtc/modules/remote_bitrate...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc:38:
TransportFeedbackAdapter::~TransportFeedbackAdapter() {
On 2016/02/17 12:47:26, the sun wrote:
> On 2016/02/17 12:17:12, pbos-webrtc wrote:
> > Remove empty dtor?
> 
> or even better: keep the dtor impl here, but forward-declare
> RemoteBitrateEstimator in the .h (remove include).

Done.

[pbos-webrtc, 2016-02-17 13:37:21]

ell gee tee emmm

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:213: } else {
remove else and just return

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.h:79:
rtc::scoped_ptr<TransportFeedbackAdapter> transport_feedback_adapter_;
const every scoped ptr that you create on construction

[the sun, 2016-02-17 13:50:55]

LGTM % nits

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/bitrate_estim...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/bitrate_estim...
webrtc/call/bitrate_estimator_tests.cc:270:
receiver_log_.PushExpectedLogLine(kAbsSendTimeLog);
Unrelated?

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:184:
RTC_DCHECK(config_thread_checker_.CalledOnValidThread());
Is this thread checker under used?

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h:34: void
AddPacket(uint16_t sequence_number,
Can we group the overrides by interface, like elsewhere?

[stefan-webrtc, 2016-02-17 14:14:07]

Comments addressed.

[stefan-webrtc, 2016-02-17 14:14:27]

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/bitrate_estim...
File webrtc/call/bitrate_estimator_tests.cc (right):

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/bitrate_estim...
webrtc/call/bitrate_estimator_tests.cc:270:
receiver_log_.PushExpectedLogLine(kAbsSendTimeLog);
On 2016/02/17 13:50:55, the sun wrote:
> Unrelated?

No, it's needed since I now always instantiate an abs send time in the
CongestionController constructor.

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.cc:184:
RTC_DCHECK(config_thread_checker_.CalledOnValidThread());
On 2016/02/17 13:50:55, the sun wrote:
> Is this thread checker under used?

Unfortunately not, but it might be that it can be utilized more in this file in
the future.

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/call/congestion_co...
webrtc/call/congestion_controller.h:79:
rtc::scoped_ptr<TransportFeedbackAdapter> transport_feedback_adapter_;
On 2016/02/17 13:37:21, pbos-webrtc wrote:
> const every scoped ptr that you create on construction

Done.

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

https://codereview.webrtc.org/1704983002/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h:34: void
AddPacket(uint16_t sequence_number,
On 2016/02/17 13:50:55, the sun wrote:
> Can we group the overrides by interface, like elsewhere?

Done.

[stefan-webrtc, 2016-02-17 14:14:32]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-02-17 14:14:33]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1704983002/#ps80001 (title:
"Comments addressed.")

[commit-bot: I haz the power, 2016-02-17 14:14:47]

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

[stefan-webrtc, 2016-02-17 14:52:29]

Description was changed from

==========
Simplify CongestionController.

- Removes the dependency on CallStats.
- Implements Module interface so that we don't have to register
  each internal component to the process thread separately.
==========

to

==========
Simplify CongestionController.

- Removes the dependency on CallStats.
- Implements Module interface so that we don't have to register
  each internal component to the process thread separately.

R=pbos@webrtc.org, solenberg@webrtc.org

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

[stefan-webrtc, 2016-02-17 14:52:31]

Committed patchset #5 (id:80001) manually as
789ba92e1462c4ad45dc13bec6616658df30c663 (presubmit successful).

[commit-bot: I haz the power, 2016-02-17 14:52:34]

Description was changed from

==========
Simplify CongestionController.

- Removes the dependency on CallStats.
- Implements Module interface so that we don't have to register
  each internal component to the process thread separately.

R=pbos@webrtc.org, solenberg@webrtc.org

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

to

==========
Simplify CongestionController.

- Removes the dependency on CallStats.
- Implements Module interface so that we don't have to register
  each internal component to the process thread separately.

R=pbos@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/789ba92e1462c4ad45dc13bec6616658df30c663
Cr-Commit-Position: refs/heads/master@{#11655}
==========

[commit-bot: I haz the power, 2016-02-17 14:52:35]

Patchset 5 (id:??) landed as
https://crrev.com/789ba92e1462c4ad45dc13bec6616658df30c663
Cr-Commit-Position: refs/heads/master@{#11655}