Include mask texture size in RenderPassDrawQuad

Include mask texture size in RenderPassDrawQuad

In order to support GL_TEXTURE_RECTANGLE_ARB, cc::GLRenderer needs to
know the size of the mask texture to unnormalize the texture
coordinates.  This has already been done for TileDrawQuad.

As an optimization, rather than adding two more members onto the already
bloated RenderPassDrawQuad, turn mask_uv_rect into mask_uv_scale.  The
position of the mask is now assumed to always be at the origin of the
quad's transform, which it always is in cc.  This is equivalent to
saying that the mask is always attached to the owning layer of a render
surface (since that's what defines the transform origin of the render
surface).

By making this assumption, the old mask_uv_rect can be calculated in
a helper function from just quad->rect and quad->mask_uv_scale.

With this patch, mask_texture_size is currently unused, but its values
are unit tested.

Depends on https://codereview.chromium.org/652393002/

R=danakj@chromium.org,jamesr@chromium.org
BUG=423533
Committed: https://crrev.com/f6f3fbba490a7529ddd9e82b979a966360e806ea
Cr-Commit-Position: refs/heads/master@{#299932}

[jamesr, 2014-10-15 03:06:29]

mojo/ lgtm

[danakj, 2014-10-15 15:33:35]

https://codereview.chromium.org/659683002/diff/1/cc/layers/render_surface_imp...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/layers/render_surface_imp...
cc/layers/render_surface_impl.cc:209: mask_texture_size =
mask_layer->content_bounds();
with implside painting this is the same as bounds(). does this not actually
represent the size of the mask texture if it's scaled then? is this okay?

https://codereview.chromium.org/659683002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (left):

https://codereview.chromium.org/659683002/diff/1/cc/output/gl_renderer.cc#old...
cc/output/gl_renderer.cc:1378: // Mask textures are oriented vertically flipped
relative to the framebuffer
what happened to this?

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

https://codereview.chromium.org/659683002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:1180: mask_sampler =
SamplerTypeFromTextureTarget(mask_resource_lock->target());
this variable exists only for DCHECK? do the lookup inside a DCHECK then?

https://codereview.chromium.org/659683002/diff/1/cc/output/renderer_pixeltest.cc
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/output/renderer_pixeltest...
cc/output/renderer_pixeltest.cc:1170: TEST_F(GLRendererPixelTest,
RenderPassAndMaskWithPartialQuad) {
?

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:523:
gfx::ToNearestRect(gfx::ScaleRect(quad->mask_uv_rect(),
the uv scale will always be an integer? and why not using the RPDQ function?

https://codereview.chromium.org/659683002/diff/1/cc/quads/draw_quad_unittest.cc
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/quads/draw_quad_unittest....
cc/quads/draw_quad_unittest.cc:448: EXPECT_EQ(mask_uv_scale.ToString(),
copy_quad->mask_uv_scale.ToString());
you don't need to ToString() anymore for these things

https://codereview.chromium.org/659683002/diff/1/cc/quads/render_pass_draw_qu...
File cc/quads/render_pass_draw_quad.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/quads/render_pass_draw_qu...
cc/quads/render_pass_draw_quad.cc:84: gfx::RectF
RenderPassDrawQuad::mask_uv_rect() const {
hacker style should have its defn in the header. i also agree this function is
small and could live in the header. but it's not an accessor so should probly
not use hacker style?

https://codereview.chromium.org/659683002/diff/1/cc/quads/render_pass_draw_qu...
cc/quads/render_pass_draw_quad.cc:85: gfx::RectF mask_uv_rect;
combine these 2 lines? no need to construct then assign. or combine the return
too?

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_com...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_com...
cc/test/render_pass_test_common.cc:127: gfx::Vector2dF(),
resource5 has a size of gfx::Size(73, 26) should we use that in these quads?

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_uti...
File cc/test/render_pass_test_utils.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_uti...
cc/test/render_pass_test_utils.cc:133: gfx::Size(),
this would be a divide by 0 when we try to use the resource making the uvrect
x/y infinity? i guess that's okay in tests atm?

should we at least TODO about it so people can discover this later when it
breaks something?

https://codereview.chromium.org/659683002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:5986: .ToString(),
not need to ToString anymore

https://codereview.chromium.org/659683002/diff/1/mojo/converters/surfaces/sur...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/659683002/diff/1/mojo/converters/surfaces/sur...
mojo/converters/surfaces/surfaces_type_converters.cc:86:
gfx::Vector2dF(mask_uv_scale_as_point.x(),
You can use mask_uv_scale_as_point.OffsetFromOrigin()

[enne (OOO), 2014-10-15 19:10:55]

https://codereview.chromium.org/659683002/diff/1/cc/layers/render_surface_imp...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/layers/render_surface_imp...
cc/layers/render_surface_impl.cc:209: mask_texture_size =
mask_layer->content_bounds();
On 2014/10/15 15:33:34, danakj wrote:
> with implside painting this is the same as bounds(). does this not actually
> represent the size of the mask texture if it's scaled then? is this okay?

Whoa.  Nice catch!

ContentsResourceId now has to return a texture size too.  Here's some more
plumbing to review.

I added some PictureLayerImpl unit tests to make sure this texture size is
legit.

Also, because tests are complicated, I removed the mask size checks from the
LayerTreeHostImpl unit tests, because they weren't creating real resources and
it'd be a lot of churn to make these tests real enough to be able to
successfully reach down into PictureLayerImpl::ContentsResourceId and get back a
size.

https://codereview.chromium.org/659683002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (left):

https://codereview.chromium.org/659683002/diff/1/cc/output/gl_renderer.cc#old...
cc/output/gl_renderer.cc:1378: // Mask textures are oriented vertically flipped
relative to the framebuffer
On 2014/10/15 15:33:35, danakj wrote:
> what happened to this?

OOPS

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

https://codereview.chromium.org/659683002/diff/1/cc/output/gl_renderer.cc#new...
cc/output/gl_renderer.cc:1180: mask_sampler =
SamplerTypeFromTextureTarget(mask_resource_lock->target());
On 2014/10/15 15:33:35, danakj wrote:
> this variable exists only for DCHECK? do the lookup inside a DCHECK then?

Sure, for now.  In the next patch for adding support for different targets, I'll
need to branch on it to scale the texture coords differently.

https://codereview.chromium.org/659683002/diff/1/cc/output/renderer_pixeltest.cc
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/output/renderer_pixeltest...
cc/output/renderer_pixeltest.cc:1170: TEST_F(GLRendererPixelTest,
RenderPassAndMaskWithPartialQuad) {
On 2014/10/15 15:33:35, danakj wrote:
> ?

Sorry for the messy patch.  I was cleaning it up and broke stuff, so reverted to
an earlier in-progress patch where I'd added printfs and hacked this to only do
GL so I knew which one I was testing.  Oops.  :(

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:523:
gfx::ToNearestRect(gfx::ScaleRect(quad->mask_uv_rect(),
On 2014/10/15 15:33:35, danakj wrote:
> the uv scale will always be an integer?

Sadly, it won't always be an integer.  Previously, we just silently floored from
floats to ints.  Skia only accepts an integer rect here, so I have to pick
something.  I figured rounding made more sense.

> and why not using the RPDQ function?

Not sure what you mean.  Isn't "quad->mask_uv_rect()" using it?  This is
converting from normalized texture coordinates to texels.

https://codereview.chromium.org/659683002/diff/1/cc/quads/draw_quad_unittest.cc
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/quads/draw_quad_unittest....
cc/quads/draw_quad_unittest.cc:448: EXPECT_EQ(mask_uv_scale.ToString(),
copy_quad->mask_uv_scale.ToString());
On 2014/10/15 15:33:35, danakj wrote:
> you don't need to ToString() anymore for these things

Punt.  The rest of the file ToStrings everything, so it's better to be
consistent.

https://codereview.chromium.org/659683002/diff/1/cc/quads/render_pass_draw_qu...
File cc/quads/render_pass_draw_quad.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/quads/render_pass_draw_qu...
cc/quads/render_pass_draw_quad.cc:84: gfx::RectF
RenderPassDrawQuad::mask_uv_rect() const {
On 2014/10/15 15:33:35, danakj wrote:
> hacker style should have its defn in the header. i also agree this function is
> small and could live in the header. but it's not an accessor so should probly
> not use hacker style?

Done.

https://codereview.chromium.org/659683002/diff/1/cc/quads/render_pass_draw_qu...
cc/quads/render_pass_draw_quad.cc:85: gfx::RectF mask_uv_rect;
On 2014/10/15 15:33:35, danakj wrote:
> combine these 2 lines? no need to construct then assign. or combine the return
> too?

Done.

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_com...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_com...
cc/test/render_pass_test_common.cc:127: gfx::Vector2dF(),
On 2014/10/15 15:33:35, danakj wrote:
> resource5 has a size of gfx::Size(73, 26) should we use that in these quads?

Done.

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_uti...
File cc/test/render_pass_test_utils.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/test/render_pass_test_uti...
cc/test/render_pass_test_utils.cc:133: gfx::Size(),
On 2014/10/15 15:33:35, danakj wrote:
> this would be a divide by 0 when we try to use the resource making the uvrect
> x/y infinity? i guess that's okay in tests atm?
> 
> should we at least TODO about it so people can discover this later when it
> breaks something?

I'll make it 1, 1.

https://codereview.chromium.org/659683002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:5986: .ToString(),
On 2014/10/15 15:33:35, danakj wrote:
> not need to ToString anymore

PUNT

https://codereview.chromium.org/659683002/diff/1/mojo/converters/surfaces/sur...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/659683002/diff/1/mojo/converters/surfaces/sur...
mojo/converters/surfaces/surfaces_type_converters.cc:86:
gfx::Vector2dF(mask_uv_scale_as_point.x(),
On 2014/10/15 15:33:35, danakj wrote:
> You can use mask_uv_scale_as_point.OffsetFromOrigin()

Done.

[danakj, 2014-10-15 19:37:26]

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:523:
gfx::ToNearestRect(gfx::ScaleRect(quad->mask_uv_rect(),
On 2014/10/15 19:10:54, enne wrote:
> On 2014/10/15 15:33:35, danakj wrote:
> > the uv scale will always be an integer?
> 
> Sadly, it won't always be an integer.  Previously, we just silently floored
from
> floats to ints.  Skia only accepts an integer rect here, so I have to pick
> something.  I figured rounding made more sense.

ToNearestRect will DCHECK if you're not "basically an int rect"

> 
> > and why not using the RPDQ function?
> 
> Not sure what you mean.  Isn't "quad->mask_uv_rect()" using it?  This is
> converting from normalized texture coordinates to texels.

Oh, I was confused. This is done here and not in GL renderer because we use a
0,1 geometry space there but we don't here right?

https://codereview.chromium.org/659683002/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/659683002/diff/20001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:355: resource_id = 0;
*resource_id

https://codereview.chromium.org/659683002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/659683002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.h:142: virtual void
ContentsResourceId(ResourceProvider::ResourceId* resource_id,
Time to name this GetContentsResourceId?

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

https://codereview.chromium.org/659683002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:1243: float device_scale = 2.7f;
these look fun but you might as well set dsf and psf to 1 since they don't
matter for this test

https://codereview.chromium.org/659683002/diff/20001/cc/quads/render_pass_dra...
File cc/quads/render_pass_draw_quad.cc (right):

https://codereview.chromium.org/659683002/diff/20001/cc/quads/render_pass_dra...
cc/quads/render_pass_draw_quad.cc:85: gfx::RectF mask_uv_rect =
nit: Foo foo = Foo(a,b,c) -> Foo foo(a, b, c) ?

[enne (OOO), 2014-10-15 19:49:03]

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer.cc
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/659683002/diff/1/cc/output/software_renderer....
cc/output/software_renderer.cc:523:
gfx::ToNearestRect(gfx::ScaleRect(quad->mask_uv_rect(),
On 2014/10/15 19:37:26, danakj wrote:
> On 2014/10/15 19:10:54, enne wrote:
> > On 2014/10/15 15:33:35, danakj wrote:
> > > the uv scale will always be an integer?
> > 
> > Sadly, it won't always be an integer.  Previously, we just silently floored
> from
> > floats to ints.  Skia only accepts an integer rect here, so I have to pick
> > something.  I figured rounding made more sense.
> 
> ToNearestRect will DCHECK if you're not "basically an int rect"

That's...awkward.  I guess I'll ToEnclosedRect this and we can revisit if it
breaks things.

> > > and why not using the RPDQ function?
> > 
> > Not sure what you mean.  Isn't "quad->mask_uv_rect()" using it?  This is
> > converting from normalized texture coordinates to texels.
> 
> Oh, I was confused. This is done here and not in GL renderer because we use a
> 0,1 geometry space there but we don't here right?

Yes.

https://codereview.chromium.org/659683002/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/659683002/diff/20001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:355: resource_id = 0;
On 2014/10/15 19:37:26, danakj wrote:
> *resource_id

Done.

https://codereview.chromium.org/659683002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/659683002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.h:142: virtual void
ContentsResourceId(ResourceProvider::ResourceId* resource_id,
On 2014/10/15 19:37:26, danakj wrote:
> Time to name this GetContentsResourceId?

Sure.

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

https://codereview.chromium.org/659683002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:1243: float device_scale = 2.7f;
On 2014/10/15 19:37:26, danakj wrote:
> these look fun but you might as well set dsf and psf to 1 since they don't
> matter for this test

Done.

https://codereview.chromium.org/659683002/diff/20001/cc/quads/render_pass_dra...
File cc/quads/render_pass_draw_quad.cc (right):

https://codereview.chromium.org/659683002/diff/20001/cc/quads/render_pass_dra...
cc/quads/render_pass_draw_quad.cc:85: gfx::RectF mask_uv_rect =
On 2014/10/15 19:37:26, danakj wrote:
> nit: Foo foo = Foo(a,b,c) -> Foo foo(a, b, c) ?

Done.

[danakj, 2014-10-15 19:53:06]

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

https://codereview.chromium.org/659683002/diff/40001/cc/output/software_rende...
cc/output/software_renderer.cc:522: SkRect mask_rect = gfx::RectToSkRect(
SkMatrix::setRectToRect takes an SkRect which is not an integer rect. Why
ToEnclosedRect at all? Just gfx::RectFToSkRect?

[enne (OOO), 2014-10-15 22:23:07]

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

https://codereview.chromium.org/659683002/diff/40001/cc/output/software_rende...
cc/output/software_renderer.cc:522: SkRect mask_rect = gfx::RectToSkRect(
On 2014/10/15 19:53:05, danakj wrote:
> SkMatrix::setRectToRect takes an SkRect which is not an integer rect. Why
> ToEnclosedRect at all? Just gfx::RectFToSkRect?

OOPS.  Good point!  Done.

[danakj, 2014-10-15 22:23:52]

LGTM

[enne (OOO), 2014-10-15 22:25:19]

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

[enne (OOO), 2014-10-15 22:26:10]

inferno: content/common/cc_messages.h OWNERS? Just changing a RectF to a
Vector2dF and a Size.

[aarya, 2014-10-15 22:29:47]

aarya@google.com changed reviewers:
+ tsepez@chromium.org
- inferno@chromium.org

[enne (OOO), 2014-10-16 17:47:09]

tsepez: content/common/cc_messages.h OWNERS?

[Tom Sepez, 2014-10-16 17:47:32]

Messages LGTM.

[enne (OOO), 2014-10-16 17:55:20]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:57:10]

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

[commit-bot: I haz the power, 2014-10-16 18:17:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-16 18:19:28]

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