MediaController/Call instantiation.

Move instantiation of webrtc::Call into a MediaController class so that it can be used for both audio and video media channels.

I'm not super happy with the GetVoE() function added on MediaEngineInterface, but this will eventually be gone, once webrtc::Call owns the shared VoE state (or initially, maps ADM* to an implicitly created VoE).

BUG=webrtc:4690
R=pbos@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/709ed67c38d0a942f3bf3e68e337cc27a27bc353
Cr-Commit-Position: refs/heads/master@{#9939}

[the sun, 2015-08-31 10:53:41]

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

[pbos-webrtc, 2015-08-31 11:21:15]

On 2015/08/31 10:53:42, the sun wrote:

Peter, can you take the first pass at this?

[pthatcher1, 2015-09-02 21:24:22]

The thing I'm most worried about is the interaction between Call, the
VideoChannel, and the VoiceEngine.  I think we can make it better.

Otherwise, seems like a good start to the MediaController.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:56: video_channel_ = video_channel;
WebRtcVideoChannel2 implements webrtc::LoadObserver, so why not just have
set_load_observer(webrtc::LoadObserver* observer) here?  I think that would be a
lot more clean.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:69: DCHECK(rtc::Thread::Current() ==
worker_thread_);
I think we usually use worker_thread_->IsCurrent() for these checks.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:71: config.overuse_callback = this;
Instead of passing in a MediaEngineInteface, webrtc::VoiceEngine directly if
that's all that we need?

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:41: class MediaController : public
webrtc::LoadObserver {
This needs a comment saying what it is.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:47: webrtc::Call* call() { return call_.get();
}
Can this be called from any thread?  I think it needs to be called on the worker
thread, so perhaps it should be called call_w() or GetCall_w() and then have a
thread-hopping version that can be called on other threads.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:48: void
set_video_channel(cricket::VideoChannel* video_channel);
Similarly here: I think this is "_w".

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:554:
media_controller_->set_video_channel(nullptr);
Why is this necessary if media_controller_ is about to be destroyed?

https://codereview.webrtc.org/1269863005/diff/200001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/libjingle.gyp#newco...
talk/libjingle.gyp:728: 'app/webrtc/mediacontroller.h',
FYI, this will break the Chrome FYI bots.  Be prepared to update the .gyp file
in Chrome as well.

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:591: external_encoder_factory_,
external_decoder_factory_);
Why not pass the WebRtcVideoChannel2 into webrtc::Call right here?

call->set_load_observer(channel);

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3556: void
WebRtcVoiceMediaChannel::RecreateCallStreams() {
I think RecreateRecvStreams or RemoveAndAddRecvStreams would be better, since
that what it does.

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3566: void
WebRtcVoiceMediaChannel::AddAudioRecvStream(uint32 ssrc) {
Since it's obviously not video streams, why not just call it AddRecvStream (and
RemoveRecvStream below)?

[the sun, 2015-09-03 15:00:47]

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:56: video_channel_ = video_channel;
On 2015/09/02 21:24:22, pthatcher1 wrote:
> WebRtcVideoChannel2 implements webrtc::LoadObserver, so why not just have
> set_load_observer(webrtc::LoadObserver* observer) here?  I think that would be
a
> lot more clean.

You have few comments about LoadObserver; let's keep the discussion here.

Indeed, the solution could be a cleaner. I don't think we should add a
Call::set_load_observer() method and start compromising the immutableness of
Call. Instead, if overuse_callback was specified per stream rather than in the
Call::Config, we could maintain the current relationship between
webrtc::VideoSendStream and WViC2 (incidentally get rid of a lock too, in
webrtc::internal::CpuOveruseObserverProxy). Call doesn't do anything smart when
overuse is detected, it merely forwards the information, funneling it through to
the same callback. We could leave it to the client to decide whether that is one
single callback instance or not.

Such a solution would fit well with what we've discussed about breaking Call
apart going forward.

pbos@, what do you think?

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:69: DCHECK(rtc::Thread::Current() ==
worker_thread_);
On 2015/09/02 21:24:22, pthatcher1 wrote:
> I think we usually use worker_thread_->IsCurrent() for these checks.

Done.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:71: config.overuse_callback = this;
On 2015/09/02 21:24:22, pthatcher1 wrote:
> Instead of passing in a MediaEngineInteface, webrtc::VoiceEngine directly if
> that's all that we need?

Done.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:41: class MediaController : public
webrtc::LoadObserver {
On 2015/09/02 21:24:22, pthatcher1 wrote:
> This needs a comment saying what it is.

Done.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:48: void
set_video_channel(cricket::VideoChannel* video_channel);
On 2015/09/02 21:24:22, pthatcher1 wrote:
> Similarly here: I think this is "_w".

Part of the LoadObserver discussion, but no, this is signaling thread - that's
why there's a lock in the impl.

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:554:
media_controller_->set_video_channel(nullptr);
On 2015/09/02 21:24:22, pthatcher1 wrote:
> Why is this necessary if media_controller_ is about to be destroyed?

Part of LoadObserver discussion. Not strictly necessary, hopefully, but I'm a
bit paranoid.

https://codereview.webrtc.org/1269863005/diff/200001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/libjingle.gyp#newco...
talk/libjingle.gyp:728: 'app/webrtc/mediacontroller.h',
On 2015/09/02 21:24:22, pthatcher1 wrote:
> FYI, this will break the Chrome FYI bots.  Be prepared to update the .gyp file
> in Chrome as well.

Right, thanks for the reminder!

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.cc:591: external_encoder_factory_,
external_decoder_factory_);
On 2015/09/02 21:24:22, pthatcher1 wrote:
> Why not pass the WebRtcVideoChannel2 into webrtc::Call right here?
> 
> call->set_load_observer(channel);

Part of LoadObserver discussion.

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3556: void
WebRtcVoiceMediaChannel::RecreateCallStreams() {
On 2015/09/02 21:24:22, pthatcher1 wrote:
> I think RecreateRecvStreams or RemoveAndAddRecvStreams would be better, since
> that what it does.

I've changed the name to RecreateAudioReceiveStreams since that is the intent of
the method. I also changed the name of the Add and Remove functions to not use
the Recv abbreviation (the objects they create/destroy are called
AudioReceiveStream after all).

https://codereview.webrtc.org/1269863005/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3566: void
WebRtcVoiceMediaChannel::AddAudioRecvStream(uint32 ssrc) {
On 2015/09/02 21:24:22, pthatcher1 wrote:
> Since it's obviously not video streams, why not just call it AddRecvStream
(and
> RemoveRecvStream below)?

Remember that "Stream" is an overloaded word here, and because we already have
WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) I think it makes
sense to add the "Audio" qualifier.

[pbos-webrtc, 2015-09-03 15:07:23]

On 2015/09/03 15:00:47, the sun wrote:
>
https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.cc (right):
> 
>
https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.cc:56: video_channel_ = video_channel;
> On 2015/09/02 21:24:22, pthatcher1 wrote:
> > WebRtcVideoChannel2 implements webrtc::LoadObserver, so why not just have
> > set_load_observer(webrtc::LoadObserver* observer) here?  I think that would
be
> a
> > lot more clean.
> 
> You have few comments about LoadObserver; let's keep the discussion here.
> 
> Indeed, the solution could be a cleaner. I don't think we should add a
> Call::set_load_observer() method and start compromising the immutableness of
> Call. Instead, if overuse_callback was specified per stream rather than in the
> Call::Config, we could maintain the current relationship between
> webrtc::VideoSendStream and WViC2 (incidentally get rid of a lock too, in
> webrtc::internal::CpuOveruseObserverProxy). Call doesn't do anything smart
when
> overuse is detected, it merely forwards the information, funneling it through
to
> the same callback. We could leave it to the client to decide whether that is
one
> single callback instance or not.
> 
> Such a solution would fit well with what we've discussed about breaking Call
> apart going forward.
> 
> pbos@, what do you think?

I think we want to keep LoadObserver as global as possible. It's supposed to
indicate that the system is overloaded which is not a per-stream decision,
otherwise we should probably discuss changing what overuse is supposed to mean.

[pbos-webrtc, 2015-09-03 16:31:40]

On 2015/09/03 15:07:23, pbos-webrtc wrote:
> On 2015/09/03 15:00:47, the sun wrote:
> >
>
https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
> > File talk/app/webrtc/mediacontroller.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
> > talk/app/webrtc/mediacontroller.cc:56: video_channel_ = video_channel;
> > On 2015/09/02 21:24:22, pthatcher1 wrote:
> > > WebRtcVideoChannel2 implements webrtc::LoadObserver, so why not just have
> > > set_load_observer(webrtc::LoadObserver* observer) here?  I think that
would
> be
> > a
> > > lot more clean.
> > 
> > You have few comments about LoadObserver; let's keep the discussion here.
> > 
> > Indeed, the solution could be a cleaner. I don't think we should add a
> > Call::set_load_observer() method and start compromising the immutableness of
> > Call. Instead, if overuse_callback was specified per stream rather than in
the
> > Call::Config, we could maintain the current relationship between
> > webrtc::VideoSendStream and WViC2 (incidentally get rid of a lock too, in
> > webrtc::internal::CpuOveruseObserverProxy). Call doesn't do anything smart
> when
> > overuse is detected, it merely forwards the information, funneling it
through
> to
> > the same callback. We could leave it to the client to decide whether that is
> one
> > single callback instance or not.
> > 
> > Such a solution would fit well with what we've discussed about breaking Call
> > apart going forward.
> > 
> > pbos@, what do you think?
> 
> I think we want to keep LoadObserver as global as possible. It's supposed to
> indicate that the system is overloaded which is not a per-stream decision,
> otherwise we should probably discuss changing what overuse is supposed to
mean.

Spoke to solenberg@ offline and have reversed my opinion. Having a load observer
per-stream permits only downscaling the larger streams (which are likelier to
trigger an overuse callback) while keeping smaller streams intact. I think this
makes sense, he'll do it in a separate CL before landing this.

[the sun, 2015-09-03 20:33:31]

On 2015/09/03 16:31:40, pbos-webrtc wrote:
> On 2015/09/03 15:07:23, pbos-webrtc wrote:
> > On 2015/09/03 15:00:47, the sun wrote:
> > >
> >
>
https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
> > > File talk/app/webrtc/mediacontroller.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1269863005/diff/200001/talk/app/webrtc/mediacon...
> > > talk/app/webrtc/mediacontroller.cc:56: video_channel_ = video_channel;
> > > On 2015/09/02 21:24:22, pthatcher1 wrote:
> > > > WebRtcVideoChannel2 implements webrtc::LoadObserver, so why not just
have
> > > > set_load_observer(webrtc::LoadObserver* observer) here?  I think that
> would
> > be
> > > a
> > > > lot more clean.
> > > 
> > > You have few comments about LoadObserver; let's keep the discussion here.
> > > 
> > > Indeed, the solution could be a cleaner. I don't think we should add a
> > > Call::set_load_observer() method and start compromising the immutableness
of
> > > Call. Instead, if overuse_callback was specified per stream rather than in
> the
> > > Call::Config, we could maintain the current relationship between
> > > webrtc::VideoSendStream and WViC2 (incidentally get rid of a lock too, in
> > > webrtc::internal::CpuOveruseObserverProxy). Call doesn't do anything smart
> > when
> > > overuse is detected, it merely forwards the information, funneling it
> through
> > to
> > > the same callback. We could leave it to the client to decide whether that
is
> > one
> > > single callback instance or not.
> > > 
> > > Such a solution would fit well with what we've discussed about breaking
Call
> > > apart going forward.
> > > 
> > > pbos@, what do you think?
> > 
> > I think we want to keep LoadObserver as global as possible. It's supposed to
> > indicate that the system is overloaded which is not a per-stream decision,
> > otherwise we should probably discuss changing what overuse is supposed to
> mean.
> 
> Spoke to solenberg@ offline and have reversed my opinion. Having a load
observer
> per-stream permits only downscaling the larger streams (which are likelier to
> trigger an overuse callback) while keeping smaller streams intact. I think
this
> makes sense, he'll do it in a separate CL before landing this.

Here's the CL: https://codereview.webrtc.org/1325263002/

[pthatcher1, 2015-09-04 04:52:31]

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:42: // MediaController owns state shared
between multiple concurrent media streams.
What does "concurrent" mean?  
And what does "media stream" mean?

Perhaps for now we should just say "currently owns shared state between media
channels, but in the future will create and own RtpSenders and RtpReceivers", or
something like that.

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:49: webrtc::Call* call() { return call_.get();
}
This still needs clear thread safety.

[pthatcher1, 2015-09-04 04:56:27]

So are you going to merge this with the load balancer redo?

[the sun, 2015-09-04 10:18:47]

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:42: // MediaController owns state shared
between multiple concurrent media streams.
On 2015/09/04 04:52:31, pthatcher1 wrote:
> What does "concurrent" mean?  
> And what does "media stream" mean?

"media stream" was referring to the actual stream objects that share the state
through the Call instance.

> Perhaps for now we should just say "currently owns shared state between media
> channels, but in the future will create and own RtpSenders and RtpReceivers",
or
> something like that.

That's slightly better, agreed, although "future" is kind of relative. Doesn't
mean much without a date attached to it. :)

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:49: webrtc::Call* call() { return call_.get();
}
On 2015/09/04 04:52:31, pthatcher1 wrote:
> This still needs clear thread safety.

Let me first say that thread safety is a very big concern for me. However, I
don't see how this construct is any more dangerous than what is already going on
with e.g. BaseSession* being passed to ChannelManager::CreateNnChannel(). In
this case, the life span of the session, MediaController and Call is close to
identical.

If your objective is that we should fetch the Call* on the worker thread, I can
see a few ways to accomplish that:

1. Forward the MediaController* instead of webrtc::Call* to
ChannelManager::CreateNnChannel(), pass it down to the _w functions and pull out
the Call* there; we could rename this getter call_w() then.

2. Add a "virtual MediaController* BaseSession::media_controller_w() = 0"; now
we can get the Call* just when the mediachannel is created, on the worker
thread.

3. Let ChannelManager keep a map BaseSession* -> Call* so we don't need the MC
at all, right now. Mapping is implicit and lookup/create/destroy Call* all
happens on the worker thread.

I'd pick #2 because that allows the ChannelManager interface to be simpler, or
leave as it is right now.

[pthatcher1, 2015-09-04 22:58:32]

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:49: webrtc::Call* call() { return call_.get();
}
On 2015/09/04 10:18:47, the sun wrote:
> On 2015/09/04 04:52:31, pthatcher1 wrote:
> > This still needs clear thread safety.
> 
> Let me first say that thread safety is a very big concern for me. However, I
> don't see how this construct is any more dangerous than what is already going
on
> with e.g. BaseSession* being passed to ChannelManager::CreateNnChannel(). In
> this case, the life span of the session, MediaController and Call is close to
> identical.
> 
> If your objective is that we should fetch the Call* on the worker thread, I
can
> see a few ways to accomplish that:
> 
> 1. Forward the MediaController* instead of webrtc::Call* to
> ChannelManager::CreateNnChannel(), pass it down to the _w functions and pull
out
> the Call* there; we could rename this getter call_w() then.
> 
> 2. Add a "virtual MediaController* BaseSession::media_controller_w() = 0"; now
> we can get the Call* just when the mediachannel is created, on the worker
> thread.
> 
> 3. Let ChannelManager keep a map BaseSession* -> Call* so we don't need the MC
> at all, right now. Mapping is implicit and lookup/create/destroy Call* all
> happens on the worker thread.
> 
> I'd pick #2 because that allows the ChannelManager interface to be simpler, or
> leave as it is right now.

That's exactly why we're refactoring ChannelManager so we don't pass in Session,
and why in the replacement (TranpsortController) we're being very careful about
which methods can be called on which threads.  If all goes well, that CL is
going in today.

Out of those, I think #1 makes a lot of sense.  I'd rather not see anything more
go into BaseSession (#2).  We're trying to get rid of BaseSession.


Another option is to do a thread hop from call() to call_w().  But I think I
like #1 better.

[the sun, 2015-09-08 13:44:29]

On 2015/09/04 22:58:32, pthatcher1 wrote:
>
https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.h (right):
> 
>
https://codereview.webrtc.org/1269863005/diff/240001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.h:49: webrtc::Call* call() { return
call_.get();
> }
> On 2015/09/04 10:18:47, the sun wrote:
> > On 2015/09/04 04:52:31, pthatcher1 wrote:
> > > This still needs clear thread safety.
> > 
> > Let me first say that thread safety is a very big concern for me. However, I
> > don't see how this construct is any more dangerous than what is already
going
> on
> > with e.g. BaseSession* being passed to ChannelManager::CreateNnChannel(). In
> > this case, the life span of the session, MediaController and Call is close
to
> > identical.
> > 
> > If your objective is that we should fetch the Call* on the worker thread, I
> can
> > see a few ways to accomplish that:
> > 
> > 1. Forward the MediaController* instead of webrtc::Call* to
> > ChannelManager::CreateNnChannel(), pass it down to the _w functions and pull
> out
> > the Call* there; we could rename this getter call_w() then.
> > 
> > 2. Add a "virtual MediaController* BaseSession::media_controller_w() = 0";
now
> > we can get the Call* just when the mediachannel is created, on the worker
> > thread.
> > 
> > 3. Let ChannelManager keep a map BaseSession* -> Call* so we don't need the
MC
> > at all, right now. Mapping is implicit and lookup/create/destroy Call* all
> > happens on the worker thread.
> > 
> > I'd pick #2 because that allows the ChannelManager interface to be simpler,
or
> > leave as it is right now.
> 
> That's exactly why we're refactoring ChannelManager so we don't pass in
Session,
> and why in the replacement (TranpsortController) we're being very careful
about
> which methods can be called on which threads.  If all goes well, that CL is
> going in today.
> 
> Out of those, I think #1 makes a lot of sense.  I'd rather not see anything
more
> go into BaseSession (#2).  We're trying to get rid of BaseSession.
> 
> 
> Another option is to do a thread hop from call() to call_w().  But I think I
> like #1 better.

1. Merged with LoadObserver change.
2. Implemented #1, which also made it evident that:
3. MediaController needed to be an abstract interface to make it mock/fakeable.

PTAL

[pbos-webrtc, 2015-09-08 14:47:41]

lgtm

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/base/videoeng...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/base/videoeng...
talk/media/base/videoengine_unittest.h:1883: rtc::scoped_ptr<webrtc::Call>
call_;
const rtc::scoped_ptr

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.h:494: webrtc::Call* call_;
Call* const

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2751:
WebRtcVideoEngine2SimulcastTest() {}
Remove ctor.

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2754: WebRtcVideoEngine2
engine_;
If this is unused now then this should just be an empty class right?

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3543: {
Remove {}s ? They're not scoping anything that should be destroyed before
engine.terminate() that's not explicitly deleted either way, right?

[the sun, 2015-09-08 15:21:35]

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/base/videoeng...
File talk/media/base/videoengine_unittest.h (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/base/videoeng...
talk/media/base/videoengine_unittest.h:1883: rtc::scoped_ptr<webrtc::Call>
call_;
On 2015/09/08 14:47:41, pbos-webrtc wrote:
> const rtc::scoped_ptr

Done.

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2.h:494: webrtc::Call* call_;
On 2015/09/08 14:47:41, pbos-webrtc wrote:
> Call* const

Do you mean I should use a reference (&) instead?

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2751:
WebRtcVideoEngine2SimulcastTest() {}
On 2015/09/08 14:47:41, pbos-webrtc wrote:
> Remove ctor.

Done.

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2754: WebRtcVideoEngine2
engine_;
On 2015/09/08 14:47:41, pbos-webrtc wrote:
> If this is unused now then this should just be an empty class right?

Done.

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1269863005/diff/300001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3543: {
On 2015/09/08 14:47:41, pbos-webrtc wrote:
> Remove {}s ? They're not scoping anything that should be destroyed before
> engine.terminate() that's not explicitly deleted either way, right? 

Done.

[pthatcher1, 2015-09-09 07:27:43]

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:40: class MediaControllerImpl : public
MediaController {
And this should be class MediaController : MediaControllerInterface

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:39: class MediaController {
If this is an interface, it should be called MediaControllerInterface.

https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:421: WebRtcVoiceEngine* const engine_;
Why 

WebRtcVoiceEngine const engine_;

Instead of 

const WebRtcVoiceEngine engine_;

?

https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
File talk/session/media/channelmanager.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
talk/session/media/channelmanager.h:113: webrtc::MediaController*
media_controller,
I think it should be the first argument.

[pbos-webrtc, 2015-09-09 07:58:46]

https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:421: WebRtcVoiceEngine* const engine_;
On 2015/09/09 07:27:43, pthatcher1 wrote:
> Why 
> 
> WebRtcVoiceEngine const engine_;
> 
> Instead of 
> 
> const WebRtcVoiceEngine engine_;
> 
> ?
> 

Pointer is const and doesn't change, but the object is not const:

Foo* const x;  // x is a const-pointer
const Foo* x;  // x points to a const Foo.

[the sun, 2015-09-09 08:24:51]

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:40: class MediaControllerImpl : public
MediaController {
On 2015/09/09 07:27:43, pthatcher1 wrote:
> And this should be class MediaController : MediaControllerInterface

Done.

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.h:39: class MediaController {
On 2015/09/09 07:27:43, pthatcher1 wrote:
> If this is an interface, it should be called MediaControllerInterface.

Doh!

Note to self: re-read the style guide.

https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:421: WebRtcVoiceEngine* const engine_;
On 2015/09/09 07:27:43, pthatcher1 wrote:
> Why 
> 
> WebRtcVoiceEngine const engine_;
> 
> Instead of 
> 
> const WebRtcVoiceEngine engine_;
> 
> ?
> 

The first one declares that the *pointer* is const. Similar to using a
(non-const) C++ reference.
The second declares that the object *pointed to* is const.

I changed it here following pbos' comment in webrtcvideoengine2.h, to be more
consistent. I prefer to keep it a pointer rather than a reference because the
dereferencing syntax makes it obvious to the reader the pointed to object isn't
necessarily owned by *this.

https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
File talk/session/media/channelmanager.h (right):

https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
talk/session/media/channelmanager.h:113: webrtc::MediaController*
media_controller,
On 2015/09/09 07:27:43, pthatcher1 wrote:
> I think it should be the first argument.  

Done.

[the sun, 2015-09-10 15:09:36]

On 2015/09/09 08:24:51, the sun wrote:
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.cc (right):
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.cc:40: class MediaControllerImpl : public
> MediaController {
> On 2015/09/09 07:27:43, pthatcher1 wrote:
> > And this should be class MediaController : MediaControllerInterface
> 
> Done.
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.h (right):
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.h:39: class MediaController {
> On 2015/09/09 07:27:43, pthatcher1 wrote:
> > If this is an interface, it should be called MediaControllerInterface.
> 
> Doh!
> 
> Note to self: re-read the style guide.
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.h:421: WebRtcVoiceEngine* const engine_;
> On 2015/09/09 07:27:43, pthatcher1 wrote:
> > Why 
> > 
> > WebRtcVoiceEngine const engine_;
> > 
> > Instead of 
> > 
> > const WebRtcVoiceEngine engine_;
> > 
> > ?
> > 
> 
> The first one declares that the *pointer* is const. Similar to using a
> (non-const) C++ reference.
> The second declares that the object *pointed to* is const.
> 
> I changed it here following pbos' comment in webrtcvideoengine2.h, to be more
> consistent. I prefer to keep it a pointer rather than a reference because the
> dereferencing syntax makes it obvious to the reader the pointed to object
isn't
> necessarily owned by *this.
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
> File talk/session/media/channelmanager.h (right):
> 
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
> talk/session/media/channelmanager.h:113: webrtc::MediaController*
> media_controller,
> On 2015/09/09 07:27:43, pthatcher1 wrote:
> > I think it should be the first argument.  
> 
> Done.

pthatcher@, is there anything else you'd like me to address?

[the sun, 2015-09-14 08:02:53]

On 2015/09/10 15:09:36, the sun wrote:
> On 2015/09/09 08:24:51, the sun wrote:
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> > File talk/app/webrtc/mediacontroller.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> > talk/app/webrtc/mediacontroller.cc:40: class MediaControllerImpl : public
> > MediaController {
> > On 2015/09/09 07:27:43, pthatcher1 wrote:
> > > And this should be class MediaController : MediaControllerInterface
> > 
> > Done.
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> > File talk/app/webrtc/mediacontroller.h (right):
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/app/webrtc/mediacon...
> > talk/app/webrtc/mediacontroller.h:39: class MediaController {
> > On 2015/09/09 07:27:43, pthatcher1 wrote:
> > > If this is an interface, it should be called MediaControllerInterface.
> > 
> > Doh!
> > 
> > Note to self: re-read the style guide.
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
> > File talk/media/webrtc/webrtcvoiceengine.h (right):
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/media/webrtc/webrtc...
> > talk/media/webrtc/webrtcvoiceengine.h:421: WebRtcVoiceEngine* const engine_;
> > On 2015/09/09 07:27:43, pthatcher1 wrote:
> > > Why 
> > > 
> > > WebRtcVoiceEngine const engine_;
> > > 
> > > Instead of 
> > > 
> > > const WebRtcVoiceEngine engine_;
> > > 
> > > ?
> > > 
> > 
> > The first one declares that the *pointer* is const. Similar to using a
> > (non-const) C++ reference.
> > The second declares that the object *pointed to* is const.
> > 
> > I changed it here following pbos' comment in webrtcvideoengine2.h, to be
more
> > consistent. I prefer to keep it a pointer rather than a reference because
the
> > dereferencing syntax makes it obvious to the reader the pointed to object
> isn't
> > necessarily owned by *this.
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
> > File talk/session/media/channelmanager.h (right):
> > 
> >
>
https://codereview.webrtc.org/1269863005/diff/320001/talk/session/media/chann...
> > talk/session/media/channelmanager.h:113: webrtc::MediaController*
> > media_controller,
> > On 2015/09/09 07:27:43, pthatcher1 wrote:
> > > I think it should be the first argument.  
> > 
> > Done.
> 
> pthatcher@, is there anything else you'd like me to address?

Ping...

[pthatcher1, 2015-09-15 00:30:56]

lgtm

[the sun, 2015-09-15 08:37:31]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-09-15 08:37:31]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1269863005/#ps460001 (title:
"Remove redundant reset(nullptr)")

[commit-bot: I haz the power, 2015-09-15 08:37:38]

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

[the sun, 2015-09-15 10:26:51]

Committed patchset #24 (id:460001) manually as
709ed67c38d0a942f3bf3e68e337cc27a27bc353 (presubmit successful).

[commit-bot: I haz the power, 2015-09-15 10:26:52]

Patchset 24 (id:??) landed as
https://crrev.com/709ed67c38d0a942f3bf3e68e337cc27a27bc353
Cr-Commit-Position: refs/heads/master@{#9939}