Calculate Background Image Geometries using sub-pixel values

Calculate Background Image Geometries using sub-pixel values

We've long had horrible issues with zooming and background images with
offsets (spriting). This is because we calculated these offsets using
integers greedily instead of snapping them directly before paint like
we do with other sub-pixel values in the layout tree.

With this change, BackgroundImageGeometry is converted entirely to
LayoutUnits, with snapping occurring after calculation in a new
pixelSnapGeometry() method. The old sub-pixel tile size heuristic was
thrown out, but replaced by a new one that puts in extra effort for
background images that fully cover the fill layer.

A number of reference tests needed to be updated to account for new
rounding values on tests with -webkit-mask-repeat or background-repeat:
round. It's worth noting that the references for these tests don't
render the same in Firefox with or without the changes made in this
patch.

The change to repeatSpacing in drawPattern and drawTiled is for
consistency only and can happily be postponed to a subsequent patch.

This patch influenced by the following changes in WebKit:
https://bugs.webkit.org/attachment.cgi?id=228591
https://bugs.webkit.org/attachment.cgi?id=222911
So mad props to Zalan Bujtas <zalan@apple.com>;, Apple's
1337 sub-pixel h4x0r.

BUG=66498
Committed: https://crrev.com/b216e38c1a1812d5761b0e78d8975c807c7f3837
Cr-Commit-Position: refs/heads/master@{#362569}

[leviw_travelin_and_unemployed, 2015-11-25 00:10:39]

Description was changed from

==========
WIP: Calculate Background Image Geometries using sub-pixel values

BUG=
==========

to

==========
Calculate Background Image Geometries using sub-pixel values

We've long had horrible issues with zooming and background images with
offsets (spriting). This is because we calculated these offsets using
integers greedily instead of snapping them directly before paint like
we do with other sub-pixel values in the layout tree.

With this change, BackgroundImageGeometry is converted entirely to
LayoutUnits, with snapping occurring after calculation in a new
pixelSnapGeometry() method. The old sub-pixel tile size heuristic was
thrown out, but replaced by a new one that puts in extra effort for
background images that fully cover the fill layer.

A number of reference tests needed to be updated to account for new
rounding values on tests with -webkit-mask-repeat or background-repeat:
round. It's worth noting that the references for these tests don't
render the same in Firefox with or without the changes made in this
patch.

The change to repeatSpacing in drawPattern and drawTiled is for
consistency only and can happily be postponed to a subsequent patch.

This patch influenced by the following changes in WebKit:
https://bugs.webkit.org/attachment.cgi?id=228591
https://bugs.webkit.org/attachment.cgi?id=222911
So mad props to Zalan Bujtas <zalan@apple.com>, Apple's
1337 sub-pixel h4x0r.

BUG=66498
==========

[leviw_travelin_and_unemployed, 2015-11-25 00:10:39]

leviw@chromium.org changed reviewers:
+ eae@chromium.org, pdr@chromium.org

[leviw_travelin_and_unemployed, 2015-11-25 00:12:04]

PTAL. The current patch uploaded *without* new test expectations to demonstrate
the pixel differences. A follow-up will be uploaded with rebaselines when the
bots are done.

This patch was more of a nightmare than expected :p

[eae, 2015-11-25 00:21:06]

So. much. better! Thanks for doing this levi!

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html
(right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html:62:
document.styleSheets[0].insertRule("body { zoom: 0.666666667; }", 0);
document.body.style.zoom = 0.666666667;
(or even 2/3 instead of 0.666666667)

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:405: IntSize
imageSize = flooredIntSize(imageBulletSize());
Not pixel snapped?

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp:138:
RefPtr<Image> image = styleImage->image(const_cast<LayoutBox*>(&m_layoutBox),
flooredIntSize(imageSize), m_layoutBox.style()->effectiveZoom());
Not roundedIntSize as before or pixel snapped?

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:165: // we
floor.
Could you expand on why we're flooring in the other case?

[leviw_travelin_and_unemployed, 2015-11-25 00:31:56]

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html
(right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html:62:
document.styleSheets[0].insertRule("body { zoom: 0.666666667; }", 0);
Fair enough. This change isn't actually necessary. I stuck it in to make testing
without DRT easier.

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:405: IntSize
imageSize = flooredIntSize(imageBulletSize());
On 2015/11/25 at 00:21:05, eae wrote:
> Not pixel snapped?

Negative. This ends up being far better than the alternative. The old code also
floored all these values or operated on integers.

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp:138:
RefPtr<Image> image = styleImage->image(const_cast<LayoutBox*>(&m_layoutBox),
flooredIntSize(imageSize), m_layoutBox.style()->effectiveZoom());
On 2015/11/25 at 00:21:05, eae wrote:
> Not roundedIntSize as before or pixel snapped?

Same as other comments.

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:165: // we
floor.
On 2015/11/25 at 00:21:05, eae wrote:
> Could you expand on why we're flooring in the other case?

Sure. We're following the same heuristic as before (see
applySubPixelHeuristicForTileSize), but doing a far better job getting there
now. Ultimately, the flooring is done to avoid picking up pixels from tiles
outside the intended bounds. This ends up being preferable to the alternative,
as frequently very different sprites are piled next to one another.

[eae, 2015-11-25 00:33:21]

On 2015/11/25 00:31:56, leviw wrote:
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html
> (right):
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html:62:
> document.styleSheets[0].insertRule("body { zoom: 0.666666667; }", 0);
> Fair enough. This change isn't actually necessary. I stuck it in to make
testing
> without DRT easier.
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right):
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:405: IntSize
> imageSize = flooredIntSize(imageBulletSize());
> On 2015/11/25 at 00:21:05, eae wrote:
> > Not pixel snapped?
> 
> Negative. This ends up being far better than the alternative. The old code
also
> floored all these values or operated on integers.
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp
(right):
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp:138:
> RefPtr<Image> image = styleImage->image(const_cast<LayoutBox*>(&m_layoutBox),
> flooredIntSize(imageSize), m_layoutBox.style()->effectiveZoom());
> On 2015/11/25 at 00:21:05, eae wrote:
> > Not roundedIntSize as before or pixel snapped?
> 
> Same as other comments.
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):
> 
>
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:165: // we
> floor.
> On 2015/11/25 at 00:21:05, eae wrote:
> > Could you expand on why we're flooring in the other case?
> 
> Sure. We're following the same heuristic as before (see
> applySubPixelHeuristicForTileSize), but doing a far better job getting there
> now. Ultimately, the flooring is done to avoid picking up pixels from tiles
> outside the intended bounds. This ends up being preferable to the alternative,
> as frequently very different sprites are piled next to one another.

Ok, would you mind adding the sentence above as a comment?

LGTM

[leviw_travelin_and_unemployed, 2015-11-25 00:34:19]

On 2015/11/25 at 00:33:21, eae wrote:
> On 2015/11/25 00:31:56, leviw wrote:
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
> > File
> >
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html
> > (right):
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/fast/backgrounds/size/contain-and-cover-zoomed.html:62:
> > document.styleSheets[0].insertRule("body { zoom: 0.666666667; }", 0);
> > Fair enough. This change isn't actually necessary. I stuck it in to make
testing
> > without DRT easier.
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right):
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:405: IntSize
> > imageSize = flooredIntSize(imageBulletSize());
> > On 2015/11/25 at 00:21:05, eae wrote:
> > > Not pixel snapped?
> > 
> > Negative. This ends up being far better than the alternative. The old code
also
> > floored all these values or operated on integers.
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp
(right):
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp:138:
> > RefPtr<Image> image =
styleImage->image(const_cast<LayoutBox*>(&m_layoutBox),
> > flooredIntSize(imageSize), m_layoutBox.style()->effectiveZoom());
> > On 2015/11/25 at 00:21:05, eae wrote:
> > > Not roundedIntSize as before or pixel snapped?
> > 
> > Same as other comments.
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
(right):
> > 
> >
https://codereview.chromium.org/1456813002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:165: // we
> > floor.
> > On 2015/11/25 at 00:21:05, eae wrote:
> > > Could you expand on why we're flooring in the other case?
> > 
> > Sure. We're following the same heuristic as before (see
> > applySubPixelHeuristicForTileSize), but doing a far better job getting there
> > now. Ultimately, the flooring is done to avoid picking up pixels from tiles
> > outside the intended bounds. This ends up being preferable to the
alternative,
> > as frequently very different sprites are piled next to one another.
> 
> Ok, would you mind adding the sentence above as a comment?
Heck no I wouldn't! :)

> 
> LGTM

[davve, 2015-11-25 15:47:58]

davve@opera.com changed reviewers:
+ davve@opera.com

[davve, 2015-11-25 15:47:59]

Obviously I like this CL.

https://codereview.chromium.org/1456813002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/NinePieceImagePainter.cpp (right):

https://codereview.chromium.org/1456813002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/NinePieceImagePainter.cpp:58:
NinePieceImageGrid grid(ninePieceImage, imageSize,
pixelSnappedIntRect(borderImageRect), borderWidths);
One idea would be to propagate LayoutUnits down to NinePieceImageGrid too,
right?

I wonder what happens if I jump down this rabbit hole...

Move NinePieceImageGrid to LayoutUnit
https://codereview.chromium.org/1477813002/ (on top of this one)

Testing that I found bug 561519 so I made another CL on top of that:

Use LayoutUnit for SVG container size
https://codereview.chromium.org/1477433004/

[leviw_travelin_and_unemployed, 2015-11-25 18:15:11]

On 2015/11/25 at 15:47:59, davve wrote:
> Obviously I like this CL.

Well then obviously I like you!

> 
>
https://codereview.chromium.org/1456813002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/NinePieceImagePainter.cpp (right):
> 
>
https://codereview.chromium.org/1456813002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/NinePieceImagePainter.cpp:58:
NinePieceImageGrid grid(ninePieceImage, imageSize,
pixelSnappedIntRect(borderImageRect), borderWidths);
> One idea would be to propagate LayoutUnits down to NinePieceImageGrid too,
right?

That's definitely on the chopping block as well.

> 
> I wonder what happens if I jump down this rabbit hole...

Besides you saving me from doing it? ;)

> 
> Move NinePieceImageGrid to LayoutUnit
> https://codereview.chromium.org/1477813002/ (on top of this one)
> 
> Testing that I found bug 561519 so I made another CL on top of that:
> 
> Use LayoutUnit for SVG container size
> https://codereview.chromium.org/1477433004/

Holy carp, davve!
http://www.pirate4x4.com/forum/attachments/general-chit-chat/354657d120360918...

[leviw_travelin_and_unemployed, 2015-11-25 22:22:22]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-11-25 22:22:22]

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

[commit-bot: I haz the power, 2015-11-25 22:24:31]

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

[commit-bot: I haz the power, 2015-11-25 23:50:57]

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

[commit-bot: I haz the power, 2015-11-25 23:50:58]

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

[leviw_travelin_and_unemployed, 2015-11-26 00:16:38]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-11-26 00:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1456813002/#ps230001
(title: "Moar test expectations")

[commit-bot: I haz the power, 2015-11-26 00:19:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456813002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456813002/230001

[commit-bot: I haz the power, 2015-11-26 01:58:57]

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

[commit-bot: I haz the power, 2015-11-26 01:58:58]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leviw_travelin_and_unemployed, 2015-12-01 18:04:55]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-12-01 18:04:55]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1456813002/#ps250001
(title: "MOAR expectations")

[commit-bot: I haz the power, 2015-12-01 18:06:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456813002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456813002/250001

[commit-bot: I haz the power, 2015-12-01 20:51:43]

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

[commit-bot: I haz the power, 2015-12-01 20:51:44]

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

[leviw_travelin_and_unemployed, 2015-12-01 21:53:13]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2015-12-01 21:55:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456813002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456813002/250001

[commit-bot: I haz the power, 2015-12-02 01:30:52]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-12-02 01:32:26]

Description was changed from

==========
Calculate Background Image Geometries using sub-pixel values

We've long had horrible issues with zooming and background images with
offsets (spriting). This is because we calculated these offsets using
integers greedily instead of snapping them directly before paint like
we do with other sub-pixel values in the layout tree.

With this change, BackgroundImageGeometry is converted entirely to
LayoutUnits, with snapping occurring after calculation in a new
pixelSnapGeometry() method. The old sub-pixel tile size heuristic was
thrown out, but replaced by a new one that puts in extra effort for
background images that fully cover the fill layer.

A number of reference tests needed to be updated to account for new
rounding values on tests with -webkit-mask-repeat or background-repeat:
round. It's worth noting that the references for these tests don't
render the same in Firefox with or without the changes made in this
patch.

The change to repeatSpacing in drawPattern and drawTiled is for
consistency only and can happily be postponed to a subsequent patch.

This patch influenced by the following changes in WebKit:
https://bugs.webkit.org/attachment.cgi?id=228591
https://bugs.webkit.org/attachment.cgi?id=222911
So mad props to Zalan Bujtas <zalan@apple.com>, Apple's
1337 sub-pixel h4x0r.

BUG=66498
==========

to

==========
Calculate Background Image Geometries using sub-pixel values

We've long had horrible issues with zooming and background images with
offsets (spriting). This is because we calculated these offsets using
integers greedily instead of snapping them directly before paint like
we do with other sub-pixel values in the layout tree.

With this change, BackgroundImageGeometry is converted entirely to
LayoutUnits, with snapping occurring after calculation in a new
pixelSnapGeometry() method. The old sub-pixel tile size heuristic was
thrown out, but replaced by a new one that puts in extra effort for
background images that fully cover the fill layer.

A number of reference tests needed to be updated to account for new
rounding values on tests with -webkit-mask-repeat or background-repeat:
round. It's worth noting that the references for these tests don't
render the same in Firefox with or without the changes made in this
patch.

The change to repeatSpacing in drawPattern and drawTiled is for
consistency only and can happily be postponed to a subsequent patch.

This patch influenced by the following changes in WebKit:
https://bugs.webkit.org/attachment.cgi?id=228591
https://bugs.webkit.org/attachment.cgi?id=222911
So mad props to Zalan Bujtas <zalan@apple.com>, Apple's
1337 sub-pixel h4x0r.

BUG=66498

Committed: https://crrev.com/b216e38c1a1812d5761b0e78d8975c807c7f3837
Cr-Commit-Position: refs/heads/master@{#362569}
==========

[commit-bot: I haz the power, 2015-12-02 01:32:27]

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