SharedContextState: Avoid unnecessary calls to MakeCurrent

The clients of SharedContextState (OOP-R, SharedImage, and SkiaRenderer)
do not care which GLSurface is bound as default FBO in the common case.
Changing the current surface has a non-trivial cost on Android Adreno
devices, even with virtual contexts enabled. This CL does a bit of
caching to avoid these unnecessary calls.

Change-Id: I0e3ec6308659644a725723295ef0b3109f22794f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1969736
Commit-Queue: Jonathan Backer <backer@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#725978}

components/viz/service/display_embedder/skia_output_device_gl.cc

@@ -12,6 +12,7 @@
 #include "gpu/command_buffer/service/feature_info.h"
 #include "gpu/command_buffer/service/gl_utils.h"
 #include "gpu/command_buffer/service/mailbox_manager.h"
+#include "gpu/command_buffer/service/shared_context_state.h"
 #include "gpu/command_buffer/service/texture_base.h"
 #include "gpu/command_buffer/service/texture_manager.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -30,6 +31,7 @@ namespace viz {
 
 SkiaOutputDeviceGL::SkiaOutputDeviceGL(
     gpu::MailboxManager* mailbox_manager,
+    gpu::SharedContextState* context_state,
     scoped_refptr<gl::GLSurface> gl_surface,
     scoped_refptr<gpu::gles2::FeatureInfo> feature_info,
     gpu::MemoryTracker* memory_tracker,
@@ -38,6 +40,7 @@ SkiaOutputDeviceGL::SkiaOutputDeviceGL(
                        memory_tracker,
                        std::move(did_swap_buffer_complete_callback)),
       mailbox_manager_(mailbox_manager),
+      context_state_(context_state),
       gl_surface_(std::move(gl_surface)) {
   capabilities_.flipped_output_surface = gl_surface_->FlipsVertically();
   capabilities_.supports_post_sub_buffer = gl_surface_->SupportsPostSubBuffer();
@@ -56,30 +59,23 @@ SkiaOutputDeviceGL::SkiaOutputDeviceGL(
   // This output device is never offscreen.
   capabilities_.supports_surfaceless = gl_surface_->IsSurfaceless();
 #endif
-}
-
-SkiaOutputDeviceGL::~SkiaOutputDeviceGL() = default;
 
-void SkiaOutputDeviceGL::Initialize(GrContext* gr_context,
-                                    gl::GLContext* gl_context) {
-  DCHECK(gr_context);
-  DCHECK(gl_context);
-  gr_context_ = gr_context;
+  DCHECK(context_state_->gr_context());
+  DCHECK(context_state_->context());
 
   if (gl_surface_->SupportsSwapTimestamps()) {
     gl_surface_->SetEnableSwapTimestamps();
 
     // Changes to swap timestamp queries are only picked up when making current.
-    gl_context->ReleaseCurrent(nullptr);
-    gl_context->MakeCurrent(gl_surface_.get());
+    context_state_->ReleaseCurrent(nullptr);
+    context_state_->MakeCurrent(gl_surface_.get());
   }
 
-  gl::CurrentGL* current_gl = gl_context->GetCurrentGL();
-  DCHECK(current_gl);
+  gl::CurrentGL* current_gl = context_state_->context()->GetCurrentGL();
 
   // Get alpha bits from the default frame buffer.
   glBindFramebufferEXT(GL_FRAMEBUFFER, 0);
-  gr_context_->resetContext(kRenderTarget_GrGLBackendState);
+  context_state_->gr_context()->resetContext(kRenderTarget_GrGLBackendState);
   const auto* version = current_gl->Version;
   GLint alpha_bits = 0;
   if (version->is_desktop_core_profile) {
@@ -93,6 +89,8 @@ void SkiaOutputDeviceGL::Initialize(GrContext* gr_context,
   supports_alpha_ = alpha_bits > 0;
 }
 
+SkiaOutputDeviceGL::~SkiaOutputDeviceGL() = default;
+
 bool SkiaOutputDeviceGL::Reshape(const gfx::Size& size,
                                  float device_scale_factor,
                                  const gfx::ColorSpace& color_space,
@@ -133,11 +131,12 @@ bool SkiaOutputDeviceGL::Reshape(const gfx::Size& size,
   auto origin = gl_surface_->FlipsVertically() ? kTopLeft_GrSurfaceOrigin
                                                : kBottomLeft_GrSurfaceOrigin;
   sk_surface_ = SkSurface::MakeFromBackendRenderTarget(
-      gr_context_, render_target, origin, color_type,
+      context_state_->gr_context(), render_target, origin, color_type,
       color_space.ToSkColorSpace(), &surface_props);
   if (!sk_surface_) {
-    LOG(ERROR) << "Couldn't create surface: " << gr_context_->abandoned() << " "
-               << color_type << " " << framebuffer_info.fFBOID << " "
+    LOG(ERROR) << "Couldn't create surface: "
+               << context_state_->gr_context()->abandoned() << " " << color_type
+               << " " << framebuffer_info.fFBOID << " "
                << framebuffer_info.fFormat << " " << color_space.ToString()
                << " " << size.ToString();
   }

components/viz/service/display_embedder/skia_output_device_gl.h

@@ -15,10 +15,7 @@
 #include "components/viz/service/display_embedder/skia_output_device.h"
 #include "gpu/command_buffer/common/mailbox.h"
 
-class GrContext;
-
 namespace gl {
-class GLContext;
 class GLImage;
 class GLSurface;
 }  // namespace gl
@@ -29,6 +26,7 @@ class GpuFence;
 
 namespace gpu {
 class MailboxManager;
+class SharedContextState;
 
 namespace gles2 {
 class FeatureInfo;
@@ -41,15 +39,14 @@ class SkiaOutputDeviceGL final : public SkiaOutputDevice {
  public:
   SkiaOutputDeviceGL(
       gpu::MailboxManager* mailbox_manager,
+      gpu::SharedContextState* context_state,
       scoped_refptr<gl::GLSurface> gl_surface,
       scoped_refptr<gpu::gles2::FeatureInfo> feature_info,
       gpu::MemoryTracker* memory_tracker,
       DidSwapBufferCompleteCallback did_swap_buffer_complete_callback);
   ~SkiaOutputDeviceGL() override;
 
-  void Initialize(GrContext* gr_context, gl::GLContext* gl_context);
   bool supports_alpha() {
-    DCHECK(gr_context_);
     return supports_alpha_;
   }
 
@@ -87,8 +84,8 @@ class SkiaOutputDeviceGL final : public SkiaOutputDevice {
 
   gpu::MailboxManager* const mailbox_manager_;
 
+  gpu::SharedContextState* const context_state_;
   scoped_refptr<gl::GLSurface> gl_surface_;
-  GrContext* gr_context_ = nullptr;
 
   sk_sp<SkSurface> sk_surface_;
 

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -707,6 +707,16 @@ class SkiaOutputSurfaceImplOnGpu::OffscreenSurface {
   sk_sp<SkPromiseImageTexture> promise_texture_;
 };
 
+SkiaOutputSurfaceImplOnGpu::ReleaseCurrent::ReleaseCurrent(
+    scoped_refptr<gl::GLSurface> gl_surface,
+    scoped_refptr<gpu::SharedContextState> context_state)
+    : gl_surface_(gl_surface), context_state_(context_state) {}
+
+SkiaOutputSurfaceImplOnGpu::ReleaseCurrent::~ReleaseCurrent() {
+  if (context_state_ && gl_surface_)
+    context_state_->ReleaseCurrent(gl_surface_.get());
+}
+
 // static
 std::unique_ptr<SkiaOutputSurfaceImplOnGpu> SkiaOutputSurfaceImplOnGpu::Create(
     SkiaOutputSurfaceDependency* deps,
@@ -785,6 +795,7 @@ SkiaOutputSurfaceImplOnGpu::~SkiaOutputSurfaceImplOnGpu() {
   if (context_state_ && MakeCurrent(false /* need_fbo0 */)) {
     // This ensures any outstanding callbacks for promise images are performed.
     gr_context()->flush();
+    release_current_last_.emplace(gl_surface_, context_state_);
   }
 
   if (copier_) {
@@ -793,6 +804,9 @@ SkiaOutputSurfaceImplOnGpu::~SkiaOutputSurfaceImplOnGpu() {
     copier_ = nullptr;
     texture_deleter_ = nullptr;
     context_provider_ = nullptr;
+
+    // Destroying context_provider_ will ReleaseCurrent. MakeCurrent again for
+    // the rest of this dtor.
     MakeCurrent(false /* need_fbo0 */);
   }
 
@@ -1447,10 +1461,9 @@ bool SkiaOutputSurfaceImplOnGpu::InitializeForGL() {
       } else {
         std::unique_ptr<SkiaOutputDeviceGL> onscreen_device =
             std::make_unique<SkiaOutputDeviceGL>(
-                dependency_->GetMailboxManager(), gl_surface_, feature_info_,
-                memory_tracker_.get(), did_swap_buffer_complete_callback_);
-
-        onscreen_device->Initialize(gr_context(), context);
+                dependency_->GetMailboxManager(), context_state_.get(),
+                gl_surface_, feature_info_, memory_tracker_.get(),
+                did_swap_buffer_complete_callback_);
         supports_alpha_ = onscreen_device->supports_alpha();
         output_device_ = std::move(onscreen_device);
       }

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -11,6 +11,7 @@
 
 #include "base/containers/circular_deque.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "base/util/type_safety/pass_key.h"
 #include "build/build_config.h"
@@ -253,6 +254,21 @@ class SkiaOutputSurfaceImplOnGpu : public gpu::ImageTransportSurfaceDelegate,
 
   void CheckReadbackCompletion();
 
+  class ReleaseCurrent {
+   public:
+    ReleaseCurrent(scoped_refptr<gl::GLSurface> gl_surface,
+                   scoped_refptr<gpu::SharedContextState> context_state);
+    ~ReleaseCurrent();
+
+   private:
+    scoped_refptr<gl::GLSurface> gl_surface_;
+    scoped_refptr<gpu::SharedContextState> context_state_;
+  };
+
+  // This must remain the first member variable to ensure that other member
+  // dtors are called first.
+  base::Optional<ReleaseCurrent> release_current_last_;
+
   SkiaOutputSurfaceDependency* const dependency_;
   scoped_refptr<gpu::gles2::FeatureInfo> feature_info_;
   scoped_refptr<gpu::SyncPointClientState> sync_point_client_state_;

gpu/command_buffer/service/raster_decoder.cc

@@ -552,8 +552,6 @@ class RasterDecoderImpl final : public RasterDecoder,
   // only if not returning an error.
   error::Error current_decoder_error_ = error::kNoError;
 
-  scoped_refptr<gl::GLContext> context_;
-
   GpuPreferences gpu_preferences_;
 
   gles2::DebugMarkerManager debug_marker_manager_;
@@ -727,8 +725,7 @@ ContextResult RasterDecoderImpl::Initialize(
     const gles2::DisallowedFeatures& disallowed_features,
     const ContextCreationAttribs& attrib_helper) {
   TRACE_EVENT0("gpu", "RasterDecoderImpl::Initialize");
-  DCHECK(shared_context_state_->IsCurrent(surface.get()));
-  DCHECK(!context_.get());
+  DCHECK(shared_context_state_->IsCurrent(nullptr));
 
   api_ = gl::g_current_gl_context;
 
@@ -746,7 +743,6 @@ ContextResult RasterDecoderImpl::Initialize(
 
   DCHECK_EQ(surface.get(), shared_context_state_->surface());
   DCHECK_EQ(context.get(), shared_context_state_->context());
-  context_ = context;
 
   // Create GPU Tracer for timing values.
   gpu_tracer_.reset(new gles2::GPUTracer(this));
@@ -831,13 +827,6 @@ void RasterDecoderImpl::Destroy(bool have_context) {
     query_manager_.reset();
   }
 
-  // Destroy the surface before the context, some surface destructors make GL
-  // calls.
-  if (context_.get()) {
-    context_->ReleaseCurrent(nullptr);
-    context_ = nullptr;
-  }
-
   font_manager_->Destroy();
   font_manager_.reset();
 }
@@ -847,9 +836,6 @@ bool RasterDecoderImpl::MakeCurrent() {
   if (!shared_context_state_->GrContextIsGL())
     return true;
 
-  if (!context_.get())
-    return false;
-
   if (context_lost_) {
     LOG(ERROR) << "  RasterDecoderImpl: Trying to make lost context current.";
     return false;
@@ -877,7 +863,7 @@ bool RasterDecoderImpl::MakeCurrent() {
 }
 
 gl::GLContext* RasterDecoderImpl::GetGLContext() {
-  return context_.get();
+  return shared_context_state_->context();
 }
 
 gl::GLSurface* RasterDecoderImpl::GetGLSurface() {

gpu/command_buffer/service/shared_context_state.cc

@@ -318,13 +318,31 @@ bool SharedContextState::MakeCurrent(gl::GLSurface* surface, bool needs_gl) {
   if (context_lost_)
     return false;
 
-  if (!context_->MakeCurrent(surface ? surface : surface_.get())) {
+  gl::GLSurface* dont_care_surface =
+      last_current_surface_ ? last_current_surface_ : surface_.get();
+  surface = surface ? surface : dont_care_surface;
+
+  if (!context_->MakeCurrent(surface)) {
     MarkContextLost();
     return false;
   }
+  last_current_surface_ = surface;
+
   return true;
 }
 
+void SharedContextState::ReleaseCurrent(gl::GLSurface* surface) {
+  if (!surface)
+    surface = last_current_surface_;
+
+  if (surface != last_current_surface_)
+    return;
+
+  last_current_surface_ = nullptr;
+  if (!context_lost_)
+    context_->ReleaseCurrent(surface);
+}
+
 void SharedContextState::MarkContextLost() {
   DCHECK(GrContextIsGL());
   if (!context_lost_) {

gpu/command_buffer/service/shared_context_state.h

@@ -86,6 +86,7 @@ class GPU_GLES2_EXPORT SharedContextState
   bool IsGLInitialized() const { return !!feature_info_; }
 
   bool MakeCurrent(gl::GLSurface* surface, bool needs_gl = false);
+  void ReleaseCurrent(gl::GLSurface* surface);
   void MarkContextLost();
   bool IsCurrent(gl::GLSurface* surface);
 
@@ -213,6 +214,12 @@ class GPU_GLES2_EXPORT SharedContextState
   scoped_refptr<gl::GLContext> context_;
   scoped_refptr<gl::GLContext> real_context_;
   scoped_refptr<gl::GLSurface> surface_;
+
+  // Most recent surface that this ShareContextState was made current with.
+  // Avoids a call to MakeCurrent with a different surface, if we don't
+  // care which surface is current.
+  gl::GLSurface* last_current_surface_ = nullptr;
+
   scoped_refptr<gles2::FeatureInfo> feature_info_;
 
   // raster decoders and display compositor share this context_state_.