Add flag for drawing layers to screen with Ganesh

Add flag for drawing layers to screen with Ganesh

The --force-direct-layer-drawing flag causes any layer that could be
drawn direct to the backbuffer to be drawn to the backbuffer using
Ganesh.  This flag also requires both the --enable-threaded-compositing
flag and the --enable-impl-side-painting flag to have any effect.

This patch also turns on testing for PictureDrawQuad using Ganesh in the
cc pixeltests.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201362

[enne (OOO), 2013-04-24 22:49:53]

This patch is an initial attempt at drawing to the backbuffer directly using
Ganesh.  It needs a stencil buffer to avoid crashes on most sites, but on poster
circle the text appears in the right place although it flashes constantly.

https://codereview.chromium.org/13863015/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/13863015/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:2640: void GLRenderer::ReinitializeGLState() {
This patch is clearly missing something here in the state setting in that
switching between ganesh and compositor drawing causes the debug clear color to
flash in and out.

[enne (OOO), 2013-04-25 16:02:37]

Uploaded a patch that adds a stencil buffer as well.  With this latest patch,
you need to run Chrome with the following flags.

--force-compositing-mode --enable-threaded-compositing
--enable-impl-side-painting --draw-layers-with-ganesh

http://www.webkit.org/blog-files/3d-transforms/poster-circle.html: flashes
between debug clear blue and text on screen.  I've tried to play whack-a-mole
with what settings need to be reset on both the compositor and Skia sides, and
can't quite figure out what's missing here.

http://google.com: Hovering over the I'm feeling lucky button makes it
disappear.  Also, lots of cyan, which is what we clear to in
picture_pile_impl.cc prior to rastering anything to make sure we've filled the
entire rect with content from the picture.  So, it means that things are getting
clipped when rastering with Ganesh that aren't being clipped with software Skia
raster.

[enne (OOO), 2013-05-13 19:33:10]

danakj: this work in progress patch is now ready for review.

https://codereview.chromium.org/13863015/diff/20001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/13863015/diff/20001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl_unittest.cc:227: EXPECT_EQ(1u,
mock_canvas.rects_.size());
This change is because the clear moved to RasterToBitmap and not Raster. 
Testing just Raster seemed cleaner.

https://codereview.chromium.org/13863015/diff/20001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/13863015/diff/20001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:918: Region outside(layer_rect);
This change is because drawing directly with Ganesh would draw this with
"kClearMode" previously, clobbering the old backbuffer.  This changes to only
drawing red outside the area we care about and leaving the union_layer_rect
unchanged.

https://codereview.chromium.org/13863015/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (left):

https://codereview.chromium.org/13863015/diff/20001/cc/resources/picture.cc#o...
cc/resources/picture.cc:203: paint.setXfermodeMode(SkXfermode::kClear_Mode);
This moved to PicturePileImpl::RasterToBitmap.

https://codereview.chromium.org/13863015/diff/20001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/13863015/diff/20001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:220: // TODO(enne): this needs to be rolled
together with the border clear
It is definitely weird to clear to transparent and then clear some border pixels
to the background color in the same function.  vmpstr tried to change this to
just clearing to the background color and got reverted.  I've left these two
clears as written previously to avoid changing behavior, and will follow up to
try to combine them.

[bsalomon, 2013-05-13 19:49:15]

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

https://codereview.chromium.org/13863015/diff/20001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2868: desc.fOrigin = kBottomLeft_GrSurfaceOrigin;
As of skia r9113 (which should get rolled to chromium tomorrow morning) you
should be able to set this to kTopLeft in order to draw in the same orientation
as the sw-rasterized layers.

[enne (OOO), 2013-05-14 18:20:09]

bsalomon: r9113 isn't quite enough.  You still assert in GrGpuGL.cpp:483 and (I
think, maybe my code is wrong) you don't flip the scissor rect when using
kTopLeft.

[bsalomon, 2013-05-14 19:57:04]

On 2013/05/14 18:20:09, enne wrote:
> bsalomon: r9113 isn't quite enough.  You still assert in GrGpuGL.cpp:483 and
(I
> think, maybe my code is wrong) you don't flip the scissor rect when using
> kTopLeft.

Hey enne, sorry about the assert. I tested my code using internally created
rendertargets rather than wrapped ones. I just landed skia r9129 to remove it. I
think we're respecting the origin when setting the scissor, though.

[enne (OOO), 2013-05-14 20:02:41]

bsalomon: Thanks.  Patch set 6 fixes the scissor issue and r9129 lets me run
with kTopLeft.

[danakj, 2013-05-15 20:33:16]

Pretty cool!

https://codereview.chromium.org/13863015/diff/33001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/base/switches.cc#newco...
cc/base/switches.cc:42: // Only valid with gl rendering / threaded compositing /
impl-side painting.
nit: s!/!+!  (and not or)

https://codereview.chromium.org/13863015/diff/33001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:563: bool LayerImpl::CanDrawDirectlyToBackbuffer() const
{
I don't like bringing a "composite draw property" into the layer level really.

Could this be a value on draw properties that is computed by LTHCommon?

Or alternatively, a function on DrawProperties? It would then instead need a
state on DrawProps to know that it draws to the root target.

And while we're here, why did you choose to limit this to the root surface only?

https://codereview.chromium.org/13863015/diff/33001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:399: tilings_->RemoveAllTilings();
Don't you also want to prevent syncing tilings to this layer?

Seems like this value should be part of the CanHaveTilings() method. And you can
do all this block if !CanHaveTilings().

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:290: bool always_clear = true;
omg thanks for this. i was meaning to do something to make this less confusing
to read

nit: can you make the release-mode the default instead of debug mode (ie use
ifndef NDEBUG)

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:297: clear_bits |= GL_STENCIL_BUFFER_BIT;
Maybe a comment why? Cuz I know why we'd do this.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1517: quad->picture_pile->Raster(
RasterToScreen?

I don't like qualifying one name and not qualifying the other, like it has some
implicit meaning.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1521: sk_canvas_->flush();
I think you want to flush the GL context after the ganesh context. And then
makeCurrent() on it. See ApplyImageFilter() and ApplyFilters() and follow the
same pattern.

(I think you have correctly avoided the flush on the compositor context /before/
using the GrContext because it won't be using any contents from the stuff drawn
so far.)

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1522: ReinitializeGLState();
Do you want to do all this flushing and state setting only when you draw
something non-ganesh for the first time after this? Maybe a TODO?

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2874: SkAutoTUnref<GrSurface> surface(
skia::RefPtr instead for these please

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2895: is_scissor_enabled_ = false;
What if scissoring is enabled to clip to some parent thing, then you draw
something with ganesh?

Changing the state of the renderer like this makes me want unit tests.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2900: return gr_context_ &&
context_->getContextAttributes().stencil;
An explanatory comment re: stencil here as well?

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

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.h#n...
cc/output/gl_renderer.h:404: skia::RefPtr<class GrContext> gr_context_;
need the "class"? I don't see a method with the name

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.h#n...
cc/output/gl_renderer.h:405: SkAutoTUnref<SkCanvas> sk_canvas_;
skia::RefPtr > SkAutoTUnref

https://codereview.chromium.org/13863015/diff/33001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:581: BackgroundFilterRendererTypes;
Should we have a test that uses ganesh and a background filter?

https://codereview.chromium.org/13863015/diff/33001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:97: SkRegion::kIntersect_Op);
Why this change?

https://codereview.chromium.org/13863015/diff/33001/cc/test/pixel_test.h
File cc/test/pixel_test.h (right):

https://codereview.chromium.org/13863015/diff/33001/cc/test/pixel_test.h#newc...
cc/test/pixel_test.h:63: void SetUp();
why isnt it virtual anymore?

https://codereview.chromium.org/13863015/diff/33001/cc/trees/layer_tree_setti...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/13863015/diff/33001/cc/trees/layer_tree_setti...
cc/trees/layer_tree_settings.h:61: bool force_direct_layer_drawing;  // with
Ganesh
With Ganesh.

https://codereview.chromium.org/13863015/diff/33001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:940:
cc::switches::kDisablePinchZoomScrollbars,
Thanks for this cleanup

[enne (OOO), 2013-05-15 22:34:38]

https://codereview.chromium.org/13863015/diff/33001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/base/switches.cc#newco...
cc/base/switches.cc:42: // Only valid with gl rendering / threaded compositing /
impl-side painting.
On 2013/05/15 20:33:16, danakj wrote:
> nit: s!/!+!  (and not or)

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/layers/layer_impl.cc#n...
cc/layers/layer_impl.cc:563: bool LayerImpl::CanDrawDirectlyToBackbuffer() const
{
On 2013/05/15 20:33:16, danakj wrote:
> I don't like bringing a "composite draw property" into the layer level really.
> 
> Could this be a value on draw properties that is computed by LTHCommon?
> 
> Or alternatively, a function on DrawProperties? It would then instead need a
> state on DrawProps to know that it draws to the root target.

Moved to DrawProperties.  PTAL.

> And while we're here, why did you choose to limit this to the root surface
only?

Limiting to non-1 opacity is because you need to composite all the draw calls in
the picture together before drawing that layer.  This is the picture analogy to
needing to create a render surface for non-1 opacity.

As for the root layer, there'd been some discussion about an initial use for
Ganesh in places where we could get LCD text where we otherwise wouldn't, such
as when you're drawing a layer directly to the backbuffer and not into a render
target.  I just figured we could start there.

What do you think?

https://codereview.chromium.org/13863015/diff/33001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:399: tilings_->RemoveAllTilings();
On 2013/05/15 20:33:16, danakj wrote:
> Don't you also want to prevent syncing tilings to this layer?
> 
> Seems like this value should be part of the CanHaveTilings() method. And you
can
> do all this block if !CanHaveTilings().

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:290: bool always_clear = true;
On 2013/05/15 20:33:16, danakj wrote:
> omg thanks for this. i was meaning to do something to make this less confusing
> to read
> 
> nit: can you make the release-mode the default instead of debug mode (ie use
> ifndef NDEBUG)

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:297: clear_bits |= GL_STENCIL_BUFFER_BIT;
On 2013/05/15 20:33:16, danakj wrote:
> Maybe a comment why? Cuz I know why we'd do this.

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1517: quad->picture_pile->Raster(
On 2013/05/15 20:33:16, danakj wrote:
> RasterToScreen?
> 
> I don't like qualifying one name and not qualifying the other, like it has
some
> implicit meaning.

It's really more "RasterDirect" vs. "RasterTo(Intermediate)Bitmap".  How does
that sound?

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1521: sk_canvas_->flush();
On 2013/05/15 20:33:16, danakj wrote:
> I think you want to flush the GL context after the ganesh context. And then
> makeCurrent() on it. See ApplyImageFilter() and ApplyFilters() and follow the
> same pattern.
> 
> (I think you have correctly avoided the flush on the compositor context
/before/
> using the GrContext because it won't be using any contents from the stuff
drawn
> so far.)

This is a canvas flush not a context flush.  Ganesh can batch up and defer some
draws and this lets it know that we're done and it should draw everything it
has.

Unlike ApplyImageFilter and ApplyFilters we are not using a different offscreen
context here.  We are using the same context that the compositor is using and so
I don't think any context flush is needed.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1522: ReinitializeGLState();
On 2013/05/15 20:33:16, danakj wrote:
> Do you want to do all this flushing and state setting only when you draw
> something non-ganesh for the first time after this? Maybe a TODO?

More than the TODO on line 1489?

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2874: SkAutoTUnref<GrSurface> surface(
On 2013/05/15 20:33:16, danakj wrote:
> skia::RefPtr instead for these please

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2895: is_scissor_enabled_ = false;
On 2013/05/15 20:33:16, danakj wrote:
> What if scissoring is enabled to clip to some parent thing, then you draw
> something with ganesh?
> 
> Changing the state of the renderer like this makes me want unit tests.

At the point this call is made, the gl scissor property is in an unknown state. 
Because we are shadowing the scissor state (like we shadow blend and program
above), we need to set the state and the shadow together.  This makes sure that
the next time we call EnsureScissorTestEnabled[0] that the GLRenderer will do
the right thing.

This is tested in layout tests, for sure.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2900: return gr_context_ &&
context_->getContextAttributes().stencil;
On 2013/05/15 20:33:16, danakj wrote:
> An explanatory comment re: stencil here as well?

Done.

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

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.h#n...
cc/output/gl_renderer.h:404: skia::RefPtr<class GrContext> gr_context_;
On 2013/05/15 20:33:16, danakj wrote:
> need the "class"? I don't see a method with the name

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/output/gl_renderer.h#n...
cc/output/gl_renderer.h:405: SkAutoTUnref<SkCanvas> sk_canvas_;
On 2013/05/15 20:33:16, danakj wrote:
> skia::RefPtr > SkAutoTUnref

Done.

https://codereview.chromium.org/13863015/diff/33001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:581: BackgroundFilterRendererTypes;
On 2013/05/15 20:33:16, danakj wrote:
> Should we have a test that uses ganesh and a background filter?

Ganesh is used to draw picture quads.  How is Ganesh involved here?

https://codereview.chromium.org/13863015/diff/33001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/13863015/diff/33001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:97: SkRegion::kIntersect_Op);
On 2013/05/15 20:33:16, danakj wrote:
> Why this change?

Because the caller could have set their own clip rect on the canvas prior to
calling raster, and I don't want to replace it here, just add to it.

This is covered by the pixel tests.  If this is replace and not intersect, then
the pixel test that involves clipping will do the wrong thing.

https://codereview.chromium.org/13863015/diff/33001/cc/test/pixel_test.h
File cc/test/pixel_test.h (right):

https://codereview.chromium.org/13863015/diff/33001/cc/test/pixel_test.h#newc...
cc/test/pixel_test.h:63: void SetUp();
On 2013/05/15 20:33:16, danakj wrote:
> why isnt it virtual anymore?

Changed back.  (At some point as I was iterating on this patch it wasn't
overriding anymore, but it clearly is now.)

https://codereview.chromium.org/13863015/diff/33001/cc/trees/layer_tree_setti...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/13863015/diff/33001/cc/trees/layer_tree_setti...
cc/trees/layer_tree_settings.h:61: bool force_direct_layer_drawing;  // with
Ganesh
On 2013/05/15 20:33:16, danakj wrote:
> With Ganesh.

Done.

[danakj, 2013-05-16 15:17:55]

LG overall! Thanks for moving that into draw properties.

I recalled that senorblanco has requested before that we not use "ganesh" in
code but instead refer to "skia" or whatnot as ganesh is a private term. What do
you think of phasing everything in terms of "draw direct to back buffer" in the
gl_renderer class etc, to match what you've done in the layer/tree code?

https://codereview.chromium.org/13863015/diff/61001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:396: if
(draw_properties().can_draw_directly_to_backbuffer &&
There's this check here, and there's a DrawsContent() check up above, which are
now 2 of the 3 CanHaveTilings() checks.

How about doing if (!CanHaveTilings()) here, and DCHECK(!num_tilings), and drop
the DrawsContent() check up above?

Then manage tilings would not need to do a CanHaveTilings() early out, and could
just DCHECK it instead.

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1489: DCHECK_EQ(quad->opacity(), 1.f) << "Need to
composite to a bitmap or a "
Hm, so this is only true if there are overlapping quads (ie children etc). And
then that is taken care of by render surfaces reparenting opacity.. It seems
okay to limit ourselves from drawing directly things we could have actually
drawn for now, but DCHECKing things that are really valid seems a bit
misleading?

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1525: ReinitializeGLState();
I think I'd like another TODO here like above.. Just so it's not lost in the mix
somehow.

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1556: quad->picture_pile->RasterToBitmap(&canvas,
quad->content_rect,
nit: one arg per line or all on one line

https://codereview.chromium.org/13863015/diff/61001/cc/quads/picture_draw_quad.h
File cc/quads/picture_draw_quad.h (right):

https://codereview.chromium.org/13863015/diff/61001/cc/quads/picture_draw_qua...
cc/quads/picture_draw_quad.h:33: bool draw_direct_to_backbuffer,
nit: can_draw_directly_to_backbuffer (like the draw prop)?

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:264: RasterDirect(canvas, layer_rect, 1.0,
NULL);
I'm a little unsure about this change and 288. Can you reassure me?

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:288: RasterDirect(&canvas, layer_rect, 1.0f,
NULL);
Say you have a tile that has nothing in it. When we raster it, we would raster
the clear to transparent, and nothing else. When we direct draw it, we would do
nothing at all.

Now when we analyze it, we'll see no drawing commands at all. Are we still going
to know this thing is clear? Do we have a test for this?

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.h:56: // Similar to the above Raster method, but
this is a convenience method
"RasterDirect"

https://codereview.chromium.org/13863015/diff/61001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1270:
layer_draw_properties.can_draw_directly_to_backbuffer =
Can you leave a comment explaining the checks done here?

https://codereview.chromium.org/13863015/diff/61001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1273: !animating_opacity_to_screen;
Why this? We'll generate a new frame with a new opacity and we can set it false
in that quad.

[enne (OOO), 2013-05-16 20:34:01]

On 2013/05/16 15:17:55, danakj wrote:

> I recalled that senorblanco has requested before that we not use "ganesh" in
> code but instead refer to "skia" or whatnot as ganesh is a private term. What
do
> you think of phasing everything in terms of "draw direct to back buffer" in
the
> gl_renderer class etc, to match what you've done in the layer/tree code?

As per an email with senorblanco, I renamed all uses of Ganesh that I've
introduced to instead be "Skia GPU backend".

However, I will point out that we use Ganesh all over the place, including code
that Stephen's written.  See:
https://code.google.com/p/chromium/codesearch#search/&q=Ganesh%5B%5Ea%5D

https://codereview.chromium.org/13863015/diff/61001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:396: if
(draw_properties().can_draw_directly_to_backbuffer &&
On 2013/05/16 15:17:55, danakj wrote:
> There's this check here, and there's a DrawsContent() check up above, which
are
> now 2 of the 3 CanHaveTilings() checks.
> 
> How about doing if (!CanHaveTilings()) here, and DCHECK(!num_tilings), and
drop
> the DrawsContent() check up above?
> 
> Then manage tilings would not need to do a CanHaveTilings() early out, and
could
> just DCHECK it instead.

Done.

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1489: DCHECK_EQ(quad->opacity(), 1.f) << "Need to
composite to a bitmap or a "
On 2013/05/16 15:17:55, danakj wrote:
> Hm, so this is only true if there are overlapping quads (ie children etc). And
> then that is taken care of by render surfaces reparenting opacity.. It seems
> okay to limit ourselves from drawing directly things we could have actually
> drawn for now, but DCHECKing things that are really valid seems a bit
> misleading?

Your assumption about overlapping quads is incorrect.  Thi is about overlapping
content inside of the layer itself.

Take a layer with two overlapping squares in it.  How do you draw that layer
(one square at a time) with an opacity applied to the entire layer and get those
two squares to blend with the backbuffer but not with each other?

If you draw to a software bitmap first, that's the analogous compositing step to
drawing two layers to a render buffer.

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1525: ReinitializeGLState();
On 2013/05/16 15:17:55, danakj wrote:
> I think I'd like another TODO here like above.. Just so it's not lost in the
mix
> somehow.

Done.

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1556: quad->picture_pile->RasterToBitmap(&canvas,
quad->content_rect,
On 2013/05/16 15:17:55, danakj wrote:
> nit: one arg per line or all on one line

nit nit: That is not in the style guide.

https://codereview.chromium.org/13863015/diff/61001/cc/quads/picture_draw_quad.h
File cc/quads/picture_draw_quad.h (right):

https://codereview.chromium.org/13863015/diff/61001/cc/quads/picture_draw_qua...
cc/quads/picture_draw_quad.h:33: bool draw_direct_to_backbuffer,
On 2013/05/16 15:17:55, danakj wrote:
> nit: can_draw_directly_to_backbuffer (like the draw prop)?

Done.

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:264: RasterDirect(canvas, layer_rect, 1.0,
NULL);
On 2013/05/16 15:17:55, danakj wrote:
> I'm a little unsure about this change and 288. Can you reassure me?

This path is going away in like the next week.  However, I will change it back
to RasterToBitmap so it will not be changed by this patch.

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.cc:288: RasterDirect(&canvas, layer_rect, 1.0f,
NULL);
On 2013/05/16 15:17:55, danakj wrote:
> Say you have a tile that has nothing in it. When we raster it, we would raster
> the clear to transparent, and nothing else. When we direct draw it, we would
do
> nothing at all.
> 
> Now when we analyze it, we'll see no drawing commands at all. Are we still
going
> to know this thing is clear? Do we have a test for this?

Added a test, ccing vmpstr.

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
File cc/resources/picture_pile_impl.h (right):

https://codereview.chromium.org/13863015/diff/61001/cc/resources/picture_pile...
cc/resources/picture_pile_impl.h:56: // Similar to the above Raster method, but
this is a convenience method
On 2013/05/16 15:17:55, danakj wrote:
> "RasterDirect"

Done.

https://codereview.chromium.org/13863015/diff/61001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1270:
layer_draw_properties.can_draw_directly_to_backbuffer =
On 2013/05/16 15:17:55, danakj wrote:
> Can you leave a comment explaining the checks done here?

Done.

https://codereview.chromium.org/13863015/diff/61001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1273: !animating_opacity_to_screen;
On 2013/05/16 15:17:55, danakj wrote:
> Why this? We'll generate a new frame with a new opacity and we can set it
false
> in that quad.

Sure, but then for animation of opacity=1 to opacity=x, on frame 1 we won't have
the texture ready for future frames where we can't draw to the backbuffer and
will jank/checkerboard.  This is just an optimization.

https://codereview.chromium.org/13863015/diff/64002/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/13863015/diff/64002/content/renderer/render_w...
content/renderer/render_widget.cc:611: attributes.stencil = true;
This change is because of https://codereview.chromium.org/15033004/, along with
some added DCHECKs elsewhere to make sure Ganesh was being used when expected.

[Stephen White, 2013-05-17 00:54:53]

On 2013/05/16 20:34:01, enne wrote:
> On 2013/05/16 15:17:55, danakj wrote:
> 
> > I recalled that senorblanco has requested before that we not use "ganesh" in
> > code but instead refer to "skia" or whatnot as ganesh is a private term.
What
> do
> > you think of phasing everything in terms of "draw direct to back buffer" in
> the
> > gl_renderer class etc, to match what you've done in the layer/tree code?
> 
> As per an email with senorblanco, I renamed all uses of Ganesh that I've
> introduced to instead be "Skia GPU backend".
> 
> However, I will point out that we use Ganesh all over the place, including
code
> that Stephen's written.  See:
> https://code.google.com/p/chromium/codesearch#search/&q=Ganesh%255B%255Ea%255D

Whoops.. busted!

[danakj, 2013-05-17 23:24:01]

https://codereview.chromium.org/13863015/diff/64002/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/64002/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:111: bool use_ganesh =
draw_properties().can_draw_directly_to_backbuffer &&
nit: ganesh

https://codereview.chromium.org/13863015/diff/64002/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:373: *contents_scale_x = ideal_contents_scale_;
the ideal value still needs to be set on the class. you're using and scaling by
0 here, no? won't this mess up AppendQuads?

[danakj, 2013-05-17 23:36:30]

LGTM when the ideal_contents_scale_ thing is resolved.

FTR I'm not a super fan of |use_skia_gpu_backend| any more than |use_ganesh| -
it's pretty abstract. I wish it was something that explained the behaviour
instead of the codepath. But I don't wanna bikeshed on it either, maybe we'll
come up with a nice term for drawing to the backbuffer directly sometime.

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/13863015/diff/61001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1489: DCHECK_EQ(quad->opacity(), 1.f) << "Need to
composite to a bitmap or a "
On 2013/05/16 20:34:11, enne wrote:
> On 2013/05/16 15:17:55, danakj wrote:
> > Hm, so this is only true if there are overlapping quads (ie children etc).
And
> > then that is taken care of by render surfaces reparenting opacity.. It seems
> > okay to limit ourselves from drawing directly things we could have actually
> > drawn for now, but DCHECKing things that are really valid seems a bit
> > misleading?
> 
> Your assumption about overlapping quads is incorrect.  Thi is about
overlapping
> content inside of the layer itself.
> 
> Take a layer with two overlapping squares in it.  How do you draw that layer
> (one square at a time) with an opacity applied to the entire layer and get
those
> two squares to blend with the backbuffer but not with each other?
> 
> If you draw to a software bitmap first, that's the analogous compositing step
to
> drawing two layers to a render buffer.

I see thanks for this explanation.

https://codereview.chromium.org/13863015/diff/64002/cc/resources/picture_pile...
File cc/resources/picture_pile_impl_unittest.cc (right):

https://codereview.chromium.org/13863015/diff/64002/cc/resources/picture_pile...
cc/resources/picture_pile_impl_unittest.cc:193: PicturePileImpl::Analysis
analysis;
nit: EXPECT_FALSE(anaysis.is_solid_color) before the Analyze call so we see it
change

https://codereview.chromium.org/13863015/diff/64002/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/13863015/diff/64002/cc/test/pixel_test.cc#new...
cc/test/pixel_test.cc:116: void PixelTest::SetUpGLRenderer(bool use_ganesh) {
nit: ganesh

[enne (OOO), 2013-05-17 23:52:49]

https://codereview.chromium.org/13863015/diff/64002/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/13863015/diff/64002/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:111: bool use_ganesh =
draw_properties().can_draw_directly_to_backbuffer &&
On 2013/05/17 23:24:01, danakj wrote:
> nit: ganesh

Done.

https://codereview.chromium.org/13863015/diff/64002/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:373: *contents_scale_x = ideal_contents_scale_;
On 2013/05/17 23:24:01, danakj wrote:
> the ideal value still needs to be set on the class. you're using and scaling
by
> 0 here, no? won't this mess up AppendQuads?

OOPS.  I will set all values here for sanity.

https://codereview.chromium.org/13863015/diff/64002/cc/resources/picture_pile...
File cc/resources/picture_pile_impl_unittest.cc (right):

https://codereview.chromium.org/13863015/diff/64002/cc/resources/picture_pile...
cc/resources/picture_pile_impl_unittest.cc:193: PicturePileImpl::Analysis
analysis;
On 2013/05/17 23:36:31, danakj wrote:
> nit: EXPECT_FALSE(anaysis.is_solid_color) before the Analyze call so we see it
> change

Done.

https://codereview.chromium.org/13863015/diff/64002/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/13863015/diff/64002/cc/test/pixel_test.cc#new...
cc/test/pixel_test.cc:116: void PixelTest::SetUpGLRenderer(bool use_ganesh) {
On 2013/05/17 23:36:31, danakj wrote:
> nit: ganesh

Done.  Nice catch.

[enne (OOO), 2013-05-20 17:37:40]

OWNERS review request:

piman: chrome/browser/chromeos/login/chrome_restart_request.cc

sky: content/browser/renderer_host/render_process_host_impl.cc

jamesr: content/renderer/*

[jamesr, 2013-05-20 17:59:54]

lgtm

[piman, 2013-05-20 18:37:07]

lgtm

[sky, 2013-05-20 19:47:59]

LGTM

[commit-bot: I haz the power, 2013-05-20 19:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/13863015/111001

[commit-bot: I haz the power, 2013-05-20 20:02:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[enne (OOO), 2013-05-20 20:30:26]

bsalomon: OWNER review for skia/ext? (or review for the rest of it too if you'd
like?)

[reed1, 2013-05-20 20:32:09]

skia/ext lgtm

[commit-bot: I haz the power, 2013-05-20 21:37:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/13863015/111001

[commit-bot: I haz the power, 2013-05-20 21:51:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-20 21:54:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/13863015/111001

[commit-bot: I haz the power, 2013-05-21 00:33:50]

Failed to apply patch for cc/output/gl_renderer_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/output/gl_renderer_unittest.cc
  Hunk #1 FAILED at 339.
  1 out of 1 hunk FAILED -- saving rejects to file
cc/output/gl_renderer_unittest.cc.rej

Patch:       cc/output/gl_renderer_unittest.cc
Index: cc/output/gl_renderer_unittest.cc
diff --git a/cc/output/gl_renderer_unittest.cc
b/cc/output/gl_renderer_unittest.cc
index
31f2603483deade017ab7a5eac28d9de3ee0352f..fecdf0f1249e0df7ead177567d3a8c17f90de67d
100644
--- a/cc/output/gl_renderer_unittest.cc
+++ b/cc/output/gl_renderer_unittest.cc
@@ -339,7 +339,8 @@ class GLRendererShaderTest : public testing::Test {
         renderer_(GLRenderer::Create(&mock_client_,
                                      output_surface_.get(),
                                      resource_provider_.get(),
-                                     0)) {}
+                                     0,
+                                     false)) {}
 
   void TestRenderPassProgram() {
     EXPECT_PROGRAM_VALID(renderer_->render_pass_program_);

[commit-bot: I haz the power, 2013-05-21 01:58:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/13863015/125003

[commit-bot: I haz the power, 2013-05-21 08:15:29]

Failed to apply patch for cc/trees/layer_tree_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host_impl.cc
  Hunk #1 FAILED at 1425.
  1 out of 1 hunk FAILED -- saving rejects to file
cc/trees/layer_tree_host_impl.cc.rej

Patch:       cc/trees/layer_tree_host_impl.cc
Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index
b6e597e86b3147e277e3735250f4651c23f1b70c..29024c884b8865bf27fa8ad5d9e79da8dd35d9a2
100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -1425,7 +1425,8 @@ bool LayerTreeHostImpl::InitializeRenderer(
     renderer_ = GLRenderer::Create(this,
                                    output_surface.get(),
                                    resource_provider.get(),
-                                   settings_.highp_threshold_min);
+                                   settings_.highp_threshold_min,
+                                   settings_.force_direct_layer_drawing);
   } else if (output_surface->software_device()) {
     renderer_ = SoftwareRenderer::Create(this,
                                          output_surface.get(),

[commit-bot: I haz the power, 2013-05-21 17:16:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/13863015/145001

[commit-bot: I haz the power, 2013-05-21 21:31:46]

Change committed as 201362