Android: Avoid blocking UI thread for GPU channel creation

Android: Avoid blocking UI thread for GPU channel creation

Use EstablishGpuChannel() with callback instead of EstablishGpuChannelSync() 
and skip and defer browser Composites until the channel is ready.

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

[no sievers, 2014-04-11 22:53:56]

ptal

[danakj, 2014-04-14 13:14:45]

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:259: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
Is the assumption that SingleThread compositor won't try to CreateOutputSurface
until Composite() happens?

Could you try this CL with https://codereview.chromium.org/134623005/ and see
what happens with this change?

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:492: if
(gpu_channel_host && !gpu_channel_host->IsLost()) {
Say we were to remove the compositor retry-CreateOutputSurface logic. How would
you do this without relying on those retries?

[no sievers, 2014-04-14 18:50:45]

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:259: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
On 2014/04/14 13:14:46, danakj wrote:
> Is the assumption that SingleThread compositor won't try to
CreateOutputSurface
> until Composite() happens?
> 


Yes, I changed it in r241897 to guarantee that behavior.
On Android and Windows w/desktop GL we cannot synchronously create a GPU channel
to a GPU child process.
Although it would be nicer if the OutputSurface creation was even more
explicitly in the client's control (for example like described in your mode
switching doc).


> Could you try this CL with https://codereview.chromium.org/134623005/ and see
> what happens with this change?

I'll give it a try. Does it respect SetLayerTreeHostClientReady()?
Also see LayerTreeHostClientNotReadyDoesNotCreateOutputSurface, which is however
only run as MULTI_THREAD_TEST_F(). If possible, we should run it singlethreaded
w/scheduler.

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:492: if
(gpu_channel_host && !gpu_channel_host->IsLost()) {
On 2014/04/14 13:14:46, danakj wrote:
> Say we were to remove the compositor retry-CreateOutputSurface logic. How
would
> you do this without relying on those retries?

Does it really change anything though from how it worked in that regard before?
It seems to call SetNeedsCommit() if the attempt failed and will try again on
next composite.

These are the basic cases I can think of:

1) The channel gets lost (let's say the GPU process crashed). But we saw it
already in Composite() and don't get in here until it's reestablished. It works
as before except for that we inhibit the Composite() until the channel is ready.

2) If the context could not get created but the GPU channel was never lost (line
493 returned NULL), then this should work exactly as before.

3) The channel was intact in CompositorImpl::Composite() but is now lost in
here. Since we come into here from the Composite() call stack, the window for
this to happen is pretty small, but it could happen. Wouldn't this still work as
before in that the compositor would SetNeedsCommit() but we would simply defer
commit/composite until the channel is ready?

[danakj, 2014-04-14 19:30:01]

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:259: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
On 2014/04/14 18:50:45, sievers wrote:
> On 2014/04/14 13:14:46, danakj wrote:
> > Is the assumption that SingleThread compositor won't try to
> CreateOutputSurface
> > until Composite() happens?
> > 
> 
> 
> Yes, I changed it in r241897 to guarantee that behavior.
> On Android and Windows w/desktop GL we cannot synchronously create a GPU
channel
> to a GPU child process.
> Although it would be nicer if the OutputSurface creation was even more
> explicitly in the client's control (for example like described in your mode
> switching doc).
> 
> 
> > Could you try this CL with https://codereview.chromium.org/134623005/ and
see
> > what happens with this change?
> 
> I'll give it a try. Does it respect SetLayerTreeHostClientReady()?
> Also see LayerTreeHostClientNotReadyDoesNotCreateOutputSurface, which is
however
> only run as MULTI_THREAD_TEST_F(). If possible, we should run it
singlethreaded
> w/scheduler.

With STP Scheduler, this method goes away entirely. Where do you see this going
then? In ScheduleComposite?

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:492: if
(gpu_channel_host && !gpu_channel_host->IsLost()) {
On 2014/04/14 18:50:45, sievers wrote:
> On 2014/04/14 13:14:46, danakj wrote:
> > Say we were to remove the compositor retry-CreateOutputSurface logic. How
> would
> > you do this without relying on those retries?
> 
> Does it really change anything though from how it worked in that regard
before?
> It seems to call SetNeedsCommit() if the attempt failed and will try again on
> next composite.
> 
> These are the basic cases I can think of:
> 
> 1) The channel gets lost (let's say the GPU process crashed). But we saw it
> already in Composite() and don't get in here until it's reestablished. It
works
> as before except for that we inhibit the Composite() until the channel is
ready.
> 
> 2) If the context could not get created but the GPU channel was never lost
(line
> 493 returned NULL), then this should work exactly as before.
> 
> 3) The channel was intact in CompositorImpl::Composite() but is now lost in
> here. Since we come into here from the Composite() call stack, the window for
> this to happen is pretty small, but it could happen. Wouldn't this still work
as
> before in that the compositor would SetNeedsCommit() but we would simply defer
> commit/composite until the channel is ready?

Ok, I think I want to make it so this method can never return NULL, but that
depends on some future work. This seems like an improvement over what we have
now, given we can already return NULL here.

[no sievers, 2014-04-14 19:40:34]

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:259: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
On 2014/04/14 19:30:01, danakj wrote:
> On 2014/04/14 18:50:45, sievers wrote:
> > On 2014/04/14 13:14:46, danakj wrote:
> > > Is the assumption that SingleThread compositor won't try to
> > CreateOutputSurface
> > > until Composite() happens?
> > > 
> > 
> > 
> > Yes, I changed it in r241897 to guarantee that behavior.
> > On Android and Windows w/desktop GL we cannot synchronously create a GPU
> channel
> > to a GPU child process.
> > Although it would be nicer if the OutputSurface creation was even more
> > explicitly in the client's control (for example like described in your mode
> > switching doc).
> > 
> > 
> > > Could you try this CL with https://codereview.chromium.org/134623005/ and
> see
> > > what happens with this change?
> > 
> > I'll give it a try. Does it respect SetLayerTreeHostClientReady()?
> > Also see LayerTreeHostClientNotReadyDoesNotCreateOutputSurface, which is
> however
> > only run as MULTI_THREAD_TEST_F(). If possible, we should run it
> singlethreaded
> > w/scheduler.
> 
> With STP Scheduler, this method goes away entirely. Where do you see this
going
> then? In ScheduleComposite?

Hmm, would I have to SetDeferCommits() also to make that work?
Also, wouldn't ScheduleComposite() go away soon too then? Seems obsolete if you
have a scheduler.

[danakj, 2014-04-14 20:29:35]

LGTM if you wanna land this for now, though I guess it gets kinda obsoleted and
has to be adjusted some when we land the scheduler for STP.

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/228663008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:259: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
On 2014/04/14 19:40:34, sievers wrote:
> On 2014/04/14 19:30:01, danakj wrote:
> > On 2014/04/14 18:50:45, sievers wrote:
> > > On 2014/04/14 13:14:46, danakj wrote:
> > > > Is the assumption that SingleThread compositor won't try to
> > > CreateOutputSurface
> > > > until Composite() happens?
> > > > 
> > > 
> > > 
> > > Yes, I changed it in r241897 to guarantee that behavior.
> > > On Android and Windows w/desktop GL we cannot synchronously create a GPU
> > channel
> > > to a GPU child process.
> > > Although it would be nicer if the OutputSurface creation was even more
> > > explicitly in the client's control (for example like described in your
mode
> > > switching doc).
> > > 
> > > 
> > > > Could you try this CL with https://codereview.chromium.org/134623005/
and
> > see
> > > > what happens with this change?
> > > 
> > > I'll give it a try. Does it respect SetLayerTreeHostClientReady()?
> > > Also see LayerTreeHostClientNotReadyDoesNotCreateOutputSurface, which is
> > however
> > > only run as MULTI_THREAD_TEST_F(). If possible, we should run it
> > singlethreaded
> > > w/scheduler.
> > 
> > With STP Scheduler, this method goes away entirely. Where do you see this
> going
> > then? In ScheduleComposite?
> 
> Hmm, would I have to SetDeferCommits() also to make that work?

Ya, I think you would to prevent races where the GPU channel doesn't come up
before the compositor gives up.

We need some kinda of blocking in the pipeline somewhere, right? Doing this
non-sync moves that blocking off into cc I guess, which is a bit of the opposite
direction I'd like it to go over time, but would also consolidate it there
instead of doing it in two places, so plus/minus.

But doing SetDeferredCommits(true) in ScheduleComposite point is kinda funny.
Maybe CreateOutputSurface wants to do that and just return false? That'd abort
the commit and prevent future commits which would prevent future attempts to
CreateOutputSurface until SetDeferredCommits(false) happened.

> Also, wouldn't ScheduleComposite() go away soon too then? Seems obsolete if
you
> have a scheduler.

The ScheduleComposite() method could call LTH::SetNeedsUpdate() or
LTH::SetNeedsCommit() similar to the ScheduleDraw() method in ui::Compositor.

But that is probably needed only because they defer damage until during the
Layout(). Since you'd be deferring things until during Layout or whatnot also,
maybe the same thing applies.

[no sievers, 2014-05-30 22:31:21]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-05-30 22:32:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sievers@chromium.org/228663008/70001

[commit-bot: I haz the power, 2014-05-31 03:49:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-31 04:58:49]

Change committed as 274032