Fix masks when scaling is in the tree

Fix masks when scaling is in the tree

Because the render surface is sized according to the screen-space footprint,
there is a scaling factor introduced between the RS and the owning layer, that
was not taken into account for masks. This fixes that.

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

[danakj, 2013-01-15 01:15:43]

Can we have a test for this, probably in LTHCommonTests?

https://codereview.chromium.org/11883027/diff/1/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11883027/diff/1/cc/render_surface_impl.cc#new...
cc/render_surface_impl.cc:233: float scaleX = contentRect().width() /
maskLayer->contentsScaleX() / maskLayer->bounds().width() / maskDrawScale.x();
Cool, but I'm wondering why it's / and not *.

If the layer will be drawing with 2x scale into the surface, then the mask layer
will need to scale up 2x as well. Won't this scale it by 0.5x?

[piman, 2013-01-15 01:35:57]

For the test, I'll see what I can do, I don't think LTHCommonTests is
appropriate since it doesn't deal with quads.

https://codereview.chromium.org/11883027/diff/1/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11883027/diff/1/cc/render_surface_impl.cc#new...
cc/render_surface_impl.cc:233: float scaleX = contentRect().width() /
maskLayer->contentsScaleX() / maskLayer->bounds().width() / maskDrawScale.x();
On 2013/01/15 01:15:44, danakj wrote:
> Cool, but I'm wondering why it's / and not *.
> 
> If the layer will be drawing with 2x scale into the surface, then the mask
layer
> will need to scale up 2x as well. Won't this scale it by 0.5x?

It changes the uv rect, so it's more like an inverse transform: the screen
footprint of the quad doesn't change, we reference a 2x smaller region of the
mask texture, 2x fewer texels per pixels mean 2x more pixels per texel, i.e.
texture gets scaled up 2x.

[danakj, 2013-01-15 01:39:05]

Thanks for doing all the legwork on this one.

https://codereview.chromium.org/11883027/diff/1/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11883027/diff/1/cc/render_surface_impl.cc#new...
cc/render_surface_impl.cc:233: float scaleX = contentRect().width() /
maskLayer->contentsScaleX() / maskLayer->bounds().width() / maskDrawScale.x();
On 2013/01/15 01:35:57, piman wrote:
> On 2013/01/15 01:15:44, danakj wrote:
> > Cool, but I'm wondering why it's / and not *.
> > 
> > If the layer will be drawing with 2x scale into the surface, then the mask
> layer
> > will need to scale up 2x as well. Won't this scale it by 0.5x?
> 
> It changes the uv rect, so it's more like an inverse transform: the screen
> footprint of the quad doesn't change, we reference a 2x smaller region of the
> mask texture, 2x fewer texels per pixels mean 2x more pixels per texel, i.e.
> texture gets scaled up 2x.

Oh, magic. Thanks for the explanation. This LGTM but would really like a test.
You're right about quads.. I think this would be a good candidate for a pixel
test, but I'm not sure if we're there yet for this kind of scenario.

[piman, 2013-01-15 03:15:37]

I added a LayerTreeHostImplTest which seemed like the best fit.

[enne (OOO), 2013-01-15 04:12:43]

lgtm to land this fix, especially since you're adding tests.  

I think maybe this could be done better in the future, but I don't see any
reason not to land this now.

https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc#...
cc/render_surface_impl.cc:232: gfx::Vector2dF maskDrawScale =
MathUtil::computeTransform2dScaleComponents(m_owningLayer->drawTransform(),
1.f);
This will work given the current implementation of LTHCommon, but I had once
hoped that we could separate the scale of the owning layer from the scale of the
surface (in case you had some image with a bizarre scale that created a surface,
and then everything inside would get scaled to it).  It almost seems like the
mask layer's content's scale should just take this surface scale into account
and then this owning layer detail could be entirely hidden in
LayerTreeHostCommon.

[commit-bot: I haz the power, 2013-01-15 04:24:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/11883027/8001

[piman, 2013-01-15 04:32:09]

On Mon, Jan 14, 2013 at 8:12 PM, <enne@chromium.org> wrote:

> lgtm to land this fix, especially since you're adding tests.
>
> I think maybe this could be done better in the future, but I don't see any
> reason not to land this now.
>
>
> https://codereview.chromium.**org/11883027/diff/8001/cc/**
>
render_surface_impl.cc<https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc>
> File cc/render_surface_impl.cc (right):
>
> https://codereview.chromium.**org/11883027/diff/8001/cc/**
>
render_surface_impl.cc#**newcode232<https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc#newcode232>
> cc/render_surface_impl.cc:232: gfx::Vector2dF maskDrawScale =
> MathUtil::**computeTransform2dScaleCompone**nts(m_owningLayer->**
> drawTransform(),
> 1.f);
> This will work given the current implementation of LTHCommon, but I had
> once hoped that we could separate the scale of the owning layer from the
> scale of the surface (in case you had some image with a bizarre scale
> that created a surface, and then everything inside would get scaled to
> it).  It almost seems like the mask layer's content's scale should just
> take this surface scale into account and then this owning layer detail
> could be entirely hidden in LayerTreeHostCommon.
>

I think it makes sense to move that logic into LTHCommon. I think we need
to be careful though, to make sure we keep the property that the mask and
the owning layer have the same content scale, because I think rasterizing
at different resolutions for the content and the mask could cause content
pixels that should be hidden to leak though or vice versa.


>
>
https://codereview.chromium.**org/11883027/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-01-15 08:30:42]

Change committed as 176850

[danakj, 2013-01-15 16:31:10]

lgtm2 and thanks for the test!

https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc#...
cc/render_surface_impl.cc:232: gfx::Vector2dF maskDrawScale =
MathUtil::computeTransform2dScaleComponents(m_owningLayer->drawTransform(),
1.f);
On 2013/01/15 04:12:43, enne wrote:
> This will work given the current implementation of LTHCommon, but I had once
> hoped that we could separate the scale of the owning layer from the scale of
the
> surface (in case you had some image with a bizarre scale that created a
surface,
> and then everything inside would get scaled to it).  It almost seems like the
> mask layer's content's scale should just take this surface scale into account
> and then this owning layer detail could be entirely hidden in
> LayerTreeHostCommon.

I'm not sure I agree. My POV is that the mask is assumed to share the same
contents scale as the owning layer, so I think the mask needs to be drawn with
the same scale as the owning layer is. The "surface's scale" is in theory
independent of the owning layer's draw transform, but the mask layer should
continue to draw at the same scale as the owning layer to have its pixel
positions aligned.

[danakj, 2013-01-15 17:11:53]

https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc#...
cc/render_surface_impl.cc:232: gfx::Vector2dF maskDrawScale =
MathUtil::computeTransform2dScaleComponents(m_owningLayer->drawTransform(),
1.f);
On 2013/01/15 16:31:11, danakj wrote:
> On 2013/01/15 04:12:43, enne wrote:
> > This will work given the current implementation of LTHCommon, but I had once
> > hoped that we could separate the scale of the owning layer from the scale of
> the
> > surface (in case you had some image with a bizarre scale that created a
> surface,
> > and then everything inside would get scaled to it).  It almost seems like
the
> > mask layer's content's scale should just take this surface scale into
account
> > and then this owning layer detail could be entirely hidden in
> > LayerTreeHostCommon.
> 
> I'm not sure I agree. My POV is that the mask is assumed to share the same
> contents scale as the owning layer, so I think the mask needs to be drawn with
> the same scale as the owning layer is. The "surface's scale" is in theory
> independent of the owning layer's draw transform, but the mask layer should
> continue to draw at the same scale as the owning layer to have its pixel
> positions aligned.

Ok I take this back. I had developed an impression that the mask only applies to
its layer. But enne@ pointed me to https://www.webkit.org/blog/181/css-masks/
where it states:

"The mask will therefore overlay the child and all of its descendants"

So I guess we want to scale it to fill the entire render surface.

Which makes me then wonder why this wasn't working for this case. Is it because
the mask layer was being added to the root layer, so the contentRect of the
surface didn't match the size of the subtree? or because aura is drawing a mask
assuming it matches the size of the owning layer and not its subtree?

[danakj, 2013-01-15 17:16:57]

On 2013/01/15 17:11:53, danakj wrote:
> So I guess we want to scale it to fill the entire render surface.
> 
> Which makes me then wonder why this wasn't working for this case. Is it
because
> the mask layer was being added to the root layer, so the contentRect of the
> surface didn't match the size of the subtree? or because aura is drawing a
mask
> assuming it matches the size of the owning layer and not its subtree?

Oh, let's call this a case of the mondays. It's the scale which it can't know
about of course, yeh. So basically I agree with enne@ and we can find a more
accurate scale value than this in l_t_h_common. Wee!

[danakj, 2013-01-15 17:19:32]

Filed http://code.google.com/p/chromium/issues/detail?id=170076 to clean this up
for m26.

[piman, 2013-01-15 19:12:43]

On Tue, Jan 15, 2013 at 9:11 AM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/11883027/diff/8001/cc/**
>
render_surface_impl.cc<https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc>
> File cc/render_surface_impl.cc (right):
>
> https://codereview.chromium.**org/11883027/diff/8001/cc/**
>
render_surface_impl.cc#**newcode232<https://codereview.chromium.org/11883027/diff/8001/cc/render_surface_impl.cc#newcode232>
> cc/render_surface_impl.cc:232: gfx::Vector2dF maskDrawScale =
> MathUtil::**computeTransform2dScaleCompone**nts(m_owningLayer->**
> drawTransform(),
> 1.f);
> On 2013/01/15 16:31:11, danakj wrote:
>
>> On 2013/01/15 04:12:43, enne wrote:
>> > This will work given the current implementation of LTHCommon, but I
>>
> had once
>
>> > hoped that we could separate the scale of the owning layer from the
>>
> scale of
>
>> the
>> > surface (in case you had some image with a bizarre scale that
>>
> created a
>
>> surface,
>> > and then everything inside would get scaled to it).  It almost seems
>>
> like the
>
>> > mask layer's content's scale should just take this surface scale
>>
> into account
>
>> > and then this owning layer detail could be entirely hidden in
>> > LayerTreeHostCommon.
>>
>
>  I'm not sure I agree. My POV is that the mask is assumed to share the
>>
> same
>
>> contents scale as the owning layer, so I think the mask needs to be
>>
> drawn with
>
>> the same scale as the owning layer is. The "surface's scale" is in
>>
> theory
>
>> independent of the owning layer's draw transform, but the mask layer
>>
> should
>
>> continue to draw at the same scale as the owning layer to have its
>>
> pixel
>
>> positions aligned.
>>
>
> Ok I take this back. I had developed an impression that the mask only
> applies to its layer. But enne@ pointed me to
>
https://www.webkit.org/blog/**181/css-masks/<https://www.webkit.org/blog/181/...
it states:
>
> "The mask will therefore overlay the child and all of its descendants"
>
> So I guess we want to scale it to fill the entire render surface.
>

The client (web contents) can do that if it desires, but I don't think we
want to do that in the compositor.

Which makes me then wonder why this wasn't working for this case.


We don't currently do that (resizing the mask to fit the surface) in the
compositor.


> Is it
> because the mask layer was being added to the root layer, so the
> contentRect of the surface didn't match the size of the subtree?


The mask isn't added to the root layer, it's added to the layer it's
intended to mask. AFAIK in our case there's a single layer with no sub-tree.
However, there is a scale higher up in the tree, that makes the render
surface size to not match the content layer size. The content layer gets
applied a scale to compensate, the mask was not - now it is.


> or
> because aura is drawing a mask assuming it matches the size of the
> owning layer and not its subtree?
>

It is allocating and drawing a mask assuming it matches the size of the
owning layer, there is no subtree to take into account.


>
>
https://codereview.chromium.**org/11883027/<https://codereview.chromium.org/1...
>