Make squashing work with subpixel layout

Make squashing work with subpixel layout

This changes GraphicsLayerPaintInfo::offsetFromSquashingCLM
from an IntSize to a LayoutSize and changes
SquashingState::offsetFromAbsoluteForSquashingCLM from an
IntPoint to a LayoutPoint. This also adds a
subpixelAccumulation field to GraphicsLayerPaintInfo; this
gets computed at the same time as
GraphicsLayerPaintInfo::offsetFromRenderer.

BUG=327387
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169584

[ajuma, 2014-02-03 21:00:44]

https://codereview.chromium.org/143283011/diff/70002/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/143283011/diff/70002/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:584: // The conversion between
m_owningLayer and the ancestor CLM is already computed above as |delta|.
What if the compositing ancestor has a clip layer? Shouldn't we position w.r.t.
that layer instead of converting using |delta|?

[ajuma, 2014-03-06 18:22:54]

PTAL.

[leviw_travelin_and_unemployed, 2014-03-07 01:03:29]

Not lgtm, sadly.

I talked to Chris Harrelson about this so I believe he understands the issue.
This won't preserve the proper sub-pixel offset of other layers than the first.
It's a bit tricky to explain. I'd be happy to do so in more detail, but it may
be easier in VC. Want to schedule one?

[ajuma, 2014-03-13 21:08:01]

PTAL. This now computes subpixel accumulation for each squashed RenderLayer
(stored in GraphicsLayerPaintInfo), and uses this subpixel accumlation in
doPaintTask.

[chrishtr, 2014-03-13 21:46:04]

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:602:
totalSquashBounds.move(m_subpixelAccumulation);
Why still use m_subpixelAccumulation at all? The accumulations vary for each
render layer that is squashed. Isn't it correct to move the subpixel
accumulation to RenderLayer instead?

[ajuma, 2014-03-13 21:55:48]

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:602:
totalSquashBounds.move(m_subpixelAccumulation);
On 2014/03/13 21:46:05, chrishtr wrote:
> Why still use m_subpixelAccumulation at all? The accumulations vary for each
> render layer that is squashed. Isn't it correct to move the subpixel
> accumulation to RenderLayer instead?

The reason we use m_subpixelAccumulation here and below is that
totalSquashBounds is in the space of m_owningLayer (whose subpixel accumulation
is m_subpixelAccumulation), so we need to account for this subpixel accumulation
when converting into the ancestor CLM's space (since the true conversion from
m_owningLayer to the ancestor is delta+m_subpixelAccumulation, not just delta).

[chrishtr, 2014-03-17 20:20:33]

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:602:
totalSquashBounds.move(m_subpixelAccumulation);
On 2014/03/13 21:55:48, ajuma wrote:
> On 2014/03/13 21:46:05, chrishtr wrote:
> > Why still use m_subpixelAccumulation at all? The accumulations vary for each
> > render layer that is squashed. Isn't it correct to move the subpixel
> > accumulation to RenderLayer instead?
> 
> The reason we use m_subpixelAccumulation here and below is that
> totalSquashBounds is in the space of m_owningLayer (whose subpixel
accumulation
> is m_subpixelAccumulation), so we need to account for this subpixel
accumulation
> when converting into the ancestor CLM's space (since the true conversion from
> m_owningLayer to the ancestor is delta+m_subpixelAccumulation, not just
delta).

Sure, but it looks like you are using that to set the subpixel accumulation of
squashed layers also, see comment below.

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:620: LayoutSize
offsetFromSquashLayerOrigin =
LayoutSize(m_squashedLayers[i].offsetFromSquashingCLM.width() -
squashLayerOrigin.x() + m_subpixelAccumulation.width(),
Isn't this wrong to use m_subpixelAccumulation? Each squashed layer could have
its own subpixel accumulation.

[ajuma, 2014-03-18 17:32:17]

PTAL.

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/143283011/diff/310001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:620: LayoutSize
offsetFromSquashLayerOrigin =
LayoutSize(m_squashedLayers[i].offsetFromSquashingCLM.width() -
squashLayerOrigin.x() + m_subpixelAccumulation.width(),
On 2014/03/17 20:20:33, chrishtr wrote:
> Isn't this wrong to use m_subpixelAccumulation? Each squashed layer could have
> its own subpixel accumulation.

I've cleaned up the code (here and above) to make the conversion between owning
layer space and ancestor space more explicit, and to also make clear that
m_subpixelAccumulation is only being used as part of such conversions.

[leviw_travelin_and_unemployed, 2014-03-18 22:30:47]

https://codereview.chromium.org/143283011/diff/220002/Source/core/rendering/c...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/143283011/diff/220002/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:656: return
roundedLayoutPoint(transformState.lastPlanarPoint());
roundedLayoutPoint is actually a no-op and should go away (it's from the
sub-pixel transition). You probably just want to construct a LayoutPoint from
the FloatPoint.

https://codereview.chromium.org/143283011/diff/220002/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:673: LayoutSize
offsetFromSquashingCLM(offsetFromAbsoluteForSquashedLayer.x() -
squashingState.offsetFromAbsoluteForSquashingCLM.x(),
I don't believe this will be guaranteed to be the same sub-pixel offset as
what's currently derived (from its composited ancestor). Either way, it's
important to add an assert that the .fraction() of this offset is the same as
the .fraction() we determine in
CompositedLayerMapping::adjustBoundsForSubPixelAccumulation. (Be aware, I'm in
the process of landing a tweak to that logic in
https://codereview.chromium.org/204053002/).

If you add the assert and validate that we don't hit it, I'm on board with the
rest of this change :)

[ajuma, 2014-03-19 19:26:42]

https://codereview.chromium.org/143283011/diff/220002/Source/core/rendering/c...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/143283011/diff/220002/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:656: return
roundedLayoutPoint(transformState.lastPlanarPoint());
On 2014/03/18 22:30:48, Levi wrote:
> roundedLayoutPoint is actually a no-op and should go away (it's from the
> sub-pixel transition). You probably just want to construct a LayoutPoint from
> the FloatPoint.

Done.

https://codereview.chromium.org/143283011/diff/220002/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:673: LayoutSize
offsetFromSquashingCLM(offsetFromAbsoluteForSquashedLayer.x() -
squashingState.offsetFromAbsoluteForSquashingCLM.x(),
On 2014/03/18 22:30:48, Levi wrote:
> I don't believe this will be guaranteed to be the same sub-pixel offset as
> what's currently derived (from its composited ancestor). Either way, it's
> important to add an assert that the .fraction() of this offset is the same as
> the .fraction() we determine in
> CompositedLayerMapping::adjustBoundsForSubPixelAccumulation. (Be aware, I'm in
> the process of landing a tweak to that logic in
> https://codereview.chromium.org/204053002/).
> 
> If you add the assert and validate that we don't hit it, I'm on board with the
> rest of this change :)

I've added the assert to CompositedLayerMapping::updateSquashingLayerGeometry. I
ran all the layout tests locally with squashing turned on, and verified that
this assert doesn't get hit.

[leviw_travelin_and_unemployed, 2014-03-19 20:30:26]

Wonderful. LGTM!

https://codereview.chromium.org/143283011/diff/370001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/143283011/diff/370001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:634:
ASSERT(m_squashedLayers[i].subpixelAccumulation ==
Perfect, thank you!

[ajuma, 2014-03-19 20:53:16]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-03-19 20:53:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/143283011/370001

[commit-bot: I haz the power, 2014-03-19 21:36:32]

Change committed as 169584