Android: Avoid blocking UI thread for GPU channel creation

content/browser/renderer_host/compositor_impl_android.cc

Index: content/browser/renderer_host/compositor_impl_android.cc
diff --git a/content/browser/renderer_host/compositor_impl_android.cc b/content/browser/renderer_host/compositor_impl_android.cc
index 2a168a89c55a45ce1cd0d9bd8ee1b7a8f0df146d..396fa2de4ca5bbc7360e80723e2de4d80f49ab86 100644
--- a/content/browser/renderer_host/compositor_impl_android.cc
+++ b/content/browser/renderer_host/compositor_impl_android.cc
@@ -238,7 +238,8 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       window_(NULL),
       surface_id_(0),
       client_(client),
-      root_window_(root_window) {
+      root_window_(root_window),
+      weak_factory_(this) {
   DCHECK(client);
   DCHECK(root_window);
   ImageTransportFactoryAndroid::AddObserver(this);
@@ -253,10 +254,26 @@ CompositorImpl::~CompositorImpl() {
 }
 
 void CompositorImpl::Composite() {
+  BrowserGpuChannelHostFactory* factory =
+      BrowserGpuChannelHostFactory::instance();
+  if (!factory->GetGpuChannel() || factory->GetGpuChannel()->IsLost()) {

[danakj, 2014/04/14 13:14:46]

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?

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

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).
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.

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

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

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

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]

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.
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.

+    CauseForGpuLaunch cause =
+        CAUSE_FOR_GPU_LAUNCH_WEBGRAPHICSCONTEXT3DCOMMANDBUFFERIMPL_INITIALIZE;
+    factory->EstablishGpuChannel(
+        cause,
+        base::Bind(&CompositorImpl::OnGpuChannelEstablished,
+                   weak_factory_.GetWeakPtr()));
+    return;
+  }
+
   if (host_)
     host_->Composite(gfx::FrameTime::Now());
 }
 
+void CompositorImpl::OnGpuChannelEstablished() {
+  ScheduleComposite();
+}
+
 void CompositorImpl::SetRootLayer(scoped_refptr<cc::Layer> root_layer) {
   root_layer_->RemoveAllChildren();
   root_layer_->AddChild(root_layer);
@@ -429,16 +446,10 @@ void CompositorImpl::DeleteUIResource(cc::UIResourceId resource_id) {
 
 static scoped_ptr<WebGraphicsContext3DCommandBufferImpl>
 CreateGpuProcessViewContext(
+    const scoped_refptr<GpuChannelHost>& gpu_channel_host,
     const blink::WebGraphicsContext3D::Attributes attributes,
     int surface_id) {
-  BrowserGpuChannelHostFactory* factory =
-      BrowserGpuChannelHostFactory::instance();
-  CauseForGpuLaunch cause =
-      CAUSE_FOR_GPU_LAUNCH_WEBGRAPHICSCONTEXT3DCOMMANDBUFFERIMPL_INITIALIZE;
-  scoped_refptr<GpuChannelHost> gpu_channel_host(
-      factory->EstablishGpuChannelSync(cause));
-  if (!gpu_channel_host)
-    return scoped_ptr<WebGraphicsContext3DCommandBufferImpl>();
+  DCHECK(gpu_channel_host);
 
   GURL url("chrome://gpu/Compositor::createContext3D");
   static const size_t kBytesPerPixel = 4;
@@ -474,9 +485,15 @@ scoped_ptr<cc::OutputSurface> CompositorImpl::CreateOutputSurface(
   DCHECK(window_);
   DCHECK(surface_id_);
 
-  scoped_refptr<ContextProviderCommandBuffer> context_provider =
-      ContextProviderCommandBuffer::Create(
-          CreateGpuProcessViewContext(attrs, surface_id_), "BrowserCompositor");
+  scoped_refptr<ContextProviderCommandBuffer> context_provider;
+  BrowserGpuChannelHostFactory* factory =
+      BrowserGpuChannelHostFactory::instance();
+  scoped_refptr<GpuChannelHost> gpu_channel_host = factory->GetGpuChannel();
+  if (gpu_channel_host && !gpu_channel_host->IsLost()) {

[danakj, 2014/04/14 13:14:46]

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]

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]

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.

+    context_provider = ContextProviderCommandBuffer::Create(
+        CreateGpuProcessViewContext(gpu_channel_host, attrs, surface_id_),
+        "BrowserCompositor");
+  }
   if (!context_provider.get()) {
     LOG(ERROR) << "Failed to create 3D context for compositor.";
     return scoped_ptr<cc::OutputSurface>();