content: Move all GpuMemoryBuffer allocation to IO thread.

content/browser/gpu/browser_gpu_channel_host_factory.cc

Index: content/browser/gpu/browser_gpu_channel_host_factory.cc
diff --git a/content/browser/gpu/browser_gpu_channel_host_factory.cc b/content/browser/gpu/browser_gpu_channel_host_factory.cc
index de4e791683b05fcc57f8184051c6a49ccacaee56..2ca5ed59db9e793338047201caf00620bf91872a 100644
--- a/content/browser/gpu/browser_gpu_channel_host_factory.cc
+++ b/content/browser/gpu/browser_gpu_channel_host_factory.cc
@@ -36,6 +36,18 @@ struct BrowserGpuChannelHostFactory::CreateRequest {
   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:
@@ -363,13 +375,74 @@ 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.

+
+  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)));
 
-  return GpuMemoryBufferImpl::Create(gfx::Size(width, height),
-                                     internalformat,
-                                     usage).PassAs<gfx::GpuMemoryBuffer>();
+  // 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
@@ -406,75 +479,33 @@ void BrowserGpuChannelHostFactory::SetHandlerForControlMessages(
                  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());
@@ -482,14 +513,4 @@ void BrowserGpuChannelHostFactory::OnGpuMemoryBufferCreated(
   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