content: Move all GpuMemoryBuffer allocation to IO thread.

content/browser/gpu/browser_gpu_channel_host_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/gpu/browser_gpu_channel_host_factory.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_restrictions.h"
@@ skipping 18 matching lines @@
   CreateRequest()
       : event(true, false), gpu_host_id(0), route_id(MSG_ROUTING_NONE),
         result(CREATE_COMMAND_BUFFER_FAILED) {}
   ~CreateRequest() {}
   base::WaitableEvent event;
   int gpu_host_id;
   int32 route_id;
   CreateCommandBufferResult result;
 };
 
+struct BrowserGpuChannelHostFactory::AllocateGpuMemoryBufferRequest {
+  AllocateGpuMemoryBufferRequest()
+      : event(true, false), width(0), height(0), internalformat(0), usage(0) {}

[piman, 2014/09/05 22:30:37]

nit: might as well providing instead a constructor that takes
widht/height/internalformat/usage since there's only one caller that will fill
all those.

[reveman, 2014/09/05 23:58:43]

Did it this way to be consistent with CreateRequest above. But I agree that we
might as well provide ctors. Change both this and CreateRequest  in latest patch
to keep things consistent.

+  ~AllocateGpuMemoryBufferRequest() {}
+  base::WaitableEvent event;
+  size_t width;
+  size_t height;
+  unsigned internalformat;
+  unsigned usage;
+  scoped_ptr<gfx::GpuMemoryBuffer> result;
+};
+
 class BrowserGpuChannelHostFactory::EstablishRequest
     : public base::RefCountedThreadSafe<EstablishRequest> {
  public:
   static scoped_refptr<EstablishRequest> Create(CauseForGpuLaunch cause,
                                                 int gpu_client_id,
                                                 int gpu_host_id);
   void Wait();
   void Cancel();
 
   int gpu_host_id() { return gpu_host_id_; }
@@ skipping 307 matching lines @@
     established_callbacks_[n].Run();
 
   established_callbacks_.clear();
 }
 
 scoped_ptr<gfx::GpuMemoryBuffer>
 BrowserGpuChannelHostFactory::AllocateGpuMemoryBuffer(size_t width,
                                                       size_t height,
                                                       unsigned internalformat,
                                                       unsigned usage) {
-  if (!GpuMemoryBufferImpl::IsFormatValid(internalformat) ||
-      !GpuMemoryBufferImpl::IsUsageValid(usage))
-    return scoped_ptr<gfx::GpuMemoryBuffer>();
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[piman, 2014/09/05 22:30:37]

Couldn't this be called on the compositor thread?

Maybe DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)) ? Since that's the
case that would cause a deadlock.

[reveman, 2014/09/05 23:58:43]

I didn't think of that. Changed DCHECKs as suggested in latest patch.

 
-  return GpuMemoryBufferImpl::Create(gfx::Size(width, height),
-                                     internalformat,
-                                     usage).PassAs<gfx::GpuMemoryBuffer>();
+  AllocateGpuMemoryBufferRequest request;
+  request.width = width;
+  request.height = height;
+  request.internalformat = internalformat;
+  request.usage = usage;
+
+  GetIOLoopProxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&BrowserGpuChannelHostFactory::AllocateGpuMemoryBufferOnIO,
+                 base::Unretained(&request)));
+
+  // We're blocking the UI thread, which is generally undesirable.
+  TRACE_EVENT0("browser",
+               "BrowserGpuChannelHostFactory::AllocateGpuMemoryBuffer");
+  base::ThreadRestrictions::ScopedAllowWait allow_wait;
+  request.event.Wait();
+  return request.result.Pass();
+}
+
+void BrowserGpuChannelHostFactory::DeleteGpuMemoryBuffer(
+    scoped_ptr<gfx::GpuMemoryBuffer> buffer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  GetIOLoopProxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&BrowserGpuChannelHostFactory::DeleteGpuMemoryBufferOnIO,
+                 base::Passed(&buffer)));
+}
+
+void BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer(
+    const gfx::GpuMemoryBufferHandle& handle,
+    const gfx::Size& size,
+    unsigned internalformat,
+    unsigned usage,
+    const CreateGpuMemoryBufferCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  GpuProcessHost* host = GpuProcessHost::FromID(gpu_host_id_);
+  if (!host) {
+    callback.Run(gfx::GpuMemoryBufferHandle());
+    return;
+  }
+
+  uint32 request_id = next_create_gpu_memory_buffer_request_id_++;
+  create_gpu_memory_buffer_requests_[request_id] = callback;
+
+  host->CreateGpuMemoryBuffer(
+      handle,
+      size,
+      internalformat,
+      usage,
+      base::Bind(&BrowserGpuChannelHostFactory::OnGpuMemoryBufferCreated,
+                 base::Unretained(this),
+                 request_id));
+}
+
+void BrowserGpuChannelHostFactory::DestroyGpuMemoryBuffer(
+    const gfx::GpuMemoryBufferHandle& handle,
+    int32 sync_point) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  GpuProcessHost* host = GpuProcessHost::FromID(gpu_host_id_);
+  if (!host)
+    return;
+
+  host->DestroyGpuMemoryBuffer(handle, sync_point);
 }
 
 // static
 void BrowserGpuChannelHostFactory::AddFilterOnIO(
     int host_id,
     scoped_refptr<IPC::MessageFilter> filter) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   GpuProcessHost* host = GpuProcessHost::FromID(host_id);
   if (host)
@@ skipping 16 matching lines @@
                                        target_task_runner);
   filter->AddRoute(MSG_ROUTING_CONTROL, handler);
 
   GetIOLoopProxy()->PostTask(
       FROM_HERE,
       base::Bind(&BrowserGpuChannelHostFactory::AddFilterOnIO,
                  gpu_host_id_,
                  filter));
 }
 
-void BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer(
-    const gfx::GpuMemoryBufferHandle& handle,
-    const gfx::Size& size,
-    unsigned internalformat,
-    unsigned usage,
-    const CreateGpuMemoryBufferCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  uint32 request_id = next_create_gpu_memory_buffer_request_id_++;
-  create_gpu_memory_buffer_requests_[request_id] = callback;
-  GetIOLoopProxy()->PostTask(
-      FROM_HERE,
-      base::Bind(&BrowserGpuChannelHostFactory::CreateGpuMemoryBufferOnIO,
-                 base::Unretained(this),
-                 handle,
-                 size,
-                 internalformat,
-                 usage,
-                 request_id));
-}
-
-void BrowserGpuChannelHostFactory::DestroyGpuMemoryBuffer(
-    const gfx::GpuMemoryBufferHandle& handle,
-    int32 sync_point) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  GetIOLoopProxy()->PostTask(
-      FROM_HERE,
-      base::Bind(&BrowserGpuChannelHostFactory::DestroyGpuMemoryBufferOnIO,
-                 base::Unretained(this),
-                 handle,
-                 sync_point));
-}
-
-void BrowserGpuChannelHostFactory::CreateGpuMemoryBufferOnIO(
-    const gfx::GpuMemoryBufferHandle& handle,
-    const gfx::Size& size,
-    unsigned internalformat,
-    unsigned usage,
-    uint32 request_id) {
-  GpuProcessHost* host = GpuProcessHost::FromID(gpu_host_id_);
-  if (!host) {
-    GpuMemoryBufferCreatedOnIO(request_id, gfx::GpuMemoryBufferHandle());
+// static
+void BrowserGpuChannelHostFactory::AllocateGpuMemoryBufferOnIO(
+    AllocateGpuMemoryBufferRequest* request) {
+  if (!GpuMemoryBufferImpl::IsFormatValid(request->internalformat) ||
+      !GpuMemoryBufferImpl::IsUsageValid(request->usage)) {
+    request->result = scoped_ptr<gfx::GpuMemoryBuffer>();
+    request->event.Signal();

[piman, 2014/09/05 22:30:37]

To confirm, if the GPU process dies while a request is pending, we'll go through
this path, correct?

[reveman, 2014/09/05 23:58:43]

No, unless I misunderstood your question. This code is not really aware of the
usage of the GPU process for allocation of some buffer types. If the GPU process
is dead or dies before it can return a valid handle, we'll still call the
platform dependent GpuMemoryBufferImpl::Create function below. If that decides
to allocate the buffer GPU process side it will call
BrowserGpuChannelHostFactory::CreateGpuMemoryBuffer above which is part of the
GpuMemoryBufferFactoryHost interface. That function calls into GpuProcessHost
which is responsible for returning a NULL handle if the GPU process dies before
processing the IPC.

However, I'm glad you asked as it looks like
GpuProcessHost::SendOutstandingReplies is missing some code for
CreateGpuMemoryBuffer requests. I'll upload another patch to fix this right
away.

[piman, 2014/09/06 00:28:23]

Then I'm confused... Shouldn't we signal the event only after we received the
reply from the GPU process?

[reveman, 2014/09/06 20:30:49]

If the format or usage is an invalid value that can't be supported by any
implementation then there's no point in moving forward. We might as well reject
the request early. This could be moved up to AllocateGpuMemoryBuffer instead but
it seemed a bit more consistent to do all these GpuMemoryBufferImpl calls on the
IO thread. Now that I think of it, it might be best to leave it up to each
implementation as we need to check these values there anyhow.

[piman, 2014/09/08 18:33:57]

ACK about early reject, but what about the general case? Should the event be
signaled in OnGpuMemoryBufferCreated?

[reveman, 2014/09/08 19:58:57]

So this patch doesn't actually make it possible to use gpu process side
allocated buffers by the browser compositor. This is still limited to
synchronous allocation using GpuMemoryBufferImpl::Create. This patch simply
moves all allocation to the IO thread so that adding support for gpu process
side allocated buffers can be done cleanly. I'll leave it up to Alex to do that
as part of https://codereview.chromium.org/540443002/.
GpuMemoryBufferImpl::Create basically need to take a callback instead of
returning a buffer and the event will then be signaled when that call back is
called.

I think the fact that this class implements both GpuChannelHostFactory and
GpuMemoryBufferFactoryHost makes things a bit confusing. Not sure what to do
about that..

     return;
   }
 
-  host->CreateGpuMemoryBuffer(
-      handle,
-      size,
-      internalformat,
-      usage,
-      base::Bind(&BrowserGpuChannelHostFactory::GpuMemoryBufferCreatedOnIO,
-                 base::Unretained(this),
-                 request_id));
+  request->result = GpuMemoryBufferImpl::Create(
+                        gfx::Size(request->width, request->height),

[piman, 2014/09/05 22:30:37]

nit: indent looks weird

[reveman, 2014/09/05 23:58:43]

I agree but this seems to be the way cl format likes it..

+                        request->internalformat,
+                        request->usage).PassAs<gfx::GpuMemoryBuffer>();
+  request->event.Signal();
 }
 
-void BrowserGpuChannelHostFactory::GpuMemoryBufferCreatedOnIO(
-    uint32 request_id,
-    const gfx::GpuMemoryBufferHandle& handle) {
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&BrowserGpuChannelHostFactory::OnGpuMemoryBufferCreated,
-                 base::Unretained(this),
-                 request_id,
-                 handle));
+// static
+void BrowserGpuChannelHostFactory::DeleteGpuMemoryBufferOnIO(
+    scoped_ptr<gfx::GpuMemoryBuffer> buffer) {
 }
 
 void BrowserGpuChannelHostFactory::OnGpuMemoryBufferCreated(
     uint32 request_id,
     const gfx::GpuMemoryBufferHandle& handle) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
   CreateGpuMemoryBufferCallbackMap::iterator iter =
       create_gpu_memory_buffer_requests_.find(request_id);
   DCHECK(iter != create_gpu_memory_buffer_requests_.end());
   iter->second.Run(handle);
   create_gpu_memory_buffer_requests_.erase(iter);
 }
 
-void BrowserGpuChannelHostFactory::DestroyGpuMemoryBufferOnIO(
-    const gfx::GpuMemoryBufferHandle& handle,
-    int32 sync_point) {
-  GpuProcessHost* host = GpuProcessHost::FromID(gpu_host_id_);
-  if (!host)
-    return;
-
-  host->DestroyGpuMemoryBuffer(handle, sync_point);
-}
-
 }  // namespace content