Change return type of CongestionController::pacer() method.

Change return type of CongestionController::pacer() method.

Move pacer_thread_ from Call to CongestionController.

BUG=webrtc:6847

[nisse-webrtc, 2016-12-15 14:59:20]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2016-12-15 14:59:21]

This cl doesn't delete the CongestionController::pacer accessor, but it changes
it to a less specific type. To make that work, also moves ownership of the
pacer_thread_.

It would make sense to rename the method and maybe some related
members/variables too, but I'm not sure if that should be in the same cl or not.

[the sun, 2016-12-15 15:11:32]

lgtm

https://codereview.webrtc.org/2580843002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2580843002/diff/1/webrtc/call/call.cc#oldcode327
webrtc/call/call.cc:327: pacer_thread_->Stop();
Are there any subtle relationship issues between the CongestionController and
Call which may bite us, now that the pacer thread will keep running until after
the explicit dtor code? Do you need to
congestion_controller_.reset(nullptr);
?

https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:124: //
TODO(nisse): Convert to TaskQueue.
Yes, please!

[nisse-webrtc, 2016-12-16 09:38:13]

https://codereview.webrtc.org/2580843002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2580843002/diff/1/webrtc/call/call.cc#oldcode327
webrtc/call/call.cc:327: pacer_thread_->Stop();
On 2016/12/15 15:11:32, the sun wrote:
> Are there any subtle relationship issues between the CongestionController and
> Call which may bite us, now that the pacer thread will keep running until
after
> the explicit dtor code? Do you need to
> congestion_controller_.reset(nullptr);
> ?

Don't know... Does the call object register any callbacks which are called on
the pacer thread?

[stefan-webrtc, 2016-12-22 09:54:58]

lgtm

https://codereview.webrtc.org/2580843002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2580843002/diff/1/webrtc/call/call.cc#oldcode327
webrtc/call/call.cc:327: pacer_thread_->Stop();
On 2016/12/16 09:38:13, nisse-webrtc (ooo 2017-01-09) wrote:
> On 2016/12/15 15:11:32, the sun wrote:
> > Are there any subtle relationship issues between the CongestionController
and
> > Call which may bite us, now that the pacer thread will keep running until
> after
> > the explicit dtor code? Do you need to
> > congestion_controller_.reset(nullptr);
> > ?
> 
> Don't know... Does the call object register any callbacks which are called on
> the pacer thread?

I don't think it does. It calls into rtp modules and transports, but not into
Call.

[perkj_webrtc, 2017-01-02 09:18:58]

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

https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:177:
pacer_thread_(ProcessThread::Create("PacerThread")),
There are upstreams projects that use CongestionController but not the pacer
that does not want an extra thread. 
Why is the thread created here when its not used in this class? Can it be
created and owned by the pacer itself?

[nisse-webrtc, 2017-01-09 13:43:46]

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

https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/congestion_controller.cc:177:
pacer_thread_(ProcessThread::Create("PacerThread")),
On 2017/01/02 09:18:58, perkj_webrtc wrote:
> There are upstreams projects that use CongestionController but not the pacer
> that does not want an extra thread. 
> Why is the thread created here when its not used in this class? Can it be
> created and owned by the pacer itself? 

I can try moving it to the pacer itself. One obvious obstacle is the calls to
RegisterModule and DeRegisterModule with &remote_estimator_proxy_.

Or is it bad to have the remote_estimator_proxy and pacer share the same thread?
If they do, it makes some sense to me to let the CongestionController own it.

[perkj_webrtc, 2017-01-10 08:18:47]

On 2017/01/09 13:43:46, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
> File webrtc/modules/congestion_controller/congestion_controller.cc (right):
> 
>
https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
> webrtc/modules/congestion_controller/congestion_controller.cc:177:
> pacer_thread_(ProcessThread::Create("PacerThread")),
> On 2017/01/02 09:18:58, perkj_webrtc wrote:
> > There are upstreams projects that use CongestionController but not the pacer
> > that does not want an extra thread. 
> > Why is the thread created here when its not used in this class? Can it be
> > created and owned by the pacer itself? 
> 
> I can try moving it to the pacer itself. One obvious obstacle is the calls to
> RegisterModule and DeRegisterModule with &remote_estimator_proxy_.
> 
> Or is it bad to have the remote_estimator_proxy and pacer share the same
thread?
> If they do, it makes some sense to me to let the CongestionController own it.

It might be a question for Stefan how they fit together. But the important thing
is that you can not add a thread inside CongestionController since that is used
for other purposes as well.

[nisse-webrtc, 2017-01-10 08:35:19]

On 2017/01/10 08:18:47, perkj_webrtc wrote:
> On 2017/01/09 13:43:46, nisse-webrtc wrote:
> >
>
https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
> > File webrtc/modules/congestion_controller/congestion_controller.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2580843002/diff/1/webrtc/modules/congestion_con...
> > webrtc/modules/congestion_controller/congestion_controller.cc:177:
> > pacer_thread_(ProcessThread::Create("PacerThread")),
> > On 2017/01/02 09:18:58, perkj_webrtc wrote:
> > > There are upstreams projects that use CongestionController but not the
pacer
> > > that does not want an extra thread. 
> > > Why is the thread created here when its not used in this class? Can it be
> > > created and owned by the pacer itself? 
> > 
> > I can try moving it to the pacer itself. One obvious obstacle is the calls
to
> > RegisterModule and DeRegisterModule with &remote_estimator_proxy_.
> > 
> > Or is it bad to have the remote_estimator_proxy and pacer share the same
> thread?
> > If they do, it makes some sense to me to let the CongestionController own
it.
> 
> It might be a question for Stefan how they fit together. But the important
thing
> is that you can not add a thread inside CongestionController since that is
used
> for other purposes as well.

I see. Can you describe the use case in some more detail? What methods of the
congestion 
controller do you need? Maybe we can separate it from the pacer and the remote
estimator (the two "modules" which want a process thread).

[nisse-webrtc, 2017-01-18 10:39:37]

Now rebased on top of the other change introducing a
congestion_controller_thread_ in Call. So no thread-related changes in this cl.

[perkj_webrtc, 2017-01-20 10:16:52]

On 2017/01/18 10:39:37, nisse-webrtc wrote:
> Now rebased on top of the other change introducing a
> congestion_controller_thread_ in Call. So no thread-related changes in this
cl.

lgtm

[nisse-webrtc, 2017-01-20 10:34:14]

On 2017/01/20 10:16:52, perkj_webrtc wrote:
> On 2017/01/18 10:39:37, nisse-webrtc wrote:
> > Now rebased on top of the other change introducing a
> > congestion_controller_thread_ in Call. So no thread-related changes in this
> cl.
> 
> lgtm

Thanks! But unfortunately, I had to revert the cl this depends on, due to
unexpected 
perf regressions. So this will have to wait a bit.