Late initialize MediaController, for less resource i.e. ProcessThread, usage by PeerConnection.

Late initialize MediaController, for less resource i.e. ProcessThread, usage by PeerConnection.

BUG=chromium:582441
Committed: https://crrev.com/03d6d57f41cd36e9cf6c9d02c2a04c2775a8611c
Cr-Commit-Position: refs/heads/master@{#11834}

[the sun, 2016-02-22 15:02:12]

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

[the sun, 2016-02-22 15:02:12]

While there are issues with bots failing and the fact that I'm not particularly
happy with the solution, I'm sharing this to get some initial feedback on the
approach, so that we increase the chances of landing a fix for M50.

Basically, media_controller_ and remote_stream_factory_ are now lazy initialized
whenever PC actually requires them, which means we can get rid of the resources
in PC::Close().

It is a bit of a hack. I would have liked to make more PC resources lazy inited
but won't be able to do so within the time frame, given how entangled
PeerConnect, WebRtcSession and friends are (and the extent of my current
knowledge).

WDYT? Can live with? Better let PC Principal tackle this? Better ideas?

[tommi, 2016-02-22 15:22:06]

I see some of the concerns you might have, but I think that your goal makes it
an acceptable approach.  Things can and will continue to be refactored and
improved.

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:858: stats_->GetStats(track, &reports);
Is this change necessary for the goal of the CL?

I worry that this could have some unwanted side effects. Today, OnComplete() is
called after GetStats() has executed, but this forces it to be _while_ GetStats
is executing.  GetStats is meant to be async, so I'd prefer to keep it working
like that and give us an opportunity to improve things such as this:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
This is btw a TODO I put down for myself after restructuring that code quite a
bit and make it obvious that it's not truly async, while it should be.

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1493:
session_->LateInitialize(media_controller_.get());
Would it make sense to media_controller_ be owned by session_? (and no
|media_controller_| member variable in PeerConnection)

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2369:
pc->CreateSender(MediaStreamTrackInterface::kAudioKind, "foo_id"));
add a comment about why this is needed?  is the |sender| variable necessary?

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:686: certificate_, this, id()));
after LateInitialize() has executed, is there need for certificate_?  I'm
wondering if it could be passed as a parameter to LateInitialize or if we should
then (at least?) set certificate_ to null at the end of LateInitialize().

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:464: MediaControllerInterface* media_controller_ =
nullptr;
wonder if ownership should simply be here.

[the sun, 2016-02-23 14:14:03]

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:858: stats_->GetStats(track, &reports);
On 2016/02/22 15:22:05, tommi-webrtc wrote:
> Is this change necessary for the goal of the CL?
> 
> I worry that this could have some unwanted side effects. Today, OnComplete()
is
> called after GetStats() has executed, but this forces it to be _while_
GetStats
> is executing.  GetStats is meant to be async, so I'd prefer to keep it working
> like that and give us an opportunity to improve things such as this:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> This is btw a TODO I put down for myself after restructuring that code quite a
> bit and make it obvious that it's not truly async, while it should be.

Duh! I thought I remembered that a Post() to current thread would be executed
synchronously, but it seems I was wrong. This wasn't strictly required for the
CL at hand so I've reverted. Thanks!

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1493:
session_->LateInitialize(media_controller_.get());
On 2016/02/22 15:22:05, tommi-webrtc wrote:
> Would it make sense to media_controller_ be owned by session_? (and no
> |media_controller_| member variable in PeerConnection)

It used to be owned by WebRtcSession: https://codereview.webrtc.org/1269863005/

I don't know the exact reason why the relationship has changed, but I would
assume it has to do with the longer term goal of merging PeerConnection and
WebRtcSession in terms of functionality provided. pthatcher@?

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:686: certificate_, this, id()));
On 2016/02/22 15:22:05, tommi-webrtc wrote:
> after LateInitialize() has executed, is there need for certificate_?  I'm
> wondering if it could be passed as a parameter to LateInitialize or if we
should
> then (at least?) set certificate_ to null at the end of LateInitialize().

I decided not to pass it as an argument as that required storing some of the
other arguments to Initialize() (constraints, rtc_config). So it is one or the
other, and I chose the path where the logic would be changed the least.

Nulling certificate_ makes sense as we don't allow multiple calls to
LateInitialize() per the DCHECKs at the start, and the transfer of ownership of
dtls_identity_store. I wonder however, could there be a situation where we get
other DOM calls after pc.close()?

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:464: MediaControllerInterface* media_controller_ =
nullptr;
On 2016/02/22 15:22:05, tommi-webrtc wrote:
> wonder if ownership should simply be here.

Ack. See comment peerconnection.cc.

[the sun, 2016-02-23 14:32:43]

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1713043002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface_unittest.cc:2369:
pc->CreateSender(MediaStreamTrackInterface::kAudioKind, "foo_id"));
On 2016/02/22 15:22:05, tommi-webrtc wrote:
> add a comment about why this is needed?  is the |sender| variable necessary?

Added comment. As for the variable, in theory we could just ignore the result,
but I think it tells the reader more to leave it in.

[the sun, 2016-02-24 13:15:20]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[the sun, 2016-02-24 13:15:21]

Adding kwiberg@ as another pair of eyes on this rather subtle change.

[kwiberg-webrtc, 2016-02-24 13:47:01]

On 2016/02/24 13:15:21, the sun wrote:
> Adding kwiberg@ as another pair of eyes on this rather subtle change.

I've had a look, and haven't spotted anything suspicious. But two comments:

  1. You could've made almost all of the statscollector changes in a
     separate CL preceding this one, if I'm not mistaken.

  2. You have a bunch of explicit calls to LateInitialize that you
     have to add at all the right places by hand. It might be a good
     idea to move the late-initialized stuff to a separate class, and
     have the only way to get a pointer to that class be through a
     method that does late initialization if required.

I understand if these aren't the suggestions you were looking for
right now, though.

[kwiberg-webrtc, 2016-02-24 14:03:22]

On 2016/02/24 13:47:01, kwiberg-webrtc wrote:
>   2. You have a bunch of explicit calls to LateInitialize that you
>      have to add at all the right places by hand. It might be a good
>      idea to move the late-initialized stuff to a separate class, and
>      have the only way to get a pointer to that class be through a
>      method that does late initialization if required.

That's for the private LateInitialize in PeerConnection; since it's private, the
outside world need never know that PeerConnection initializes parts of itself
lazily.

For WebRtcSession, on the other hand, you have a public LateInitialize that
needs to be public because it takes an argument. This means that everyone that
handles a WebRtcSession needs to be aware that it has two states, and when to
make it change state. I don't see a pretty way to do this other than to split it
into two objects, each of which is always completely initialized.

Not saying you absolutely have to do this right this minute, but it might be
good to keep in mind for later.

[the sun, 2016-02-24 14:30:02]

On 2016/02/24 13:47:01, kwiberg-webrtc wrote:
> On 2016/02/24 13:15:21, the sun wrote:
> > Adding kwiberg@ as another pair of eyes on this rather subtle change.
> 
> I've had a look, and haven't spotted anything suspicious. But two comments:
> 
>   1. You could've made almost all of the statscollector changes in a
>      separate CL preceding this one, if I'm not mistaken.

Yes, I did those changes (using thread checker instead of fetching a pointer to
singnal_thread from session() as an aside, while getting to know the code. I was
trying to see how changing the way WebRtcSession relates to MediaController
might affect StatsCollector. I could revert it I guess.

>   2. You have a bunch of explicit calls to LateInitialize that you
>      have to add at all the right places by hand. It might be a good
>      idea to move the late-initialized stuff to a separate class, and
>      have the only way to get a pointer to that class be through a
>      method that does late initialization if required.
> 
> I understand if these aren't the suggestions you were looking for
> right now, though.

[the sun, 2016-02-24 14:56:52]

On 2016/02/24 14:03:22, kwiberg-webrtc wrote:
> On 2016/02/24 13:47:01, kwiberg-webrtc wrote:
> >   2. You have a bunch of explicit calls to LateInitialize that you
> >      have to add at all the right places by hand. It might be a good
> >      idea to move the late-initialized stuff to a separate class, and
> >      have the only way to get a pointer to that class be through a
> >      method that does late initialization if required.
> 
> That's for the private LateInitialize in PeerConnection; since it's private,
the
> outside world need never know that PeerConnection initializes parts of itself
> lazily.
> 
> For WebRtcSession, on the other hand, you have a public LateInitialize that
> needs to be public because it takes an argument. This means that everyone that
> handles a WebRtcSession needs to be aware that it has two states, and when to
> make it change state. I don't see a pretty way to do this other than to split
it
> into two objects, each of which is always completely initialized.
> 
> Not saying you absolutely have to do this right this minute, but it might be
> good to keep in mind for later.

Got it.

I think the long term goal is to remove WebRtcSession, essentially folding it
into PeerConnection, with the parts depending on MediaController split out into
RtpSender/RtpReceiver classes (and similarly with the transport classes,
depending on TransportController) so I doubt splitting it up into two classes is
what we want to do right now. (See:
http://ortc.org/wp-content/uploads/2014/08/ortc.html)

Likewise with PeerConnection. Like I said in an initial comment I would have
liked for much more of the PC state to be lazy initialized, but it feels the
only way to do it "properly" would require significant refactoring of the
relationship between PC, WebRtcSession, StatsCollector and more; definitely more
work than can be handled within the time frame for the M50 cut. As it stands,
and as you note, even if breaking out a class to keep the calls that rely on
LateInitialize() to be called, what we're actually doing then is splitting up PC
into two classes, to handle the two different states of WebRtcSession.

While this change frankly gives me the creeps, I believe it will reduce the
number of resource starvation issues that we've seen reported. I would like to
commit it with a TBR for pthatcher@ and am ready to revert if there are more
concerns or if it proves to cause problems.

[the sun, 2016-02-24 16:29:15]

On 2016/02/24 14:56:52, the sun wrote:
> On 2016/02/24 14:03:22, kwiberg-webrtc wrote:
> > On 2016/02/24 13:47:01, kwiberg-webrtc wrote:
> > >   2. You have a bunch of explicit calls to LateInitialize that you
> > >      have to add at all the right places by hand. It might be a good
> > >      idea to move the late-initialized stuff to a separate class, and
> > >      have the only way to get a pointer to that class be through a
> > >      method that does late initialization if required.
> > 
> > That's for the private LateInitialize in PeerConnection; since it's private,
> the
> > outside world need never know that PeerConnection initializes parts of
itself
> > lazily.
> > 
> > For WebRtcSession, on the other hand, you have a public LateInitialize that
> > needs to be public because it takes an argument. This means that everyone
that
> > handles a WebRtcSession needs to be aware that it has two states, and when
to
> > make it change state. I don't see a pretty way to do this other than to
split
> it
> > into two objects, each of which is always completely initialized.
> > 
> > Not saying you absolutely have to do this right this minute, but it might be
> > good to keep in mind for later.
> 
> Got it.
> 
> I think the long term goal is to remove WebRtcSession, essentially folding it
> into PeerConnection, with the parts depending on MediaController split out
into
> RtpSender/RtpReceiver classes (and similarly with the transport classes,
> depending on TransportController) so I doubt splitting it up into two classes
is
> what we want to do right now. (See:
> http://ortc.org/wp-content/uploads/2014/08/ortc.html)
> 
> Likewise with PeerConnection. Like I said in an initial comment I would have
> liked for much more of the PC state to be lazy initialized, but it feels the
> only way to do it "properly" would require significant refactoring of the
> relationship between PC, WebRtcSession, StatsCollector and more; definitely
more
> work than can be handled within the time frame for the M50 cut. As it stands,
> and as you note, even if breaking out a class to keep the calls that rely on
> LateInitialize() to be called, what we're actually doing then is splitting up
PC
> into two classes, to handle the two different states of WebRtcSession.
> 
> While this change frankly gives me the creeps, I believe it will reduce the
> number of resource starvation issues that we've seen reported. I would like to
> commit it with a TBR for pthatcher@ and am ready to revert if there are more
> concerns or if it proves to cause problems.

Ping pthatcher@; your input on this would be much appreciated.

[pthatcher1, 2016-02-24 23:10:09]

I think there are several changes that we can make that would make this a lot
less ugly and error-prone.

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:745: LateInitialize();
Instead of calling LateInitialize everywhere (which is really error-prone), why
not just have the media_controller created on demand in the media_controller()
method?

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1511:
session_->LateInitialize(media_controller_.get());
This would make a lot more sense if WebrtcSession and PeerConnection were one
class.  And it's our intention to merge them, we just haven't done it yet.  If
you'd like to do this "lazy load" thing, I think it would be much better to
merge the two classes first to avoid having to initialize methods in
WebRtcSession.

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1513: factory_->signaling_thread(),
media_controller_->channel_manager()));
I looked into this, and found that the ChannelManager here is only needed for
the SignalVideoCaptureStateChange event, which isn't really needed.  I think we
can remove the signal and remove the need to pass the ChannelManager into the
RemoteMedisStreamFactory.

More info here:
https://docs.google.com/document/d/12LMKcZlcLnw62ijmIiLzjJ6GW_cDhGtOUgKiCVKw9...

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1715: video_channel_.release());
Why not just make a channel_manager() method?

ChannelManager* channel_manager() {
  RTC_DCHECK(media_controller_);
  return media_controller_->channel_manager();
}

Then all of these would just change from channel_manager_-> to
channel_manager()->

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:151: void
LateInitialize(webrtc::MediaControllerInterface* media_controller);
It's not clear what you can do with a WebRtcSession that has been Initialized,
but not LateInitialized.  Is that a state that even has any value?  If not, why
bother have two Initialize methods?  If so, what things can you do in one place
and not the other.

Honestly, this doesn't smell right at all, to have two initialize methods.

[the sun, 2016-02-25 13:15:39]

See new approach in latest patch set. Still incomplete as there are tests
failing, but I think I like it better.

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:745: LateInitialize();
On 2016/02/24 23:10:09, pthatcher1 wrote:
> Instead of calling LateInitialize everywhere (which is really error-prone),
why
> not just have the media_controller created on demand in the media_controller()
> method?
> 

That's a good suggestion, but unfortunately it doesn't apply here as this
function does not reference media_controller_ directly. LateInitialize() is
called here because of how we use WebRtcSession; in essence it is there to
manage the dual states of WebRtcSession.

I guess another option would be to add an internal fully_initialized_session()
helper, which would be slightly better explicit calls to LateInitialize(), but
still very error prone.

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1511:
session_->LateInitialize(media_controller_.get());
On 2016/02/24 23:10:09, pthatcher1 wrote:
> This would make a lot more sense if WebrtcSession and PeerConnection were one
> class.  And it's our intention to merge them, we just haven't done it yet.  If
> you'd like to do this "lazy load" thing, I think it would be much better to
> merge the two classes first to avoid having to initialize methods in
> WebRtcSession.

Agree. Like I noted in an initial comment, that is not an option if we are to
land a fix for crbug:582441 for the M50 cut. So the question is whether we want
to land this interim fix or not. Myself, I'm torn.

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1513: factory_->signaling_thread(),
media_controller_->channel_manager()));
On 2016/02/24 23:10:09, pthatcher1 wrote:
> I looked into this, and found that the ChannelManager here is only needed for
> the SignalVideoCaptureStateChange event, which isn't really needed.  I think
we
> can remove the signal and remove the need to pass the ChannelManager into the
> RemoteMedisStreamFactory.
> 
> More info here:
>
https://docs.google.com/document/d/12LMKcZlcLnw62ijmIiLzjJ6GW_cDhGtOUgKiCVKw9...

Wow, thanks for taking such a deep dive!

What about the calls from VideoSource to channel_manager_->AddVideoSink() and
channel_manager_->RemoveVideoSink()? Or the calls from
ChannelManager::StartVideoCapture() onto CaptureManager::StartVideoCapture()
(and similarly for Stop)?

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:151: void
LateInitialize(webrtc::MediaControllerInterface* media_controller);
On 2016/02/24 23:10:09, pthatcher1 wrote:
> It's not clear what you can do with a WebRtcSession that has been Initialized,
> but not LateInitialized.  Is that a state that even has any value?  If not,
why
> bother have two Initialize methods?  If so, what things can you do in one
place
> and not the other.

Right. When reading the code I was under the impression that StatsCollector
would be fetching info about transports even if there are no active channels,
but that turns out not being correct. I've reworked the code to lazy-init
WebRtcSession, but get stuck on the
PeerConnectionInterfaceTest.CloseAndTestMethods test case. Apparently it is
expected that PC methods continue to return results even after the PC has been
closed.

I'll need to meditate a little on that. If you have any ideas let me know. 

> Honestly, this doesn't smell right at all, to have two initialize methods.

Agree it reeks.

[pthatcher1, 2016-02-26 21:51:20]

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1511:
session_->LateInitialize(media_controller_.get());
On 2016/02/25 13:15:39, the sun wrote:
> On 2016/02/24 23:10:09, pthatcher1 wrote:
> > This would make a lot more sense if WebrtcSession and PeerConnection were
one
> > class.  And it's our intention to merge them, we just haven't done it yet. 
If
> > you'd like to do this "lazy load" thing, I think it would be much better to
> > merge the two classes first to avoid having to initialize methods in
> > WebRtcSession.
> 
> Agree. Like I noted in an initial comment, that is not an option if we are to
> land a fix for crbug:582441 for the M50 cut. So the question is whether we
want
> to land this interim fix or not. Myself, I'm torn.

I appreciate that we'd like to fix the bug quickly, but this is would be quite a
nasty temporary fix as-is, and it seems like the bug is a very rare one.  

Maybe if we can fix this CL enough to be not so nasty....

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1513: factory_->signaling_thread(),
media_controller_->channel_manager()));
On 2016/02/25 13:15:39, the sun wrote:
> On 2016/02/24 23:10:09, pthatcher1 wrote:
> > I looked into this, and found that the ChannelManager here is only needed
for
> > the SignalVideoCaptureStateChange event, which isn't really needed.  I think
> we
> > can remove the signal and remove the need to pass the ChannelManager into
the
> > RemoteMedisStreamFactory.
> > 
> > More info here:
> >
>
https://docs.google.com/document/d/12LMKcZlcLnw62ijmIiLzjJ6GW_cDhGtOUgKiCVKw9...
> 
> Wow, thanks for taking such a deep dive!
> 
> What about the calls from VideoSource to channel_manager_->AddVideoSink() and
> channel_manager_->RemoveVideoSink()? Or the calls from
> ChannelManager::StartVideoCapture() onto CaptureManager::StartVideoCapture()
> (and similarly for Stop)?

ChannelManager::StartVideoCapture() is only called by
VideoSource::Initialize/Create and VideoSource::Restart.

ChannelManager::StopVideoCapture() is only called by VideoSource::Stop() and
VideoSource::~VideoSource.

ChannelManager::AddVideoSink is only called by VideoSource::AddSink.

ChannelManager::RemoveVideoSink is only called by VideoSource::RemoveSink.


In all of these cases, VideoSource knows that it changed something. So I think
it could just do SetState(GetReadyState(capture_state)) at the end of the method
instead of having the change trigger an event which does that.

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:562: class PeerConnection::LiveSession {
What is a LiveSession?  It appears to be a random bag of stuff with a funny
name.

The semantics seem to be "a MediaController plus everything that needs a
MediaController within a PeerConnection".

I think it would be much cleaner to:

1.  Add a WebRtcSession::SetMediaController, and have WebRtcSession act
reasonably when the MediaController is unset (not explode).

2.  Remove the dependency of RemoteMediaStreamFactory on MediaController.  If
that's too hard, also make a RemoteMediaStreamFactory::SetMediaController, like
for WebRtcSession.

3.  When it's time to create a MediaController, do so and set it on the
WebRtcSession (and the RemoteMediaStreamFactory if needed).



As another alternative, why don't we just create a MediaController which the
WebRtcSession and the RemoteMediaStreamFactory can reference immediately, but
not Init the MediaController until later?  It might be easier to work with a
MediaController that hasn't been Inited than with a WebRtcSession without a
MediaController.

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/statscollecto...
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/statscollecto...
webrtc/api/statscollector.cc:472: // when there isn't a session.
I think we can just make this:

TODO(pthatcher): Merge PeerConnection and WebRtcSession so there is no
pc_->session().

[the sun, 2016-02-29 15:58:23]

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1511:
session_->LateInitialize(media_controller_.get());
On 2016/02/26 21:51:20, pthatcher1 wrote:
> On 2016/02/25 13:15:39, the sun wrote:
> > On 2016/02/24 23:10:09, pthatcher1 wrote:
> > > This would make a lot more sense if WebrtcSession and PeerConnection were
> one
> > > class.  And it's our intention to merge them, we just haven't done it yet.

> If
> > > you'd like to do this "lazy load" thing, I think it would be much better
to
> > > merge the two classes first to avoid having to initialize methods in
> > > WebRtcSession.
> > 
> > Agree. Like I noted in an initial comment, that is not an option if we are
to
> > land a fix for crbug:582441 for the M50 cut. So the question is whether we
> want
> > to land this interim fix or not. Myself, I'm torn.
> 
> I appreciate that we'd like to fix the bug quickly, but this is would be quite
a
> nasty temporary fix as-is, and it seems like the bug is a very rare one.  
> 
> Maybe if we can fix this CL enough to be not so nasty....
> 

Acknowledged.

https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:1513: factory_->signaling_thread(),
media_controller_->channel_manager()));
On 2016/02/26 21:51:20, pthatcher1 wrote:
> On 2016/02/25 13:15:39, the sun wrote:
> > On 2016/02/24 23:10:09, pthatcher1 wrote:
> > > I looked into this, and found that the ChannelManager here is only needed
> for
> > > the SignalVideoCaptureStateChange event, which isn't really needed.  I
think
> > we
> > > can remove the signal and remove the need to pass the ChannelManager into
> the
> > > RemoteMedisStreamFactory.
> > > 
> > > More info here:
> > >
> >
>
https://docs.google.com/document/d/12LMKcZlcLnw62ijmIiLzjJ6GW_cDhGtOUgKiCVKw9...
> > 
> > Wow, thanks for taking such a deep dive!
> > 
> > What about the calls from VideoSource to channel_manager_->AddVideoSink()
and
> > channel_manager_->RemoveVideoSink()? Or the calls from
> > ChannelManager::StartVideoCapture() onto CaptureManager::StartVideoCapture()
> > (and similarly for Stop)?
> 
> ChannelManager::StartVideoCapture() is only called by
> VideoSource::Initialize/Create and VideoSource::Restart.
> 
> ChannelManager::StopVideoCapture() is only called by VideoSource::Stop() and
> VideoSource::~VideoSource.
> 
> ChannelManager::AddVideoSink is only called by VideoSource::AddSink.
> 
> ChannelManager::RemoveVideoSink is only called by VideoSource::RemoveSink.
> 
> 
> In all of these cases, VideoSource knows that it changed something. So I think
> it could just do SetState(GetReadyState(capture_state)) at the end of the
method
> instead of having the change trigger an event which does that.
> 

Acknowledged.

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:562: class PeerConnection::LiveSession {
On 2016/02/26 21:51:20, pthatcher1 wrote:
> What is a LiveSession?  It appears to be a random bag of stuff with a funny
> name.
> 
> The semantics seem to be "a MediaController plus everything that needs a
> MediaController within a PeerConnection".
> 
> I think it would be much cleaner to:
> 
> 1.  Add a WebRtcSession::SetMediaController, and have WebRtcSession act
> reasonably when the MediaController is unset (not explode).
> 
> 2.  Remove the dependency of RemoteMediaStreamFactory on MediaController.  If
> that's too hard, also make a RemoteMediaStreamFactory::SetMediaController,
like
> for WebRtcSession.
> 
> 3.  When it's time to create a MediaController, do so and set it on the
> WebRtcSession (and the RemoteMediaStreamFactory if needed).
> 
> 
> 
> As another alternative, why don't we just create a MediaController which the
> WebRtcSession and the RemoteMediaStreamFactory can reference immediately, but
> not Init the MediaController until later?  It might be easier to work with a
> MediaController that hasn't been Inited than with a WebRtcSession without a
> MediaController.

Indeed. Great suggestion! Look at the current solution.

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/statscollecto...
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/statscollecto...
webrtc/api/statscollector.cc:472: // when there isn't a session.
On 2016/02/26 21:51:20, pthatcher1 wrote:
> I think we can just make this:
> 
> TODO(pthatcher): Merge PeerConnection and WebRtcSession so there is no
> pc_->session().  

Done.

[the sun, 2016-02-29 16:18:16]

On 2016/02/29 15:58:23, the sun wrote:
>
https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection.cc
> File webrtc/api/peerconnection.cc (right):
> 
>
https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
> webrtc/api/peerconnection.cc:1511:
> session_->LateInitialize(media_controller_.get());
> On 2016/02/26 21:51:20, pthatcher1 wrote:
> > On 2016/02/25 13:15:39, the sun wrote:
> > > On 2016/02/24 23:10:09, pthatcher1 wrote:
> > > > This would make a lot more sense if WebrtcSession and PeerConnection
were
> > one
> > > > class.  And it's our intention to merge them, we just haven't done it
yet.
> 
> > If
> > > > you'd like to do this "lazy load" thing, I think it would be much better
> to
> > > > merge the two classes first to avoid having to initialize methods in
> > > > WebRtcSession.
> > > 
> > > Agree. Like I noted in an initial comment, that is not an option if we are
> to
> > > land a fix for crbug:582441 for the M50 cut. So the question is whether we
> > want
> > > to land this interim fix or not. Myself, I'm torn.
> > 
> > I appreciate that we'd like to fix the bug quickly, but this is would be
quite
> a
> > nasty temporary fix as-is, and it seems like the bug is a very rare one.  
> > 
> > Maybe if we can fix this CL enough to be not so nasty....
> > 
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1713043002/diff/60001/webrtc/api/peerconnection...
> webrtc/api/peerconnection.cc:1513: factory_->signaling_thread(),
> media_controller_->channel_manager()));
> On 2016/02/26 21:51:20, pthatcher1 wrote:
> > On 2016/02/25 13:15:39, the sun wrote:
> > > On 2016/02/24 23:10:09, pthatcher1 wrote:
> > > > I looked into this, and found that the ChannelManager here is only
needed
> > for
> > > > the SignalVideoCaptureStateChange event, which isn't really needed.  I
> think
> > > we
> > > > can remove the signal and remove the need to pass the ChannelManager
into
> > the
> > > > RemoteMedisStreamFactory.
> > > > 
> > > > More info here:
> > > >
> > >
> >
>
https://docs.google.com/document/d/12LMKcZlcLnw62ijmIiLzjJ6GW_cDhGtOUgKiCVKw9...
> > > 
> > > Wow, thanks for taking such a deep dive!
> > > 
> > > What about the calls from VideoSource to channel_manager_->AddVideoSink()
> and
> > > channel_manager_->RemoveVideoSink()? Or the calls from
> > > ChannelManager::StartVideoCapture() onto
CaptureManager::StartVideoCapture()
> > > (and similarly for Stop)?
> > 
> > ChannelManager::StartVideoCapture() is only called by
> > VideoSource::Initialize/Create and VideoSource::Restart.
> > 
> > ChannelManager::StopVideoCapture() is only called by VideoSource::Stop() and
> > VideoSource::~VideoSource.
> > 
> > ChannelManager::AddVideoSink is only called by VideoSource::AddSink.
> > 
> > ChannelManager::RemoveVideoSink is only called by VideoSource::RemoveSink.
> > 
> > 
> > In all of these cases, VideoSource knows that it changed something. So I
think
> > it could just do SetState(GetReadyState(capture_state)) at the end of the
> method
> > instead of having the change trigger an event which does that.
> > 
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/peerconnectio...
> File webrtc/api/peerconnection.cc (right):
> 
>
https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/peerconnectio...
> webrtc/api/peerconnection.cc:562: class PeerConnection::LiveSession {
> On 2016/02/26 21:51:20, pthatcher1 wrote:
> > What is a LiveSession?  It appears to be a random bag of stuff with a funny
> > name.
> > 
> > The semantics seem to be "a MediaController plus everything that needs a
> > MediaController within a PeerConnection".
> > 
> > I think it would be much cleaner to:
> > 
> > 1.  Add a WebRtcSession::SetMediaController, and have WebRtcSession act
> > reasonably when the MediaController is unset (not explode).
> > 
> > 2.  Remove the dependency of RemoteMediaStreamFactory on MediaController. 
If
> > that's too hard, also make a RemoteMediaStreamFactory::SetMediaController,
> like
> > for WebRtcSession.
> > 
> > 3.  When it's time to create a MediaController, do so and set it on the
> > WebRtcSession (and the RemoteMediaStreamFactory if needed).
> > 
> > 
> > 
> > As another alternative, why don't we just create a MediaController which the
> > WebRtcSession and the RemoteMediaStreamFactory can reference immediately,
but
> > not Init the MediaController until later?  It might be easier to work with a
> > MediaController that hasn't been Inited than with a WebRtcSession without a
> > MediaController.
> 
> Indeed. Great suggestion! Look at the current solution.
> 
>
https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/statscollecto...
> File webrtc/api/statscollector.cc (right):
> 
>
https://codereview.webrtc.org/1713043002/diff/100001/webrtc/api/statscollecto...
> webrtc/api/statscollector.cc:472: // when there isn't a session.
> On 2016/02/26 21:51:20, pthatcher1 wrote:
> > I think we can just make this:
> > 
> > TODO(pthatcher): Merge PeerConnection and WebRtcSession so there is no
> > pc_->session().  
> 
> Done.

I feel a lot better about the most recent patch set. PTAL.

[the sun, 2016-02-29 16:18:46]

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

[commit-bot: I haz the power, 2016-02-29 16:18:56]

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

[commit-bot: I haz the power, 2016-02-29 18:19:44]

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

[commit-bot: I haz the power, 2016-02-29 18:19:45]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[tommi, 2016-03-01 09:57:41]

wow, that looks impressively simple :-|  lgtm!

https://codereview.webrtc.org/1713043002/diff/140001/webrtc/api/mediacontroll...
File webrtc/api/mediacontroller.cc (right):

https://codereview.webrtc.org/1713043002/diff/140001/webrtc/api/mediacontroll...
webrtc/api/mediacontroller.cc:42: void Close() override {
nit: there's sometimes an empty line between methods in the class and sometimes
not.  I'm not sure what the style is but it files like Close() should be
separate from the ctor/dtor at least.  Feel free to make the whole thing
consistent though :)

[the sun, 2016-03-01 10:52:30]

https://codereview.webrtc.org/1713043002/diff/140001/webrtc/api/mediacontroll...
File webrtc/api/mediacontroller.cc (right):

https://codereview.webrtc.org/1713043002/diff/140001/webrtc/api/mediacontroll...
webrtc/api/mediacontroller.cc:42: void Close() override {
On 2016/03/01 09:57:41, tommi-webrtc wrote:
> nit: there's sometimes an empty line between methods in the class and
sometimes
> not.  I'm not sure what the style is but it files like Close() should be
> separate from the ctor/dtor at least.  Feel free to make the whole thing
> consistent though :)

I've now formatted like you would normally format a class definition - grouped
the interface methods under a comment, ctor+dtor together, and the private
methods together. Let me know whether you find this acceptable. :)

[the sun, 2016-03-01 10:57:47]

On 2016/03/01 10:52:30, the sun wrote:
>
https://codereview.webrtc.org/1713043002/diff/140001/webrtc/api/mediacontroll...
> File webrtc/api/mediacontroller.cc (right):
> 
>
https://codereview.webrtc.org/1713043002/diff/140001/webrtc/api/mediacontroll...
> webrtc/api/mediacontroller.cc:42: void Close() override {
> On 2016/03/01 09:57:41, tommi-webrtc wrote:
> > nit: there's sometimes an empty line between methods in the class and
> sometimes
> > not.  I'm not sure what the style is but it files like Close() should be
> > separate from the ctor/dtor at least.  Feel free to make the whole thing
> > consistent though :)
> 
> I've now formatted like you would normally format a class definition - grouped
> the interface methods under a comment, ctor+dtor together, and the private
> methods together. Let me know whether you find this acceptable. :)

s/definition/declaration

As a note, yes, I'm embarrassed I didn't think of this simple solution initially
- I guess my focus was to lazy-init as much of PC as possible.

Thanks to the reviewers for pushing back! I'm glad we didn't end up with the
initial two abominations on the code base. :)

[tommi, 2016-03-01 13:21:56]

lgtm. thanks!

[kwiberg-webrtc, 2016-03-01 13:35:31]

lgtm!

[the sun, 2016-03-01 16:31:51]

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

[commit-bot: I haz the power, 2016-03-01 16:31:58]

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

[commit-bot: I haz the power, 2016-03-01 18:33:19]

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

[commit-bot: I haz the power, 2016-03-01 18:33:20]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2016-03-01 20:08:45]

lgtm

WOW, that's a lot better.

[the sun, 2016-03-01 20:39:41]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-03-01 20:39:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1713043002/#ps200001 (title:
"rebase")

[commit-bot: I haz the power, 2016-03-01 20:39:54]

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

[commit-bot: I haz the power, 2016-03-01 20:42:07]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-01 20:42:15]

Description was changed from

==========
Late initialize MediaController, for less resource i.e. ProcessThread, usage by
PeerConnection.

BUG=chromium:582441
==========

to

==========
Late initialize MediaController, for less resource i.e. ProcessThread, usage by
PeerConnection.

BUG=chromium:582441

Committed: https://crrev.com/03d6d57f41cd36e9cf6c9d02c2a04c2775a8611c
Cr-Commit-Position: refs/heads/master@{#11834}
==========

[commit-bot: I haz the power, 2016-03-01 20:42:16]

Patchset 11 (id:??) landed as
https://crrev.com/03d6d57f41cd36e9cf6c9d02c2a04c2775a8611c
Cr-Commit-Position: refs/heads/master@{#11834}