mus: Get GpuMemoryBuffer from gpu process when possible.

services/ui/ws/gpu_service_proxy.cc

Index: services/ui/ws/gpu_service_proxy.cc
diff --git a/services/ui/ws/gpu_service_proxy.cc b/services/ui/ws/gpu_service_proxy.cc
index e1dd4aaeb65ede578976a1698a5883082d40bd42..308e5661e30100d31c7b84a912487659a806df61 100644
--- a/services/ui/ws/gpu_service_proxy.cc
+++ b/services/ui/ws/gpu_service_proxy.cc
@@ -10,6 +10,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "gpu/ipc/client/gpu_channel_host.h"
 #include "gpu/ipc/client/gpu_memory_buffer_impl_shared_memory.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
 #include "mojo/public/cpp/system/buffer.h"
 #include "mojo/public/cpp/system/platform_handle.h"
 #include "services/service_manager/public/cpp/connection.h"
@@ -25,6 +26,71 @@ namespace {
 const int32_t kInternalGpuChannelClientId = 1;
 const uint64_t kInternalGpuChannelClientTracingId = 1;
 
+// The implementation that relays requests from clients to the real
+// service implementation in the GPU process over mojom.GpuServiceInternal.
+class GpuServiceImpl : public mojom::GpuService {

[reveman, 2016/11/22 08:18:56]

I'm confused by this. Who is supposed to use this interface? Why are they not
using the GpuMemoryBufferManager directly? The purpose of the GMBM interface is
to hide all complicated out-of-process logic that might be needed and it's what
chromium code should be using in general. Here you seem to be wrapping that
interface in a GpuService while I would expected something more of the opposite;
GMBM using some mojo to allocate native buffers through another process.

[sadrul, 2016/11/22 18:45:41]

I have shared a doc [1] with you that hopefully explains this a little better.
But briefly: mus-clients use MojoGMBManager to talk to the mus-window-server.
This GpuServiceImpl (which lives in the mus-ws) is the end-point of that IPC
channel. This receives the request from the mus-clients, and forwards that to
the mus-gpu through MusGpuMemoryBufferManager (which lives in the mus-ws).
ui::GpuServiceInternal (which lives in mus-gpu) receives that message, and uses
GMBFactory to do actual memory allocation.

The names maybe confusing. I am open to suggestions for better names.

[1]
https://docs.google.com/presentation/d/1Ms1kgyg_nCxdbskxV4M9op5OtklKDcmkIk56m...

[reveman, 2016/11/22 23:25:04]

Ok, makes sense. I guess MojoGMBManager can't talk to MusGMBManager directly? In
a mus world, are we supposed to survive mus-gpu process crashes or is that
considered fatal? Curious as if the latter we need to make sure mus-gpu is
respawned correctly for GMBs allocation to never fail.

[sadrul, 2016/11/24 02:02:41]

It could if we really wanted to. But that would mean an extra message-pipe
between the two, whereas with the current set-up, the same message-pipe can be
used to establish the gpu-channel too.
We need to survive a mus-gpu process crash.

[reveman, 2016/11/30 17:32:45]

Ok, what happens if CreateGpuMemoryBuffer is called just after the mus-gpu
process has crashed but a new connection has not been established? Is there code
in place that will try to reestablish a new connection of GMB allocation fails.
Similar to what we have for content/ here:
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b...

[sadrul, 2016/11/30 19:54:31]

The short answer is: yes. But let me explain:

if the gpu process dies, then |gpu_service_internal_| of this object will not be
able to make any requests (or, the requests it makes will not go anywhere). As
the owner of the connection, GpuServiceProxy will have to recreate that
connection. Here is an example of how that would look like:
https://codereview.chromium.org/2546563002 This process of reconnecting will
respawn the gpu process.

+ public:
+  GpuServiceImpl(int client_id,
+                 gpu::GPUInfo* gpu_info,
+                 MusGpuMemoryBufferManager* gpu_memory_buffer_manager,
+                 mojom::GpuServiceInternal* gpu_service_internal)
+      : client_id_(client_id),
+        gpu_info_(gpu_info),
+        gpu_memory_buffer_manager_(gpu_memory_buffer_manager),
+        gpu_service_internal_(gpu_service_internal) {
+    DCHECK(gpu_memory_buffer_manager_);
+    DCHECK(gpu_service_internal_);
+  }
+  ~GpuServiceImpl() override {}
+
+ private:
+  void OnGpuChannelEstablished(const EstablishGpuChannelCallback& callback,
+                               mojo::ScopedMessagePipeHandle channel_handle) {
+    callback.Run(client_id_, std::move(channel_handle), *gpu_info_);
+  }
+
+  // mojom::GpuService overrides:
+  void EstablishGpuChannel(
+      const EstablishGpuChannelCallback& callback) override {
+    // TODO(sad): crbug.com/617415 figure out how to generate a meaningful
+    // tracing id.
+    const uint64_t client_tracing_id = 0;
+    constexpr bool is_gpu_host = false;
+    gpu_service_internal_->EstablishGpuChannel(
+        client_id_, client_tracing_id, is_gpu_host,
+        base::Bind(&GpuServiceImpl::OnGpuChannelEstablished,
+                   base::Unretained(this), callback));
+  }
+
+  void CreateGpuMemoryBuffer(
+      gfx::GpuMemoryBufferId id,
+      const gfx::Size& size,
+      gfx::BufferFormat format,
+      gfx::BufferUsage usage,
+      const mojom::GpuService::CreateGpuMemoryBufferCallback& callback)
+      override {
+    auto handle = gpu_memory_buffer_manager_->AllocateGpuMemoryBufferHandle(
+        id, client_id_, size, format, usage, gpu::kNullSurfaceHandle);
+    callback.Run(handle);
+  }
+
+  void DestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id,
+                              const gpu::SyncToken& sync_token) override {
+    gpu_memory_buffer_manager_->DestroyGpuMemoryBuffer(id, client_id_,
+                                                       sync_token);
+  }
+
+  const int client_id_;
+
+  // The objects these pointers refer to are owned by the GpuServiceProxy
+  // object.
+  const gpu::GPUInfo* gpu_info_;
+  MusGpuMemoryBufferManager* gpu_memory_buffer_manager_;
+  mojom::GpuServiceInternal* gpu_service_internal_;
+
+  DISALLOW_COPY_AND_ASSIGN(GpuServiceImpl);
+};
+
 }  // namespace
 
 GpuServiceProxy::GpuServiceProxy(GpuServiceProxyDelegate* delegate)
@@ -39,6 +105,9 @@ GpuServiceProxy::GpuServiceProxy(GpuServiceProxyDelegate* delegate)
   gpu_main_.Create(GetProxy(&gpu_service_));
   gpu_service_->Initialize(
       base::Bind(&GpuServiceProxy::OnInitialized, base::Unretained(this)));
+
+  gpu_memory_buffer_manager_ = base::MakeUnique<MusGpuMemoryBufferManager>(
+      gpu_service_.get(), kInternalGpuChannelClientId);
 }
 
 GpuServiceProxy::~GpuServiceProxy() {
@@ -47,7 +116,11 @@ GpuServiceProxy::~GpuServiceProxy() {
 }
 
 void GpuServiceProxy::Add(mojom::GpuServiceRequest request) {
-  bindings_.AddBinding(this, std::move(request));
+  mojo::MakeStrongBinding(
+      base::MakeUnique<GpuServiceImpl>(next_client_id_++, &gpu_info_,
+                                       gpu_memory_buffer_manager_.get(),
+                                       gpu_service_.get()),
+      std::move(request));
 }
 
 void GpuServiceProxy::OnInitialized(const gpu::GPUInfo& gpu_info) {
@@ -66,9 +139,6 @@ void GpuServiceProxy::OnInternalGpuChannelEstablished(
   base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
   thread_options.priority = base::ThreadPriority::NORMAL;
   CHECK(io_thread_->StartWithOptions(thread_options));
-
-  gpu_memory_buffer_manager_ = base::MakeUnique<MusGpuMemoryBufferManager>(
-      gpu_service_.get(), kInternalGpuChannelClientId);
   gpu_channel_ = gpu::GpuChannelHost::Create(
       this, kInternalGpuChannelClientId, gpu_info_,
       IPC::ChannelHandle(channel_handle.release()), &shutdown_event_,
@@ -77,48 +147,6 @@ void GpuServiceProxy::OnInternalGpuChannelEstablished(
     delegate_->OnGpuChannelEstablished(gpu_channel_);
 }
 
-void GpuServiceProxy::OnGpuChannelEstablished(
-    const EstablishGpuChannelCallback& callback,
-    int32_t client_id,
-    mojo::ScopedMessagePipeHandle channel_handle) {
-  callback.Run(client_id, std::move(channel_handle), gpu_info_);
-}
-
-void GpuServiceProxy::EstablishGpuChannel(
-    const EstablishGpuChannelCallback& callback) {
-  const int client_id = next_client_id_++;
-  // TODO(sad): crbug.com/617415 figure out how to generate a meaningful tracing
-  // id.
-  const uint64_t client_tracing_id = 0;
-  constexpr bool is_gpu_host = false;
-  gpu_service_->EstablishGpuChannel(
-      client_id, client_tracing_id, is_gpu_host,
-      base::Bind(&GpuServiceProxy::OnGpuChannelEstablished,
-                 base::Unretained(this), callback, client_id));
-}
-
-void GpuServiceProxy::CreateGpuMemoryBuffer(
-    gfx::GpuMemoryBufferId id,
-    const gfx::Size& size,
-    gfx::BufferFormat format,
-    gfx::BufferUsage usage,
-    const mojom::GpuService::CreateGpuMemoryBufferCallback& callback) {
-  // TODO(sad): Check to see if native gpu memory buffer can be used first.
-  if (!gpu::GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage) ||
-      !gpu::GpuMemoryBufferImplSharedMemory::IsSizeValidForFormat(size,
-                                                                  format)) {
-    callback.Run(gfx::GpuMemoryBufferHandle());
-    return;
-  }
-  callback.Run(gpu::GpuMemoryBufferImplSharedMemory::AllocateForChildProcess(
-      id, size, format));
-}
-
-void GpuServiceProxy::DestroyGpuMemoryBuffer(gfx::GpuMemoryBufferId id,
-                                             const gpu::SyncToken& sync_token) {
-  //  NOTIMPLEMENTED();
-}
-
 bool GpuServiceProxy::IsMainThread() {
   return main_thread_task_runner_->BelongsToCurrentThread();
 }