Make lost context and error message callbacks on GpuControl go to client

gpu/command_buffer/client/gl_in_process_context.cc

Index: gpu/command_buffer/client/gl_in_process_context.cc
diff --git a/gpu/command_buffer/client/gl_in_process_context.cc b/gpu/command_buffer/client/gl_in_process_context.cc
index 99e4b786a0bdcbe9312bddc259358f093c175a9c..21d8924c8dff052716de382decb2c6aa146847bf 100644
--- a/gpu/command_buffer/client/gl_in_process_context.cc
+++ b/gpu/command_buffer/client/gl_in_process_context.cc
@@ -49,6 +49,7 @@ const unsigned int kDefaultMaxTransferBufferSize = 16 * 1024 * 1024;
 
 class GLInProcessContextImpl
     : public GLInProcessContext,
+      public GpuControlClient,
       public base::SupportsWeakPtr<GLInProcessContextImpl> {
  public:
   explicit GLInProcessContextImpl(
@@ -67,12 +68,16 @@ class GLInProcessContextImpl
                   GpuMemoryBufferManager* gpu_memory_buffer_manager,
                   ImageFactory* image_factory);
 
-  // GLInProcessContext implementation:
-  void SetContextLostCallback(const base::Closure& callback) override;
+  // GLInProcessContext implementation.
   gles2::GLES2Implementation* GetImplementation() override;
   size_t GetMappedMemoryLimit() override;
   void SetLock(base::Lock* lock) override;
 
+  // GpuControlClient implementation. These methods are called on the GPU thread

[danakj, 2016/04/06 02:14:15]

It all gets quite hairy in here. The InProcessCommandBuffer runs on the GPU
thread, so these methods get called on the GPU thread. Which is ok I guess in
theory.

Except, in this patchset I have GLInProcessContextImpl as the GpuControlClient,
but the correct relationship is to have GLES2Implementation as the client. So I
should be calling

command_buffer_->SetGpuControlClient(gles2_implementation_.get());

But.. GLES2Implementation is not expecting those methods to be called on a GPU
thread.

And it turns out we want to intercept the lost context event in here somehow and
store lost_context_ = true on the context...

Alternatively we need multiple lost context callbacks on GLES2Implementation I
guess. But still we're calling into GLES2Implementation on the wrong thread
then..

[danakj, 2016/04/06 02:19:58]

Oh no I realized this class should not be calling SetGpuControlClient() on the
InProcessCommandBuffer at all. GLES2Implementation will already do that.

But GLES2Implementation is going to get called on the wrong thread then.

So.. do we have to do this with callbacks (wrapped in callbacks inside
callbacks) to thread hop here? :/

[danakj, 2016/04/06 02:25:44]

It seems like what's weird here (or different) is that InProcessCommandBuffer is
entirely living on the GPU thread (aka server side), whereas in the OOP version
we have CommandBufferProxyImpl on client side.

Perhaps we should have a thread split on InProcessCommandBuffer, and the
GpuControl parts should be on the main thread, and the rest of it should be on
the GPU thread? Then calling back to the GpuControlClient would be a thread hop
in the InProcessCommandBuffer (but wouldn't need nested callback dolls yay)
since it would be going to the "GpuControl" thread.

+  // by the in-process GpuControl.
+  void OnGpuControlErrorMessage(const char* message, int32_t id) override {}
+  void OnGpuControlLostContext() override;
+
 #if defined(OS_ANDROID)
   scoped_refptr<gfx::SurfaceTexture> GetSurfaceTexture(
       uint32_t stream_id) override;
@@ -81,7 +86,7 @@ class GLInProcessContextImpl
 
  private:
   void Destroy();
-  void OnContextLost();
+  bool IsContextLost() const;
   void OnSignalSyncPoint(const base::Closure& callback);
 
   scoped_ptr<gles2::GLES2CmdHelper> gles2_helper_;
@@ -90,9 +95,8 @@ class GLInProcessContextImpl
   scoped_ptr<InProcessCommandBuffer> command_buffer_;
 
   const GLInProcessContextSharedMemoryLimits mem_limits_;
+  mutable base::Lock context_lost_lock_;

[danakj, 2016/04/06 02:14:15]

We used to wrap callbacks inside callbacks to get to the right thread, but all
that just to set a context_lost_ bool.. so, I just added a lock instead.

Where I start to get lost is why we had this other |lock_|, which we'd grab when
setting the context_lost_ bool. The lock_ is set via SetLock in the
GLInProcessContext API, and passed to the InProcessCommandBuffer, but never
used!

The lock_ is actually owned out in (for example) AwRenderThreadContextProvider,
which calls SetLock in its own public SetupLock which is part of the
ContextProvider API.

So.. I don't get the point of this lock_ at all basically, and I removed it
(replaced with this new locally-owned lock). But then.. why have a lock on
AwRenderThreadContextProvider? Everything is confusing, halp.

   bool context_lost_;
-  base::Closure context_lost_callback_;
-  base::Lock* lock_;
 
   DISALLOW_COPY_AND_ASSIGN(GLInProcessContextImpl);
 };
@@ -104,8 +108,7 @@ base::LazyInstance<std::set<GLInProcessContextImpl*> > g_all_shared_contexts =
 
 GLInProcessContextImpl::GLInProcessContextImpl(
     const GLInProcessContextSharedMemoryLimits& mem_limits)
-    : mem_limits_(mem_limits), context_lost_(false), lock_(nullptr) {
-}
+    : mem_limits_(mem_limits) {}
 
 GLInProcessContextImpl::~GLInProcessContextImpl() {
   {
@@ -124,23 +127,19 @@ size_t GLInProcessContextImpl::GetMappedMemoryLimit() {
 }
 
 void GLInProcessContextImpl::SetLock(base::Lock* lock) {
+  // TODO(danakj): This isn't used... remove from the GLInProcessContext api?
   command_buffer_->SetLock(lock);
-  lock_ = lock;
 }
 
-void GLInProcessContextImpl::SetContextLostCallback(
-    const base::Closure& callback) {
-  context_lost_callback_ = callback;
+void GLInProcessContextImpl::OnGpuControlLostContext() {
+  // This is called on the GPU thread.
+  base::AutoLock lock(context_lost_lock_);
+  context_lost_ = true;
 }
 
-void GLInProcessContextImpl::OnContextLost() {
-  scoped_ptr<base::AutoLock> lock;
-  if (lock_)
-    lock.reset(new base::AutoLock(*lock_));
-  context_lost_ = true;
-  if (!context_lost_callback_.is_null()) {
-    context_lost_callback_.Run();
-  }
+bool GLInProcessContextImpl::IsContextLost() const {
+  base::AutoLock lock(context_lost_lock_);
+  return context_lost_;
 }
 
 bool GLInProcessContextImpl::Initialize(
@@ -161,8 +160,6 @@ bool GLInProcessContextImpl::Initialize(
   std::vector<int32_t> attrib_vector;
   attribs.Serialize(&attrib_vector);
 
-  base::Closure wrapped_callback =
-      base::Bind(&GLInProcessContextImpl::OnContextLost, AsWeakPtr());
   command_buffer_.reset(new InProcessCommandBuffer(service));
 
   scoped_ptr<base::AutoLock> scoped_shared_context_lock;
@@ -176,7 +173,7 @@ bool GLInProcessContextImpl::Initialize(
          it != g_all_shared_contexts.Get().end();
          it++) {
       const GLInProcessContextImpl* context = *it;
-      if (!context->context_lost_) {
+      if (!context->IsContextLost()) {
         share_group = context->gles2_implementation_->share_group();
         share_command_buffer = context->command_buffer_.get();
         DCHECK(share_group.get());
@@ -193,13 +190,14 @@ bool GLInProcessContextImpl::Initialize(
     DCHECK(share_command_buffer);
   }
 
+  command_buffer_->SetGpuControlClient(this);
+
   if (!command_buffer_->Initialize(surface,
                                    is_offscreen,
                                    window,
                                    size,
                                    attrib_vector,
                                    gpu_preference,
-                                   wrapped_callback,
                                    share_command_buffer,
                                    gpu_memory_buffer_manager,
                                    image_factory)) {