cc: Implement overdraw feedback debugging feature.

cc/output/gl_renderer.cc

Index: cc/output/gl_renderer.cc
diff --git a/cc/output/gl_renderer.cc b/cc/output/gl_renderer.cc
index c1937fd9d314f3883b208b7239ad4e06d802166c..b576cdcf13fa677ba900c78633af7f2ffa56e3c5 100644
--- a/cc/output/gl_renderer.cc
+++ b/cc/output/gl_renderer.cc
@@ -14,6 +14,7 @@
 #include <string>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -370,6 +371,16 @@ class GLRenderer::SyncQuery {
   DISALLOW_COPY_AND_ASSIGN(SyncQuery);
 };
 
+struct GLRenderer::PendingOverdrawQuery {
+  explicit PendingOverdrawQuery(unsigned query) : query(query) {}
+
+  base::CancelableClosure overdraw_callback;

[danakj, 2017/01/12 00:42:58]

this makes a weakptrfactory for each query, is that overkill?

[reveman, 2017/01/13 01:27:43]

This code has been removed from latest patch.

+  const unsigned query;

[danakj, 2017/01/12 00:42:57]

query is a GLuint, can you use that instead of "unsigned" throughout?

[reveman, 2017/01/13 01:27:42]

This code has been removed from latest patch but unsigned instead of GLuint
seems very common in GLRenderer so I used that for consistency.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PendingOverdrawQuery);
+};
+
 GLRenderer::GLRenderer(const RendererSettings* settings,
                        OutputSurface* output_surface,
                        ResourceProvider* resource_provider,
@@ -415,6 +426,13 @@ GLRenderer::~GLRenderer() {
     pending_async_read_pixels_.pop_back();
   }
 
+  while (!pending_overdraw_queries_.empty()) {
+    PendingOverdrawQuery* pending_query =
+        pending_overdraw_queries_.back().get();
+    pending_query->overdraw_callback.Cancel();

[danakj, 2017/01/12 00:42:58]

destroying a WeakPtrFactory (inside the CancelableCallback) invalidates the
weakptr, no need to Cancel. I guess I did not know these things when I wrote
pending_async_read_pixels_ stuff.

[reveman, 2017/01/13 01:27:42]

This code has been removed from latest patch.

+    pending_overdraw_queries_.pop_back();
+  }
+
   CleanupSharedObjects();
 
   if (context_visibility_) {
@@ -495,7 +513,9 @@ void GLRenderer::ClearFramebuffer(DrawingFrame* frame) {
   else
     gl_->ClearColor(0, 0, 1, 1);
 
-  bool always_clear = false;
+  gl_->ClearStencil(0);

[danakj, 2017/01/12 00:42:58]

Does this add cost when not using overdraw feedback?

[reveman, 2017/01/13 01:27:42]

Pretty sure it doesn't and doing this unconditionally is consistent with doing
the ClearColor above even if we're not going to do a clear below.

+
+  bool always_clear = overdraw_feedback_;
 #ifndef NDEBUG
   always_clear = true;
 #endif
@@ -2764,8 +2784,11 @@ void GLRenderer::FinishDrawingFrame(DrawingFrame* frame) {
     pending_sync_queries_.push_back(std::move(current_sync_query_));
   }
 
-  current_framebuffer_lock_ = nullptr;
   swap_buffer_rect_.Union(frame->root_damage_rect);
+  if (overdraw_feedback_)
+    FlushOverdrawFeedback(frame, swap_buffer_rect_);
+
+  current_framebuffer_lock_ = nullptr;
 
   gl_->Disable(GL_BLEND);
   blend_shadow_ = false;
@@ -3003,6 +3026,9 @@ void GLRenderer::GetFramebufferPixelsAsync(
   if (rect.IsEmpty())
     return;
 
+  if (overdraw_feedback_)

[danakj, 2017/01/12 00:42:58]

how come you do this into readbacks?

[reveman, 2017/01/13 01:27:42]

For screenshots. That's very important for debugging purposes.

[danakj, 2017/01/16 16:02:57]

Do you need it in screenshots of non-root render surfaces? Just noting that
non-root stuff seems like a good 50% of the code in here..

[reveman, 2017/01/16 18:02:26]

This is needed for all types of screenshots on ChromeOS as discussed.

+    FlushOverdrawFeedback(frame, rect);
+
   gfx::Rect window_rect = MoveFromDrawToWindowSpace(frame, rect);
   DCHECK_GE(window_rect.x(), 0);
   DCHECK_GE(window_rect.y(), 0);
@@ -3203,7 +3229,10 @@ void GLRenderer::BindFramebufferToOutputSurface(DrawingFrame* frame) {
   current_framebuffer_lock_ = nullptr;
   output_surface_->BindFramebuffer();
 
-  if (output_surface_->HasExternalStencilTest()) {
+  if (overdraw_feedback_) {
+    SetupOverdrawFeedback();
+    SetStencilEnabled(true);
+  } else if (output_surface_->HasExternalStencilTest()) {

[danakj, 2017/01/12 00:42:58]

using else here means turning on overdraw feedback does bad things to webview,
which is okay I guess, is this what you intended?

[reveman, 2017/01/13 01:27:42]

I'm expecting an output surface that might have an external stencil test to
never report that stencil operations are available. overdraw_feedback_ should in
that case never be true. Added a DCHECK here to verify that.

     output_surface_->ApplyExternalStencil();
     SetStencilEnabled(true);
   } else {
@@ -3219,7 +3248,6 @@ bool GLRenderer::BindFramebufferToTexture(DrawingFrame* frame,
   // same texture again.
   current_framebuffer_lock_ = nullptr;
 
-  SetStencilEnabled(false);
   gl_->BindFramebuffer(GL_FRAMEBUFFER, offscreen_framebuffer_id_);
   current_framebuffer_lock_ =
       base::MakeUnique<ResourceProvider::ScopedWriteLockGL>(
@@ -3228,10 +3256,33 @@ bool GLRenderer::BindFramebufferToTexture(DrawingFrame* frame,
   unsigned texture_id = current_framebuffer_lock_->texture_id();
   gl_->FramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D,
                             texture_id, 0);
+  if (overdraw_feedback_) {
+    if (!offscreen_stencil_renderbuffer_id_)
+      gl_->GenRenderbuffers(1, &offscreen_stencil_renderbuffer_id_);
+    if (texture->size() != offscreen_stencil_renderbuffer_size_) {
+      gl_->BindRenderbuffer(GL_RENDERBUFFER,
+                            offscreen_stencil_renderbuffer_id_);
+      gl_->RenderbufferStorage(GL_RENDERBUFFER, GL_STENCIL_INDEX8,
+                               texture->size().width(),
+                               texture->size().height());
+      gl_->BindRenderbuffer(GL_RENDERBUFFER, 0);
+      offscreen_stencil_renderbuffer_size_ = texture->size();
+    }
+    gl_->FramebufferRenderbuffer(GL_FRAMEBUFFER, GL_STENCIL_ATTACHMENT,
+                                 GL_RENDERBUFFER,
+                                 offscreen_stencil_renderbuffer_id_);
+  }
 
   DCHECK(gl_->CheckFramebufferStatus(GL_FRAMEBUFFER) ==
              GL_FRAMEBUFFER_COMPLETE ||
          IsContextLost());
+
+  if (overdraw_feedback_) {

[danakj, 2017/01/12 00:42:58]

Hm why do you do stencil and overdraw stuff for child render passes, is this
just for copy requests? What's the use case for that?

[reveman, 2017/01/13 01:27:42]

Yes, this is for copy requests.

+    SetupOverdrawFeedback();
+    SetStencilEnabled(true);
+  } else {
+    SetStencilEnabled(false);
+  }
   return true;
 }
 
@@ -3733,6 +3784,9 @@ void GLRenderer::CleanupSharedObjects() {
   if (offscreen_framebuffer_id_)
     gl_->DeleteFramebuffers(1, &offscreen_framebuffer_id_);
 
+  if (offscreen_stencil_renderbuffer_id_)
+    gl_->DeleteRenderbuffers(1, &offscreen_stencil_renderbuffer_id_);
+
   ReleaseRenderPassTextures();
 }
 
@@ -4065,4 +4119,139 @@ void GLRenderer::ScheduleRenderPassDrawQuad(
       ca_layer_overlay->edge_aa_mask, bounds_rect, filter);
 }
 
+void GLRenderer::SetupOverdrawFeedback() {
+  gl_->StencilFunc(GL_ALWAYS, 1, 0xffffffff);
+  // First two values are ignored as test always passes.
+  gl_->StencilOp(GL_KEEP, GL_KEEP, GL_INCR);
+  gl_->StencilMask(0xff);
+}
+
+void GLRenderer::FlushOverdrawFeedback(const DrawingFrame* frame,
+                                       const gfx::Rect& output_rect) {
+  // Return early if already flushed.

[danakj, 2017/01/12 00:42:58]

I don't understand how we can flush twice for a single renderpass/frame? Maybe
DCHECK or can you explain to me?

[reveman, 2017/01/13 01:27:42]

Good point. Now a DCHECK.

+  if (!stencil_shadow_)
+    return;
+
+  // Test only, keep everything.
+  gl_->StencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
+  gl_->StencilMask(0);
+
+  EnsureScissorTestDisabled();
+  SetBlendEnabled(true);
+
+  PrepareGeometry(SHARED_BINDING);
+
+  const DebugBorderProgram* program = GetDebugBorderProgram();
+  DCHECK(program && (program->initialized() || IsContextLost()));
+  SetUseProgram(program->program());
+
+  gfx::Transform render_matrix;
+  render_matrix.Translate(0.5 * output_rect.width() + output_rect.x(),
+                          0.5 * output_rect.height() + output_rect.y());
+  render_matrix.Scale(output_rect.width(), output_rect.height());
+  static float gl_matrix[16];
+  GLRenderer::ToGLMatrix(&gl_matrix[0],
+                         frame->projection_matrix * render_matrix);
+  gl_->UniformMatrix4fv(program->vertex_shader().matrix_location(), 1, false,
+                        &gl_matrix[0]);
+
+  // Produce hinting for the amount of overdraw on screen for each pixel by
+  // drawing hint colors to the framebuffer based on the current stencil value.
+  struct {
+    int category;
+    GLenum func;
+    GLint ref;
+    SkColor color;
+  } stencil_tests[] = {
+      {1, GL_EQUAL, 2, 0x2f0000ff},  // Blue: Overdrawn once.
+      {2, GL_EQUAL, 3, 0x2f00ff00},  // Green: Overdrawn twice.
+      {3, GL_EQUAL, 4, 0x3fff0000},  // Pink: Overdrawn three times.
+      {4, GL_LESS, 4, 0x7fff0000},   // Red: Overdrawn four or more times.
+  };
+
+  // Occlusion queries can be expensive, so only collect trace data if we select
+  // cc.debug.overdraw.
+  bool tracing_enabled;
+  TRACE_EVENT_CATEGORY_GROUP_ENABLED(
+      TRACE_DISABLED_BY_DEFAULT("cc.debug.overdraw"), &tracing_enabled);
+
+  // Trace only the root render pass.
+  if (frame->current_render_pass != frame->root_render_pass)
+    tracing_enabled = false;
+
+  base::Closure barrier = base::BarrierClosure(
+      arraysize(stencil_tests),
+      base::Bind(&GLRenderer::UpdateOverdrawCounter, base::Unretained(this)));
+
+  for (const auto& test : stencil_tests) {
+    if (tracing_enabled) {
+      GLuint query = 0;
+      gl_->GenQueriesEXT(1, &query);
+      // TODO(reveman): Use SAMPLES_PASSED_ARB when available for exact amount
+      // of overdraw.
+      gl_->BeginQueryEXT(GL_ANY_SAMPLES_PASSED_EXT, query);

[danakj, 2017/01/12 00:42:57]

would it not work to do one query around the whole loop? what would that do?

[reveman, 2017/01/13 01:27:43]

We want samples for each overdraw category and with SAMPLES_PASSED_ARB even the
exact number of samples for each category. One query outside the loop would just
give us samples for all categories.

+
+      std::unique_ptr<PendingOverdrawQuery> pending_overdraw_query(
+          new PendingOverdrawQuery(query));
+      pending_overdraw_queries_.push_back(std::move(pending_overdraw_query));
+    }
+
+    gl_->StencilFunc(test.func, test.ref, 0xffffffff);
+    // Transparent color unless color-coding of overdraw is enabled.
+    Float4 color =
+        PremultipliedColor(settings_->show_overdraw_feedback ? test.color : 0);
+    gl_->Uniform4fv(program->fragment_shader().color_location(), 1, color.data);
+    gl_->DrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_SHORT, 0);
+
+    if (tracing_enabled) {
+      base::Closure overdraw_feedback_callback = base::Bind(
+          &GLRenderer::OverdrawFeedback, base::Unretained(this),
+          pending_overdraw_queries_.back()->query, test.category, barrier);
+
+      // Save the overdraw_feedback_callback so it can be cancelled.

[danakj, 2017/01/12 00:42:57]

If you just put a WeakPtrFactory on GLRenderer and post with that as the |this|
instead of Unretained, then you don't need to save the closure at all.

[reveman, 2017/01/13 01:27:42]

Done but updated to be less complicated as a result of removing
PendingOverdrawQuery.

+      pending_overdraw_queries_.back()->overdraw_callback.Reset(
+          overdraw_feedback_callback);
+      base::Closure cancelable_callback =
+          pending_overdraw_queries_.back()->overdraw_callback.callback();
+
+      gl_->EndQueryEXT(GL_ANY_SAMPLES_PASSED_EXT);
+      context_support_->SignalQuery(pending_overdraw_queries_.back()->query,
+                                    cancelable_callback);
+    }
+  }
+
+  gl_->StencilFunc(GL_ALWAYS, 0, 0xffffffff);
+  gl_->Disable(GL_STENCIL_TEST);
+  stencil_shadow_ = false;

[danakj, 2017/01/12 00:42:58]

It feels weird that this method is changing this, call SetStencilEnabled
instead?

But also can you maybe do that at the caller instead of hiding it inside this
method, whose name does not sound like it will change stencil states?

And why do we do this at all, we did not disable stencil at the end of the frame
(or render pass) when it was enabled for webview?

[reveman, 2017/01/13 01:27:42]

Good point. Removed it.

+}
+
+void GLRenderer::OverdrawFeedback(unsigned query,
+                                  int category,
+                                  base::Closure barrier) {

[danakj, 2017/01/12 00:42:58]

const&

[reveman, 2017/01/13 01:27:42]

Done.

+  DCHECK(!pending_overdraw_queries_.empty());
+  DCHECK_EQ(pending_overdraw_queries_.front()->query, query);
+  pending_overdraw_queries_.pop_front();
+
+  unsigned result = 0;
+  if (query) {
+    gl_->GetQueryObjectuivEXT(query, GL_QUERY_RESULT_EXT, &result);
+    gl_->DeleteQueriesEXT(1, &query);
+  }
+
+  if (result)
+    current_overdraw_.insert(category);
+  else
+    current_overdraw_.erase(category);
+
+  barrier.Run();
+}
+
+void GLRenderer::UpdateOverdrawCounter() const {
+  // Report overdraw as multiple of the screen size. ie. 1x for the whole
+  // screen is 1.0. Note: this will always report the worst overdraw on screen
+  // as the overdraw for the whole screen until SAMPLES_PASSED_ARB is supported.
+  TRACE_COUNTER1(TRACE_DISABLED_BY_DEFAULT("cc.debug.overdraw"), "GPU Overdraw",
+                 current_overdraw_.empty() ? 0 : *current_overdraw_.rbegin());
+}
+
 }  // namespace cc