cc: Support anti-aliasing for solid color layers.

cc: Support anti-aliasing for solid color layers.

Render solid color layers using the same edge AA method used for
tiled layers and render surfaces.

BUG=166570
TEST=manual

[reveman, 2012-12-21 20:46:53]

This is work in progress but I'd like some feedback before moving forward.

Latest patch adds per-edge anti-aliasing to solid color layers. Looking at
SolidColorLayerImpl we need this as these layers are currently split up in tiles
for the sake of efficient occlusion culling.

There's currently a large amount of code duplication between
GLRenderer::drawSolidColorQuad and GLRenderer::drawTileQuad, which I'm planning
to reduce. Should we support anti-aliasing for other layer types too? Seems like
any layer that can be transformed need aa support. i.e. checkerboard and video
layers.

[danakj, 2012-12-21 20:50:18]

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h
File cc/solid_color_draw_quad.h (right):

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h...
cc/solid_color_draw_quad.h:40: // TODO(danakj): Stick the data used to compute
these things in the quad
Maybe we can address this TODO at the same time?

It seems like the data could/should go in shared_quad_state, then all the solid
color quad callsites won't need to change in this CL. And the gl_renderer can do
AA for all quad types given the right inputs in the shared_quad_state.

[reveman, 2012-12-21 20:58:05]

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h
File cc/solid_color_draw_quad.h (right):

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h...
cc/solid_color_draw_quad.h:40: // TODO(danakj): Stick the data used to compute
these things in the quad
On 2012/12/21 20:50:18, danakj wrote:
> Maybe we can address this TODO at the same time?
> 
> It seems like the data could/should go in shared_quad_state, then all the
solid
> color quad callsites won't need to change in this CL. And the gl_renderer can
do
> AA for all quad types given the right inputs in the shared_quad_state.

hm, not sure I understand. isn't the shared state shared between all tiles?
these aa properties will be different between each tile as they are set based on
if the tile is at the edge of the layer or not.

[danakj, 2012-12-21 22:18:57]

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h
File cc/solid_color_draw_quad.h (right):

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h...
cc/solid_color_draw_quad.h:40: // TODO(danakj): Stick the data used to compute
these things in the quad
On 2012/12/21 20:58:05, David Reveman wrote:
> On 2012/12/21 20:50:18, danakj wrote:
> > Maybe we can address this TODO at the same time?
> > 
> > It seems like the data could/should go in shared_quad_state, then all the
> solid
> > color quad callsites won't need to change in this CL. And the gl_renderer
can
> do
> > AA for all quad types given the right inputs in the shared_quad_state.
> 
> hm, not sure I understand. isn't the shared state shared between all tiles?
> these aa properties will be different between each tile as they are set based
on
> if the tile is at the edge of the layer or not.

Ah, yes. I guess we need some piece of data on each tile (if it's the edge). But
other data used in the decision could go on the shared_quad_state (maybe it's
all there already, I didn't look at this too hard yet).

[reveman, 2012-12-21 23:33:45]

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h
File cc/solid_color_draw_quad.h (right):

https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h...
cc/solid_color_draw_quad.h:40: // TODO(danakj): Stick the data used to compute
these things in the quad
On 2012/12/21 22:18:58, danakj wrote:
> On 2012/12/21 20:58:05, David Reveman wrote:
> > On 2012/12/21 20:50:18, danakj wrote:
> > > Maybe we can address this TODO at the same time?
> > > 
> > > It seems like the data could/should go in shared_quad_state, then all the
> > solid
> > > color quad callsites won't need to change in this CL. And the gl_renderer
> can
> > do
> > > AA for all quad types given the right inputs in the shared_quad_state.
> > 
> > hm, not sure I understand. isn't the shared state shared between all tiles?
> > these aa properties will be different between each tile as they are set
based
> on
> > if the tile is at the edge of the layer or not.
> 
> Ah, yes. I guess we need some piece of data on each tile (if it's the edge).
But
> other data used in the decision could go on the shared_quad_state (maybe it's
> all there already, I didn't look at this too hard yet).

the only piece of data shared between tiles is:
bool useAA = m_tiler->hasBorderTexels() && !isAxisAlignedInTarget;

which I don't think is necessary. The isAxisAlignedInTarget check is done in
drawTileQuad already so I think we change this into:
DCHECK(m_tiler->hasBorderTexels() || isAxisAlignedInTarget)

[danakj, 2012-12-21 23:38:47]

On 2012/12/21 23:33:45, David Reveman wrote:
> https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h
> File cc/solid_color_draw_quad.h (right):
> 
>
https://codereview.chromium.org/11649005/diff/2001/cc/solid_color_draw_quad.h...
> cc/solid_color_draw_quad.h:40: // TODO(danakj): Stick the data used to compute
> these things in the quad
> On 2012/12/21 22:18:58, danakj wrote:
> > On 2012/12/21 20:58:05, David Reveman wrote:
> > > On 2012/12/21 20:50:18, danakj wrote:
> > > > Maybe we can address this TODO at the same time?
> > > > 
> > > > It seems like the data could/should go in shared_quad_state, then all
the
> > > solid
> > > > color quad callsites won't need to change in this CL. And the
gl_renderer
> > can
> > > do
> > > > AA for all quad types given the right inputs in the shared_quad_state.
> > > 
> > > hm, not sure I understand. isn't the shared state shared between all
tiles?
> > > these aa properties will be different between each tile as they are set
> based
> > on
> > > if the tile is at the edge of the layer or not.
> > 
> > Ah, yes. I guess we need some piece of data on each tile (if it's the edge).
> But
> > other data used in the decision could go on the shared_quad_state (maybe
it's
> > all there already, I didn't look at this too hard yet).
> 
> the only piece of data shared between tiles is:
> bool useAA = m_tiler->hasBorderTexels() && !isAxisAlignedInTarget;

I think we're going to have to turn on border texels for all layers under
ubercomp, at least in the renderer compositor. So I think it's fair to have the
gl_renderer assume that if !isAxisAligned then hasBorderTexels() is true.

> which I don't think is necessary. The isAxisAlignedInTarget check is done in
> drawTileQuad already so I think we change this into:
> DCHECK(m_tiler->hasBorderTexels() || isAxisAlignedInTarget)

That'd be cool. Even better would be a DCHECK in gl_renderer that border texels
are present, if it can discern that somehow from tile size or something? So we
can notice when we screw up ubercomp :)

[enne (OOO), 2012-12-27 02:03:37]

What's the use case here? When do we have solid color quads that aren't axis
aligned?

Also, we already have border texels on everything (see
https://chromiumcodereview.appspot.com/11360205).

[reveman, 2012-12-27 08:05:09]

On 2012/12/27 02:03:37, enne wrote:
> What's the use case here? When do we have solid color quads that aren't axis
> aligned?

WebKit has apparently learned how to use solid color layers as backgrounds. If
we hook that up we'll have non-axis aligned solid color quads. More details:
http://code.google.com/p/chromium/issues/detail?id=166570

> 
> Also, we already have border texels on everything (see
> https://chromiumcodereview.appspot.com/11360205).

Ok, great. I hope the dead border texel code is removed soon too.

[jamesr, 2013-01-03 00:58:55]

Can we use a struct with reasonable names and a default constructor instead of
passing groups of 4 bools around everywhere?

[reveman, 2013-01-03 17:07:00]

On 2013/01/03 00:58:55, jamesr wrote:
> Can we use a struct with reasonable names and a default constructor instead of
> passing groups of 4 bools around everywhere?

Sure.

I'd also like to move these aa properties to the DrawQuad base class as from a
quad api perspective, any quad that can be transformed should also be allowed to
support anti-aliasing.

[danakj, 2013-01-03 17:34:57]

On Thu, Jan 3, 2013 at 12:07 PM, <reveman@chromium.org> wrote:

> On 2013/01/03 00:58:55, jamesr wrote:
>
>> Can we use a struct with reasonable names and a default constructor
>> instead of
>> passing groups of 4 bools around everywhere?
>>
>
> Sure.
>
> I'd also like to move these aa properties to the DrawQuad base class as
> from a
> quad api perspective, any quad that can be transformed should also be
> allowed to
> support anti-aliasing.
>

Sounds nice.

[reveman, 2013-01-08 22:23:15]

Added DrawQuad::AntiAliasing struct. PTAL.

[jamesr, 2013-01-08 22:29:26]

https://codereview.chromium.org/11649005/diff/16001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11649005/diff/16001/cc/gl_renderer.cc#newcode763
cc/gl_renderer.cc:763: LayerQuad::Edge rightEdge(topRight, bottomRight);
Tons of this code is copy-pasted from drawTileQuad.  Please extract the common
code out

[danakj, 2013-01-08 22:30:46]

https://codereview.chromium.org/11649005/diff/16001/cc/draw_quad.h
File cc/draw_quad.h (right):

https://codereview.chromium.org/11649005/diff/16001/cc/draw_quad.h#newcode31
cc/draw_quad.h:31: struct AntiAliasing {
Should this instead be "Layer Edgeness" or something? And when it's true and the
transform is does not preserve axis-alignment, then you'd end up with
antialiasing?

Hardcoding the full "is antialiased" decision in the quad conflicts with the
browser process modifying the transforms used to draw the quad.

[danakj, 2013-01-08 22:34:16]

https://codereview.chromium.org/11649005/diff/16001/cc/draw_quad.h
File cc/draw_quad.h (right):

https://codereview.chromium.org/11649005/diff/16001/cc/draw_quad.h#newcode37
cc/draw_quad.h:37: AntiAliasing(bool left_edge,
I'm also wondering what is the benefit of this struct instead of 4 bools on the
DrawQuad? It will appear in only one place, and forces subclasses to construct
an additional thing and pass it in, instead of passing in the bools directly.

[jamesr, 2013-01-08 23:01:58]

https://codereview.chromium.org/11649005/diff/16001/content/common/cc_message...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/11649005/diff/16001/content/common/cc_message...
content/common/cc_messages.cc:660: void
ParamTraits<cc::DrawQuad::AntiAliasing>::Write(
yeah this really doesn't make sense to serialize since AA decisions depend on
the draw transforms, which (by definition) can't be accurate in a child
compositor since it has no idea how it will be rendered

[reveman, 2013-01-09 14:20:05]

https://chromiumcodereview.appspot.com/11649005/diff/16001/cc/draw_quad.h
File cc/draw_quad.h (right):

https://chromiumcodereview.appspot.com/11649005/diff/16001/cc/draw_quad.h#new...
cc/draw_quad.h:31: struct AntiAliasing {
On 2013/01/08 22:30:46, danakj wrote:
> Should this instead be "Layer Edgeness" or something? And when it's true and
the
> transform is does not preserve axis-alignment, then you'd end up with
> antialiasing?
> 
> Hardcoding the full "is antialiased" decision in the quad conflicts with the
> browser process modifying the transforms used to draw the quad.

Yea, this is not the full anti-aliasing decision. This is "what edges are
allowed to be anti-aliased". The real decision about aa is done by the parent
compositor based on these properties and the transform.

We could rename this to LayerEdges or in that case I prefer to just store the
layer content bounds in the shared quad state and the parent compositor can
determine if it's an edge by comparing the quad rect to the layer content rect.

Separate aa properties makes sense if we want to be able to tell the parent
compositor that some edge should never be anti-aliased. Our current code
(PictureLayerImpl, TiledLayerImpl) does that but I think that's incorrect. Maybe
I should start by fixing that?

https://chromiumcodereview.appspot.com/11649005/diff/16001/cc/draw_quad.h#new...
cc/draw_quad.h:37: AntiAliasing(bool left_edge,
On 2013/01/08 22:34:16, danakj wrote:
> I'm also wondering what is the benefit of this struct instead of 4 bools on
the
> DrawQuad? It will appear in only one place, and forces subclasses to construct
> an additional thing and pass it in, instead of passing in the bools directly.

Maybe. The default constructor is somewhat useful but if we use the layer
content bounds none of this is of course needed.

https://chromiumcodereview.appspot.com/11649005/diff/16001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://chromiumcodereview.appspot.com/11649005/diff/16001/cc/gl_renderer.cc#...
cc/gl_renderer.cc:763: LayerQuad::Edge rightEdge(topRight, bottomRight);
On 2013/01/08 22:29:26, jamesr wrote:
> Tons of this code is copy-pasted from drawTileQuad.  Please extract the common
> code out

Definitely. I'm saving this until the rest of the patch is done to avoid having
to do it twice.

https://chromiumcodereview.appspot.com/11649005/diff/16001/content/common/cc_...
File content/common/cc_messages.cc (right):

https://chromiumcodereview.appspot.com/11649005/diff/16001/content/common/cc_...
content/common/cc_messages.cc:660: void
ParamTraits<cc::DrawQuad::AntiAliasing>::Write(
On 2013/01/08 23:01:58, jamesr wrote:
> yeah this really doesn't make sense to serialize since AA decisions depend on
> the draw transforms, which (by definition) can't be accurate in a child
> compositor since it has no idea how it will be rendered

This is not the full AA decision. It tells the parent compositor that it's
allowed to use AA if the layer is transformed but as mentioned in my other
comments we hopefully don't need this.

[enne (OOO), 2013-04-16 20:25:56]

Closing this patch, as AA for solid color layers has already landed.