Splitting of layers for correct intersections

Splitting of layers for correct intersections

Sorting 3d-sorted layers and rendering them in that order causes issues
when layers intersect. Instead place 3d-sorted layers in a bsp tree and
fragment any intersecting layers into non-rectangular quads. We can then
render the fragments in the correct sorted order regardless of
intersections.

BUG=455918, 159225, 132122, 230833
Committed: https://crrev.com/3d8eb1d841ec1ea36906d351bc65af4d7d27718a
Cr-Commit-Position: refs/heads/master@{#320276}

[enne (OOO), 2014-09-24 17:35:28]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2014-09-24 17:35:30]

Not sure if you wanted this for review yet, but here's an initial pass.   Let me
know when you want me to take a closer look.

https://codereview.chromium.org/595593002/diff/1/cc/output/geometry_binding.cc
File cc/output/geometry_binding.cc (right):

https://codereview.chromium.org/595593002/diff/1/cc/output/geometry_binding.c...
cc/output/geometry_binding.cc:125: void
GeometryBindingQuad::InitializeCustomQuad(const gfx::QuadF& quad) {
Can the GeometryBinding stuff become two classes, one for base and one for
custom? I'm not sure that there's shared functionality here that's really
benefiting from the two of them being together.

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

https://codereview.chromium.org/595593002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:1093: gfx::RectF
content_rect(quad->shared_quad_state->content_bounds);
I'm not sure I understand why this is needed.  Also, if you're going to scale to
ShaderGeometryQuad, you should use those values indirectly rather than assuming
them.

https://codereview.chromium.org/595593002/diff/1/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/595593002/diff/1/cc/quads/draw_polygon.cc#new...
cc/quads/draw_polygon.cc:15: static const float coplanar_dot_epsilon =
0.000001f;
Can you give me some examples that helped motivate these heuristics?

https://codereview.chromium.org/595593002/diff/1/cc/quads/draw_polygon.cc#new...
cc/quads/draw_polygon.cc:28: static const float split_threshold = 0.00005f;
Should this split threshold be relative to the compare_threshold?

[enne (OOO), 2015-02-04 21:24:01]

Sorry for the super belated review.

https://codereview.chromium.org/595593002/diff/70033/cc/output/bsp_walk_actio...
File cc/output/bsp_walk_action.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/bsp_walk_actio...
cc/output/bsp_walk_action.cc:29: if
(item->original_ref()->quadTransform().GetInverse(&inverse_transform)) {
Isn't it true that we don't draw things that don't have an inverse? If so,
should this be a dcheck?

https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
File cc/output/geometry_binding.h (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
cc/output/geometry_binding.h:37: class StaticGeometryBinding {
Can you put the different bindings in different headers?

https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
cc/output/geometry_binding.h:59: void InitializeCustomQuadWithUVs(const
gfx::QuadF& quad, const float uv[8]);
const float& uv[8]

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:840: bool GetScaledRegion(const gfx::Rect& rect,
This function needs a comment about what it's doing.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1471: // If an edge is degenerate we do not want to
replace it with a "proper" edge
Oh!  Good catch.  I didn't think about that.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1511: void RotateQuad(gfx::QuadF* clipped_quad) {
Maybe call this RotateQuadToMatchBoudingQuad? (Or some other word than rotate,
given my comments elsewhere?)

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1523: clipped_quad->Rotate((least_error + 1) % 4);
This maybe needs some explanation? Also, a unit test?

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1561: const gfx::QuadF* clip_region_,
clip_region (no underscore)

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2721: PrepareGeometry(NoBinding);
Why does this need to happen? Does NoBinding need to exist?

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:159: void
CreateTestYUVVideoDrawQuad_FromVideoFrame(
Ack, I touched some of these functions in a recent patch, so you're going to
have to eat some conflicts.  :(

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:460: "intersecting_blue_green_texture.png",
What's with the weird aliasing in this image and the software one? It'd really
be nice if all of these used the same image, with fuzz.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:461: "intersecting_blue_green_render_pass.png",
The intersecting_blue_green_render_pass.png looks the same as
intersecting_blue_green.png to me.  Are they that different? Can the same image
be used with fuzz?

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:474: GLImageIndex(TextureDrawQuad, 1);
Whoa, this is a bit obtuse to get a string into a test based on two types.

What about:

template <typename RendererType, typename QuadType>
static const char* IntersectingQuadImage() {
  return "some explicit string";
};

...and then repeat that as needed?

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:509: gfx::Transform trans;
Can you leave a large comment about what this is setting up and what the result
should look like?

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:511: trans.RotateAboutZAxis(46.0);
Why 46 degrees? Is that why there's the weird aliasing in the output?

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:590: this->quad_rect_,
GetColor<TypeParam>(SkColorSetARGB(255, 0, 0, 255)),
Do you think the texture/video/picture/content quads need to be not solid color
(even just two tone, if a split intersects that change in color) in order to
test that uv splitting works correctly? If the quad is solid color, then the uvs
could be totally bogus and the test would still pass.

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
cc/output/software_renderer.cc:294: // TODO(thildebr) For now since
checkerboards shouldn't be part of a 3D
Maybe assign this TODO to somebody else.  Me?

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
File cc/output/software_renderer.h (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
cc/output/software_renderer.h:58: const gfx::QuadF* draw_region = NULL)
override;
style nit: no default parameters in Chromium

https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:103: DCHECK_GE(0.001f,
std::abs(a.normal_.LengthSquared() - 1.0f));
Make a constant for this too?

https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:278: if (current_vertex++ > (points_size + 1)) {
Can you explain this? Maybe also put the increment on a separate line?

https://codereview.chromium.org/595593002/diff/70033/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/595593002/diff/70033/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:18: #include "cc/trees/layer_sorter.h"
Can layer sorter get deleted too?

https://codereview.chromium.org/595593002/diff/70033/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/70033/ui/gfx/geometry/quad_f.h...
ui/gfx/geometry/quad_f.h:70: void Rotate(size_t times) {
Can you leave a comment about what this does? Rotate is also a bit of a weird
word on a quad class, as it could be confused with a spatial transformation. 
Like, if you saw Rotate(2) at a callsite, what would the expectation be?

[awoloszyn, 2015-02-11 21:17:36]

On 2015/02/04 21:24:01, enne wrote:
> Sorry for the super belated review.
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/bsp_walk_actio...
> File cc/output/bsp_walk_action.cc (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/bsp_walk_actio...
> cc/output/bsp_walk_action.cc:29: if
> (item->original_ref()->quadTransform().GetInverse(&inverse_transform)) {
> Isn't it true that we don't draw things that don't have an inverse? If so,
> should this be a dcheck?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
> File cc/output/geometry_binding.h (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
> cc/output/geometry_binding.h:37: class StaticGeometryBinding {
> Can you put the different bindings in different headers?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
> cc/output/geometry_binding.h:59: void InitializeCustomQuadWithUVs(const
> gfx::QuadF& quad, const float uv[8]);
> const float& uv[8]
> 
> https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:840: bool GetScaledRegion(const gfx::Rect& rect,
> This function needs a comment about what it's doing.
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:1471: // If an edge is degenerate we do not want to
> replace it with a "proper" edge
> Oh!  Good catch.  I didn't think about that.
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:1511: void RotateQuad(gfx::QuadF* clipped_quad) {
> Maybe call this RotateQuadToMatchBoudingQuad? (Or some other word than rotate,
> given my comments elsewhere?)
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:1523: clipped_quad->Rotate((least_error + 1) % 4);
> This maybe needs some explanation? Also, a unit test?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:1561: const gfx::QuadF* clip_region_,
> clip_region (no underscore)
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:2721: PrepareGeometry(NoBinding);
> Why does this need to happen? Does NoBinding need to exist?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> File cc/output/renderer_pixeltest.cc (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:159: void
> CreateTestYUVVideoDrawQuad_FromVideoFrame(
> Ack, I touched some of these functions in a recent patch, so you're going to
> have to eat some conflicts.  :(
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:460: "intersecting_blue_green_texture.png",
> What's with the weird aliasing in this image and the software one? It'd really
> be nice if all of these used the same image, with fuzz.
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:461:
"intersecting_blue_green_render_pass.png",
> The intersecting_blue_green_render_pass.png looks the same as
> intersecting_blue_green.png to me.  Are they that different? Can the same
image
> be used with fuzz?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:474: GLImageIndex(TextureDrawQuad, 1);
> Whoa, this is a bit obtuse to get a string into a test based on two types.
> 
> What about:
> 
> template <typename RendererType, typename QuadType>
> static const char* IntersectingQuadImage() {
>   return "some explicit string";
> };
> 
> ...and then repeat that as needed?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:509: gfx::Transform trans;
> Can you leave a large comment about what this is setting up and what the
result
> should look like?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:511: trans.RotateAboutZAxis(46.0);
> Why 46 degrees? Is that why there's the weird aliasing in the output?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:590: this->quad_rect_,
> GetColor<TypeParam>(SkColorSetARGB(255, 0, 0, 255)),
> Do you think the texture/video/picture/content quads need to be not solid
color
> (even just two tone, if a split intersects that change in color) in order to
> test that uv splitting works correctly? If the quad is solid color, then the
uvs
> could be totally bogus and the test would still pass.
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
> File cc/output/software_renderer.cc (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
> cc/output/software_renderer.cc:294: // TODO(thildebr) For now since
> checkerboards shouldn't be part of a 3D
> Maybe assign this TODO to somebody else.  Me?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
> File cc/output/software_renderer.h (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
> cc/output/software_renderer.h:58: const gfx::QuadF* draw_region = NULL)
> override;
> style nit: no default parameters in Chromium
> 
> https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc
> File cc/quads/draw_polygon.cc (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc...
> cc/quads/draw_polygon.cc:103: DCHECK_GE(0.001f,
> std::abs(a.normal_.LengthSquared() - 1.0f));
> Make a constant for this too?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc...
> cc/quads/draw_polygon.cc:278: if (current_vertex++ > (points_size + 1)) {
> Can you explain this? Maybe also put the increment on a separate line?
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_common.cc (left):
> 
>
https://codereview.chromium.org/595593002/diff/70033/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common.cc:18: #include "cc/trees/layer_sorter.h"
> Can layer sorter get deleted too?
> 
> https://codereview.chromium.org/595593002/diff/70033/ui/gfx/geometry/quad_f.h
> File ui/gfx/geometry/quad_f.h (right):
> 
>
https://codereview.chromium.org/595593002/diff/70033/ui/gfx/geometry/quad_f.h...
> ui/gfx/geometry/quad_f.h:70: void Rotate(size_t times) {
> Can you leave a comment about what this does? Rotate is also a bit of a weird
> word on a quad class, as it could be confused with a spatial transformation. 
> Like, if you saw Rotate(2) at a callsite, what would the expectation be?

Put up CL to deal with most comments, just have to remove default parameters +
add multi-colored tests to the pixeltests.

[enne (OOO), 2015-02-11 21:26:16]

Thanks.  I'll take a look once you make the other changes.

Re: pixel tests.  I said two colors, but I guess four colors is necessary to
make sure both u and v stretch ok.

Also, if it's easier, I think I had originally suggested that the pixel tests
just be a single quad (taking up the whole viewport) but then a bunch of other
quads perpendicular (and thus invisible) to the camera, splitting that viewport
quad.  The end result is that it should look the same with and without
splitting, and would probably have less aliasing than rotating.  I don't feel
strongly about doing it this way, but it's a suggestion to make the tests a
little easier to generate consistent results for if you wanted to.

[awoloszyn, 2015-02-12 16:48:52]

https://codereview.chromium.org/595593002/diff/70033/cc/output/bsp_walk_actio...
File cc/output/bsp_walk_action.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/bsp_walk_actio...
cc/output/bsp_walk_action.cc:29: if
(item->original_ref()->quadTransform().GetInverse(&inverse_transform)) {
On 2015/02/04 21:23:59, enne wrote:
> Isn't it true that we don't draw things that don't have an inverse? If so,
> should this be a dcheck?

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
File cc/output/geometry_binding.h (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
cc/output/geometry_binding.h:37: class StaticGeometryBinding {
On 2015/02/04 21:23:59, enne wrote:
> Can you put the different bindings in different headers?

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/geometry_bindi...
cc/output/geometry_binding.h:59: void InitializeCustomQuadWithUVs(const
gfx::QuadF& quad, const float uv[8]);
On 2015/02/04 21:23:59, enne wrote:
> const float& uv[8]

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:840: bool GetScaledRegion(const gfx::Rect& rect,
On 2015/02/04 21:24:00, enne wrote:
> This function needs a comment about what it's doing.

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1511: void RotateQuad(gfx::QuadF* clipped_quad) {
On 2015/02/04 21:23:59, enne wrote:
> Maybe call this RotateQuadToMatchBoudingQuad? (Or some other word than rotate,
> given my comments elsewhere?)

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1523: clipped_quad->Rotate((least_error + 1) % 4);
On 2015/02/04 21:24:00, enne wrote:
> This maybe needs some explanation? Also, a unit test?

Replaced this with a temporary, it is much clearer, and I expect better for
performance than having to rotate the quad some more.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1561: const gfx::QuadF* clip_region_,
On 2015/02/04 21:24:00, enne wrote:
> clip_region (no underscore)

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2721: PrepareGeometry(NoBinding);
On 2015/02/04 21:24:00, enne wrote:
> Why does this need to happen? Does NoBinding need to exist?

Once we have called GetFramebufferPixelsAsync, we have to rebind the geometry.
Currently if SharedBinding is active, and we call
PrepareGeometry(SharedBinding), nothing happens, but this will force it to get
rebound. That being said, it may make sense to move this to EnforceMemoryPolicy.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:159: void
CreateTestYUVVideoDrawQuad_FromVideoFrame(
On 2015/02/04 21:24:00, enne wrote:
> Ack, I touched some of these functions in a recent patch, so you're going to
> have to eat some conflicts.  :(

Turns out it wasn't too bad!

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:460: "intersecting_blue_green_texture.png",
On 2015/02/04 21:24:00, enne wrote:
> What's with the weird aliasing in this image and the software one? It'd really
> be nice if all of these used the same image, with fuzz.

I replaced these images with more sensible ones. We still have 3, but they make
more sense.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:461: "intersecting_blue_green_render_pass.png",
On 2015/02/04 21:24:00, enne wrote:
> The intersecting_blue_green_render_pass.png looks the same as
> intersecting_blue_green.png to me.  Are they that different? Can the same
image
> be used with fuzz?

I replaced most of the pictures with fuzz.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:474: GLImageIndex(TextureDrawQuad, 1);
On 2015/02/04 21:24:00, enne wrote:
> Whoa, this is a bit obtuse to get a string into a test based on two types.
> 
> What about:
> 
> template <typename RendererType, typename QuadType>
> static const char* IntersectingQuadImage() {
>   return "some explicit string";
> };
> 
> ...and then repeat that as needed?

Fixed.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:509: gfx::Transform trans;
On 2015/02/04 21:24:00, enne wrote:
> Can you leave a large comment about what this is setting up and what the
result
> should look like?

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:511: trans.RotateAboutZAxis(46.0);
On 2015/02/04 21:24:00, enne wrote:
> Why 46 degrees? Is that why there's the weird aliasing in the output?

I wanted to avoid picking a very common rotation, it turns out it does not
matter, switched back to 45degs.

https://codereview.chromium.org/595593002/diff/70033/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:590: this->quad_rect_,
GetColor<TypeParam>(SkColorSetARGB(255, 0, 0, 255)),
On 2015/02/04 21:24:00, enne wrote:
> Do you think the texture/video/picture/content quads need to be not solid
color
> (even just two tone, if a split intersects that change in color) in order to
> test that uv splitting works correctly? If the quad is solid color, then the
uvs
> could be totally bogus and the test would still pass.

Fixed, now have a small square inside a large square, should show any problems
with UVs. It exposed a bug in YUVDrawQuads, so thanks for the suggestion!

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
cc/output/software_renderer.cc:294: // TODO(thildebr) For now since
checkerboards shouldn't be part of a 3D
On 2015/02/04 21:24:00, enne wrote:
> Maybe assign this TODO to somebody else.  Me?

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
File cc/output/software_renderer.h (right):

https://codereview.chromium.org/595593002/diff/70033/cc/output/software_rende...
cc/output/software_renderer.h:58: const gfx::QuadF* draw_region = NULL)
override;
On 2015/02/04 21:24:00, enne wrote:
> style nit: no default parameters in Chromium

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:103: DCHECK_GE(0.001f,
std::abs(a.normal_.LengthSquared() - 1.0f));
On 2015/02/04 21:24:00, enne wrote:
> Make a constant for this too?

Done.

https://codereview.chromium.org/595593002/diff/70033/cc/quads/draw_polygon.cc...
cc/quads/draw_polygon.cc:278: if (current_vertex++ > (points_size + 1)) {
On 2015/02/04 21:24:00, enne wrote:
> Can you explain this? Maybe also put the increment on a separate line?

I think this is actually un-needed. I had some really good reason for it a few
months ago, I will remove and make sure it is actually not needed, because I
cannot see any reason at the moment.

https://codereview.chromium.org/595593002/diff/70033/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/595593002/diff/70033/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:18: #include "cc/trees/layer_sorter.h"
On 2015/02/04 21:24:00, enne wrote:
> Can layer sorter get deleted too?

Done.

https://codereview.chromium.org/595593002/diff/70033/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/70033/ui/gfx/geometry/quad_f.h...
ui/gfx/geometry/quad_f.h:70: void Rotate(size_t times) {
On 2015/02/04 21:24:00, enne wrote:
> Can you leave a comment about what this does? Rotate is also a bit of a weird
> word on a quad class, as it could be confused with a spatial transformation. 
> Like, if you saw Rotate(2) at a callsite, what would the expectation be?

Renamed to Realign and added comment.

[awoloszyn, 2015-02-12 16:54:52]

On 2015/02/11 21:26:16, enne wrote:
> Thanks.  I'll take a look once you make the other changes.
> 
> Re: pixel tests.  I said two colors, but I guess four colors is necessary to
> make sure both u and v stretch ok.
> 
> Also, if it's easier, I think I had originally suggested that the pixel tests
> just be a single quad (taking up the whole viewport) but then a bunch of other
> quads perpendicular (and thus invisible) to the camera, splitting that
viewport
> quad.  The end result is that it should look the same with and without
> splitting, and would probably have less aliasing than rotating.  I don't feel
> strongly about doing it this way, but it's a suggestion to make the tests a
> little easier to generate consistent results for if you wanted to.

I think my only problem with having the single quad, and invisible perpendicular
quads is that it will completely miss the cases where we split the quad, and
then just draw the whole
thing twice, rather than the expected pieces, which turned out to be one of the
most common issue that I had. If you can see one quad clearly split by the
other, then it becomes easier to validate that case.

[enne (OOO), 2015-02-12 18:00:27]

Looking really good, mostly just style nits at this point to clean up.

Also, thanks for the test changes; I'm glad they caught something.  :)

https://codereview.chromium.org/595593002/diff/130001/cc/output/bsp_walk_acti...
File cc/output/bsp_walk_action.cc (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/bsp_walk_acti...
cc/output/bsp_walk_action.cc:29: bool b =
item->original_ref()->quadTransform().GetInverse(&inverse_transform);
naming nit: b => invertible?

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

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:354: DoDrawQuad(frame, poly.original_ref(), NULL);
style nit: early return, no else case below

(also NULL => nullptr)

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:358: for (unsigned int i = 0; i < quads.size();
++i) {
unsigned int => size_t

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:377: poly_list->clear();
Wouldn't the BspTree consume the entire list? If so, maybe it should DCHECK
itself?  If not, maybe it should clear it itself rather than making callers do
it?

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:421: int count = 0;
Can you give this a better name? polygon_count? Or next_polygon_id, even?

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:422: int last_context_id = 0;
Please call this last_sorting_context_id.  I know it's wordy, but in
DirectRenderer "context" means different things in different contexts.  ;)

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:427: gfx::Transform quad_transform =
quad.quadTransform();
Maybe no need for this temporary, since it's only used once below.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:459: DoDrawQuad(frame, &quad, NULL);
NULL => nullptr

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:461: FlushPolygons(&poly_list, frame,
render_pass_scissor,
Maybe this should go in FinishDrawingQuadList with the texture cache flush.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.h:119: const gfx::QuadF* draw_region) = 0;
naming: clip_region?

Can you also leave a comment here that clip_region is optional as well as what
it does?

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
File cc/output/dynamic_geometry_binding.h (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
cc/output/dynamic_geometry_binding.h:10: using gpu::gles2::GLES2Interface;
style nit: don't use "using" in a header.

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
cc/output/dynamic_geometry_binding.h:19: void InitializeCustomQuadWithUVs(const
gfx::QuadF& quad, const float* uv);
Can you make this const float uv[8], to be better documented?

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
cc/output/dynamic_geometry_binding.h:19: void InitializeCustomQuadWithUVs(const
gfx::QuadF& quad, const float* uv);
Can you make this const float uv[8], to be better documented?

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
cc/output/dynamic_geometry_binding.h:26:
DISALLOW_COPY_AND_ASSIGN(DynamicGeometryBinding);
style nit: blank line before DISALLOW

https://codereview.chromium.org/595593002/diff/130001/cc/output/geometry_bind...
File cc/output/geometry_binding.h (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/geometry_bind...
cc/output/geometry_binding.h:21: using gpu::gles2::GLES2Interface;
style nit: don't use "using" here either

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:507: const gfx::QuadF* draw_region) {
clip_region?

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:565: // TODO(thildebr) For now since checkerboards
shouldn't be part of a 3D
TODO(enne) here too?

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:846: if (clip) {
style nit: too much indenting.  "if !clip return false" first, then don't indent
the rest of the function.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:862: // and returns a the proportional uv's in the
space 0->1.
"a the"

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:864: if (clip) {
style nit: early out here too.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1407: bool is_top(const gfx::QuadF* clip_region, const
DrawQuad* quad) {
Can you leave a comment for what these functions are doing?

Also, if you want free-floating functions, put them in an anonymous namespace or
make them static.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1410: if (!clip_region) {
style nit: be consistent with using braces or not with one liners.  Most places
in the code don't.  I don't care in general, other than not having a function
use both styles right next to each other.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1625: bool use_aa_on_all_four_edges =
Can you break up this boolean into some temporaries? Maybe an
"all_outside_edges" one?

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1625: bool use_aa_on_all_four_edges =
Can you break up this boolean into some temporaries? Maybe an
"all_outside_edges" one?

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2157: // The transform and vertex data are used to
figure out the extents that the
Thanks for this comment.  Sorry for the confusion about geometry.  GLRenderer is
a bit of a mess in that sense.  <_<

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2305: void GLRenderer::FlushTextureQuadCache(bool
using_clip_region) {
I think this function would be more clear if it took the binding enum instead of
a bool.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2466: uv[0] = scaled_region.p1().x() + 0.5f;
Maybe a comment about why 0.5f here?

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

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:100: enum BoundGeometry { NoBinding, SharedBinding,
ClippedBinding };
style nit: Chromium enums are NO_BINDING, SHARED_BINDING, CLIPPED_BINDING

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:229: const float* uv);
const float uv[8]

https://codereview.chromium.org/595593002/diff/130001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:153: premultiplied_alpha, gfx::PointF(0.0f,
0.0f),  // uv_top_left
If you feel like you need inline comments about what variables are, then maybe
make temporaries with those names instead?

https://codereview.chromium.org/595593002/diff/130001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:221: if (with_alpha)
style nit: needs curly braces for multi-line body

https://codereview.chromium.org/595593002/diff/130001/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/130001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:70: // Realigns the points in the quad by rotating them
n point to the right.
points?

[enne (OOO), 2015-02-12 18:02:52]

> I think my only problem with having the single quad, and invisible
perpendicular quads is that it will completely miss the cases where we split the
quad, and then just draw the whole
> thing twice, rather than the expected pieces, which turned out to be one of
the most common issue that I had. If you can see one quad clearly split by the
other, then it becomes easier to validate that case.

Huh! That's interesting to hear that was such a common failure case.  Then, what
you have seems like the best kind of test.

cc pixel tests are a bit rudimentary in terms of infrastructure, so I always try
to push back on anything that's not axis-aligned (and thus likely to need
painful rebaselining in the future) unless there's a good reason for it, like
there seems to be here.

[awoloszyn, 2015-02-23 22:28:27]

There is an accompanying blink-side change to NeedsManuallyRebaseline I think 4
tests while this goes in, and then once everything is merged properly, I can
commit and rebaseline the changes. (952593004)

https://codereview.chromium.org/595593002/diff/130001/cc/output/bsp_walk_acti...
File cc/output/bsp_walk_action.cc (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/bsp_walk_acti...
cc/output/bsp_walk_action.cc:29: bool b =
item->original_ref()->quadTransform().GetInverse(&inverse_transform);
On 2015/02/12 18:00:26, enne wrote:
> naming nit: b => invertible?

Done.

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

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:354: DoDrawQuad(frame, poly.original_ref(), NULL);
On 2015/02/12 18:00:26, enne wrote:
> style nit: early return, no else case below
> 
> (also NULL => nullptr)

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:358: for (unsigned int i = 0; i < quads.size();
++i) {
On 2015/02/12 18:00:26, enne wrote:
> unsigned int => size_t

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:377: poly_list->clear();
On 2015/02/12 18:00:26, enne wrote:
> Wouldn't the BspTree consume the entire list? If so, maybe it should DCHECK
> itself?  If not, maybe it should clear it itself rather than making callers do
> it?

Actually it does make sure the list is empty. Loops until the list is empty so
should be pretty safe to assume that here. Will switch to dcheck.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:421: int count = 0;
On 2015/02/12 18:00:26, enne wrote:
> Can you give this a better name? polygon_count? Or next_polygon_id, even?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:422: int last_context_id = 0;
On 2015/02/12 18:00:26, enne wrote:
> Please call this last_sorting_context_id.  I know it's wordy, but in
> DirectRenderer "context" means different things in different contexts.  ;)

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:427: gfx::Transform quad_transform =
quad.quadTransform();
On 2015/02/12 18:00:26, enne wrote:
> Maybe no need for this temporary, since it's only used once below.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:459: DoDrawQuad(frame, &quad, NULL);
On 2015/02/12 18:00:26, enne wrote:
> NULL => nullptr

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:461: FlushPolygons(&poly_list, frame,
render_pass_scissor,
On 2015/02/12 18:00:26, enne wrote:
> Maybe this should go in FinishDrawingQuadList with the texture cache flush.

I worried about passing the poly_list, frame, render_pass_scissor, and
using_scissor_as_optimization down to gl_renderer and then making the call back
up. It seemed to add more complication.

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.h:119: const gfx::QuadF* draw_region) = 0;
On 2015/02/12 18:00:26, enne wrote:
> naming: clip_region?
> 
> Can you also leave a comment here that clip_region is optional as well as what
> it does?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
File cc/output/dynamic_geometry_binding.h (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
cc/output/dynamic_geometry_binding.h:10: using gpu::gles2::GLES2Interface;
On 2015/02/12 18:00:26, enne wrote:
> style nit: don't use "using" in a header.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/dynamic_geome...
cc/output/dynamic_geometry_binding.h:26:
DISALLOW_COPY_AND_ASSIGN(DynamicGeometryBinding);
On 2015/02/12 18:00:26, enne wrote:
> style nit: blank line before DISALLOW

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/geometry_bind...
File cc/output/geometry_binding.h (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/geometry_bind...
cc/output/geometry_binding.h:21: using gpu::gles2::GLES2Interface;
On 2015/02/12 18:00:26, enne wrote:
> style nit: don't use "using" here either

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:507: const gfx::QuadF* draw_region) {
On 2015/02/12 18:00:26, enne wrote:
> clip_region?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:565: // TODO(thildebr) For now since checkerboards
shouldn't be part of a 3D
On 2015/02/12 18:00:26, enne wrote:
> TODO(enne) here too?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:846: if (clip) {
On 2015/02/12 18:00:27, enne wrote:
> style nit: too much indenting.  "if !clip return false" first, then don't
indent
> the rest of the function.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:862: // and returns a the proportional uv's in the
space 0->1.
On 2015/02/12 18:00:26, enne wrote:
> "a the"

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:864: if (clip) {
On 2015/02/12 18:00:27, enne wrote:
> style nit: early out here too.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1407: bool is_top(const gfx::QuadF* clip_region, const
DrawQuad* quad) {
On 2015/02/12 18:00:27, enne wrote:
> Can you leave a comment for what these functions are doing?
> 
> Also, if you want free-floating functions, put them in an anonymous namespace
or
> make them static.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1410: if (!clip_region) {
On 2015/02/12 18:00:27, enne wrote:
> style nit: be consistent with using braces or not with one liners.  Most
places
> in the code don't.  I don't care in general, other than not having a function
> use both styles right next to each other.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1625: bool use_aa_on_all_four_edges =
On 2015/02/12 18:00:27, enne wrote:
> Can you break up this boolean into some temporaries? Maybe an
> "all_outside_edges" one?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1625: bool use_aa_on_all_four_edges =
On 2015/02/12 18:00:27, enne wrote:
> Can you break up this boolean into some temporaries? Maybe an
> "all_outside_edges" one?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2305: void GLRenderer::FlushTextureQuadCache(bool
using_clip_region) {
On 2015/02/12 18:00:27, enne wrote:
> I think this function would be more clear if it took the binding enum instead
of
> a bool.

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:2466: uv[0] = scaled_region.p1().x() + 0.5f;
On 2015/02/12 18:00:27, enne wrote:
> Maybe a comment about why 0.5f here?

Done.

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

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:100: enum BoundGeometry { NoBinding, SharedBinding,
ClippedBinding };
On 2015/02/12 18:00:27, enne wrote:
> style nit: Chromium enums are NO_BINDING, SHARED_BINDING, CLIPPED_BINDING

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:229: const float* uv);
On 2015/02/12 18:00:27, enne wrote:
> const float uv[8]

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/595593002/diff/130001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:153: premultiplied_alpha, gfx::PointF(0.0f,
0.0f),  // uv_top_left
On 2015/02/12 18:00:27, enne wrote:
> If you feel like you need inline comments about what variables are, then maybe
> make temporaries with those names instead?

Done.

https://codereview.chromium.org/595593002/diff/130001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:221: if (with_alpha)
On 2015/02/12 18:00:27, enne wrote:
> style nit: needs curly braces for multi-line body

Done.

https://codereview.chromium.org/595593002/diff/130001/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/130001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:70: // Realigns the points in the quad by rotating them
n point to the right.
On 2015/02/12 18:00:27, enne wrote:
> points?

Done.

[enne (OOO), 2015-02-23 23:51:36]

This seems really close.  Can you write a more thorough CL description too?

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

https://codereview.chromium.org/595593002/diff/130001/cc/output/direct_render...
cc/output/direct_renderer.cc:461: FlushPolygons(&poly_list, frame,
render_pass_scissor,
On 2015/02/23 22:28:26, awoloszyn wrote:
> On 2015/02/12 18:00:26, enne wrote:
> > Maybe this should go in FinishDrawingQuadList with the texture cache flush.
> 
> I worried about passing the poly_list, frame, render_pass_scissor, and
> using_scissor_as_optimization down to gl_renderer and then making the call
back
> up. It seemed to add more complication.

Ok, sounds good!

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

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
cc/output/direct_renderer.cc:377: // Clear the polygon list for the next
transformation layer block
Remove this comment?

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
cc/output/direct_renderer.cc:430: if (ShouldSkipQuad(quad, render_pass_scissor))
{
Combine these two conditionals into one if statement if there's no other logic
going on.

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
cc/output/direct_renderer.h:117: // Clip_region is a (possibly null) pointer to
a quad in the same
Clip_region => clip_region

https://codereview.chromium.org/595593002/diff/150001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/150001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1631: (is_top(local_clip_region, quad) &&
is_left(local_clip_region, quad) &&
This logic seems redundant.  If local_clip_region is valid, then
use_aa_on_all_four_edges is always false.  If local_clip_region is nullptr, then
is_top/left/bottom/right is always true.  So, I think maybe this is just:

  bool is_full_rect = quad->visible_rect == quad->rect;
  // If we have a clip region then we are split, and therefore
  // by necessity, at least one of our edges is not an external
  // one.
  bool use_aa_on_all_four_edges =
      !local_clip_region &&
      (is_render_pass_quad || is_full_rect);

https://codereview.chromium.org/595593002/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/595593002/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:9: #include <tuple>
This doesn't compile on Android, no matter what the linter tells you.

[awoloszyn, 2015-02-24 21:28:49]

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

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
cc/output/direct_renderer.cc:377: // Clear the polygon list for the next
transformation layer block
On 2015/02/23 23:51:36, enne wrote:
> Remove this comment?

Done.

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
cc/output/direct_renderer.cc:430: if (ShouldSkipQuad(quad, render_pass_scissor))
{
On 2015/02/23 23:51:36, enne wrote:
> Combine these two conditionals into one if statement if there's no other logic
> going on.

Done.

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/595593002/diff/150001/cc/output/direct_render...
cc/output/direct_renderer.h:117: // Clip_region is a (possibly null) pointer to
a quad in the same
On 2015/02/23 23:51:36, enne wrote:
> Clip_region => clip_region

Done.

https://codereview.chromium.org/595593002/diff/150001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/150001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1631: (is_top(local_clip_region, quad) &&
is_left(local_clip_region, quad) &&
On 2015/02/23 23:51:36, enne wrote:
> This logic seems redundant.  If local_clip_region is valid, then
> use_aa_on_all_four_edges is always false.  If local_clip_region is nullptr,
then
> is_top/left/bottom/right is always true.  So, I think maybe this is just:

If local_clip_region is nullptr then you still might not be (is_top).
This can happen if the quad is not rendering the entire layer (basically if it
is partially occluded).

Iirc the is_full_rect is also needed because your visible_rect && quad->rect are
related to your occluded area rather than your unoccluded area.


> 
>   bool is_full_rect = quad->visible_rect == quad->rect;
>   // If we have a clip region then we are split, and therefore
>   // by necessity, at least one of our edges is not an external
>   // one.
>   bool use_aa_on_all_four_edges =
>       !local_clip_region &&
>       (is_render_pass_quad || is_full_rect);

https://codereview.chromium.org/595593002/diff/150001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/595593002/diff/150001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:9: #include <tuple>
On 2015/02/23 23:51:36, enne wrote:
> This doesn't compile on Android, no matter what the linter tells you.

Done.

[enne (OOO), 2015-02-24 21:35:17]

https://codereview.chromium.org/595593002/diff/150001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/595593002/diff/150001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1631: (is_top(local_clip_region, quad) &&
is_left(local_clip_region, quad) &&
Oh, quite right! My misreading it.

[enne (OOO), 2015-02-24 21:37:37]

lgtm

[awoloszyn, 2015-02-26 17:33:22]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-02-26 17:33:23]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps210001
(title: "Rebase")

[commit-bot: I haz the power, 2015-02-26 17:34:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/210001

[commit-bot: I haz the power, 2015-02-26 17:54:52]

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

[commit-bot: I haz the power, 2015-02-26 17:54:53]

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

[awoloszyn, 2015-02-26 18:30:29]

awoloszyn@chromium.org changed reviewers:
+ danakj@chromium.org

[awoloszyn, 2015-02-26 18:30:47]

Added dana for
    ui/gfx/geometry/quad_f.h
    ui/gfx/skia_util.cc
    ui/gfx/skia_util.h

[danakj, 2015-02-26 18:33:40]

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:72: for (size_t i = 0; i < times % 4; ++i) {
How about DCHECK(times < 4) and remove the mod (mod is expensive on android)?
Will that work?

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/skia_util.h
File ui/gfx/skia_util.h (right):

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/skia_util.h#newc...
ui/gfx/skia_util.h:82: GFX_EXPORT void QuadFToSkPoints(const gfx::QuadF& quad,
SkPoint points[4]);
move this up closer to the Rect/Size methods?

[awoloszyn, 2015-02-26 19:03:40]

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:72: for (size_t i = 0; i < times % 4; ++i) {
On 2015/02/26 18:33:39, danakj wrote:
> How about DCHECK(times < 4) and remove the mod (mod is expensive on android)?
> Will that work?

Should be perfectly fine. Good to know % is slow on android.

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/skia_util.h
File ui/gfx/skia_util.h (right):

https://codereview.chromium.org/595593002/diff/230001/ui/gfx/skia_util.h#newc...
ui/gfx/skia_util.h:82: GFX_EXPORT void QuadFToSkPoints(const gfx::QuadF& quad,
SkPoint points[4]);
On 2015/02/26 18:33:39, danakj wrote:
> move this up closer to the Rect/Size methods?

Done.

[danakj, 2015-02-26 19:04:50]

LGTM

https://codereview.chromium.org/595593002/diff/250001/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/250001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:73: DCHECK(times < 4);
use DCHECK_LT to do this please

https://codereview.chromium.org/595593002/diff/250001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:74: for (size_t i = 0; i < times % 4; ++i) {
remove the mod then? :)

[awoloszyn, 2015-02-26 19:40:41]

https://codereview.chromium.org/595593002/diff/250001/ui/gfx/geometry/quad_f.h
File ui/gfx/geometry/quad_f.h (right):

https://codereview.chromium.org/595593002/diff/250001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:73: DCHECK(times < 4);
On 2015/02/26 19:04:49, danakj wrote:
> use DCHECK_LT to do this please

Done.

https://codereview.chromium.org/595593002/diff/250001/ui/gfx/geometry/quad_f....
ui/gfx/geometry/quad_f.h:74: for (size_t i = 0; i < times % 4; ++i) {
On 2015/02/26 19:04:49, danakj wrote:
> remove the mod then? :)

AKK! Sorry, fixed now.

[danakj, 2015-02-26 19:41:14]

Thanks LGTM

[awoloszyn, 2015-02-26 19:42:53]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-02-26 19:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps270001
(title: " ")

[commit-bot: I haz the power, 2015-02-26 19:44:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/270001

[commit-bot: I haz the power, 2015-02-26 23:09:08]

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

[commit-bot: I haz the power, 2015-02-26 23:09:09]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[awoloszyn, 2015-02-27 14:00:37]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-02-27 14:00:38]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps290001
(title: "Fix windows build.")

[commit-bot: I haz the power, 2015-02-27 14:01:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/290001

[awoloszyn, 2015-02-27 19:12:49]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-02-27 19:12:50]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps310001
(title: "Update to calm down the windows testers.")

[commit-bot: I haz the power, 2015-02-27 19:14:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/310001

[commit-bot: I haz the power, 2015-02-27 20:32:17]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2015-02-27 20:32:48]

Patchset 17 (id:??) landed as
https://crrev.com/ac754705ceaa5bc4cb8dc8df9a733b4b68396399
Cr-Commit-Position: refs/heads/master@{#318506}

[eroman, 2015-02-27 21:45:54]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2015-02-27 21:45:55]

This appears to have triggered a static initializers error on builds, although
not sure why..

# draw_polygon.cc cc::DrawPolygon::default_normal
# draw_polygon.cc gfx::Vector3dF::Vector3dF(float, float, float)

Curiously there was already a static initializer for default_normal prior to
this change which is why I don't understand the alert

[Nico, 2015-02-27 21:48:42]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-02-27 21:48:43]

https://codereview.chromium.org/595593002/diff/310001/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/595593002/diff/310001/cc/quads/draw_polygon.c...
cc/quads/draw_polygon.cc:28: static const float split_threshold =
compare_threshold / 2.0f;
this computation wasn't here before this patch. that's probably what causes the
initializer

[eroman, 2015-02-27 22:09:57]

A revert of this CL (patchset #17 id:310001) has been created in
https://codereview.chromium.org/960893004/ by eroman@chromium.org.

The reason for reverting is: Static initializers count increased.

[awoloszyn, 2015-03-10 21:46:24]

Merge https://codereview.chromium.org/962363002/
and     https://codereview.chromium.org/959403002/

[enne (OOO), 2015-03-10 21:56:41]

Changes lgtm.

[awoloszyn, 2015-03-11 12:43:30]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-03-11 12:43:31]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps330001
(title: "Fixing both g++ errors and static initializers.")

[commit-bot: I haz the power, 2015-03-11 12:44:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/330001

[commit-bot: I haz the power, 2015-03-11 13:14:06]

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

[commit-bot: I haz the power, 2015-03-11 13:14:07]

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

[awoloszyn, 2015-03-11 13:31:37]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-03-11 13:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps350001
(title: "Fixed removed file in gn file")

[commit-bot: I haz the power, 2015-03-11 13:31:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/350001

[awoloszyn, 2015-03-11 15:36:09]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-03-11 15:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps370001
(title: "Added missing image files.")

[commit-bot: I haz the power, 2015-03-11 15:36:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/370001

[commit-bot: I haz the power, 2015-03-11 16:54:31]

Committed patchset #20 (id:370001)

[commit-bot: I haz the power, 2015-03-11 16:56:50]

Patchset 20 (id:??) landed as
https://crrev.com/7e2af77dcd3b5df086be3421fad40b719779381c
Cr-Commit-Position: refs/heads/master@{#320096}

[gab, 2015-03-11 19:32:22]

A revert of this CL (patchset #20 id:370001) has been created in
https://codereview.chromium.org/998023002/ by gab@chromium.org.

The reason for reverting is: Appears to be adding new static initializers and is
breaking Mac/Linux builders, e.g.:
http://build.chromium.org/p/chromium/buildstatus?builder=Mac&number=34562.

[awoloszyn, 2015-03-12 01:01:59]

Removed the default_normal. It was only being used in one place, and was showing
up as a static initializer, (causing the linux and mac builders to complain)

[enne (OOO), 2015-03-12 01:20:55]

Is there a try bot or a local way to see if static_initializers is clean?

[tfarina, 2015-03-12 01:27:56]

On Wed, Mar 11, 2015 at 10:20 PM, <enne@chromium.org> wrote:

> Is there a try bot or a local way to see if static_initializers is clean?
>
> there is a tools/linux/dump-static-initializers.py that I think you could
use. Scott (scottmg) knows more of it.


> https://codereview.chromium.org/595593002/
>



-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2015-03-12 01:35:34]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-03-12 01:35:35]

I think Nico or Lei would be better bets, but yes, you can run that script
locally to see if you've added static initializers (I doubt many people think to
do that before landing though.)

I think for simple cases like this we would normally just leave it red and let
the author fix as we don't want it to regress, but it's not urgent.

[Nico, 2015-03-12 02:50:47]

https://codereview.chromium.org/595593002/diff/410001/cc/quads/draw_polygon.cc
File cc/quads/draw_polygon.cc (right):

https://codereview.chromium.org/595593002/diff/410001/cc/quads/draw_polygon.c...
cc/quads/draw_polygon.cc:58: : normal_(0.0f, 0.0f, 1.0f),
Alternatively, you could do

gfx::Vector3dF kDefaultNormal() { return gfx::Vector3dF(0, 0, 1); }

and then write normal_(kDefaultNormal()) here.

Then the constant is initialized when DrawPolygon objects are created, not on
program startup.


The stdout of the sizes step of the linux and mac bots print a list of static
intializers. You can compare that output to the previous build to see how much
you added. This definitely removes one of them, so if it was just one you should
be ok to reland.

[awoloszyn, 2015-03-12 03:24:37]

On 2015/03/12 02:50:47, Nico (traveling) wrote:
> https://codereview.chromium.org/595593002/diff/410001/cc/quads/draw_polygon.cc
> File cc/quads/draw_polygon.cc (right):
> 
>
https://codereview.chromium.org/595593002/diff/410001/cc/quads/draw_polygon.c...
> cc/quads/draw_polygon.cc:58: : normal_(0.0f, 0.0f, 1.0f),
> Alternatively, you could do
> 
> gfx::Vector3dF kDefaultNormal() { return gfx::Vector3dF(0, 0, 1); }
> 
> and then write normal_(kDefaultNormal()) here.
> 
> Then the constant is initialized when DrawPolygon objects are created, not on
> program startup.
> 
> 
> The stdout of the sizes step of the linux and mac bots print a list of static
> intializers. You can compare that output to the previous build to see how much
> you added. This definitely removes one of them, so if it was just one you
should
> be ok to reland.

I ran dump_static_initializers.py on linux with and without the latest patch,
and got the same results, so it looks like this should be good to go.
I will try to re-land tomorrow morning so I am around if anything else shows up,
unless there are any objections.

[awoloszyn, 2015-03-12 12:24:29]

The CQ bit was checked by awoloszyn@chromium.org

[awoloszyn, 2015-03-12 12:24:30]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/595593002/#ps410001
(title: "Removed the static that was only used in ony place anyway")

[commit-bot: I haz the power, 2015-03-12 12:24:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595593002/410001

[commit-bot: I haz the power, 2015-03-12 14:39:03]

Committed patchset #22 (id:410001)

[commit-bot: I haz the power, 2015-03-12 14:39:49]

Patchset 22 (id:??) landed as
https://crrev.com/3d8eb1d841ec1ea36906d351bc65af4d7d27718a
Cr-Commit-Position: refs/heads/master@{#320276}

[alexst (slow to review), 2015-03-16 19:16:43]

On 2015/03/12 14:39:49, I haz the power (commit-bot) wrote:
> Patchset 22 (id:??) landed as
> https://crrev.com/3d8eb1d841ec1ea36906d351bc65af4d7d27718a
> Cr-Commit-Position: refs/heads/master@{#320276}

Breaks http://www.quirksmode.org/html5/tests/video.html by changing video layer
size when scrolling past the bottom of the screen.

[enne (OOO), 2015-03-16 19:38:46]

alexst: Do you think this needs to be rolled out? If no, can you file a bug and
assign it to awoloszyn?

[xing.xu, 2017-05-03 07:07:17]

xing.xu@intel.com changed reviewers:
+ xing.xu@intel.com

[xing.xu, 2017-05-03 07:07:18]

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

https://codereview.chromium.org/595593002/diff/410001/cc/output/direct_render...
cc/output/direct_renderer.cc:435: FlushPolygons(&poly_list, frame,
render_pass_scissor,
I didn't understand why L460 did the FlushPolygon, and L435 did it again?

[xing.xu, 2017-05-03 12:45:31]

The L460 is for case with sorting context.

But I didn't understand,why the following is for case without sorting context? 
last_sorting_context_id != quad.shared_quad_state->sorting_context_id

Can we use quad.shared_quad_state->sorting_context_id == 0 to judge if this is
case without sorting context?

[xing.xu, 2017-05-03 12:45:34]

The L460 is for case with sorting context.

But I didn't understand,why the following is for case without sorting context? 
last_sorting_context_id != quad.shared_quad_state->sorting_context_id

Can we use quad.shared_quad_state->sorting_context_id == 0 to judge if this is
case without sorting context?

[enne (OOO), 2017-05-03 19:00:14]

I think it's that there can be two different sorting contexts back to back in
the quad list.  Not just a transition from context=X to context=0 but from
context=X to context=Y.  FlushPolygons does a BSP tree walk and sorts/splits
polygons from a single context.  So, if the context ID changes, that means that
one context's quads will not split or sort with the quads from another context,
which is the intended behavior.

[xing.xu, 2017-05-04 06:06:50]

Thanks enne!