avoid Skia floating point position fixups for rectangles (tdf#137329)

Avoid to toSkX()/toSkY() tweaks for rectangular areas, so with AA
enabled it leads to fuzzy edges, and rectangles should line up
perfectly with all pixels even without tweaks.

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

2 files changed, 64 insertions, 16 deletions

diff --git a/vcl/qa/cppunit/skia/skia.cxx b/vcl/qa/cppunit/skia/skia.cxx
index d13e1530f95e..1f04c5825584 100644
--- a/vcl/qa/cppunit/skia/skia.cxx
+++ b/vcl/qa/cppunit/skia/skia.cxx
@@ -35,6 +35,7 @@ public:
     void testInterpretAs8Bit();
     void testAlphaBlendWith();
     void testBitmapCopyOnWrite();
+    void testTdf137329();
 
     CPPUNIT_TEST_SUITE(SkiaTest);
     CPPUNIT_TEST(testBitmapErase);
@@ -42,6 +43,7 @@ public:
     CPPUNIT_TEST(testInterpretAs8Bit);
     CPPUNIT_TEST(testAlphaBlendWith);
     CPPUNIT_TEST(testBitmapCopyOnWrite);
+    CPPUNIT_TEST(testTdf137329);
     CPPUNIT_TEST_SUITE_END();
 
 private:
@@ -303,6 +305,27 @@ void SkiaTest::testBitmapCopyOnWrite()
     CPPUNIT_ASSERT(bitmap.unittestGetAlphaImage() != oldAlphaImage);
 }
 
+void SkiaTest::testTdf137329()
+{
+    // Draw a filled polygon in the entire device, with AA enabled.
+    // All pixels in the device should be black, even those at edges (i.e. not affected by AA).
+    ScopedVclPtr<VirtualDevice> device = VclPtr<VirtualDevice>::Create(DeviceFormat::DEFAULT);
+    device->SetOutputSizePixel(Size(10, 10));
+    device->SetBackground(Wallpaper(COL_WHITE));
+    device->SetAntialiasing(AntialiasingFlags::Enable);
+    device->Erase();
+    device->SetLineColor();
+    device->SetFillColor(COL_BLACK);
+    device->DrawPolyPolygon(
+        basegfx::B2DPolyPolygon(basegfx::B2DPolygon{ { 0, 0 }, { 10, 0 }, { 10, 10 }, { 0, 10 } }));
+    // savePNG("/tmp/tdf137329.png", device);
+    CPPUNIT_ASSERT_EQUAL(COL_BLACK, device->GetPixel(Point(0, 0)));
+    CPPUNIT_ASSERT_EQUAL(COL_BLACK, device->GetPixel(Point(9, 0)));
+    CPPUNIT_ASSERT_EQUAL(COL_BLACK, device->GetPixel(Point(9, 9)));
+    CPPUNIT_ASSERT_EQUAL(COL_BLACK, device->GetPixel(Point(0, 9)));
+    CPPUNIT_ASSERT_EQUAL(COL_BLACK, device->GetPixel(Point(4, 4)));
+}
+
 } // namespace
 
 CPPUNIT_TEST_SUITE_REGISTRATION(SkiaTest);
diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx
index 1f1c4002f94d..c7035e7d3de7 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -53,7 +53,8 @@ namespace
 // bottom-most line of pixels of the bounding rectangle (see
 // https://lists.freedesktop.org/archives/libreoffice/2019-November/083709.html).
 // So be careful with rectangle->polygon conversions (generally avoid them).
-void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
+void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath,
+                      bool* hasOnlyOrthogonal = nullptr)
 {
     const sal_uInt32 nPointCount(rPolygon.count());
 
@@ -88,6 +89,11 @@ void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
         else if (!bHasCurves)
         {
             rPath.lineTo(aCurrentPoint.getX(), aCurrentPoint.getY());
+            // If asked for, check whether the polygon has a line that is not
+            // strictly horizontal or vertical.
+            if (hasOnlyOrthogonal != nullptr && aCurrentPoint.getX() != aPreviousPoint.getX()
+                && aCurrentPoint.getY() != aPreviousPoint.getY())
+                *hasOnlyOrthogonal = false;
         }
         else
         {
@@ -96,7 +102,12 @@ void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
 
             if (aPreviousControlPoint.equal(aPreviousPoint)
                 && aCurrentControlPoint.equal(aCurrentPoint))
+            {
                 rPath.lineTo(aCurrentPoint.getX(), aCurrentPoint.getY()); // a straight line
+                if (hasOnlyOrthogonal != nullptr && aCurrentPoint.getX() != aPreviousPoint.getX()
+                    && aCurrentPoint.getY() != aPreviousPoint.getY())
+                    *hasOnlyOrthogonal = false;
+            }
             else
             {
                 if (aPreviousControlPoint.equal(aPreviousPoint))
@@ -112,6 +123,8 @@ void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
                 rPath.cubicTo(aPreviousControlPoint.getX(), aPreviousControlPoint.getY(),
                               aCurrentControlPoint.getX(), aCurrentControlPoint.getY(),
                               aCurrentPoint.getX(), aCurrentPoint.getY());
+                if (hasOnlyOrthogonal != nullptr)
+                    *hasOnlyOrthogonal = false;
             }
         }
         aPreviousPoint = aCurrentPoint;
@@ -123,7 +136,8 @@ void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
     }
 }
 
-void addPolyPolygonToPath(const basegfx::B2DPolyPolygon& rPolyPolygon, SkPath& rPath)
+void addPolyPolygonToPath(const basegfx::B2DPolyPolygon& rPolyPolygon, SkPath& rPath,
+                          bool* hasOnlyOrthogonal = nullptr)
 {
     const sal_uInt32 nPolygonCount(rPolyPolygon.count());
 
@@ -132,7 +146,7 @@ void addPolyPolygonToPath(const basegfx::B2DPolyPolygon& rPolyPolygon, SkPath& r
 
     for (const auto& rPolygon : rPolyPolygon)
     {
-        addPolygonToPath(rPolygon, rPath);
+        addPolygonToPath(rPolygon, rPath, hasOnlyOrthogonal);
     }
 }
 
@@ -852,36 +866,47 @@ void SkiaSalGraphicsImpl::performDrawPolyPolygon(const basegfx::B2DPolyPolygon&
     preDraw();
 
     SkPath polygonPath;
-    addPolyPolygonToPath(aPolyPolygon, polygonPath);
+    bool hasOnlyOrthogonal = true;
+    addPolyPolygonToPath(aPolyPolygon, polygonPath, &hasOnlyOrthogonal);
     polygonPath.setFillType(SkPathFillType::kEvenOdd);
     addUpdateRegion(polygonPath.getBounds());
 
     SkPaint aPaint;
     aPaint.setAntiAlias(useAA);
-    // 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 = useAA ? toSkXYFix : 0;
+
+    // For lines we use toSkX()/toSkY() in order to pass centers of pixels to Skia,
+    // as that leads to better results with floating-point coordinates
+    // (e.g. https://bugs.chromium.org/p/skia/issues/detail?id=9611).
+    // But that means that we generally need to use it also for areas, so that they
+    // line up properly if used together (tdf#134346).
+    // On the other hand, with AA enabled and rectangular areas, this leads to fuzzy
+    // edges (tdf#137329). But since rectangular areas line up perfectly to pixels
+    // everywhere, it shouldn't be necessary to do this for them.
+    // So if AA is enabled, avoid this fixup for rectangular areas.
+    if (!useAA || !hasOnlyOrthogonal)
+    {
+        // 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 = useAA ? toSkXYFix : 0;
+        polygonPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
+    }
     if (mFillColor != SALCOLOR_NONE)
     {
-        SkPath path;
-        polygonPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, &path);
         aPaint.setColor(toSkColorWithTransparency(mFillColor, fTransparency));
         aPaint.setStyle(SkPaint::kFill_Style);
         // HACK: If the polygon is just a line, it still should be drawn. But when filling
         // Skia doesn't draw empty polygons, so in that case ensure the line is drawn.
-        if (mLineColor == SALCOLOR_NONE && path.getBounds().isEmpty())
+        if (mLineColor == SALCOLOR_NONE && polygonPath.getBounds().isEmpty())
             aPaint.setStyle(SkPaint::kStroke_Style);
-        getDrawCanvas()->drawPath(path, aPaint);
+        getDrawCanvas()->drawPath(polygonPath, aPaint);
     }
     if (mLineColor != SALCOLOR_NONE)
     {
-        SkPath path;
-        polygonPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, &path);
         aPaint.setColor(toSkColorWithTransparency(mLineColor, fTransparency));
         aPaint.setStyle(SkPaint::kStroke_Style);
-        getDrawCanvas()->drawPath(path, aPaint);
+        getDrawCanvas()->drawPath(polygonPath, aPaint);
     }
     postDraw();
 #if defined LINUX