SkaOutputSurfaceImpl: add EnqueueGpuTask() and FlushGpuTasks() methods.

SkaOutputSurfaceImpl defers draw until SwapBuffers. However SKiaRenderer
may issue other GPU tasks between Draw and SwapBuffers for example:
ScheduleOveralys(), ScheduleOutputSurfaceAsOverlay(), etc. Those tasks
will be executed before drawing task. It may causes problems.
Fix the issue by en-queuing all GPU tasks, and then defer and execute
them in order.

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

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -143,18 +143,14 @@ SkiaOutputSurfaceImpl::~SkiaOutputSurfaceImpl() {
   }
   DCHECK(render_pass_image_cache_.empty());
 
-  // Post a task to destroy |impl_on_gpu_| on the GPU thread and block until
-  // that is finished.
-  base::WaitableEvent event;
+  // Post a task to destroy |impl_on_gpu_| on the GPU thread.
   auto task = base::BindOnce(
-      [](std::unique_ptr<SkiaOutputSurfaceImplOnGpu> impl_on_gpu,
-         base::WaitableEvent* event) {
-        impl_on_gpu.reset();
-        event->Signal();
-      },
-      std::move(impl_on_gpu_), &event);
-  ScheduleGpuTask(std::move(task), {});
-  event.Wait();
+      [](std::unique_ptr<SkiaOutputSurfaceImplOnGpu> impl_on_gpu) {},
+      std::move(impl_on_gpu_));
+  EnqueueGpuTask(std::move(task), {}, /*make_current=*/false,
+                 /*need_framebuffer=*/false);
+  // Flush GPU tasks and block until all tasks are finished.
+  FlushGpuTasks(/*wait_for_finish=*/true);
 }
 
 gpu::SurfaceHandle SkiaOutputSurfaceImpl::GetSurfaceHandle() const {
@@ -192,7 +188,8 @@ void SkiaOutputSurfaceImpl::SetEnableDCLayers(bool enable) {
 
   auto task = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::SetEnableDCLayers,
                              base::Unretained(impl_on_gpu_.get()), enable);
-  ScheduleGpuTask(std::move(task), {});
+  EnqueueGpuTask(std::move(task), {}, /*make_current=*/true,
+                 /*need_framebuffer=*/false);
 }
 
 void SkiaOutputSurfaceImpl::EnsureBackbuffer() {
@@ -247,7 +244,9 @@ void SkiaOutputSurfaceImpl::Reshape(const gfx::Size& size,
                              base::Unretained(impl_on_gpu_.get()), size,
                              device_scale_factor, color_space, format,
                              use_stencil, GetDisplayTransform());
-  ScheduleGpuTask(std::move(task), {});
+  EnqueueGpuTask(std::move(task), {}, /*make_current=*/true,
+                 /*need_framebuffer=*/!dependency_->IsOffscreen());
+  FlushGpuTasks(/*wait_for_finish=*/false);
 
   color_space_ = color_space;
   is_hdr_ = color_space_.IsHDR();
@@ -412,7 +411,10 @@ gpu::SyncToken SkiaOutputSurfaceImpl::ReleaseImageContexts(
       base::BindOnce(&SkiaOutputSurfaceImplOnGpu::ReleaseImageContexts,
                      base::Unretained(impl_on_gpu_.get()),
                      std::move(image_contexts), sync_fence_release_);
-  gpu_task_scheduler_->ScheduleGpuTask(std::move(callback), {});
+  EnqueueGpuTask(std::move(callback), {}, /*make_current=*/true,
+                 /*need_framebuffer=*/false);
+  // Defer ReleaseImageContexts.
+  FlushGpuTasks(/*wait_for_finish=*/false);
   return sync_token;
 }
 
@@ -465,9 +467,11 @@ void SkiaOutputSurfaceImpl::SwapBuffers(OutputSurfaceFrame frame) {
   // SkiaOutputSurfaceImpl::dtor. So it is safe to use base::Unretained.
   auto callback = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::SwapBuffers,
                                  base::Unretained(impl_on_gpu_.get()),
-                                 post_task_timestamp, std::move(frame),
-                                 std::move(deferred_framebuffer_draw_closure_));
-  ScheduleGpuTask(std::move(callback), std::move(resource_sync_tokens_));
+                                 post_task_timestamp, std::move(frame));
+  EnqueueGpuTask(std::move(callback), std::move(resource_sync_tokens_),
+                 /*make_current=*/true,
+                 /*need_framebuffer=*/!dependency_->IsOffscreen());
+  FlushGpuTasks(/*wait_for_finish=*/false);
 
   // Recreate |root_recorder_| after SwapBuffers has been scheduled on GPU
   // thread to save some time in BeginPaintCurrentFrame
@@ -479,9 +483,10 @@ void SkiaOutputSurfaceImpl::SwapBuffersSkipped() {
   // PostTask to the GPU thread to deal with freeing resources and running
   // callbacks.
   auto task = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::SwapBuffersSkipped,
-                             base::Unretained(impl_on_gpu_.get()),
-                             std::move(deferred_framebuffer_draw_closure_));
-  ScheduleGpuTask(std::move(task), std::move(resource_sync_tokens_));
+                             base::Unretained(impl_on_gpu_.get()));
+  EnqueueGpuTask(std::move(task), std::move(resource_sync_tokens_),
+                 /*make_current=*/false, /*need_framebuffer=*/false);
+  FlushGpuTasks(/*wait_for_finish=*/false);
 
   // TODO(vasilyt): reuse root recorder
   RecreateRootRecorder();
@@ -495,7 +500,8 @@ void SkiaOutputSurfaceImpl::ScheduleOutputSurfaceAsOverlay(
   auto callback = base::BindOnce(
       &SkiaOutputSurfaceImplOnGpu::ScheduleOutputSurfaceAsOverlay,
       base::Unretained(impl_on_gpu_.get()), std::move(output_surface_plane));
-  ScheduleGpuTask(std::move(callback), {});
+  EnqueueGpuTask(std::move(callback), {}, /*make_current=*/false,
+                 /*need_framebuffer=*/false);
 }
 
 SkCanvas* SkiaOutputSurfaceImpl::BeginPaintRenderPass(
@@ -555,7 +561,6 @@ gpu::SyncToken SkiaOutputSurfaceImpl::SubmitPaint(
     base::OnceClosure on_finished) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(current_paint_);
-  DCHECK(!deferred_framebuffer_draw_closure_);
   // If current_render_pass_id_ is not null, we are painting a render pass.
   // Otherwise we are painting a frame.
 
@@ -586,13 +591,16 @@ gpu::SyncToken SkiaOutputSurfaceImpl::SubmitPaint(
       post_task_timestamp = base::TimeTicks::Now();
     }
 
-    auto closure =
+    auto task =
         base::BindOnce(&SkiaOutputSurfaceImplOnGpu::FinishPaintRenderPass,
                        base::Unretained(impl_on_gpu_.get()),
                        post_task_timestamp, current_paint_->render_pass_id(),
                        std::move(ddl), std::move(images_in_current_paint_),
                        resource_sync_tokens_, sync_fence_release_);
-    ScheduleGpuTask(std::move(closure), std::move(resource_sync_tokens_));
+    EnqueueGpuTask(std::move(task), std::move(resource_sync_tokens_),
+                   /*make_current=*/true, /*need_framebuffer=*/false);
+    // Maybe not flush here to avoid an extra MakeCurrent() call.
+    FlushGpuTasks(/*wait_for_finish=*/false);
   } else {
     // Draw on the root render pass.
     current_buffer_modified_ = true;
@@ -605,12 +613,14 @@ gpu::SyncToken SkiaOutputSurfaceImpl::SubmitPaint(
       overdraw_surface_recorder_.reset();
     }
 
-    deferred_framebuffer_draw_closure_ = base::BindOnce(
+    auto task = base::BindOnce(
         &SkiaOutputSurfaceImplOnGpu::FinishPaintCurrentFrame,
         base::Unretained(impl_on_gpu_.get()), std::move(ddl),
         std::move(overdraw_ddl), std::move(images_in_current_paint_),
         resource_sync_tokens_, sync_fence_release_, std::move(on_finished),
         draw_rectangle_);
+    EnqueueGpuTask(std::move(task), std::move(resource_sync_tokens_),
+                   /*make_current=*/true, /*need_framebuffer=*/true);
     draw_rectangle_.reset();
   }
   images_in_current_paint_.clear();
@@ -677,7 +687,8 @@ void SkiaOutputSurfaceImpl::RemoveRenderPassResource(
       base::BindOnce(&SkiaOutputSurfaceImplOnGpu::RemoveRenderPassResource,
                      base::Unretained(impl_on_gpu_.get()), std::move(ids),
                      std::move(image_contexts));
-  ScheduleGpuTask(std::move(callback), {});
+  EnqueueGpuTask(std::move(callback), {}, /*make_current=*/false,
+                 /*need_framebuffer=*/false);
 }
 
 void SkiaOutputSurfaceImpl::CopyOutput(
@@ -686,22 +697,11 @@ void SkiaOutputSurfaceImpl::CopyOutput(
     const gfx::ColorSpace& color_space,
     std::unique_ptr<CopyOutputRequest> request) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-
-  // Defer CopyOutput for root render pass with draw framebuffer to
-  // SwapBuffers() or SwapBuffersSkipped().
-  if (!id) {
-    deferred_framebuffer_draw_closure_ = base::BindOnce(
-        &SkiaOutputSurfaceImplOnGpu::CopyOutput,
-        base::Unretained(impl_on_gpu_.get()), id, geometry, color_space,
-        std::move(request), std::move(deferred_framebuffer_draw_closure_));
-  } else {
-    DCHECK(!deferred_framebuffer_draw_closure_);
-    auto callback = base::BindOnce(
-        base::IgnoreResult(&SkiaOutputSurfaceImplOnGpu::CopyOutput),
-        base::Unretained(impl_on_gpu_.get()), id, geometry, color_space,
-        std::move(request), base::OnceCallback<bool()>());
-    ScheduleGpuTask(std::move(callback), std::move(resource_sync_tokens_));
-  }
+  auto callback = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::CopyOutput,
+                                 base::Unretained(impl_on_gpu_.get()), id,
+                                 geometry, color_space, std::move(request));
+  EnqueueGpuTask(std::move(callback), std::move(resource_sync_tokens_),
+                 /*make_current=*/true, /*need_framebuffer=*/!id);
 }
 
 void SkiaOutputSurfaceImpl::ScheduleOverlays(
@@ -711,7 +711,19 @@ void SkiaOutputSurfaceImpl::ScheduleOverlays(
       base::BindOnce(&SkiaOutputSurfaceImplOnGpu::ScheduleOverlays,
                      base::Unretained(impl_on_gpu_.get()), std::move(overlays),
                      std::move(images_in_current_paint_));
-  ScheduleGpuTask(std::move(task), std::move(sync_tokens));
+#if defined(OS_APPLE)
+  DCHECK_EQ(dependency_->gr_context_type(), gpu::GrContextType::kGL);
+  // If there are render pass overlays, then a gl context is needed for drawing
+  // the overlay render passes to a backing for being scanned out.
+  bool make_current = std::find_if(overlays.begin(), overlays.end(),
+                                   [](const CALayerOverlay& overlay) {
+                                     return !!overlay.ddl;
+                                   }) != overlays.end();
+#else
+  bool make_current = false;
+#endif
+  EnqueueGpuTask(std::move(task), std::move(sync_tokens), make_current,
+                 /*need_framebuffer=*/false);
   images_in_current_paint_.clear();
 }
 
@@ -724,7 +736,8 @@ gpu::MemoryTracker* SkiaOutputSurfaceImpl::GetMemoryTracker() {
 void SkiaOutputSurfaceImpl::SetFrameRate(float frame_rate) {
   auto task = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::SetFrameRate,
                              base::Unretained(impl_on_gpu_.get()), frame_rate);
-  ScheduleGpuTask(std::move(task), {});
+  EnqueueGpuTask(std::move(task), {}, /*make_current=*/false,
+                 /*need_framebuffer=*/false);
 }
 
 void SkiaOutputSurfaceImpl::SetCapabilitiesForTesting(
@@ -735,7 +748,8 @@ void SkiaOutputSurfaceImpl::SetCapabilitiesForTesting(
   auto callback =
       base::BindOnce(&SkiaOutputSurfaceImplOnGpu::SetCapabilitiesForTesting,
                      base::Unretained(impl_on_gpu_.get()), capabilities_);
-  ScheduleGpuTask(std::move(callback), {});
+  EnqueueGpuTask(std::move(callback), {}, /*make_current=*/true,
+                 /*need_framebuffer=*/false);
 }
 
 bool SkiaOutputSurfaceImpl::Initialize() {
@@ -746,12 +760,12 @@ bool SkiaOutputSurfaceImpl::Initialize() {
   // This runner could be called from vsync or GPU thread after |this| is
   // destroyed. We post directly to display compositor thread to check
   // |weak_ptr_| as |dependency_| may have been destroyed.
-  GpuVSyncCallback vsync_callback_runner =
 #if defined(OS_ANDROID)
-      // Callback is never used on Android. Doesn't work with WebView because
-      // calling it bypasses SkiaOutputSurfaceDependency.
-      base::DoNothing();
+  // Callback is never used on Android. Doesn't work with WebView because
+  // calling it bypasses SkiaOutputSurfaceDependency.
+  GpuVSyncCallback vsync_callback_runner = base::DoNothing();
 #else
+  GpuVSyncCallback vsync_callback_runner =
       base::BindRepeating(
           [](scoped_refptr<base::SingleThreadTaskRunner> runner,
              base::WeakPtr<SkiaOutputSurfaceImpl> weak_ptr,
@@ -763,13 +777,15 @@ bool SkiaOutputSurfaceImpl::Initialize() {
           base::ThreadTaskRunnerHandle::Get(), weak_ptr_);
 #endif
 
-  base::WaitableEvent event;
   bool result = false;
-  auto callback = base::BindOnce(&SkiaOutputSurfaceImpl::InitializeOnGpuThread,
-                                 base::Unretained(this), vsync_callback_runner,
-                                 &event, &result);
-  ScheduleGpuTask(std::move(callback), {});
-  event.Wait();
+  auto callback =
+      base::BindOnce(&SkiaOutputSurfaceImpl::InitializeOnGpuThread,
+                     base::Unretained(this), vsync_callback_runner, &result);
+  EnqueueGpuTask(std::move(callback), {}, /*make_current=*/false,
+                 /*need_framebuffer=*/false);
+  // |capabilities_| will be initialized in InitializeOnGpuThread(), so have to
+  // wait.
+  FlushGpuTasks(/*wait_for_finish=*/true);
 
   if (capabilities_.preserve_buffer_content &&
       capabilities_.supports_post_sub_buffer) {
@@ -791,14 +807,7 @@ bool SkiaOutputSurfaceImpl::Initialize() {
 
 void SkiaOutputSurfaceImpl::InitializeOnGpuThread(
     GpuVSyncCallback vsync_callback_runner,
-    base::WaitableEvent* event,
     bool* result) {
-  base::Optional<base::ScopedClosureRunner> scoped_runner;
-  if (event) {
-    scoped_runner.emplace(
-        base::BindOnce(&base::WaitableEvent::Signal, base::Unretained(event)));
-  }
-
   auto did_swap_buffer_complete_callback = base::BindRepeating(
       &SkiaOutputSurfaceImpl::DidSwapBuffersComplete, weak_ptr_);
   auto buffer_presented_callback =
@@ -959,20 +968,64 @@ void SkiaOutputSurfaceImpl::OnGpuVSync(base::TimeTicks timebase,
 void SkiaOutputSurfaceImpl::ScheduleGpuTaskForTesting(
     base::OnceClosure callback,
     std::vector<gpu::SyncToken> sync_tokens) {
-  ScheduleGpuTask(std::move(callback), std::move(sync_tokens));
+  EnqueueGpuTask(std::move(callback), std::move(sync_tokens),
+                 /*make_current=*/false, /*need_framebuffer=*/false);
+  FlushGpuTasks(/*wait_for_finish=*/false);
 }
 
-void SkiaOutputSurfaceImpl::ScheduleGpuTask(
-    base::OnceClosure callback,
-    std::vector<gpu::SyncToken> sync_tokens) {
-  auto wrapped_closure = base::BindOnce(
-      [](base::OnceClosure callback) {
+void SkiaOutputSurfaceImpl::EnqueueGpuTask(
+    GpuTask task,
+    std::vector<gpu::SyncToken> sync_tokens,
+    bool make_current,
+    bool need_framebuffer) {
+  gpu_tasks_.push_back(std::move(task));
+  std::move(sync_tokens.begin(), sync_tokens.end(),
+            std::back_inserter(gpu_task_sync_tokens_));
+
+  // Set |make_current_|, so MakeCurrent() will be called before executing all
+  // enqueued GPU tasks.
+  make_current_ |= make_current;
+  need_framebuffer_ |= need_framebuffer;
+}
+
+void SkiaOutputSurfaceImpl::FlushGpuTasks(bool wait_for_finish) {
+  // If |wait_for_finish| is true, a GPU task will be always scheduled to make
+  // sure all pending tasks are finished on the GPU thread.
+  if (gpu_tasks_.empty() && !wait_for_finish)
+    return;
+
+  auto event =
+      wait_for_finish ? std::make_unique<base::WaitableEvent>() : nullptr;
+
+  auto callback = base::BindOnce(
+      [](std::vector<GpuTask> tasks, base::WaitableEvent* event,
+         SkiaOutputSurfaceImplOnGpu* impl_on_gpu, bool need_framebuffer) {
         gpu::ContextUrl::SetActiveUrl(GetActiveUrl());
-        std::move(callback).Run();
+        // MakeCurrent() will mark context lost in SkiaOutputSurfaceImplOnGpu,
+        // if it fails.
+        if (impl_on_gpu)
+          impl_on_gpu->MakeCurrent(need_framebuffer);
+        // Each task can check SkiaOutputSurfaceImplOnGpu::contest_is_lost_
+        // to detect errors.
+        for (auto& task : tasks) {
+          std::move(task).Run();
+        }
+        if (event)
+          event->Signal();
       },
-      std::move(callback));
-  gpu_task_scheduler_->ScheduleGpuTask(std::move(wrapped_closure),
-                                       std::move(sync_tokens));
+      std::move(gpu_tasks_), event.get(),
+      make_current_ ? impl_on_gpu_.get() : nullptr, need_framebuffer_);
+
+  gpu_task_scheduler_->ScheduleGpuTask(std::move(callback),
+                                       std::move(gpu_task_sync_tokens_));
+
+  make_current_ = false;
+  need_framebuffer_ = false;
+  gpu_task_sync_tokens_.clear();
+  gpu_tasks_.clear();
+
+  if (event)
+    event->Wait();
 }
 
 GrBackendFormat SkiaOutputSurfaceImpl::GetGrBackendFormatForTexture(
@@ -1122,4 +1175,5 @@ gfx::Rect SkiaOutputSurfaceImpl::GetCurrentFramebufferDamage() const {
 void SkiaOutputSurfaceImpl::SetNeedsMeasureNextDrawLatency() {
   should_measure_next_post_task_ = true;
 }
+
 }  // namespace viz

components/viz/service/display_embedder/skia_output_surface_impl.h

@@ -26,10 +26,6 @@
 #include "third_party/skia/include/core/SkSurfaceCharacterization.h"
 #include "third_party/skia/include/core/SkYUVAIndex.h"
 
-namespace base {
-class WaitableEvent;
-}
-
 namespace viz {
 
 class ImageContextImpl;
@@ -164,7 +160,6 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
  private:
   bool Initialize();
   void InitializeOnGpuThread(GpuVSyncCallback vsync_callback_runner,
-                             base::WaitableEvent* event,
                              bool* result);
   SkSurfaceCharacterization CreateSkSurfaceCharacterization(
       const gfx::Size& surface_size,
@@ -179,8 +174,12 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
   // Provided as a callback for the GPU thread.
   void OnGpuVSync(base::TimeTicks timebase, base::TimeDelta interval);
 
-  void ScheduleGpuTask(base::OnceClosure callback,
-                       std::vector<gpu::SyncToken> sync_tokens);
+  using GpuTask = base::OnceClosure;
+  void EnqueueGpuTask(GpuTask task,
+                      std::vector<gpu::SyncToken> sync_tokens,
+                      bool make_current,
+                      bool need_framebuffer);
+  void FlushGpuTasks(bool wait_for_finish);
   GrBackendFormat GetGrBackendFormatForTexture(
       ResourceFormat resource_format,
       uint32_t gl_texture_target,
@@ -292,9 +291,14 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
 
   bool should_measure_next_post_task_ = false;
 
-  // We defer the draw to the framebuffer until SwapBuffers or CopyOutput
-  // to avoid the expense of posting a task and calling MakeCurrent.
-  base::OnceCallback<bool()> deferred_framebuffer_draw_closure_;
+  // GPU tasks pending for flush.
+  std::vector<GpuTask> gpu_tasks_;
+  // GPU sync tokens which are depended by |gpu_tasks_|.
+  std::vector<gpu::SyncToken> gpu_task_sync_tokens_;
+  // True if _any_ of |gpu_tasks_| need a GL context.
+  bool make_current_ = false;
+  // True if _any_ of |gpu_tasks_| need to access the framebuffer.
+  bool need_framebuffer_ = false;
 
   bool use_damage_area_from_skia_output_device_ = false;
   // Damage area of the current buffer. Differ to the last submit buffer.

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -443,7 +443,7 @@ SkiaOutputSurfaceImplOnGpu::~SkiaOutputSurfaceImplOnGpu() {
 
     // |context_provider_| and clients want either the context to be lost or
     // made current on destruction.
-    if (MakeCurrent(false /* need_fbo0 */)) {
+    if (MakeCurrent(/*need_framebuffer=*/false)) {
       // This ensures any outstanding callbacks for promise images are
       // performed.
       gr_context()->flushAndSubmit();
@@ -464,7 +464,7 @@ void SkiaOutputSurfaceImplOnGpu::Reshape(const gfx::Size& size,
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(gr_context());
 
-  if (!MakeCurrent(!dependency_->IsOffscreen() /* need_fbo0 */))
+  if (context_is_lost_)
     return;
 
   size_ = size;
@@ -472,11 +472,10 @@ void SkiaOutputSurfaceImplOnGpu::Reshape(const gfx::Size& size,
   if (!output_device_->Reshape(size_, device_scale_factor, color_space, format,
                                transform)) {
     MarkContextLost(CONTEXT_LOST_RESHAPE_FAILED);
-    return;
   }
 }
 
-bool SkiaOutputSurfaceImplOnGpu::FinishPaintCurrentFrame(
+void SkiaOutputSurfaceImplOnGpu::FinishPaintCurrentFrame(
     sk_sp<SkDeferredDisplayList> ddl,
     sk_sp<SkDeferredDisplayList> overdraw_ddl,
     std::vector<ImageContextImpl*> image_contexts,
@@ -488,20 +487,19 @@ bool SkiaOutputSurfaceImplOnGpu::FinishPaintCurrentFrame(
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(!scoped_output_device_paint_);
 
-  bool need_fbo0 = gl_surface_ && !gl_surface_->IsSurfaceless();
-  if (!MakeCurrent(need_fbo0))
-    return false;
+  if (context_is_lost_)
+    return;
 
   if (!ddl) {
     MarkContextLost(CONTEXT_LOST_UNKNOWN);
-    return false;
+    return;
   }
 
   if (draw_rectangle) {
     if (!output_device_->SetDrawRectangle(*draw_rectangle)) {
       MarkContextLost(
           ContextLostReason::CONTEXT_LOST_SET_DRAW_RECTANGLE_FAILED);
-      return false;
+      return;
     }
   }
 
@@ -576,11 +574,10 @@ bool SkiaOutputSurfaceImplOnGpu::FinishPaintCurrentFrame(
         !(begin_semaphores.empty() && end_semaphores.empty())) {
       // TODO(penghuang): handle vulkan device lost.
       DLOG(ERROR) << "output_sk_surface()->flush() failed.";
-      return false;
+      return;
     }
   }
   ReleaseFenceSyncAndPushTextureUpdates(sync_fence_release);
-  return true;
 }
 
 void SkiaOutputSurfaceImplOnGpu::ScheduleOutputSurfaceAsOverlay(
@@ -592,8 +589,7 @@ void SkiaOutputSurfaceImplOnGpu::ScheduleOutputSurfaceAsOverlay(
 
 void SkiaOutputSurfaceImplOnGpu::SwapBuffers(
     base::TimeTicks post_task_timestamp,
-    OutputSurfaceFrame frame,
-    base::OnceCallback<bool()> deferred_framebuffer_draw_closure) {
+    OutputSurfaceFrame frame) {
   TRACE_EVENT0("viz", "SkiaOutputSurfaceImplOnGpu::SwapBuffers");
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
@@ -601,13 +597,12 @@ void SkiaOutputSurfaceImplOnGpu::SwapBuffers(
     output_device_->SetDrawTimings(post_task_timestamp, base::TimeTicks::Now());
   }
 
-  SwapBuffersInternal(std::move(deferred_framebuffer_draw_closure), &frame);
+  SwapBuffersInternal(&frame);
 }
 
-void SkiaOutputSurfaceImplOnGpu::SwapBuffersSkipped(
-    base::OnceCallback<bool()> deferred_framebuffer_draw_closure) {
+void SkiaOutputSurfaceImplOnGpu::SwapBuffersSkipped() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  SwapBuffersInternal(std::move(deferred_framebuffer_draw_closure));
+  SwapBuffersInternal();
 }
 
 void SkiaOutputSurfaceImplOnGpu::FinishPaintRenderPass(
@@ -625,7 +620,7 @@ void SkiaOutputSurfaceImplOnGpu::FinishPaintRenderPass(
     output_device_->SetDrawTimings(post_task_timestamp, base::TimeTicks::Now());
   }
 
-  if (!MakeCurrent(false /* need_fbo0 */))
+  if (context_is_lost_)
     return;
 
   if (!ddl) {
@@ -707,38 +702,31 @@ static void PostTaskFromMainToImplThread(
       FROM_HERE, base::BindOnce(std::move(callback), sync_token, is_lost));
 }
 
-bool SkiaOutputSurfaceImplOnGpu::CopyOutput(
+void SkiaOutputSurfaceImplOnGpu::CopyOutput(
     AggregatedRenderPassId id,
     copy_output::RenderPassGeometry geometry,
     const gfx::ColorSpace& color_space,
-    std::unique_ptr<CopyOutputRequest> request,
-    base::OnceCallback<bool()> deferred_framebuffer_draw_closure) {
+    std::unique_ptr<CopyOutputRequest> request) {
   TRACE_EVENT0("viz", "SkiaOutputSurfaceImplOnGpu::CopyOutput");
   // TODO(crbug.com/898595): Do this on the GPU instead of CPU with Vulkan.
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
-  if (deferred_framebuffer_draw_closure) {
-    // returns false if context not set to current, i.e lost
-    if (!std::move(deferred_framebuffer_draw_closure).Run())
-      return false;
-    DCHECK(context_state_->IsCurrent(nullptr /* surface */));
-  } else {
-    if (!MakeCurrent(true /* need_fbo0 */))
-      return false;
-  }
+  if (context_is_lost_)
+    return;
 
-  bool from_fbo0 = !id;
-  DCHECK(scoped_output_device_paint_ || !from_fbo0);
+  bool from_framebuffer = !id;
+  DCHECK(scoped_output_device_paint_ || !from_framebuffer);
 
-  DCHECK(from_fbo0 ||
+  DCHECK(from_framebuffer ||
          offscreen_surfaces_.find(id) != offscreen_surfaces_.end());
-  auto* surface =
-      from_fbo0 ? output_sk_surface() : offscreen_surfaces_[id].surface();
+  auto* surface = from_framebuffer ? output_sk_surface()
+                                   : offscreen_surfaces_[id].surface();
 
   // If a platform doesn't support RGBX_8888 format, we will use RGBA_8888
   // instead. In this case, we need discard alpha channel (modify the alpha
   // value to 0xff, but keep other channel not changed).
-  bool need_discard_alpha = from_fbo0 && (output_device_->is_emulated_rgbx());
+  bool need_discard_alpha =
+      from_framebuffer && (output_device_->is_emulated_rgbx());
   if (need_discard_alpha) {
     base::Optional<gpu::raster::GrShaderCache::ScopedCacheUse> cache_use;
     if (dependency_->GetGrShaderCache()) {
@@ -845,7 +833,7 @@ bool SkiaOutputSurfaceImplOnGpu::CopyOutput(
         gpu::kNullSurfaceHandle, kUsage);
     if (!result) {
       DLOG(ERROR) << "Failed to create shared image.";
-      return false;
+      return;
     }
 
     auto representation = dependency_->GetSharedImageManager()->ProduceSkia(
@@ -888,7 +876,7 @@ bool SkiaOutputSurfaceImplOnGpu::CopyOutput(
         !(begin_semaphores.empty() && end_semaphores.empty())) {
       // TODO(penghuang): handle vulkan device lost.
       DLOG(ERROR) << "dest_surface->flush() failed.";
-      return false;
+      return;
     }
     auto release_callback = base::BindOnce(
         &SkiaOutputSurfaceImplOnGpu::DestroySharedImageOnImplThread,
@@ -904,7 +892,6 @@ bool SkiaOutputSurfaceImplOnGpu::CopyOutput(
     NOTREACHED();
   }
   ScheduleCheckReadbackCompletion();
-  return true;
 }
 
 void SkiaOutputSurfaceImplOnGpu::DestroySharedImageOnImplThread(
@@ -978,7 +965,7 @@ void SkiaOutputSurfaceImplOnGpu::ReleaseImageContexts(
   DCHECK(!image_contexts.empty());
   // The window could be destroyed already, and the MakeCurrent will fail with
   // an destroyed window, so MakeCurrent without requiring the fbo0.
-  if (!MakeCurrent(false /* need_fbo0 */)) {
+  if (context_is_lost_) {
     for (const auto& context : image_contexts)
       context->OnContextLost();
   }
@@ -991,20 +978,7 @@ void SkiaOutputSurfaceImplOnGpu::ScheduleOverlays(
     SkiaOutputSurface::OverlayList overlays,
     std::vector<ImageContextImpl*> image_contexts) {
 #if defined(OS_APPLE)
-  DCHECK(context_state_->GrContextIsGL());
-
-  // If |image_contexts| is not empty, it means there is at least one render
-  // pass overlays
-  bool has_render_pass_overlays =
-      !image_contexts.empty() ||
-      std::find_if(overlays.begin(), overlays.end(),
-                   [](const CALayerOverlay& overlay) {
-                     return !!overlay.ddl;
-                   }) != overlays.end();
-
-  // If there are render pass overlays, then a gl context is needed for drawing
-  // the overlay render passes to a backing for being scanned out.
-  if (has_render_pass_overlays && !MakeCurrent(false /* need_fbo0 */))
+  if (context_is_lost_)
     return;
 
   std::vector<GrBackendSemaphore> begin_semaphores;
@@ -1056,7 +1030,7 @@ void SkiaOutputSurfaceImplOnGpu::ScheduleOverlays(
 }
 
 void SkiaOutputSurfaceImplOnGpu::SetEnableDCLayers(bool enable) {
-  if (!MakeCurrent(false /* need_fbo0 */))
+  if (context_is_lost_)
     return;
   output_device_->SetEnableDCLayers(enable);
 }
@@ -1072,7 +1046,6 @@ void SkiaOutputSurfaceImplOnGpu::SetFrameRate(float frame_rate) {
 
 void SkiaOutputSurfaceImplOnGpu::SetCapabilitiesForTesting(
     const OutputSurface::Capabilities& capabilities) {
-  MakeCurrent(false /* need_fbo0 */);
   // Check that we're using an offscreen surface.
   DCHECK(dependency_->IsOffscreen());
   output_device_ = std::make_unique<SkiaOutputDeviceOffscreen>(
@@ -1145,7 +1118,7 @@ bool SkiaOutputSurfaceImplOnGpu::InitializeForGL() {
     if (!gl_surface_)
       return false;
 
-    if (MakeCurrent(true /* need_fbo0 */)) {
+    if (MakeCurrent(/*need_framebuffer=*/true)) {
       if (gl_surface_->IsSurfaceless()) {
 #if !defined(OS_WIN)
         output_device_ = std::make_unique<SkiaOutputDeviceBufferQueue>(
@@ -1289,9 +1262,15 @@ bool SkiaOutputSurfaceImplOnGpu::InitializeForDawn() {
   return true;
 }
 
-bool SkiaOutputSurfaceImplOnGpu::MakeCurrent(bool need_fbo0) {
+bool SkiaOutputSurfaceImplOnGpu::MakeCurrent(bool need_framebuffer) {
+  // If GL is not being used or GLSurface is not surfaceless, we can ignore
+  // making current the GLSurface for better performance.
+  bool need_fbo0 = need_framebuffer && context_state_->GrContextIsGL() &&
+                   gl_surface_ && !gl_surface_->IsSurfaceless();
+
   // need_fbo0 implies need_gl too.
   bool need_gl = need_fbo0;
+
   // Only make current with |gl_surface_|, if following operations will use
   // fbo0.
   auto* gl_surface = need_fbo0 ? gl_surface_.get() : nullptr;
@@ -1304,6 +1283,13 @@ bool SkiaOutputSurfaceImplOnGpu::MakeCurrent(bool need_fbo0) {
         gpu::error::kLostContext, *context_state_->context_lost_reason()));
     return false;
   }
+
+  // Some GLSurface implements OnMakeCurrent() to tracing current GLContext,
+  // even if framebuffer is not needed, we still call OnMakeCurrent() so
+  // GLSurface implementation will know the current GLContext.
+  if (gl_surface_ && !need_fbo0)
+    gl_surface_->OnMakeCurrent(context_state_->context());
+
   context_state_->set_need_context_state_reset(true);
   return true;
 }
@@ -1342,20 +1328,12 @@ void SkiaOutputSurfaceImplOnGpu::ReleaseFenceSyncAndPushTextureUpdates(
 }
 
 void SkiaOutputSurfaceImplOnGpu::SwapBuffersInternal(
-    base::OnceCallback<bool()> deferred_framebuffer_draw_closure,
     OutputSurfaceFrame* frame) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(output_device_);
 
-  if (deferred_framebuffer_draw_closure) {
-    // Returns false if context not set to current, i.e lost
-    if (!std::move(deferred_framebuffer_draw_closure).Run())
-      return;
-    DCHECK(context_state_->IsCurrent(nullptr /* surface */));
-  } else {
-    if (!MakeCurrent(!dependency_->IsOffscreen() /* need_fbo0 */))
-      return;
-  }
+  if (context_is_lost_)
+    return;
 
   ResetStateOfImages();
   output_device_->PreGrContextSubmit();
@@ -1519,7 +1497,7 @@ void SkiaOutputSurfaceImplOnGpu::CheckReadbackCompletion() {
 
   // If there are no pending readback requests or we can't make the context
   // current then exit. There is no thing to do here.
-  if (num_readbacks_pending_ == 0 || !MakeCurrent(/*need_fbo0=*/false))
+  if (num_readbacks_pending_ == 0 || !MakeCurrent(/*need_framebuffer=*/false))
     return;
 
   gr_context()->checkAsyncWorkCompletion();

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -119,7 +119,7 @@ class SkiaOutputSurfaceImplOnGpu
                gfx::BufferFormat format,
                bool use_stencil,
                gfx::OverlayTransform transform);
-  bool FinishPaintCurrentFrame(sk_sp<SkDeferredDisplayList> ddl,
+  void FinishPaintCurrentFrame(sk_sp<SkDeferredDisplayList> ddl,
                                sk_sp<SkDeferredDisplayList> overdraw_ddl,
                                std::vector<ImageContextImpl*> image_contexts,
                                std::vector<gpu::SyncToken> sync_tokens,
@@ -129,14 +129,11 @@ class SkiaOutputSurfaceImplOnGpu
   void ScheduleOutputSurfaceAsOverlay(
       const OverlayProcessorInterface::OutputSurfaceOverlayPlane&
           output_surface_plane);
-  void SwapBuffers(
-      base::TimeTicks post_task_timestamp,
-      OutputSurfaceFrame frame,
-      base::OnceCallback<bool()> deferred_framebuffer_draw_closure);
+  void SwapBuffers(base::TimeTicks post_task_timestamp,
+                   OutputSurfaceFrame frame);
   // Runs |deferred_framebuffer_draw_closure| when SwapBuffers() or CopyOutput()
   // will not.
-  void SwapBuffersSkipped(
-      base::OnceCallback<bool()> deferred_framebuffer_draw_closure);
+  void SwapBuffersSkipped();
   void EnsureBackbuffer() { output_device_->EnsureBackbuffer(); }
   void DiscardBackbuffer() { output_device_->DiscardBackbuffer(); }
   void FinishPaintRenderPass(base::TimeTicks post_task_timestamp,
@@ -150,11 +147,10 @@ class SkiaOutputSurfaceImplOnGpu
   void RemoveRenderPassResource(
       std::vector<AggregatedRenderPassId> ids,
       std::vector<std::unique_ptr<ImageContextImpl>> image_contexts);
-  bool CopyOutput(AggregatedRenderPassId id,
+  void CopyOutput(AggregatedRenderPassId id,
                   copy_output::RenderPassGeometry geometry,
                   const gfx::ColorSpace& color_space,
-                  std::unique_ptr<CopyOutputRequest> request,
-                  base::OnceCallback<bool()> deferred_framebuffer_draw_closure);
+                  std::unique_ptr<CopyOutputRequest> request);
 
   void BeginAccessImages(const std::vector<ImageContextImpl*>& image_contexts,
                          std::vector<GrBackendSemaphore>* begin_semaphores,
@@ -209,6 +205,10 @@ class SkiaOutputSurfaceImplOnGpu
 
   gpu::MemoryTracker* GetMemoryTracker() { return memory_tracker_; }
 
+  // Make context current for GL, and return false if the context is lost.
+  // It will do nothing when Vulkan is used.
+  bool MakeCurrent(bool need_framebuffer);
+
  private:
   class OffscreenSurface;
   class DisplayContext;
@@ -224,9 +224,6 @@ class SkiaOutputSurfaceImplOnGpu
 
   DidSwapBufferCompleteCallback GetDidSwapBuffersCompleteCallback();
 
-  // Make context current for GL, and return false if the context is lost.
-  // It will do nothing when Vulkan is used.
-  bool MakeCurrent(bool need_fbo0);
   void MarkContextLost(ContextLostReason reason);
 
   void DestroySharedImageOnImplThread(
@@ -239,9 +236,7 @@ class SkiaOutputSurfaceImplOnGpu
 
   void ReleaseFenceSyncAndPushTextureUpdates(uint64_t sync_fence_release);
 
-  void SwapBuffersInternal(
-      base::OnceCallback<bool()> deferred_framebuffer_draw_closure,
-      OutputSurfaceFrame* frame = nullptr);
+  void SwapBuffersInternal(OutputSurfaceFrame* frame = nullptr);
 
   GrDirectContext* gr_context() { return context_state_->gr_context(); }