Fix pixel snapping issues when layers become composited

Fix pixel snapping issues when layers become composited

Within the render tree, locations and sizes are stored in LayoutUnits
and determining the final destination for an element involves walking
the render tree and accumulating the offset. When a layer becomes
composited, we start mid-way in the render tree, and don't accumulate
our offsets.

This change has us using LayoutUnits in RenderLayerBacking to store
our sub-pixel offset so our painting when composited pixel snaps the
same as when we aren't composited.

This patch is based on work Allan Sandfeld Jensen started on
https://bugs.webkit.org/show_bug.cgi?id=115304.

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

[eae, 2013-07-26 20:30:50]

Yay! Got a couple of questions and suggestions for you though.

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:369:
innerCompositor->frameViewDidChangeLocation(flooredIntPoint(contentsBox().location()));
Don't you need to account for the subpixel accumulation here?

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:536: m_subpixelAccumulation =
toLayoutSize(rawDelta.fraction());
Wouldn't it make more sense to have the fraction method on LayoutSize? A
fractional size makes more sense (to me at least) than a fractional point.

toLayoutSize(rawDelta).fraction();

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:540: IntRect localCompositingBounds
= pixelSnappedIntRect(localRawCompositingBounds);
How about moving all of this snapping logic into a (static) helper method?

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:701:
m_scrollingLayer->setPosition(FloatPoint(clientBox.location() -
localCompositingBounds.location() + roundedIntSize(m_subpixelAccumulation)));
Why round here?

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:496: LOG(Compositing, "%*p %dx%d
%.2fKB (%s) %s\n", 12 + depth * 2, layer,
backing->compositedBounds().width().round(),
backing->compositedBounds().height().round(),
Why not log the precise values?

[leviw_travelin_and_unemployed, 2013-07-26 20:36:21]

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:536: m_subpixelAccumulation =
toLayoutSize(rawDelta.fraction());
On 2013/07/26 20:30:51, eae wrote:
> Wouldn't it make more sense to have the fraction method on LayoutSize? A
> fractional size makes more sense (to me at least) than a fractional point.
> 
> toLayoutSize(rawDelta).fraction();

We certainly can. It makes sense to me to request the fraction of a LayoutPoint
or Size.

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerBacking.cpp:540: IntRect localCompositingBounds
= pixelSnappedIntRect(localRawCompositingBounds);
On 2013/07/26 20:30:51, eae wrote:
> How about moving all of this snapping logic into a (static) helper method?

Sure.

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/20723003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:496: LOG(Compositing, "%*p %dx%d
%.2fKB (%s) %s\n", 12 + depth * 2, layer,
backing->compositedBounds().width().round(),
backing->compositedBounds().height().round(),
On 2013/07/26 20:30:51, eae wrote:
> Why not log the precise values?

Good question :)

[leviw_travelin_and_unemployed, 2013-07-26 21:47:50]

ptal

[eae, 2013-07-26 21:53:16]

LGTM!

[commit-bot: I haz the power, 2013-07-26 21:58:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/20723003/9001

[commit-bot: I haz the power, 2013-07-27 02:25:41]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-29 17:14:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/20723003/9001

[commit-bot: I haz the power, 2013-07-29 17:14:53]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 succeeded at 1220 (offset -54 lines).
  Hunk #2 FAILED at 1607.
  1 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
b18a50d8add63898b57b580c3de0f3c5ed1c9374..bf2cbda37fded59aed71cafad63e8a5c94fc9e8a
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1274,7 +1274,6 @@ crbug.com/237250 [ SnowLeopard Debug ]
media/video-set-rate-from-pause.html [ Cr
 crbug.com/237270 [ Win7 ] transforms/2d/transform-2d.html [ Pass Timeout ]
 crbug.com/237270 [ Debug ] fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml [
Slow ]
 
-crbug.com/237270 [ Debug ]
fast/sub-pixel/transformed-iframe-copy-on-scroll.html [ Pass Timeout ]
 crbug.com/251129 [ Linux Win Debug ]
http/tests/history/redirect-js-form-submit-0-seconds.html [ Failure Pass ]
 
 # Gets a "Maximum call stack size exceeded" exception in debug that doesn't
happen in release.
@@ -1608,6 +1607,14 @@ crbug.com/247889 [ Win ]
http/tests/navigation/back-send-referrer.html [ Pass Ti
 Bug(jamesr) compositing/video/video-controls-layer-creation.html [
ImageOnlyFailure Pass ]
 Bug(jamesr)
virtual/softwarecompositing/video/video-controls-layer-creation.html [
ImageOnlyFailure Pass ]
 
+Bug(leviw) compositing/direct-image-compositing.html [ NeedsRebaseline ]
+Bug(leviw) css3/filters/effect-reference-zoom.html [ NeedsRebaseline ]
+Bug(leviw) css3/filters/filter-repaint-blur.html [ NeedsRebaseline ]
+Bug(leviw) css3/filters/filter-repaint-child-layers.html [ NeedsRebaseline ]
+Bug(leviw) css3/filters/filter-repaint-shadow.html [ NeedsRebaseline ]
+Bug(leviw) css3/filters/nested-filter.html [ NeedsRebaseline ]
+Bug(leviw) fast/sub-pixel/transformed-iframe-copy-on-scroll.html [
NeedsManualRebaseline ]
+
 # New baseline might fail on Mac according to trybots.
 crbug.com/245520 [ Mac ] compositing/repaint/transform-style-change.html [
ImageOnlyFailure ]

[leviw_travelin_and_unemployed, 2013-11-13 00:16:27]

I've updated this patch. It's a pretty nasty issue and I think the one failure I
can't seem to fix is less bad than the current state of affairs. Unfortunately,
it appears to be quite gerbroken on Windows... I may need a hand with that :(

Either way, PTAL.

[eae, 2013-11-13 00:29:00]

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:162: #crbug.com/310679 [ Mac ]
virtual/gpu/compositedscrolling/scrollbars/custom-scrollbar-with-incomplete-style.html
[ ImageOnlyFailure Pass ]
You should probably just remove this line.

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/sub-pixel-composited-layers-expected.html
(right):

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/sub-pixel-composited-layers-expected.html:1: <html>
Add a DOCTYPE to trigger standards mode for at least one of the tests please.

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/sub-pixel-composited-layers.html (right):

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/sub-pixel-composited-layers.html:32: function
setupGrid10x10(leftOffset, topOffset, leftFraction, topFraction)
Instead of duplicating the code why not do setupGrid(columns, rows, ...)

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

https://codereview.chromium.org/20723003/diff/105001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:373: // We can floor this point
because our frameviews are always aligned to pixel boundaries.
Could you add an ASSERT ensuring that this is the case?

https://codereview.chromium.org/20723003/diff/105001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:738:
m_scrollingLayer->setPosition(FloatPoint(clientBox.location() -
localCompositingBounds.location() + roundedIntSize(m_subpixelAccumulation)));
"clientBox.location() - localCompositingBounds.location()" is int so that
rounding the m_subpixelAccumulatio has the same effect as pixel snapping the
entire value, right? (If so, all good. If not then please explain why you round
the value).

[leviw_travelin_and_unemployed, 2013-11-13 00:31:50]

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/sub-pixel-composited-layers.html (right):

https://codereview.chromium.org/20723003/diff/105001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/sub-pixel-composited-layers.html:32: function
setupGrid10x10(leftOffset, topOffset, leftFraction, topFraction)
On 2013/11/13 00:29:01, eae wrote:
> Instead of duplicating the code why not do setupGrid(columns, rows, ...)

Sorry, yeah. Copied this from the WK bug :D Will fix!

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

https://codereview.chromium.org/20723003/diff/105001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:373: // We can floor this point
because our frameviews are always aligned to pixel boundaries.
On 2013/11/13 00:29:01, eae wrote:
> Could you add an ASSERT ensuring that this is the case?

Very good point. Yes.

https://codereview.chromium.org/20723003/diff/105001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:738:
m_scrollingLayer->setPosition(FloatPoint(clientBox.location() -
localCompositingBounds.location() + roundedIntSize(m_subpixelAccumulation)));
On 2013/11/13 00:29:01, eae wrote:
> "clientBox.location() - localCompositingBounds.location()" is int so that
> rounding the m_subpixelAccumulatio has the same effect as pixel snapping the
> entire value, right? (If so, all good. If not then please explain why you
round
> the value).

Precisely.

[leviw_travelin_and_unemployed, 2013-11-13 21:01:26]

Adding some other folks to
- validate the approach of this patch
- help me understand how this can possibly break all things compositing in
Windows but nowhere else.

[leviw_travelin_and_unemployed, 2013-11-18 21:41:20]

I added an assert that doesn't modify any values, and the tests passed on the
debug bot and failed the release one. My mind is officially boggled.

[leviw_travelin_and_unemployed, 2013-11-18 21:49:43]

On 2013/11/18 21:41:20, Levi wrote:
> I added an assert that doesn't modify any values, and the tests passed on the
> debug bot and failed the release one. My mind is officially boggled.

Nevermind, they're just skipped :(

[eae, 2014-01-07 01:49:51]

AWESOME!

LGTM

[commit-bot: I haz the power, 2014-01-07 09:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/20723003/455001

[commit-bot: I haz the power, 2014-01-07 10:14:51]

Change committed as 164561