GPU: Reduce MakeCurrent calls to fix Orange San Diego

gpu/command_buffer/service/gl_context_virtual.cc

Index: gpu/command_buffer/service/gl_context_virtual.cc
diff --git a/gpu/command_buffer/service/gl_context_virtual.cc b/gpu/command_buffer/service/gl_context_virtual.cc
index 5503a94b76307c974910d7e1ae3133b46ab9eac6..b7a90ce9b52b2fec95dd28e2faad1813848c611e 100644
--- a/gpu/command_buffer/service/gl_context_virtual.cc
+++ b/gpu/command_buffer/service/gl_context_virtual.cc
@@ -29,12 +29,12 @@ bool GLContextVirtual::Initialize(
     gfx::GLSurface* compatible_surface, gfx::GpuPreference gpu_preference) {
   display_ = static_cast<gfx::Display*>(compatible_surface->GetDisplay());
 
-  if (!shared_context_->MakeCurrent(compatible_surface))
-    return false;
+  if (!IsCurrent(compatible_surface)) {

[no sievers, 2013/04/10 22:44:31]

shared_context_->MakeCurrent() here is called to find out if the surface is
compatible with the real context (it's not for example on Linux if the pbuffer
does not match view config).
So we'd have to do it (at least for non-Android) regardless of what
GLContextVirtual::IsCurrent() says (which is optimized to return 'true'
falsely).

[epennerAtGoogle, 2013/04/11 03:23:43]

Hmm, if you want to legitimately bind a pbuffer surface for rendering, and
Surface::IsOffscreen() denotes such a surface, then it seems is IsCurrent is at
fault? IsCurrent should return false, which would allow the offscreen surface to
be truly made current for rendering?

Alternatively, if the surface has GetBackingFrameBufferObject(), then the phony
pbuffer surface backing should *not* need to be verified, right? That is implied
to be a phony singleton pbuffer, which can be verified as compatible at startup.

Can you foresee any other problems with this?

[no sievers, 2013/04/11 19:56:19]

Discussed optimizing this by having the caller pass in NULL for
compatible_surface if that surface is the same that the shared real context was
created with, and then skipping the test here (since we know it's compatible
then).

[epenner, 2013/05/02 04:13:11]

Sorry but that won't work. Initialize always need to insure a context/surface
current before for SetupForVirtualization(). So we always need the surface to be
passed in.

Also, it seems checking MakeCurrent to insure the pbuffer is compatible isn't
correct to do in this function anyway, since the real context is already created
and we can't recover from the error.

So I think the best thing to do is move this validation outside of Initialize as
I mentioned above, and leave the code as is, to insure a valid context/surface
is current.

If you look at the call-site, the real context is created with the
default-offscreen-surface, so it's already guaranteed to be compatible with that
surface.

+    if (!shared_context_->MakeCurrent(compatible_surface))
+      return false;
+  }
 
   shared_context_->SetupForVirtualization();
-
-  shared_context_->ReleaseCurrent(compatible_surface);

[no sievers, 2013/04/10 22:44:31]

This can probably just go away.

[epennerAtGoogle, 2013/04/11 03:23:43]

Done.

[epennerAtGoogle, 2013/04/11 03:23:43]

SGTM

   return true;
 }
 
@@ -45,7 +45,7 @@ void GLContextVirtual::Destroy() {
 }
 
 bool GLContextVirtual::MakeCurrent(gfx::GLSurface* surface) {
-  if (decoder_.get() && decoder_->initialized())
+  if (IsCurrent(surface) || (decoder_.get() && decoder_->initialized()))
     shared_context_->MakeVirtuallyCurrent(this, surface);
   else
     shared_context_->MakeCurrent(surface);
@@ -53,8 +53,8 @@ bool GLContextVirtual::MakeCurrent(gfx::GLSurface* surface) {
 }
 
 void GLContextVirtual::ReleaseCurrent(gfx::GLSurface* surface) {
-  if (IsCurrent(surface))

[no sievers, 2013/04/10 22:44:31]

shared_context_->ReleaseCurrent() still needs to be called at least when the
actual shared view surface goes away, which is not straightforward to find out
from here (for example ~GpuCommandBuffer stub passes NULL in here because it
specifically has to release its GLSurface ref before calling into here, as
~GLSurface might need a current context).

[no sievers, 2013/04/10 22:47:23]

Or to generalize, ReleaseCurrent() would have to be called whenever *any*
surface goes away that is still current.
So maybe it should happen in a different place.

But it does not have to be called here if an offscreen context + offscreen
surface goes away and that offscreen surface is not current (but some shared
surface is).

[epennerAtGoogle, 2013/04/11 03:23:43]

Okay I see the problem there. That's a pretty nasty dependency :P I'll think
about but conceptually we simply want this right?
if (!surface->GetBackingFrameBufferObject())
  shared_context_->ReleaseCurrent(surface);

Any objections assuming I can figure out the NULL case? That seems like it
should be fixed anyway.

[no sievers, 2013/04/11 19:56:19]

Discussed optimizing this by moving the call site from ~GpuCommandBufferStub()
to GLSurface::Destroy(), which is called only during surface destruction (so the
context will only get released when *all* references to the surface go away) and
further optimizing it for wrapper surface types (image transport) to skip
release if they don't have their own real (E)GL surface (but for example use the
shared offscreen surface as their handle).

[epenner, 2013/05/02 04:13:11]

To clarify, are you suggesting that every derived Surface::Destroy function also
does eglMakeCurrent(null,null) or equivalent if the surface is bound?

Hmm, an alternative would be that we get a higher level callback when the app is
minimized to do a real release current?

Out of interest, what is the problem with just leaving the old surface bound
until the new one arrives (on app minimize/restore)? The eglDestroySurface spec
says the surface isn't destroyed until it is made not-current, which likely
means the surface just becomes zero size but the context stays alive while the
app is minimized. Have you ever tried it?

-    shared_context_->ReleaseCurrent(surface);
+  // This is intentionally a no-op. When using virtual contexts
+  // the shared context should always stay current.
 }
 
 bool GLContextVirtual::IsCurrent(gfx::GLSurface* surface) {