cc: Implement overdraw feedback debugging feature.

cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

The --ui-disable-partial-swap flag is also added to about:flags in
this patch. Disabling of partial swap is not required for overdraw
feedback but often what you want to make the output easier to diagnose.

BUG=678234
TEST=chrome --show-overdraw-feedback, cc_unittests --gtest_filter=GLRendererPixelTestWithOverdrawFeedback.TranslucentRectangles
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2612023002
Cr-Commit-Position: refs/heads/master@{#444256}
Committed: https://chromium.googlesource.com/chromium/src/+/9413cffb4fdc7dbcfc40a8da0530942e2f6ae0f2

[reveman, 2017-01-04 13:42:31]

Description was changed from

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

BUG=678234
TEST=chrome --overdraw-feedback
==========

to

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

BUG=678234
TEST=chrome --overdraw-feedback
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-01-09 11:07:24]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org

[Daniele Castagna, 2017-01-09 18:53:06]

Can you mentioned the tracing support in the CL description?

https://codereview.chromium.org/2612023002/diff/20001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/base/switches.cc#new...
cc/base/switches.cc:111: const char kOverdrawFeedback[] = "overdraw-feedback";
Shouldn't flag names be imperative? (i.e: show-overdraw-feedback.)

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3321: gl_->GenRenderbuffers(1,
&offscreen_stencil_renderbuffer_id_);
Why not if (overdraw_feedback_) ?

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:556: std::set<int> current_overdraw_;
Why don't you just use std::bitset here?

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:556: std::set<int> current_overdraw_;
Why don't you just use std::bitset here?

[reveman, 2017-01-10 23:38:03]

Description was changed from

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

BUG=678234
TEST=chrome --overdraw-feedback
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

BUG=678234
TEST=chrome --show-overdraw-feedback
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-01-10 23:52:01]

On 2017/01/09 at 18:53:06, dcastagna wrote:
> Can you mentioned the tracing support in the CL description?

Done.

https://codereview.chromium.org/2612023002/diff/20001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/base/switches.cc#new...
cc/base/switches.cc:111: const char kOverdrawFeedback[] = "overdraw-feedback";
On 2017/01/09 at 18:53:06, Daniele Castagna wrote:
> Shouldn't flag names be imperative? (i.e: show-overdraw-feedback.)

Done.

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3321: gl_->GenRenderbuffers(1,
&offscreen_stencil_renderbuffer_id_);
On 2017/01/09 at 18:53:06, Daniele Castagna wrote:
> Why not if (overdraw_feedback_) ?

Done. By deferring this until needed.

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:556: std::set<int> current_overdraw_;
On 2017/01/09 at 18:53:06, Daniele Castagna wrote:
> Why don't you just use std::bitset here?

That requires knowing the number of overdraw categories here at compile time
(ie. std::bitset<num_categories>) or use a magic value here. I prefer to keep
that as simple as a size of an array in the .cc file.

[Daniele Castagna, 2017-01-10 23:52:52]

lgtm

[reveman, 2017-01-11 03:21:37]

Description was changed from

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

BUG=678234
TEST=chrome --show-overdraw-feedback
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

The --ui-disable-partial-swap flag is also added to about:flags in
this patch. Disabling of partial swap is not required for overdraw
feedback but often what you want to make the output easier to diagnose.

BUG=678234
TEST=chrome --show-overdraw-feedback
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-01-11 03:22:02]

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/20001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:556: std::set<int> current_overdraw_;
On 2017/01/10 at 23:52:01, reveman wrote:
> On 2017/01/09 at 18:53:06, Daniele Castagna wrote:
> > Why don't you just use std::bitset here?
> 
> That requires knowing the number of overdraw categories here at compile time
(ie. std::bitset<num_categories>) or use a magic value here. I prefer to keep
that as simple as a size of an array in the .cc file.

Fyi, follow up patch that uses SAMPLES_PASSED_ARB for more accurate trace events
will also require this to become a map instead of a set.

[reveman, 2017-01-11 03:46:40]

reveman@chromium.org changed reviewers:
+ asvitkine@chromium.org, danakj@chromium.org, dtrainor@chromium.org,
fsamuel@chromium.org

[reveman, 2017-01-11 03:46:41]

+dtrainor for blimp/
+danakj for cc/, components/display_compositor/ and ui/compositor/
+asvitkine for tools/metrics/histograms/
+fsamuel for services/ui/surfaces/

Two important features have been saved for follow up patches:

1. Extend support beyond ChromeOS Ozone and the display compositor.
2. Use SAMPLES_PASSED_ARB instead of ANY_SAMPLES_PASSED_EXT when available.

2) will allow us to create continuously running performance tests that measure
the exact amount of overdraw.

[Fady Samuel, 2017-01-11 16:12:01]

services/ui/surfaces lgtm

[Alexei Svitkine (slow), 2017-01-11 16:41:30]

lgtm

[David Trainor- moved to gerrit, 2017-01-11 18:22:37]

blimp/ lgtm

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

How can we unit test the code in DirectRenderer/GLRenderer? Can we look at what
we draw and verify it does anything? Maybe also verify it finds overdraw? We'd
need to make stencils work in the TestContextProvider probably? Or maybe it
should be a pixel-test?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:90: LOG_IF(WARNING,
settings_->show_overdraw_feedback &&
use DLOG_IF instead.

why doesn't this happen for tracing path too?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:92: << "Overdraw feedback enabled without output
surface stencil support.";
This error is pretty vague, I think it might be nicer to say on a platform
without support? WDYT? Also if you disable GPU you lose this.. I guess that's
not feasible to do there, not sure if we can include that in the error?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:377: base::CancelableClosure overdraw_callback;
this makes a weakptrfactory for each query, is that overkill?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:378: const unsigned query;
query is a GLuint, can you use that instead of "unsigned" throughout?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:432: pending_query->overdraw_callback.Cancel();
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.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:516: gl_->ClearStencil(0);
Does this add cost when not using overdraw feedback?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3029: if (overdraw_feedback_)
how come you do this into readbacks?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3235: } else if
(output_surface_->HasExternalStencilTest()) {
using else here means turning on overdraw feedback does bad things to webview,
which is okay I guess, is this what you intended?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3280: if (overdraw_feedback_) {
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?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4131: // Return early if already flushed.
I don't understand how we can flush twice for a single renderpass/frame? Maybe
DCHECK or can you explain to me?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4192: gl_->BeginQueryEXT(GL_ANY_SAMPLES_PASSED_EXT,
query);
would it not work to do one query around the whole loop? what would that do?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4211: // Save the overdraw_feedback_callback so it can
be cancelled.
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.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4225: stencil_shadow_ = false;
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?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4230: base::Closure barrier) {
const&

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:556: std::set<int> current_overdraw_;
set is very slow, is there a better structure you can use?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:560: std::deque<std::unique_ptr<PendingOverdrawQuery>>
pending_overdraw_queries_;
deque is very costly for memory
https://bugs.chromium.org/p/chromium/issues/detail?id=674287 is there something
better you can use?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/renderer_sett...
File cc/output/renderer_settings.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/renderer_sett...
cc/output/renderer_settings.cc:36: bool RendererSettings::operator==(const
RendererSettings& other) const {
can this just =default?

https://codereview.chromium.org/2612023002/diff/80001/components/display_comp...
File components/display_compositor/buffer_queue.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/components/display_comp...
components/display_compositor/buffer_queue.cc:137:
gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
I don't get why this stuff is in Reshape and in BindFramebuffer?

[reveman, 2017-01-13 01:17:21]

Description was changed from

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

The --ui-disable-partial-swap flag is also added to about:flags in
this patch. Disabling of partial swap is not required for overdraw
feedback but often what you want to make the output easier to diagnose.

BUG=678234
TEST=chrome --show-overdraw-feedback
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

The --ui-disable-partial-swap flag is also added to about:flags in
this patch. Disabling of partial swap is not required for overdraw
feedback but often what you want to make the output easier to diagnose.

BUG=678234
TEST=chrome --show-overdraw-feedback, cc_unittests
--gtest_filter=GLRendererPixelTestWithOverdrawFeedback.TranslucentRectangles
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

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

On 2017/01/12 at 00:42:57, danakj wrote:
> How can we unit test the code in DirectRenderer/GLRenderer? Can we look at
what we draw and verify it does anything? Maybe also verify it finds overdraw?
We'd need to make stencils work in the TestContextProvider probably? Or maybe it
should be a pixel-test?

I've added a pixel test to latest patch. Ptal.

FYI, I'm also working on a guide for how to use this tool that I'm going to add
to chromium.org. Current draft is
https://docs.google.com/a/chromium.org/document/d/1UyVwJAdeAvkRW-jn3S4ABh75zK...
and it should be ready by the time follow up patches that provide support for
non-ChromeOS platforms are ready.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:90: LOG_IF(WARNING,
settings_->show_overdraw_feedback &&
On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> use DLOG_IF instead.

Done.

> 
> why doesn't this happen for tracing path too?

Tracing is turned on/off so it would need to be logged every frame or we'd need
some more complicated mechanism to handle that. Overdraw trace counter just not
showing up in traces when not supported is fine imo. Someone enabling visual
feedback in about:flags but nothing happens is more surprising so I figured that
might deserve a LOG statement.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:92: << "Overdraw feedback enabled without output
surface stencil support.";
On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> This error is pretty vague, I think it might be nicer to say on a platform
without support? WDYT? Also if you disable GPU you lose this.. I guess that's
not feasible to do there, not sure if we can include that in the error?

Makes sense. Done.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:377: base::CancelableClosure overdraw_callback;
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> this makes a weakptrfactory for each query, is that overkill?

This code has been removed from latest patch.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:378: const unsigned query;
On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> query is a GLuint, can you use that instead of "unsigned" throughout?

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

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:432: pending_query->overdraw_callback.Cancel();
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> 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.

This code has been removed from latest patch.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:516: gl_->ClearStencil(0);
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> Does this add cost when not using overdraw feedback?

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.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3029: if (overdraw_feedback_)
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> how come you do this into readbacks?

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

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3235: } else if
(output_surface_->HasExternalStencilTest()) {
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> using else here means turning on overdraw feedback does bad things to webview,
which is okay I guess, is this what you intended?

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.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3280: if (overdraw_feedback_) {
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> 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?

Yes, this is for copy requests.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4131: // Return early if already flushed.
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> I don't understand how we can flush twice for a single renderpass/frame? Maybe
DCHECK or can you explain to me?

Good point. Now a DCHECK.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4192: gl_->BeginQueryEXT(GL_ANY_SAMPLES_PASSED_EXT,
query);
On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> would it not work to do one query around the whole loop? what would that do?

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.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4211: // Save the overdraw_feedback_callback so it can
be cancelled.
On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> 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.

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

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4225: stencil_shadow_ = false;
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> 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?

Good point. Removed it.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:4230: base::Closure barrier) {
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> const&

Done.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:556: std::set<int> current_overdraw_;
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> set is very slow, is there a better structure you can use?

FYI, this will become a map in a follow up patch where we track the exact amount
of overdraw.

I think set/map is fine. It makes for clean and easy to read code and it's only
used when overdraw tracing is enabled so it doesn't effect normal chrome
performance.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:560: std::deque<std::unique_ptr<PendingOverdrawQuery>>
pending_overdraw_queries_;
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> deque is very costly for memory
https://bugs.chromium.org/p/chromium/issues/detail?id=674287 is there something
better you can use?

Removed this completely from latest version of the patch.

https://codereview.chromium.org/2612023002/diff/80001/cc/output/renderer_sett...
File cc/output/renderer_settings.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/renderer_sett...
cc/output/renderer_settings.cc:36: bool RendererSettings::operator==(const
RendererSettings& other) const {
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> can this just =default?

I don't think there's default comparisons in c++ yet. Looks like there's a
proposal for adding it to c++17 here:
http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0221r0.html

https://codereview.chromium.org/2612023002/diff/80001/components/display_comp...
File components/display_compositor/buffer_queue.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/components/display_comp...
components/display_compositor/buffer_queue.cc:137:
gl_->BindFramebuffer(GL_FRAMEBUFFER, fbo_);
On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> I don't get why this stuff is in Reshape and in BindFramebuffer?

What stuff? Allocation and binding of stencil buffers? That's needed for
GpuSurfacelessBrowserCompositorOutputSurface to be able to claim that it
supports stencil operations.

[danakj, 2017-01-16 16:02:58]

Moving to the new patchset some first comments

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3029: if (overdraw_feedback_)
On 2017/01/13 01:27:42, reveman wrote:
> On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> > how come you do this into readbacks?
> 
> For screenshots. That's very important for debugging purposes.

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..

https://codereview.chromium.org/2612023002/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3029:
DCHECK(!output_surface_->HasExternalStencilTest());
Can you put in a comment that the output surface should not allow
overdraw_feedback_ by not saying |supports_stencil|.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/output_surfa...
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/output_surfa...
cc/output/output_surface.h:47: // Whether this OutputSurface supports stencil
operations or not.
Can you mention this is mutually exclusive with HasExternalStencilTest?

[danakj, 2017-01-16 16:22:01]

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:90: LOG_IF(WARNING,
settings_->show_overdraw_feedback &&
On 2017/01/13 01:27:42, reveman wrote:
> On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> > use DLOG_IF instead.
> 
> Done.
> 
> > 
> > why doesn't this happen for tracing path too?
> 
> Tracing is turned on/off so it would need to be logged every frame or we'd
need
> some more complicated mechanism to handle that. Overdraw trace counter just
not
> showing up in traces when not supported is fine imo. Someone enabling visual
> feedback in about:flags but nothing happens is more surprising so I figured
that
> might deserve a LOG statement.

What about just a simple static bool? It's okay to only report once per process.

[reveman, 2017-01-16 18:02:27]

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:90: LOG_IF(WARNING,
settings_->show_overdraw_feedback &&
On 2017/01/16 at 16:22:01, danakj (slow) wrote:
> On 2017/01/13 01:27:42, reveman wrote:
> > On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> > > use DLOG_IF instead.
> > 
> > Done.
> > 
> > > 
> > > why doesn't this happen for tracing path too?
> > 
> > Tracing is turned on/off so it would need to be logged every frame or we'd
need
> > some more complicated mechanism to handle that. Overdraw trace counter just
not
> > showing up in traces when not supported is fine imo. Someone enabling visual
> > feedback in about:flags but nothing happens is more surprising so I figured
that
> > might deserve a LOG statement.
> 
> What about just a simple static bool? It's okay to only report once per
process.

Done. Although "call once" execution of code using a static bool without a
subtle race condition is not pretty imo. Happy to change it to use
std::call_once if that's OK with you?

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3029: if (overdraw_feedback_)
On 2017/01/16 at 16:02:57, danakj (slow) wrote:
> On 2017/01/13 01:27:42, reveman wrote:
> > On 2017/01/12 at 00:42:58, danakj (slow) wrote:
> > > how come you do this into readbacks?
> > 
> > For screenshots. That's very important for debugging purposes.
> 
> 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..

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

https://codereview.chromium.org/2612023002/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3029:
DCHECK(!output_surface_->HasExternalStencilTest());
On 2017/01/16 at 16:02:57, danakj (slow) wrote:
> Can you put in a comment that the output surface should not allow
overdraw_feedback_ by not saying |supports_stencil|.

Done.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/output_surfa...
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/output_surfa...
cc/output/output_surface.h:47: // Whether this OutputSurface supports stencil
operations or not.
On 2017/01/16 at 16:02:57, danakj (slow) wrote:
> Can you mention this is mutually exclusive with HasExternalStencilTest?

Done.

[danakj, 2017-01-16 18:17:57]

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:90: LOG_IF(WARNING,
settings_->show_overdraw_feedback &&
On 2017/01/16 18:02:26, reveman wrote:
> On 2017/01/16 at 16:22:01, danakj (slow) wrote:
> > On 2017/01/13 01:27:42, reveman wrote:
> > > On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> > > > use DLOG_IF instead.
> > > 
> > > Done.
> > > 
> > > > 
> > > > why doesn't this happen for tracing path too?
> > > 
> > > Tracing is turned on/off so it would need to be logged every frame or we'd
> need
> > > some more complicated mechanism to handle that. Overdraw trace counter
just
> not
> > > showing up in traces when not supported is fine imo. Someone enabling
visual
> > > feedback in about:flags but nothing happens is more surprising so I
figured
> that
> > > might deserve a LOG statement.
> > 
> > What about just a simple static bool? It's okay to only report once per
> process.
> 
> Done. Although "call once" execution of code using a static bool without a
> subtle race condition is not pretty imo. Happy to change it to use
> std::call_once if that's OK with you?

What race are you worried about, DirectRenderer being used on multiple threads
in the same process? There's no place that happens right? Maybe put a static
thread-checker that runs in debug mode only if you're worried about it. I'd
rather debug-only verify this than use more complicated code for a whatif. wdyt?

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3543: gl_->StencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
These two lines here seem redundant with each other, or no?

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3544: gl_->StencilMask(0);
The above and this one

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3552: DCHECK(program && (program->initialized() ||
IsContextLost()));
don't use && in a DCHECK, split it into 2 DCHECKs so we can tell which one
failed

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3635: *std::max_element(overdraw->begin(),
overdraw->end()));
Why is it max_element that you report?

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.h:9: #include <set>
unused

https://codereview.chromium.org/2612023002/diff/160001/cc/test/pixel_test_out...
File cc/test/pixel_test_output_surface.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/test/pixel_test_out...
cc/test/pixel_test_output_surface.cc:61: return external_stencil_test_;
This can be true, but supports_stencil is true. Some DCHECKs or something else
to help?

https://codereview.chromium.org/2612023002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1057: IDS_FLAGS_UI_PARTIAL_SWAP_DESCRIPTION,
kOsCrOS,
Why is this chromeos only, the flag works anywhere. Could be useful for software
compositing on linux and such for example too?

But then also.. why is it in this CL? Do you need to use this when showing
overdraw?

[reveman, 2017-01-16 22:46:19]

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/80001/cc/output/direct_render...
cc/output/direct_renderer.cc:90: LOG_IF(WARNING,
settings_->show_overdraw_feedback &&
On 2017/01/16 at 18:17:52, danakj (slow) wrote:
> On 2017/01/16 18:02:26, reveman wrote:
> > On 2017/01/16 at 16:22:01, danakj (slow) wrote:
> > > On 2017/01/13 01:27:42, reveman wrote:
> > > > On 2017/01/12 at 00:42:57, danakj (slow) wrote:
> > > > > use DLOG_IF instead.
> > > > 
> > > > Done.
> > > > 
> > > > > 
> > > > > why doesn't this happen for tracing path too?
> > > > 
> > > > Tracing is turned on/off so it would need to be logged every frame or
we'd
> > need
> > > > some more complicated mechanism to handle that. Overdraw trace counter
just
> > not
> > > > showing up in traces when not supported is fine imo. Someone enabling
visual
> > > > feedback in about:flags but nothing happens is more surprising so I
figured
> > that
> > > > might deserve a LOG statement.
> > > 
> > > What about just a simple static bool? It's okay to only report once per
> > process.
> > 
> > Done. Although "call once" execution of code using a static bool without a
> > subtle race condition is not pretty imo. Happy to change it to use
> > std::call_once if that's OK with you?
> 
> What race are you worried about, DirectRenderer being used on multiple threads
in the same process? There's no place that happens right? Maybe put a static
thread-checker that runs in debug mode only if you're worried about it. I'd
rather debug-only verify this than use more complicated code for a whatif. wdyt?

I'd rather not introduce the assumption that this class is never instantiated on
more than one thread just for this debugging feature and this log statement. I
changed the code to use a member variable in latest patch. I can continue to
propose alternatives until we find one that we're both happy with but we might
be overthinking this as it's really just a log statement for a debug feature.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3543: gl_->StencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
On 2017/01/16 at 18:17:54, danakj (slow) wrote:
> These two lines here seem redundant with each other, or no?

Yes, second line should not be needed.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3544: gl_->StencilMask(0);
On 2017/01/16 at 18:17:54, danakj (slow) wrote:
> The above and this one

Removed this line.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3552: DCHECK(program && (program->initialized() ||
IsContextLost()));
On 2017/01/16 at 18:17:53, danakj (slow) wrote:
> don't use && in a DCHECK, split it into 2 DCHECKs so we can tell which one
failed

Done. Changed all other occurrences of this too for consistency.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3635: *std::max_element(overdraw->begin(),
overdraw->end()));
On 2017/01/16 at 18:17:55, danakj (slow) wrote:
> Why is it max_element that you report?

I think that's the most useful with GL_ANY_SAMPLES_PASSED_EXT as the result for
each category is going to be 0 or 1. I'm changing this to std::accumulate in a
follow up patch that makes use of GL_SAMPLES_PASSED_ARB as the queries are then
returning the exact number of samples in each category.

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.h:9: #include <set>
On 2017/01/16 at 18:17:55, danakj (slow) wrote:
> unused

Removed.

https://codereview.chromium.org/2612023002/diff/160001/cc/test/pixel_test_out...
File cc/test/pixel_test_output_surface.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/test/pixel_test_out...
cc/test/pixel_test_output_surface.cc:61: return external_stencil_test_;
On 2017/01/16 at 18:17:55, danakj (slow) wrote:
> This can be true, but supports_stencil is true. Some DCHECKs or something else
to help?

Added DCHECK to PixelTestOutputSurface::Reshape.

https://codereview.chromium.org/2612023002/diff/160001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1057: IDS_FLAGS_UI_PARTIAL_SWAP_DESCRIPTION,
kOsCrOS,
On 2017/01/16 at 18:17:55, danakj (slow) wrote:
> Why is this chromeos only, the flag works anywhere. Could be useful for
software compositing on linux and such for example too?

Made it available on all platforms in latest patch.

> 
> But then also.. why is it in this CL? Do you need to use this when showing
overdraw?

This is useful when debugging overdraw. Overdraw feedback works correctly with
partial updates and sometimes that's useful for debugging but it often results
in harder to understand output as you're not seeing the overdraw result for the
complete screen at the current compositor state. E.g. if you move a window and
that makes the compositor decide to use a new approach for occlusion culling
then only the partially updated area of the screen will show overdraw feedback
that match that new approach while the rest will show the correct overdraw for
the old approach. I'll be explaining this in how-to going up on chromium.org.

[danakj, 2017-01-17 15:36:38]

LGTM

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2612023002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3635: *std::max_element(overdraw->begin(),
overdraw->end()));
On 2017/01/16 22:46:18, reveman wrote:
> On 2017/01/16 at 18:17:55, danakj (slow) wrote:
> > Why is it max_element that you report?
> 
> I think that's the most useful with GL_ANY_SAMPLES_PASSED_EXT as the result
for
> each category is going to be 0 or 1. I'm changing this to std::accumulate in a
> follow up patch that makes use of GL_SAMPLES_PASSED_ARB as the queries are
then
> returning the exact number of samples in each category.

Ah I see, thanks. That sounds good!

[reveman, 2017-01-17 17:02:07]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-17 17:02:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, dtrainor@chromium.org, asvitkine@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps200001
(title: "address more feedback")

[commit-bot: I haz the power, 2017-01-17 17:02:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 17:04:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 17:04:30]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[reveman, 2017-01-17 17:24:25]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-17 17:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
asvitkine@chromium.org, dtrainor@chromium.org, dcastagna@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-17 17:24:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 17:37:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 17:37:42]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[reveman, 2017-01-17 17:54:50]

reveman@chromium.org changed reviewers:
+ boliu@chromium.org

[reveman, 2017-01-17 17:54:50]

+boliu for content/renderer/android

[boliu, 2017-01-17 17:56:22]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-01-17 17:56:23]

lgtm

[boliu, 2017-01-17 17:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
asvitkine@chromium.org, dtrainor@chromium.org, dcastagna@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps240001
(title: "fix android build")

[commit-bot: I haz the power, 2017-01-17 17:57:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 18:11:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:11:29]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-01-17 20:41:57]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-17 20:41:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, dcastagna@chromium.org, boliu@chromium.org,
danakj@chromium.org, asvitkine@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps260001
(title: "fix buffer queue unit test and renderer pixel test build on android")

[commit-bot: I haz the power, 2017-01-17 20:42:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 21:00:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 21:00:33]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-01-17 21:09:53]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-17 21:09:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, dcastagna@chromium.org, boliu@chromium.org,
danakj@chromium.org, asvitkine@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps280001
(title: "fix partial swap resource")

[commit-bot: I haz the power, 2017-01-17 21:10:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 21:33:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 21:33:34]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[reveman, 2017-01-17 22:14:39]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-17 22:14:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, dcastagna@chromium.org, boliu@chromium.org,
danakj@chromium.org, asvitkine@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps300001
(title: "fix another android build issue")

[commit-bot: I haz the power, 2017-01-17 22:15:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 22:39:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 22:39:55]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[reveman, 2017-01-17 22:48:14]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-17 22:48:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, dcastagna@chromium.org, boliu@chromium.org,
danakj@chromium.org, asvitkine@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps310001
(title: "another android build fix")

[commit-bot: I haz the power, 2017-01-17 22:48:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 00:52:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 00:52:48]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-01-18 01:57:49]

The CQ bit was checked by reveman@chromium.org

[reveman, 2017-01-18 01:57:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, dcastagna@chromium.org, boliu@chromium.org,
danakj@chromium.org, asvitkine@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2612023002/#ps330001
(title: "make sure overdraw_feedback_ is initialized and reset properly")

[commit-bot: I haz the power, 2017-01-18 01:58:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 03:05:52]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1484704669409020,
"parent_rev": "a19f1df53e1967de0ae549ca1bcdf2a1f0ef47a7", "commit_rev":
"9413cffb4fdc7dbcfc40a8da0530942e2f6ae0f2"}

[commit-bot: I haz the power, 2017-01-18 03:06:41]

Description was changed from

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

The --ui-disable-partial-swap flag is also added to about:flags in
this patch. Disabling of partial swap is not required for overdraw
feedback but often what you want to make the output easier to diagnose.

BUG=678234
TEST=chrome --show-overdraw-feedback, cc_unittests
--gtest_filter=GLRendererPixelTestWithOverdrawFeedback.TranslucentRectangles
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Implement overdraw feedback debugging feature.

This adds a overdraw feedback debug flag to the GL renderer. This feature
is inspired by the GPU overdraw debug feature that exists on Android and
developers familiar with that feature will feel at home using this flag
when debugging overdraw in web-pages or in the ChromeOS UI.

When enabled, this flag visualize overdraw by color-coding areas of the
output based on how much overdraw is taking place. The colors are
hinting at the amount of overdraw on the screen for each pixel, as follows:

True color: No overdraw
Blue: Overdrawn once
Green: Overdrawn twice
Pink: Overdrawn three times
Red: Overdrawn four or more times

This also adds an overdraw tracing category (cc.debug.overdraw) that
provides a trace counter for the current amount overdraw.

The --ui-disable-partial-swap flag is also added to about:flags in
this patch. Disabling of partial swap is not required for overdraw
feedback but often what you want to make the output easier to diagnose.

BUG=678234
TEST=chrome --show-overdraw-feedback, cc_unittests
--gtest_filter=GLRendererPixelTestWithOverdrawFeedback.TranslucentRectangles
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2612023002
Cr-Commit-Position: refs/heads/master@{#444256}
Committed:
https://chromium.googlesource.com/chromium/src/+/9413cffb4fdc7dbcfc40a8da0530...
==========

[commit-bot: I haz the power, 2017-01-18 03:06:42]

Committed patchset #18 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/9413cffb4fdc7dbcfc40a8da0530...