Fix rounding of phase for background image

third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp

Index: third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
diff --git a/third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp b/third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
index a3369ee9ea4a72499f2c73b18eebaa350fbf151f..b5293fa8380c0129b1e49a35761f1b5c4d950608 100644
--- a/third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
+++ b/third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp
@@ -146,8 +146,8 @@ IntPoint accumulatedScrollOffsetForFixedBackground(const LayoutBoxModelObject& o
 // snap the same way. This commonly occurs when the background is meant to fill the
 // padding box but there's a border (which in Blink is always stored as an integer).
 // Otherwise we floor to avoid growing our tile size. Often these tiles are from a
-// sprite map, and bleeding adjactent sprites is visually worse than clipping the
-// intenteded one.
+// sprite map, and bleeding adjacent sprites is visually worse than clipping the
+// intended one.
 LayoutSize applySubPixelHeuristicToImageSize(const LayoutSize& size, const LayoutRect& destination)
 {
     LayoutSize snappedSize = LayoutSize(
@@ -160,17 +160,19 @@ LayoutSize applySubPixelHeuristicToImageSize(const LayoutSize& size, const Layou
 
 void BackgroundImageGeometry::setNoRepeatX(LayoutUnit xOffset)
 {
-    m_destRect.move(std::max(xOffset, LayoutUnit()), LayoutUnit());
-    m_phase.setX(-std::min(xOffset, LayoutUnit()));
-    m_destRect.setWidth(m_tileSize.width() + std::min(xOffset, LayoutUnit()));
+    int roundedOffset = roundToInt(xOffset);
+    m_destRect.move(std::max(roundedOffset, 0), LayoutUnit());
+    m_phase.setX(LayoutUnit(-std::min(roundedOffset, 0)));

[f(malita), 2016/06/30 15:03:05]

Nit: use setDestRect()/setPhaseX() for consistency?

+    m_destRect.setWidth(m_tileSize.width() + std::min(roundedOffset, 0));
     setSpaceSize(LayoutSize(LayoutUnit(), spaceSize().height()));
 }
 
 void BackgroundImageGeometry::setNoRepeatY(LayoutUnit yOffset)
 {
-    m_destRect.move(LayoutUnit(), std::max(yOffset, LayoutUnit()));
-    m_phase.setY(-std::min(yOffset, LayoutUnit()));
-    m_destRect.setHeight(m_tileSize.height() + std::min(yOffset, LayoutUnit()));
+    int roundedOffset = roundToInt(yOffset);
+    m_destRect.move(LayoutUnit(), std::max(roundedOffset, 0));
+    m_phase.setY(LayoutUnit(-std::min(roundedOffset, 0)));

[f(malita), 2016/06/30 15:03:05]

Ditto (setDestRect/setPhaseY).

+    m_destRect.setHeight(m_tileSize.height() + std::min(roundedOffset, 0));
     setSpaceSize(LayoutSize(spaceSize().width(), LayoutUnit()));
 }
 
@@ -188,11 +190,11 @@ void BackgroundImageGeometry::setRepeatX(
         if (fillLayer.backgroundXOrigin() == RightEdge) {
             float numberOfTilesInPosition = (snappedAvailableWidth - computedXPosition + extraOffset).toFloat() / unsnappedTileWidth.toFloat();
             float fractionalPositionWithinTile = numberOfTilesInPosition - truncf(numberOfTilesInPosition);
-            setPhaseX(LayoutUnit(fractionalPositionWithinTile * tileSize().width()));
+            setPhaseX(LayoutUnit(round(fractionalPositionWithinTile * tileSize().width())));

[f(malita), 2016/06/30 15:03:05]

Nit: (here and throughout) roundf

[Stephen Chennney, 2016/06/30 18:50:55]

Done.

         } else {
             float numberOfTilesInPosition = (computedXPosition + extraOffset).toFloat() / unsnappedTileWidth.toFloat();
             float fractionalPositionWithinTile = 1.0f - (numberOfTilesInPosition - truncf(numberOfTilesInPosition));
-            setPhaseX(LayoutUnit(fractionalPositionWithinTile * tileSize().width()));
+            setPhaseX(LayoutUnit(round(fractionalPositionWithinTile * tileSize().width())));
         }
     } else {
         setPhaseX(LayoutUnit());
@@ -214,11 +216,11 @@ void BackgroundImageGeometry::setRepeatY(
         if (fillLayer.backgroundYOrigin() == BottomEdge) {
             float numberOfTilesInPosition = (snappedAvailableHeight - computedYPosition + extraOffset).toFloat() / unsnappedTileHeight.toFloat();
             float fractionalPositionWithinTile = numberOfTilesInPosition - truncf(numberOfTilesInPosition);
-            setPhaseY(LayoutUnit(fractionalPositionWithinTile * tileSize().height()));
+            setPhaseY(LayoutUnit(round(fractionalPositionWithinTile * tileSize().height())));
         } else {
             float numberOfTilesInPosition = (computedYPosition + extraOffset).toFloat() / unsnappedTileHeight.toFloat();
             float fractionalPositionWithinTile = 1.0f - (numberOfTilesInPosition - truncf(numberOfTilesInPosition));
-            setPhaseY(LayoutUnit(fractionalPositionWithinTile * tileSize().height()));
+            setPhaseY(LayoutUnit(round(fractionalPositionWithinTile * tileSize().height())));
         }
     } else {
         setPhaseY(LayoutUnit());
@@ -231,7 +233,7 @@ void BackgroundImageGeometry::setSpaceX(LayoutUnit space, LayoutUnit availableWi
     LayoutUnit computedXPosition = roundedMinimumValueForLength(Length(), availableWidth);
     setSpaceSize(LayoutSize(space.round(), spaceSize().height()));
     LayoutUnit actualWidth = tileSize().width() + space;
-    setPhaseX(actualWidth ? LayoutUnit(actualWidth - fmodf((computedXPosition + extraOffset), actualWidth)) : LayoutUnit());
+    setPhaseX(actualWidth ? LayoutUnit(round(actualWidth - fmodf((computedXPosition + extraOffset), actualWidth))) : LayoutUnit());

[f(malita), 2016/06/30 15:03:05]

Sice round/roundf has a different bias vs. LayoutUnit::round() for negative
half-values, I'm wary of mixing the two.  But I guess it's provable that the arg
is positive in all these cases?

[Stephen Chennney, 2016/06/30 18:50:55]

Rounding with LayoutUnit will cause all sorts of type cast issues. I think we're
dealing with LayoutUnits and floats in this line. It would take
LayoutUnit(rountToInt(LayoutUnit(float thing))). It can't be negative though,
because fmodf(blah, actualWidth) < actualWidth.

[f(malita), 2016/06/30 19:01:54]

I'm mostly worried about inconsistencies vs. other code paths - say phase
rounding - which use LayoutUnit rounding rules.  But yeah, I think these are all
safe because the args are positive.

(if we had to, floorf(x + 0.5f) should give us equivalent results without
LayoutUnit gymnastics)

 }
 
 void BackgroundImageGeometry::setSpaceY(LayoutUnit space, LayoutUnit availableHeight, LayoutUnit extraOffset)
@@ -239,13 +241,14 @@ void BackgroundImageGeometry::setSpaceY(LayoutUnit space, LayoutUnit availableHe
     LayoutUnit computedYPosition = roundedMinimumValueForLength(Length(), availableHeight);
     setSpaceSize(LayoutSize(spaceSize().width(), space.round()));
     LayoutUnit actualHeight = tileSize().height() + space;
-    setPhaseY(actualHeight ? LayoutUnit(actualHeight - fmodf((computedYPosition + extraOffset), actualHeight)) : LayoutUnit());
+    setPhaseY(actualHeight ? LayoutUnit(round(actualHeight - fmodf((computedYPosition + extraOffset), actualHeight))) : LayoutUnit());
 }
 
 void BackgroundImageGeometry::useFixedAttachment(const LayoutPoint& attachmentPoint)
 {
     LayoutPoint alignedPoint = attachmentPoint;
     m_phase.move(std::max(alignedPoint.x() - m_destRect.x(), LayoutUnit()), std::max(alignedPoint.y() - m_destRect.y(), LayoutUnit()));
+    m_phase = LayoutPoint(roundedIntPoint(m_phase));

[pdr., 2016/06/30 06:01:02]

Can m_phase be refactored to be an IntPoint?

[Stephen Chennney, 2016/06/30 18:50:55]

Yes, and so can lots of other things. But it's a big change that should be done
separately to avoid obfuscating the actual change here.

[pdr., 2016/06/30 20:14:35]

SG, can you add a TODO above m_phase for this (and any other useful refactorings
this patch makes possible)?

 }
 
 void BackgroundImageGeometry::calculate(const LayoutBoxModelObject& obj, const LayoutBoxModelObject* paintContainer,
@@ -337,6 +340,9 @@ void BackgroundImageGeometry::calculate(const LayoutBoxModelObject& obj, const L
     // incorrectly using sub-pixel values that won't be present in the painted tile.
     setTileSize(applySubPixelHeuristicToImageSize(fillTileSize, m_destRect));
 
+    setDestRect(LayoutRect(pixelSnappedIntRect(m_destRect)));
+
+
     EFillRepeat backgroundRepeatX = fillLayer.repeatX();
     EFillRepeat backgroundRepeatY = fillLayer.repeatY();
     LayoutUnit unsnappedAvailableWidth = positioningAreaSize.width() - fillTileSize.width();
@@ -356,7 +362,8 @@ void BackgroundImageGeometry::calculate(const LayoutBoxModelObject& obj, const L
             fillTileSize.setHeight(fillTileSize.height() * positioningAreaSize.width() / (nrTiles * fillTileSize.width()));
         }
         setTileSize(applySubPixelHeuristicToImageSize(fillTileSize, m_destRect));
-        setPhaseX(tileSize().width() ? LayoutUnit(tileSize().width() - fmodf((computedXPosition + left), tileSize().width()))
+        setPhaseX(tileSize().width()
+            ? LayoutUnit(round(tileSize().width() - fmodf((computedXPosition + left), tileSize().width())))
             : LayoutUnit());
         setSpaceSize(LayoutSize());
     }
@@ -372,7 +379,8 @@ void BackgroundImageGeometry::calculate(const LayoutBoxModelObject& obj, const L
             fillTileSize.setWidth(fillTileSize.width() * positioningAreaSize.height() / (nrTiles * fillTileSize.height()));
         }
         setTileSize(applySubPixelHeuristicToImageSize(fillTileSize, m_destRect));
-        setPhaseY(tileSize().height() ? LayoutUnit(tileSize().height() - fmodf((computedYPosition + top), tileSize().height()))
+        setPhaseY(tileSize().height()
+            ? LayoutUnit(round(tileSize().height() - fmodf((computedYPosition + top), tileSize().height())))
             : LayoutUnit());
         setSpaceSize(LayoutSize());
     }
@@ -412,7 +420,7 @@ void BackgroundImageGeometry::calculate(const LayoutBoxModelObject& obj, const L
     m_destRect.intersect(paintRect);
 
     // Snap as-yet unsnapped values.
-    setPhase(LayoutPoint(roundedIntPoint(m_phase)));
+    DCHECK(m_phase == LayoutPoint(roundedIntPoint(m_phase)));
     setDestRect(LayoutRect(pixelSnappedIntRect(m_destRect)));

[f(malita), 2016/06/30 15:03:05]

So we round the dest rect offset, then we round the dest rect, then we intersect
and round the dest rect again.  Methinks we round too much :)

I think all this logic needs to be rethought at some point - perform all
computations in layout units, in a common coordinate space, and only snap at the
end?

Also possible regression for background-repeat-space-padding-box-expected (moar
red with the patch), but it's minor and since the change fixes some obvious bugs
I'm fine with it.

[Stephen Chennney, 2016/06/30 18:50:55]

That's the way snapping was originally done before I got into this, and it
causes a whole set of other problems. Basically the changes have been toward
snapping as early as possible so that subsequent calculations are done with the
input values that will actually be used when painting, allowing for the most
accurate sizing and rounding for what appears on the screen.

Areas for improving are cases where downstream code can do a better job rounding
some things, in particular the space between images, to avoid nasty
discretization issues. 

 
 }