Improve Ganesh helpers.

mojo/skia/ganesh_context.cc

Index: mojo/skia/ganesh_context.cc
diff --git a/mojo/skia/ganesh_context.cc b/mojo/skia/ganesh_context.cc
index 76b7c597332914472b8d6a52ecb903a2a0fb7e80..8a4057c187a77a400d5178db2d16c77b40ab7aa1 100644
--- a/mojo/skia/ganesh_context.cc
+++ b/mojo/skia/ganesh_context.cc
@@ -9,53 +9,48 @@
 #include "third_party/skia/include/gpu/gl/GrGLInterface.h"
 
 namespace mojo {
-namespace {
+namespace skia {
 
 // The limit of the number of GPU resources we hold in the GrContext's
 // GPU cache.
-const int kMaxGaneshResourceCacheCount = 2048;
+constexpr int kMaxGaneshResourceCacheCount = 2048;
 
 // The limit of the bytes allocated toward GPU resources in the GrContext's
 // GPU cache.
-const size_t kMaxGaneshResourceCacheBytes = 96 * 1024 * 1024;
-}
+constexpr size_t kMaxGaneshResourceCacheBytes = 96 * 1024 * 1024;
 
-GaneshContext::Scope::Scope(GaneshContext* context)
-    : previous_(gles2::GetGLContext()) {
-  auto gl = context->gl_context_->gl();
-  DCHECK(gl);
-  gles2::SetGLContext(gl);
-  DCHECK(gles2::GetGLContext());
+GaneshContext::Scope::Scope(GaneshContext* context) : context_(context) {
+  DCHECK(context_);

[viettrungluu, 2015/12/18 16:34:48]

I wonder if we shouldn't just inline the definition of Scope.

(The DCHECK is of pretty limited value, since it'll almost certainly crash in an
obvious way if |context_| is null.)

[jeffbrown, 2015/12/18 17:48:24]

Yeah that's a good idea.  Now that it's just calling EnterScope / ExitScope
there isn't much else for this function to do.

+  context_->EnterScope();
 }
 
 GaneshContext::Scope::~Scope() {
-  gles2::SetGLContext(previous_);
+  context_->ExitScope();
 }
 
 GaneshContext::GaneshContext(base::WeakPtr<GLContext> gl_context)
     : gl_context_(gl_context) {
   DCHECK(gl_context_);
   gl_context_->AddObserver(this);
+
   Scope scope(this);
 
-  skia::RefPtr<GrGLInterface> interface =
-      skia::AdoptRef(skia_bindings::CreateMojoSkiaGLBinding());
+  ::skia::RefPtr<GrGLInterface> interface =
+      ::skia::AdoptRef(CreateMojoSkiaGLBinding());
   DCHECK(interface);
 
-  context_ = skia::AdoptRef(GrContext::Create(
+  gr_context_ = ::skia::AdoptRef(GrContext::Create(
       kOpenGL_GrBackend, reinterpret_cast<GrBackendContext>(interface.get())));
-  DCHECK(context_);
-  context_->setResourceCacheLimits(kMaxGaneshResourceCacheCount,
-                                   kMaxGaneshResourceCacheBytes);
+  DCHECK(gr_context_);
+  gr_context_->setResourceCacheLimits(kMaxGaneshResourceCacheCount,
+                                      kMaxGaneshResourceCacheBytes);
 }
 
 GaneshContext::~GaneshContext() {
-  if (context_) {
-    Scope scope(this);
-    context_.clear();
-  }
-  if (gl_context_.get())
+  if (gl_context_)
     gl_context_->RemoveObserver(this);
+
+  ReleaseContext();
 }
 
 bool GaneshContext::InScope() const {
@@ -63,9 +58,44 @@ bool GaneshContext::InScope() const {
 }
 
 void GaneshContext::OnContextLost() {
-  context_->abandonContext();
-  context_.clear();
+  ReleaseContext();
+}
+
+void GaneshContext::ReleaseContext() {
+  Scope(this);
+  gr_context_->abandonContext();
+  gr_context_.clear();
   gl_context_.reset();
 }
 
+void GaneshContext::EnterScope() {
+  if (!scope_nest_count_) {

[viettrungluu, 2015/12/18 16:34:48]

(jamesr or abarth should probably look at this, and decide if we really want to
allow nested scopes, but see my comments below.)

+    previous_mgl_context_ = MGLGetCurrentContext();
+    if (gl_context_)
+      gl_context_->MakeCurrent();
+
+    // Reset the Ganesh context when entering its scope in case the caller
+    // performed low-level GL operations which might interfere with Ganesh's
+    // state expectations.
+    if (gr_context_)
+      gr_context_->resetContext();

[viettrungluu, 2015/12/18 16:34:48]

If the intent is to allow scopes from different GaneshContexts to be nested,
then you'd have to flush the previous "gr_context_" here, right? (And then, on
exiting the scope, you'd have to reset the previous gr_context_ as well.)

So I guess that doesn't work/isn't allowed, so you're only allowed to nest
scopes from the GaneshContext? I'm not sure that's a good idea, since it
encourages a certain level of sloppiness (for reasons similar to why I don't
like recursive mutexes).

(TBH, I'm not a big fan of GaneshContext::Scope in general. IIRC, in some
previous life, I was going to delete it.)

[jeffbrown, 2015/12/18 17:48:24]

FWIW, I'm with you on recursive mutexes (yuck).  Open to suggestion as to
whether to support recursive GaneshContexts.

The use-case this appeared in was in some helper code which gets passed a
GaneshContext* to do a little bit of work.  For safety, it enters a Scope at the
very beginning of the function.  If the caller also happened to be in the same
scope then recursion occurs and it would be unfortunate if we flushed / reset
the context repeatedly across this transition.

An alternative solution would be to change the helpers to take a GrContext*
instead so we move responsibility for managing the scope to a higher level.  A
little inconvenient but perhaps better.

Another approach would be to pass in the Scope itself.  We could move the
"gr_context()" and "gl_context()" methods to the Scope so there's never any
ambiguity about whether the scope has been entered, assuming it exists. 
IsInScope() can go away.

Thoughts?

[viettrungluu, 2015/12/18 18:05:41]

I think my tendency would be to go with something like this.

I think I dislike GaneshContext::Scope because entering/leaving a Ganesh context
(well, really, using Ganesh at all) actually has side effects on the GL context,
so callers of any function that uses Ganesh need to know that.

[jeffbrown, 2015/12/18 18:12:57]

Sounds good.  I'll do that.

+  }
+  scope_nest_count_++;
+  DCHECK(scope_nest_count_);
+}
+
+void GaneshContext::ExitScope() {
+  DCHECK(scope_nest_count_);
+  scope_nest_count_--;
+  if (!scope_nest_count_) {
+    // Flush the Ganesh context when exiting its scope.
+    if (gr_context_)
+      gr_context_->flush();
+
+    MGLMakeCurrent(previous_mgl_context_);
+    previous_mgl_context_ = MGL_NO_CONTEXT;
+  }
+}
+
+}  // namespace skia
 }  // namespace mojo