use consistent Skia pixel position adjustments (tdf#134346)

What happens in tdf#134346 is that the same shape was drawn twice,
once using drawPolyPolygon() and once as an outline using
drawPolyLine(). Those had different offsets, so with AA enabled
the tiny difference could be actually visible. Be more consistent
with how the pixels are positioned in float coordinates.

Change-Id: Id852fef70e7bd829ff0108a86d1ebee29c300e3a
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/98745
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>

2 files changed, 19 insertions, 21 deletions

diff --git a/vcl/inc/skia/gdiimpl.hxx b/vcl/inc/skia/gdiimpl.hxx
index c98038807287..344509fe301e 100644
--- a/vcl/inc/skia/gdiimpl.hxx
+++ b/vcl/inc/skia/gdiimpl.hxx
@@ -272,12 +272,14 @@ protected:
     sk_sp<SkImage> mergeCacheBitmaps(const SkiaSalBitmap& bitmap, const SkiaSalBitmap* alphaBitmap,
                                      const Size targetSize);
 
-    // When drawing using GPU, rounding errors may result in off-by-one errors,
+    // Skia uses floating point coordinates, so when we use integer coordinates, sometimes
+    // rounding results in off-by-one errors (down), especially when drawing using GPU,
     // see https://bugs.chromium.org/p/skia/issues/detail?id=9611 . Compensate for
-    // it by using centers of pixels (Skia uses float coordinates). In raster case
-    // it seems better to not do this though.
-    SkScalar toSkX(long x) const { return mIsGPU ? x + 0.5 : x; }
-    SkScalar toSkY(long y) const { return mIsGPU ? y + 0.5 : y; }
+    // it by using centers of pixels. Using 0.5 may sometimes round up, so go with 0.495 .
+    static constexpr SkScalar toSkX(long x) { return x + 0.495; }
+    static constexpr SkScalar toSkY(long y) { return y + 0.495; }
+    // Value to add to be exactly in the middle of the pixel.
+    static constexpr SkScalar toSkXYFix = SkScalar(0.005);
 
     template <typename charT, typename traits>
     friend inline std::basic_ostream<charT, traits>&
diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx
index bb837b03493b..09dd924b6a2c 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -597,11 +597,7 @@ void SkiaSalGraphicsImpl::drawLine(long nX1, long nY1, long nX2, long nY2)
     SkPaint paint;
     paint.setColor(toSkColor(mLineColor));
     paint.setAntiAlias(mParent.getAntiAliasB2DDraw());
-    // Raster has better results if shifted by 0.25 (unlike the 0.5 done by toSkX/toSkY).
-    if (!isGPU())
-        getDrawCanvas()->drawLine(nX1 + 0.25, nY1 + 0.25, nX2 + 0.25, nY2 + 0.25, paint);
-    else
-        getDrawCanvas()->drawLine(toSkX(nX1), toSkY(nY1), toSkX(nX2), toSkY(nY2), paint);
+    getDrawCanvas()->drawLine(toSkX(nX1), toSkY(nY1), toSkX(nX2), toSkY(nY2), paint);
     addXorRegion(SkRect::MakeLTRB(nX1, nY1, nX2 + 1, nY2 + 1));
     postDraw();
 }
@@ -708,19 +704,21 @@ bool SkiaSalGraphicsImpl::drawPolyPolygon(const basegfx::B2DHomMatrix& rObjectTo
 
     SkPaint aPaint;
     aPaint.setAntiAlias(mParent.getAntiAliasB2DDraw());
+    // We normally use pixel at their center positions, but slightly off (see toSkX/Y()).
+    // With AA lines that "slightly off" causes tiny changes of color, making some tests
+    // fail. Since moving AA-ed line slightly to a side doesn't cause any real visual
+    // difference, just place exactly at the center. tdf#134346
+    const SkScalar posFix = mParent.getAntiAliasB2DDraw() ? toSkXYFix : 0;
     if (mFillColor != SALCOLOR_NONE)
     {
         aPaint.setColor(toSkColorWithTransparency(mFillColor, fTransparency));
         aPaint.setStyle(SkPaint::kFill_Style);
+        aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
         getDrawCanvas()->drawPath(aPath, aPaint);
     }
     if (mLineColor != SALCOLOR_NONE)
     {
-        // Raster has better results if shifted by 0.25 (unlike the 0.5 done by toSkX/toSkY).
-        if (!isGPU())
-            aPath.offset(0.25, 0.25, nullptr);
-        else // Apply the same adjustment as toSkX()/toSkY() do.
-            aPath.offset(0.5, 0.5, nullptr);
+        aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
         aPaint.setColor(toSkColorWithTransparency(mLineColor, fTransparency));
         aPaint.setStyle(SkPaint::kStroke_Style);
         getDrawCanvas()->drawPath(aPath, aPaint);
@@ -811,6 +809,8 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
     aPaint.setStrokeMiter(fMiterLimit);
     aPaint.setStrokeWidth(fLineWidth);
     aPaint.setAntiAlias(mParent.getAntiAliasB2DDraw());
+    // See the tdf#134346 comment above.
+    const SkScalar posFix = mParent.getAntiAliasB2DDraw() ? toSkXYFix : 0;
 
     if (pStroke && std::accumulate(pStroke->begin(), pStroke->end(), 0.0) != 0)
     {
@@ -829,9 +829,7 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
         aPath.setFillType(SkPathFillType::kEvenOdd);
         for (sal_uInt32 a(0); a < aPolyPolygonLine.count(); a++)
             addPolygonToPath(aPolyPolygonLine.getB2DPolygon(a), aPath);
-        // Apply the same adjustment as toSkX()/toSkY() do. Do it here even in the non-GPU
-        // case as it seems to produce better results.
-        aPath.offset(0.5, 0.5, nullptr);
+        aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
         getDrawCanvas()->drawPath(aPath, aPaint);
         addXorRegion(aPath.getBounds());
     }
@@ -852,9 +850,7 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
                 aPath.lineTo(rPolygon.getB2DPoint(index2).getX(),
                              rPolygon.getB2DPoint(index2).getY());
 
-                // Apply the same adjustment as toSkX()/toSkY() do. Do it here even in the non-GPU
-                // case as it seems to produce better results.
-                aPath.offset(0.5, 0.5, nullptr);
+                aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
                 getDrawCanvas()->drawPath(aPath, aPaint);
                 addXorRegion(aPath.getBounds());
             }