Move GLFences from GLSurface to GLContext

The GLFences used for backpressure in ImageTransportSurfaceOverlayMac
are objects that should be scoped to a GLContext, not a GLSurface. Add
methods to GLContext to allow creation and waiting on fences, and call
those methods from GLSurface.

Bug: 863817
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ib0fc8cac3efdd50f88c197c6bdeb2ee2e22696bd
Reviewed-on: https://chromium-review.googlesource.com/1146483
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577321}

gpu/command_buffer/service/gl_context_virtual.cc

@@ -5,6 +5,7 @@
 #include "gpu/command_buffer/service/gl_context_virtual.h"
 
 #include "base/callback.h"
+#include "build/build_config.h"
 #include "gpu/command_buffer/service/decoder_context.h"
 #include "gpu/command_buffer/service/gl_state_restorer_impl.h"
 #include "ui/gl/gl_gl_api_implementation.h"
@@ -101,6 +102,16 @@ void GLContextVirtual::ForceReleaseVirtuallyCurrent() {
   shared_context_->OnReleaseVirtuallyCurrent(this);
 }
 
+#if defined(OS_MACOSX)
+uint64_t GLContextVirtual::BackpressureFenceCreate() {
+  return shared_context_->BackpressureFenceCreate();
+}
+
+void GLContextVirtual::BackpressureFenceWait(uint64_t fence) {
+  shared_context_->BackpressureFenceWait(fence);
+}
+#endif
+
 GLContextVirtual::~GLContextVirtual() {
   Destroy();
 }

gpu/command_buffer/service/gl_context_virtual.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "build/build_config.h"
 #include "gpu/gpu_gles2_export.h"
 #include "ui/gl/gl_context.h"
 
@@ -46,6 +47,10 @@ class GPU_GLES2_EXPORT GLContextVirtual : public gl::GLContext {
   gl::YUVToRGBConverter* GetYUVToRGBConverter(
       const gfx::ColorSpace& color_space) override;
   void ForceReleaseVirtuallyCurrent() override;
+#if defined(OS_MACOSX)
+  uint64_t BackpressureFenceCreate() override;
+  void BackpressureFenceWait(uint64_t fence) override;
+#endif
 
  protected:
   ~GLContextVirtual() override;

gpu/ipc/service/image_transport_surface_overlay_mac.h

@@ -92,10 +92,9 @@ class ImageTransportSurfaceOverlayMac : public gl::GLSurface,
 
   std::vector<CALayerInUseQuery> ca_layer_in_use_queries_;
 
-  // A GLFence marking the end of the previous frame. Must only be accessed
-  // while the associated |previous_frame_context_| is bound.
-  std::unique_ptr<gl::GLFence> previous_frame_fence_;
-  base::ScopedTypeRef<CGLContextObj> fence_context_obj_;
+  // A GLFence marking the end of the previous frame, used for applying
+  // backpressure.
+  uint64_t previous_frame_fence_ = 0;
 
   // The renderer ID that all contexts made current to this surface should be
   // targeting.

gpu/ipc/service/image_transport_surface_overlay_mac.mm

@@ -19,22 +19,10 @@
 #include "ui/base/ui_base_switches.h"
 #include "ui/gl/ca_renderer_layer_params.h"
 #include "ui/gl/gl_context.h"
-#include "ui/gl/gl_fence.h"
 #include "ui/gl/gl_image_io_surface.h"
 #include "ui/gl/gpu_switching_manager.h"
 #include "ui/gl/scoped_cgl.h"
 
-namespace {
-
-void CheckGLErrors(const char* msg) {
-  GLenum gl_error;
-  while ((gl_error = glGetError()) != GL_NO_ERROR) {
-    LOG(ERROR) << "OpenGL error hit " << msg << ": " << gl_error;
-  }
-}
-
-}  // namespace
-
 namespace gpu {
 
 ImageTransportSurfaceOverlayMac::ImageTransportSurfaceOverlayMac(
@@ -78,25 +66,10 @@ bool ImageTransportSurfaceOverlayMac::Initialize(gl::GLSurfaceFormat format) {
 }
 
 void ImageTransportSurfaceOverlayMac::PrepareToDestroy(bool have_context) {
-  if (!previous_frame_fence_)
-    return;
-  if (!have_context) {
-    // If we have no context, leak the GL objects, since we have no way to
-    // delete them.
-    DLOG(ERROR) << "Leaking GL fences.";
-    previous_frame_fence_.release();
-    return;
-  }
-  // Ensure we are using the context with which the fence was created.
-  DCHECK_EQ(fence_context_obj_, CGLGetCurrentContext());
-  CheckGLErrors("Before destroy fence");
-  previous_frame_fence_.reset();
-  CheckGLErrors("After destroy fence");
 }
 
 void ImageTransportSurfaceOverlayMac::Destroy() {
   ca_layer_tree_coordinator_.reset();
-  DCHECK(!previous_frame_fence_);
 }
 
 bool ImageTransportSurfaceOverlayMac::IsOffscreen() {
@@ -105,104 +78,16 @@ bool ImageTransportSurfaceOverlayMac::IsOffscreen() {
 
 void ImageTransportSurfaceOverlayMac::ApplyBackpressure() {
   TRACE_EVENT0("gpu", "ImageTransportSurfaceOverlayMac::ApplyBackpressure");
-
-  // TODO(ccameron): If this fixes the crashes in GLFence::Create, then
-  // determine how we are getting into this situation.
-  // https://crbug.com/863817
-  CGLContextObj current_context = CGLGetCurrentContext();
-  if (!current_context) {
-    // TODO(ccameron): ImageTransportSurfaceOverlayMac assumes that it is only
-    // ever created with a GLContextCGL. This is no longer the case for layout
-    // tests, and possibly in other situations.
-    // https://crbug.com/866520
-    return;
-  }
-
-  // If supported, use GLFence to ensure that we haven't gotten more than one
-  // frame ahead of GL.
-  if (gl::GLFence::IsSupported()) {
-    CheckGLErrors("Before fence/flush");
-
-    // All GL writes to IOSurfaces prior to a glFlush will appear when that
-    // IOSurface is next displayed as a CALayer.
-    // TODO(ccameron): This is unnecessary if the below GLFence is a GLFenceARB,
-    // as that will call glFlush after creating the sync object.
-    {
-      TRACE_EVENT0("gpu", "glFlush");
-      base::TimeTicks before_flush_time = base::TimeTicks::Now();
-      glFlush();
-      base::TimeTicks after_flush_time = base::TimeTicks::Now();
-      CheckGLErrors("After fence/flush");
-      UMA_HISTOGRAM_TIMES("GPU.IOSurface.GLFlushTime",
-                          after_flush_time - before_flush_time);
-    }
-
-    // Create a fence for the current frame's work.
-    std::unique_ptr<gl::GLFence> this_frame_fence;
-    {
-      TRACE_EVENT0("gpu", "Create GLFence");
-      this_frame_fence = gl::GLFence::Create();
-    }
-
-    // If we have gotten more than one frame ahead of GL, wait for the previous
-    // frame to complete.
-    bool fence_context_changed = true;
-    if (previous_frame_fence_) {
-      fence_context_changed = fence_context_obj_.get() != current_context;
-      TRACE_EVENT0("gpu", "ClientWait");
-
-      // Ensure we are using the context with which the fence was created.
-      base::Optional<gl::ScopedCGLSetCurrentContext> scoped_set_current;
-      if (fence_context_changed) {
-        TRACE_EVENT0("gpu", "SetCurrentContext");
-        scoped_set_current.emplace(fence_context_obj_);
-      }
-
-      // While we could call GLFence::ClientWait, this performs a busy wait on
-      // Mac, leading to high CPU usage. Instead we poll with a 1ms delay. This
-      // should have minimal impact, as we will only hit this path when we are
-      // more than one frame (16ms) behind.
-      //
-      // Note that on some platforms (10.9), fences appear to sometimes get
-      // lost and will never pass. Add a 32ms timout to prevent these
-      // situations from causing a GPU process hang. crbug.com/618075
-      bool fence_completed = false;
-      for (int poll_iter = 0; !fence_completed && poll_iter < 32; ++poll_iter) {
-        if (poll_iter > 0) {
-          base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
-        }
-        {
-          TRACE_EVENT0("gpu", "GLFence::HasCompleted");
-          fence_completed = previous_frame_fence_->HasCompleted();
-        }
-      }
-      if (!fence_completed) {
-        TRACE_EVENT0("gpu", "Finish");
-        // We timed out waiting for the above fence, just issue a glFinish.
-        glFinish();
-      }
-
-      // Ensure that the GLFence object be destroyed while its context is
-      // current, lest we crash.
-      // https://crbug.com/863817
-      previous_frame_fence_.reset();
-    }
-
-    // Update the previous fame fence and save the context that we will be
-    // using for the GLFence object we will create (or just leave the
-    // previously saved one, if it is unchanged).
-    previous_frame_fence_ = std::move(this_frame_fence);
-    if (fence_context_changed) {
-      fence_context_obj_.reset(current_context, base::scoped_policy::RETAIN);
-    }
-  } else {
-    // GLFence isn't supported - issue a glFinish on each frame to ensure
-    // there is backpressure from GL.
-    TRACE_EVENT0("gpu", "glFinish");
-    CheckGLErrors("Before finish");
-    glFinish();
-    CheckGLErrors("After finish");
-  }
+  gl::GLContext* current_context = gl::GLContext::GetCurrent();
+  // TODO(ccameron): Remove these CHECKs.
+  CHECK(current_context);
+  CHECK(current_context->IsCurrent(this));
+
+  // Create the fence for the current frame before waiting on the previous
+  // frame's fence (to maximize CPU and GPU execution overlap).
+  uint64_t this_frame_fence = current_context->BackpressureFenceCreate();
+  current_context->BackpressureFenceWait(previous_frame_fence_);
+  previous_frame_fence_ = this_frame_fence;
 }
 
 void ImageTransportSurfaceOverlayMac::BufferPresented(

ui/gl/gl_context.cc

@@ -14,6 +14,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_local.h"
+#include "build/build_config.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_gl_api_implementation.h"
 #include "ui/gl/gl_implementation.h"
@@ -187,6 +188,14 @@ void GLContext::ForceReleaseVirtuallyCurrent() {
   NOTREACHED();
 }
 
+#if defined(OS_MACOSX)
+uint64_t GLContext::BackpressureFenceCreate() {
+  return 0;
+}
+
+void GLContext::BackpressureFenceWait(uint64_t fence) {}
+#endif
+
 bool GLContext::HasExtension(const char* name) {
   return gfx::HasExtension(GetExtensions(), name);
 }

ui/gl/gl_context.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/cancellation_flag.h"
+#include "build/build_config.h"
 #include "ui/gfx/extension_set.h"
 #include "ui/gl/gl_export.h"
 #include "ui/gl/gl_share_group.h"
@@ -193,6 +194,16 @@ class GL_EXPORT GLContext : public base::RefCounted<GLContext> {
   // current.
   virtual void ForceReleaseVirtuallyCurrent();
 
+#if defined(OS_MACOSX)
+  // Create a fence for all work submitted to this context so far, and return a
+  // monotonically increasing handle to it. This returned handle never needs to
+  // be freed. This method is used to create backpressure to throttle GL work
+  // on macOS, so that we do not starve CoreAnimation.
+  virtual uint64_t BackpressureFenceCreate();
+  // Perform a client-side wait on a previously-created fence.
+  virtual void BackpressureFenceWait(uint64_t fence);
+#endif
+
  protected:
   virtual ~GLContext();
 

ui/gl/gl_context_cgl.cc

@@ -17,6 +17,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "ui/gl/gl_bindings.h"
+#include "ui/gl/gl_fence.h"
 #include "ui/gl/gl_gl_api_implementation.h"
 #include "ui/gl/gl_implementation.h"
 #include "ui/gl/gl_surface.h"
@@ -148,9 +149,9 @@ bool GLContextCGL::Initialize(GLSurface* compatible_surface,
 }
 
 void GLContextCGL::Destroy() {
-  if (!yuv_to_rgb_converters_.empty()) {
+  if (!yuv_to_rgb_converters_.empty() || !backpressure_fences_.empty()) {
     // If this context is not current, bind this context's API so that the YUV
-    // converter can safely destruct
+    // converter and GLFences can safely destruct
     GLContext* current_context = GetRealCurrent();
     if (current_context != this) {
       SetCurrentGL(GetCurrentGL());
@@ -159,6 +160,7 @@ void GLContextCGL::Destroy() {
     ScopedCGLSetCurrentContext scoped_set_current(
         static_cast<CGLContextObj>(context_));
     yuv_to_rgb_converters_.clear();
+    backpressure_fences_.clear();
 
     // Rebind the current context's API if needed.
     if (current_context && current_context != this) {
@@ -234,6 +236,72 @@ YUVToRGBConverter* GLContextCGL::GetYUVToRGBConverter(
   return yuv_to_rgb_converter.get();
 }
 
+// If the GLFence extensions are not supported or are unstable, fall back to
+// using a glFinish. Use this id to denote a fence that should be implemented
+// as glFinish.
+constexpr uint64_t kFinishFenceId = -1;
+
+uint64_t GLContextCGL::BackpressureFenceCreate() {
+  TRACE_EVENT0("gpu", "GLContextCGL::BackpressureFenceCreate");
+  // TODO(ccameron): Remove this CHECK.
+  CHECK(CGLGetCurrentContext() == context_);
+  CHECK(context_);
+  if (gl::GLFence::IsSupported()) {
+    backpressure_fences_[next_backpressure_fence_] = GLFence::Create();
+    return next_backpressure_fence_++;
+  }
+  return kFinishFenceId;
+}
+
+void GLContextCGL::BackpressureFenceWait(uint64_t fence_id) {
+  TRACE_EVENT0("gpu", "GLContextCGL::BackpressureFenceWait");
+  // TODO(ccameron): Remove this CHECK.
+  CHECK(CGLGetCurrentContext() == context_);
+  CHECK(context_);
+  if (fence_id == kFinishFenceId) {
+    glFinish();
+    return;
+  }
+
+  // If a fence is not found, then it has already been waited on.
+  auto found = backpressure_fences_.find(fence_id);
+  if (found == backpressure_fences_.end())
+    return;
+  std::unique_ptr<GLFence> fence = std::move(found->second);
+  backpressure_fences_.erase(found);
+
+  // While we could call GLFence::ClientWait, this performs a busy wait on
+  // Mac, leading to high CPU usage. Instead we poll with a 1ms delay. This
+  // should have minimal impact, as we will only hit this path when we are
+  // more than one frame (16ms) behind.
+  //
+  // Note that on some platforms (10.9), fences appear to sometimes get
+  // lost and will never pass. Add a 32ms timout to prevent these
+  // situations from causing a GPU process hang.
+  // https://crbug.com/618075
+  bool fence_completed = false;
+  for (int poll_iter = 0; !fence_completed && poll_iter < 32; ++poll_iter) {
+    if (poll_iter > 0) {
+      base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+    }
+    {
+      TRACE_EVENT0("gpu", "GLFence::HasCompleted");
+      fence_completed = fence->HasCompleted();
+    }
+  }
+  if (!fence_completed) {
+    TRACE_EVENT0("gpu", "Finish");
+    // We timed out waiting for the above fence, just issue a glFinish.
+    glFinish();
+  }
+  fence.reset();
+
+  // Waiting on |fence_id| has implicitly waited on all previous fences, so
+  // remove them.
+  while (backpressure_fences_.begin()->first < fence_id)
+    backpressure_fences_.erase(backpressure_fences_.begin());
+}
+
 bool GLContextCGL::MakeCurrent(GLSurface* surface) {
   DCHECK(context_);
 

ui/gl/gl_context_cgl.h

@@ -17,6 +17,7 @@
 
 namespace gl {
 
+class GLFence;
 class GLSurface;
 
 // Encapsulates a CGL OpenGL context.
@@ -35,6 +36,8 @@ class GL_EXPORT GLContextCGL : public GLContextReal {
   bool ForceGpuSwitchIfNeeded() override;
   YUVToRGBConverter* GetYUVToRGBConverter(
       const gfx::ColorSpace& color_space) override;
+  uint64_t BackpressureFenceCreate() override;
+  void BackpressureFenceWait(uint64_t fence) override;
 
  protected:
   ~GLContextCGL() override;
@@ -48,6 +51,9 @@ class GL_EXPORT GLContextCGL : public GLContextReal {
   std::map<gfx::ColorSpace, std::unique_ptr<YUVToRGBConverter>>
       yuv_to_rgb_converters_;
 
+  std::map<uint64_t, std::unique_ptr<GLFence>> backpressure_fences_;
+  uint64_t next_backpressure_fence_ = 0;
+
   CGLPixelFormatObj discrete_pixelformat_;
 
   int screen_;