Make cc output surface creation async

Make cc output surface creation async

As a part of making Android CompositorImpl use cc's scheduler and not
post its own composite calls manually, cc needs to be able to handle
asynchronous output surface creation.

This change modifies CreateOutputSurface to instead be
RequestNewOutputSurface, which the LayerTreeHostClient must respond to
and call SetOutputSurface on the host with the new output surface once
it is ready.

Because the LayerTreeHostClient must now talk to the host as a part of
output surface creation, a bunch of unit test code needed to be
refactored to handle this.

BUG=none

Committed: https://crrev.com/2097cab4982cc0ad92b878ad88e39d724dd047f7
Cr-Commit-Position: refs/heads/master@{#296780}

[enne (OOO), 2014-06-20 22:42:04]

sievers: Is this something you think might solve what you were worried about
with the early out in CompositorImpl::Composite regarding a missing GPU channel?

[danakj, 2014-06-20 23:19:10]

You could put bug 276411 on this or block that on something to attach to this
CL.

[danakj, 2014-06-20 23:36:34]

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host.cc
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host...
cc/test/fake_layer_tree_host.cc:8:
FakeLayerTreeHost::FakeLayerTreeHost(FakeLayerTreeHostClient* client,
Maybe we should consider having FakeLayerTreeHost own a LayerTreeHost so it can
own the client instead of making us change every unit test. For the things it
overrides it could have a subclass of LTH that it actually owns internally.
WDYT?

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host...
File cc/test/fake_layer_tree_host_client.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host...
cc/test/fake_layer_tree_host_client.cc:24: void
FakeLayerTreeHostClient::RequestNewOutputSurface(bool fallback) {
(Next step is to move this to an impl-side client, and then we can do the thread
hopping as necessary out there instead in the proxy!)

https://codereview.chromium.org/348093004/diff/1/cc/trees/single_thread_proxy.cc
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:117: if (success) {
Do we want the embedder to be able to call back with NULL?

https://codereview.chromium.org/348093004/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:228: if (output_surface) {
Do we want the embedder to be able to call back with NULL?

[enne (OOO), 2014-06-23 16:57:39]

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host.cc
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host...
cc/test/fake_layer_tree_host.cc:8:
FakeLayerTreeHost::FakeLayerTreeHost(FakeLayerTreeHostClient* client,
On 2014/06/20 23:36:34, danakj wrote:
> Maybe we should consider having FakeLayerTreeHost own a LayerTreeHost so it
can
> own the client instead of making us change every unit test. For the things it
> overrides it could have a subclass of LTH that it actually owns internally.
> WDYT?

I went down that path a little, but it was an even bigger change than this one.

https://codereview.chromium.org/348093004/diff/1/cc/trees/single_thread_proxy.cc
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:117: if (success) {
On 2014/06/20 23:36:34, danakj wrote:
> Do we want the embedder to be able to call back with NULL?

I think the ultimate answer is no, but I don't know that I want to change that
in this patch too, since it'll require a little more refactoring.

[no sievers, 2014-06-23 19:53:58]

Thanks, yes this should do the trick.

https://codereview.chromium.org/348093004/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:687: if (output_surface_lost_)
nit: combine if-clauses

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

https://codereview.chromium.org/348093004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:535:
host_->SetOutputSurface(scoped_ptr<cc::OutputSurface>());
either return here or if-else

[enne (OOO), 2014-06-24 22:20:06]

https://codereview.chromium.org/348093004/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:687: if (output_surface_lost_)
On 2014/06/23 19:53:58, sievers wrote:
> nit: combine if-clauses

Done.

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

https://codereview.chromium.org/348093004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:535:
host_->SetOutputSurface(scoped_ptr<cc::OutputSurface>());
On 2014/06/23 19:53:58, sievers wrote:
> either return here or if-else

Oops!  Good catch.  Done.

[danakj, 2014-06-25 17:43:09]

LGTM

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host.cc
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/1/cc/test/fake_layer_tree_host...
cc/test/fake_layer_tree_host.cc:8:
FakeLayerTreeHost::FakeLayerTreeHost(FakeLayerTreeHostClient* client,
On 2014/06/23 16:57:39, enne wrote:
> On 2014/06/20 23:36:34, danakj wrote:
> > Maybe we should consider having FakeLayerTreeHost own a LayerTreeHost so it
> can
> > own the client instead of making us change every unit test. For the things
it
> > overrides it could have a subclass of LTH that it actually owns internally.
> > WDYT?
> 
> I went down that path a little, but it was an even bigger change than this
one.

Darn :( I don't like forcing so many unrelated-to-OS-creation tests to have to
change for this. Maybe we can look into isolating this better in the future.

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:688: }
This is a change of behaviour where if they synchronously set an OS, we'd allow
the composite to go through before. I don't mind the change, but hopefully
nothing breaks! :D

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:112: void
SetOutputSurface(scoped_ptr<OutputSurface>);
needs a variable name

https://codereview.chromium.org/348093004/diff/40001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/proxy.h#newcode55
cc/trees/proxy.h:55: virtual void SetOutputSurface(scoped_ptr<OutputSurface>) =
0;
Can you add comment about how this will cause
LayerTreeHost::OnCreateAndInitializeOutputSurfaceAttempted to be called with the
result.

[enne (OOO), 2014-06-25 21:25:05]

Hmm, this patch seems to create some cc_unittest flakiness.  I'll look into it.

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:688: }
On 2014/06/25 17:43:09, danakj wrote:
> This is a change of behaviour where if they synchronously set an OS, we'd
allow
> the composite to go through before. I don't mind the change, but hopefully
> nothing breaks! :D

I'll change that back and add a comment.  It should be legit to create one
synchronously and be ok here.  sievers asked me to combine the conditionals, but
on second thought I think that's wrong.

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:112: void
SetOutputSurface(scoped_ptr<OutputSurface>);
On 2014/06/25 17:43:09, danakj wrote:
> needs a variable name

Done.

https://codereview.chromium.org/348093004/diff/40001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/proxy.h#newcode55
cc/trees/proxy.h:55: virtual void SetOutputSurface(scoped_ptr<OutputSurface>) =
0;
On 2014/06/25 17:43:09, danakj wrote:
> Can you add comment about how this will cause
> LayerTreeHost::OnCreateAndInitializeOutputSurfaceAttempted to be called with
the
> result.

Done.  After this patch, I want to revist that and maybe just have LTH call it
directly (since this is really indirect).

[danakj, 2014-06-25 21:25:50]

https://codereview.chromium.org/348093004/diff/40001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/348093004/diff/40001/cc/trees/proxy.h#newcode55
cc/trees/proxy.h:55: virtual void SetOutputSurface(scoped_ptr<OutputSurface>) =
0;
On 2014/06/25 21:25:05, enne wrote:
> On 2014/06/25 17:43:09, danakj wrote:
> > Can you add comment about how this will cause
> > LayerTreeHost::OnCreateAndInitializeOutputSurfaceAttempted to be called with
> the
> > result.
> 
> Done.  After this patch, I want to revist that and maybe just have LTH call it
> directly (since this is really indirect).

+1

[enne (OOO), 2014-08-13 17:51:01]

boliu: android_webview/
jamesr: mojo/

[jamesr, 2014-08-13 17:52:52]

lgtm

[boliu, 2014-08-13 17:57:04]

lgtm

[enne (OOO), 2014-09-24 23:27:50]

Finally rebased this patch from the past.  It can land now that
SingleThreadProxy+SchedulerClient has landed and stuck.  This is the same as the
previous patch, but with a few more tests updated to use the
FakeLayerTreeHostClient.

[danakj, 2014-09-25 16:50:21]

LGTM

[enne (OOO), 2014-09-25 17:15:49]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-09-25 17:17:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/348093004/220001

[commit-bot: I haz the power, 2014-09-25 17:27:15]

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

[commit-bot: I haz the power, 2014-09-25 17:27:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[enne (OOO), 2014-09-25 17:37:17]

enne@chromium.org changed reviewers:
+ jcivelli@chromium.org

[enne (OOO), 2014-09-25 17:38:02]

OWNERS approval needed:

sievers: content/browser/renderer_host/
jcivelli: content/test/

[no sievers, 2014-09-25 17:59:29]

android lgtm!

https://codereview.chromium.org/348093004/diff/220001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/348093004/diff/220001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:301: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
Can 301-308 actually be removed now that LTH::Composite() will already bail-out
if RequestOutputSurface() does not result in the immediate and synchronous
creation of an OutputSurface?

[Jay Civelli, 2014-09-25 18:20:19]

LGTM for content/tests

[enne (OOO), 2014-09-25 19:05:05]

https://codereview.chromium.org/348093004/diff/220001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/348093004/diff/220001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:301: if
(!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
On 2014/09/25 17:59:29, sievers wrote:
> Can 301-308 actually be removed now that LTH::Composite() will already
bail-out
> if RequestOutputSurface() does not result in the immediate and synchronous
> creation of an OutputSurface?

If you don't mind me punting, I was going to remove it entirely here:
https://codereview.chromium.org/344743002/

[enne (OOO), 2014-09-25 19:06:14]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-09-25 19:08:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/348093004/220001

[no sievers, 2014-09-25 19:21:33]

On 2014/09/25 19:05:05, enne wrote:
>
https://codereview.chromium.org/348093004/diff/220001/content/browser/rendere...
> File content/browser/renderer_host/compositor_impl_android.cc (right):
> 
>
https://codereview.chromium.org/348093004/diff/220001/content/browser/rendere...
> content/browser/renderer_host/compositor_impl_android.cc:301: if
> (!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {
> On 2014/09/25 17:59:29, sievers wrote:
> > Can 301-308 actually be removed now that LTH::Composite() will already
> bail-out
> > if RequestOutputSurface() does not result in the immediate and synchronous
> > creation of an OutputSurface?
> 
> If you don't mind me punting, I was going to remove it entirely here:
> https://codereview.chromium.org/344743002/

Sounds good!

[commit-bot: I haz the power, 2014-09-25 20:16:41]

Committed patchset #12 (id:220001) as 66afc525101bf90b64604f90197edd896960d498

[commit-bot: I haz the power, 2014-09-25 20:17:30]

Patchset 12 (id:??) landed as
https://crrev.com/2097cab4982cc0ad92b878ad88e39d724dd047f7
Cr-Commit-Position: refs/heads/master@{#296780}

[kareng, 2014-09-26 20:22:15]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/606113003/ by kareng@google.com.

The reason for reverting is: possibly causing bug=418206 crash..