Create the desktop environment before any of the channel were connected.

Create the desktop environment before any of the channel were connected.

ClientSession now creates the desktop environment and the corresponding video scheduler, event executor, etc. before any of the channel were created but after the connection has been authenticated and allowed by the host. This arrangement has two benefits:
  - Messages arriving before all channels are ready could be successfully processed instead of being ignored.
  - Desktop session can be created in parallel with channel creation reducing the overall connection time. 

This CL also splits initialization of the audio and video schedulers in two phases. The schedulers created and prepared to handle incoming messages once the connection has been authenticated. Then, ClientSession starts them (so they can start sending messages to the client) once all channels have been set up.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197689

[alexeypa (please no reviews), 2013-04-26 22:39:41]

PTAL.

[alexeypa (please no reviews), 2013-04-29 23:02:51]

Ping.

[Wez, 2013-04-30 13:33:18]

On 2013/04/29 23:02:51, alexeypa wrote:
> Ping.

/me drives by.

We specifically want to avoid creating back-end components for channels that
aren't going to be used, so we can support configurations such as no-audio or
no-video efficiently in future; his does this CL affect that?

[Wez, 2013-04-30 13:48:39]

Wouldn't it be better to split out channel-connected notifications to be
per-channel, and initialize the relevant components as each channel becomes
connected?  That would break down the stub connection logic per-channel, and
avoid creation/startup needing to be split.

OTOH, if we're worried that stubs might take a long time to start up in some
cases (e.g. if spinning up the multi-process host's agent process can be
time-consuming) then the creation/startup split makes more sense, and we should
be explicit about that being a goal.

https://chromiumcodereview.appspot.com/14520018/diff/1/remoting/host/client_s...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/14520018/diff/1/remoting/host/client_s...
remoting/host/client_session.cc:217: input_injector_ =
desktop_environment_->CreateInputInjector();
nit: You don't really need to do this at this point, since the input pipeline
specifically ignores input until SetDisableInputs(false) is called.

[Sergey Ulanov, 2013-04-30 17:19:26]

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session....
remoting/host/client_session.cc:256: DCHECK(video_scheduler_);
nit: Many of these dchecks are not necessary - scoped_ptr<> dchecks in
operator-> anyway.

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session....
remoting/host/client_session.cc:270: desktop_environment_->SetCapabilities(
Do you need this here? ClientSession::SetCapabilities() always calls
desktop_environment_->SetCapabilities(), so if client_capabilities_ is set, it
has already been called.

[Sergey Ulanov, 2013-04-30 17:42:32]

On 2013/04/30 13:48:39, Wez wrote:
> Wouldn't it be better to split out channel-connected notifications to be
> per-channel, and initialize the relevant components as each channel becomes
> connected?  That would break down the stub connection logic per-channel, and
> avoid creation/startup needing to be split.

This would make the code more complicated without giving much in return. Also,
there can be some components that depend on multiple channels (e.g. control
channel is used to control video stream). 

I think that ClientSession class should be agnostic to the details of the
protocol, so it shouldn't need to know that some channels may be connected
before others and the fact that protocol may use multiple channels exist
shouldn't matter at that layer. Better solution would be to push this logic down
to the protocol layer, i.e. to ConnectionToClient: it shouldn't start reading
from any channels until all of them are connected. WDYT? Alex, I can make this
fix, if you agree that it's better.


> 
> OTOH, if we're worried that stubs might take a long time to start up in some
> cases (e.g. if spinning up the multi-process host's agent process can be
> time-consuming) then the creation/startup split makes more sense, and we
should
> be explicit about that being a goal.
> 
>
https://chromiumcodereview.appspot.com/14520018/diff/1/remoting/host/client_s...
> File remoting/host/client_session.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14520018/diff/1/remoting/host/client_s...
> remoting/host/client_session.cc:217: input_injector_ =
> desktop_environment_->CreateInputInjector();
> nit: You don't really need to do this at this point, since the input pipeline
> specifically ignores input until SetDisableInputs(false) is called.

[alexeypa (please no reviews), 2013-04-30 17:55:32]

> > OTOH, if we're worried that stubs might take a long time to start up in some
> > cases (e.g. if spinning up the multi-process host's agent process can be
> > time-consuming) then the creation/startup split makes more sense, and we
> > should be explicit about that being a goal.

I actually didn't think about it but that is a good point. I agree it is better
to start creating resources for the connection early (in case it takes time).
Once a connection is authenticated and allowed by the host we can be almost 100%
that the connection is going to be completely set up and the host will start
pumping video to back to the client. I'll add this to the CL description.

> Better solution would be to push this logic down
> to the protocol layer, i.e. to ConnectionToClient: it shouldn't start reading
> from any channels until all of them are connected. WDYT? Alex, I can make this
> fix, if you agree that it's better.

It simplifies ClientSession's code but it makes the connection time a little
longer. I think we should not do that.

[alexeypa (please no reviews), 2013-04-30 18:16:20]

PTAL.

PS. I had to upload the same patchset twice for some reason. Look at the #3 for
the diff.

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session....
remoting/host/client_session.cc:217: input_injector_ =
desktop_environment_->CreateInputInjector();
On 2013/04/30 13:48:39, Wez wrote:
> nit: You don't really need to do this at this point, since the input pipeline
> specifically ignores input until SetDisableInputs(false) is called.

Yes, but it allows the host to create the event executor in parallel with
channel creation.

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session....
remoting/host/client_session.cc:256: DCHECK(video_scheduler_);
On 2013/04/30 17:19:26, Sergey Ulanov wrote:
> nit: Many of these dchecks are not necessary - scoped_ptr<> dchecks in
> operator-> anyway.

Done.

https://codereview.chromium.org/14520018/diff/1/remoting/host/client_session....
remoting/host/client_session.cc:270: desktop_environment_->SetCapabilities(
On 2013/04/30 17:19:26, Sergey Ulanov wrote:
> Do you need this here? ClientSession::SetCapabilities() always calls
> desktop_environment_->SetCapabilities(), so if client_capabilities_ is set, it
> has already been called.

Done.

[Sergey Ulanov, 2013-04-30 18:53:17]

On 2013/04/30 17:55:32, alexeypa wrote:
> > Better solution would be to push this logic down
> > to the protocol layer, i.e. to ConnectionToClient: it shouldn't start
reading
> > from any channels until all of them are connected. WDYT? Alex, I can make
this
> > fix, if you agree that it's better.
> 
> It simplifies ClientSession's code but it makes the connection time a little
> longer. I think we should not do that.

I don't see any way it could impact performance. We don't start video capturer
and don't send capabilities until all channels are connected, so even if we
create reader and writer the channels are not used before all of them are
connected. Even if did utilize channels before they are connected, normally both
non-multiplex channels are connected simultaneously, so the performance impact
would not be measurable.
Also note that if we don't read data from a buffer, doesn't mean the sender
cannot send it. It will be buffered in the receive buffer until it's read.

[alexeypa (please no reviews), 2013-04-30 19:07:07]

On 2013/04/30 18:53:17, Sergey Ulanov wrote:
> I don't see any way it could impact performance. We don't start video capturer
> and don't send capabilities until all channels are connected, so even if we
> create reader and writer the channels are not used before all of them are
> connected. Even if did utilize channels before they are connected, normally
both
> non-multiplex channels are connected simultaneously, so the performance impact
> would not be measurable.
> Also note that if we don't read data from a buffer, doesn't mean the sender
> cannot send it. It will be buffered in the receive buffer until it's read.

If capabilities are received early enough, we could initiate session creation (a
Windows session for instance) earlier than all channels are connected. Session
creation is a heavy-weight operation. The sooner we can start it the better.

I don't think the channel creation takes much time now but it still can be
useful to do explicit early allocation of resources. One reason is that it makes
us think front-loading heavy-weight work as much as we can. Second is that if
channel creation depends on receiving something from the network  it can take
noticeable time.

WDYT?

[Sergey Ulanov, 2013-04-30 23:59:20]

On 2013/04/30 19:07:07, alexeypa wrote:
> On 2013/04/30 18:53:17, Sergey Ulanov wrote:
> > I don't see any way it could impact performance. We don't start video
capturer
> > and don't send capabilities until all channels are connected, so even if we
> > create reader and writer the channels are not used before all of them are
> > connected. Even if did utilize channels before they are connected, normally
> both
> > non-multiplex channels are connected simultaneously, so the performance
impact
> > would not be measurable.
> > Also note that if we don't read data from a buffer, doesn't mean the sender
> > cannot send it. It will be buffered in the receive buffer until it's read.
> 
> If capabilities are received early enough, we could initiate session creation
(a
> Windows session for instance) earlier than all channels are connected. Session
> creation is a heavy-weight operation. The sooner we can start it the better.

Optimizing for the case when by some chance mux (and hence control) channel is
connected before the video channels doesn't make sense because normally all
channels connected at the same time, so you are not winning anything. As we
discussed before, if you worried that we should initialize desktop environment
asap, then we should try to get capabilities over signaling channel during
session negotiation when it is possible (and use control channel for the case
when capabilities change later).
Anyway, this change LGTM, as it still makes sense to create desktop environment
before channels are connected.

> 
> I don't think the channel creation takes much time now but it still can be
> useful to do explicit early allocation of resources. One reason is that it
makes
> us think front-loading heavy-weight work as much as we can. Second is that if
> channel creation depends on receiving something from the network  it can take
> noticeable time.
> 
> WDYT?

[commit-bot: I haz the power, 2013-05-01 15:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/14520018/7025

[commit-bot: I haz the power, 2013-05-01 16:25:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-01 16:32:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/14520018/7025

[commit-bot: I haz the power, 2013-05-01 19:30:26]

Change committed as 197689