merge needlessly split polygons back in Skia drawing (tdf#133016)

There are places in LO code that needlessly split polygons into
a group of adjacent polygons. These should theoretically result
in the same, but only if antialiasing is not used (where Skia
has a problem and according to Skia developers that's not really
Skia's fault). So whenever a possibly problematic polygon is
asked to be drawn, delay it and try to merge it with followup
polygons back into one polygon where those needlessly created
problematic edges do not exist.
This is indeed just a hack and those problematic places should
be fixed, but oh well :/.

Change-Id: I1b03fe7c2f5e8c962b0dcb8962196b7fea090146
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/98887
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
(cherry picked from commit 12147e0322e0fdd1b561c94e7ebd3fdd69ceaac0)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/98841
Reviewed-by: Adolfo Jayme Barrientos <fitojb@ubuntu.com>

4 files changed, 238 insertions, 11 deletions

diff --git a/basegfx/CppunitTest_basegfx.mk b/basegfx/CppunitTest_basegfx.mk
index 91cfc2226774..231854cdf057 100644
--- a/basegfx/CppunitTest_basegfx.mk
+++ b/basegfx/CppunitTest_basegfx.mk
@@ -29,6 +29,7 @@ $(eval $(call gb_CppunitTest_add_exception_objects,basegfx,\
     basegfx/test/B2DPolygonTest \
     basegfx/test/B2DPolygonToolsTest \
     basegfx/test/B2DPolyPolygonTest \
+    basegfx/test/B2DPolyPolygonCutterTest \
     basegfx/test/B1DRangeTest \
     basegfx/test/B2XRangeTest \
     basegfx/test/B2IBoxTest \
diff --git a/basegfx/test/B2DPolyPolygonCutterTest.cxx b/basegfx/test/B2DPolyPolygonCutterTest.cxx
new file mode 100644
index 000000000000..b111bf469d5e
--- /dev/null
+++ b/basegfx/test/B2DPolyPolygonCutterTest.cxx
@@ -0,0 +1,103 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * This file incorporates work covered by the following license notice:
+ *
+ *   Licensed to the Apache Software Foundation (ASF) under one or more
+ *   contributor license agreements. See the NOTICE file distributed
+ *   with this work for additional information regarding copyright
+ *   ownership. The ASF licenses this file to you under the Apache
+ *   License, Version 2.0 (the "License"); you may not use this file
+ *   except in compliance with the License. You may obtain a copy of
+ *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
+ */
+
+#include <cppunit/TestAssert.h>
+#include <cppunit/TestFixture.h>
+#include <cppunit/extensions/HelperMacros.h>
+
+#include <basegfx/polygon/b2dpolypolygoncutter.hxx>
+
+namespace basegfx
+{
+class b2dpolypolygoncutter : public CppUnit::TestFixture
+{
+public:
+    void testMergeToSinglePolyPolygon()
+    {
+        { // Adjacent polygons merged to one, closed manually.
+            B2DPolygon poly1{ { 0, 0 }, { 1, 0 }, { 1, 1 }, { 0, 1 }, { 0, 0 } };
+            B2DPolygon poly2{ { 0, 1 }, { 1, 1 }, { 1, 2 }, { 0, 2 }, { 0, 1 } };
+            B2DPolyPolygon expected(
+                B2DPolygon{ { 1, 0 }, { 1, 1 }, { 1, 2 }, { 0, 2 }, { 0, 1 }, { 0, 0 } });
+            expected.setClosed(true);
+            B2DPolyPolygon result
+                = utils::mergeToSinglePolyPolygon({ B2DPolyPolygon(poly1), B2DPolyPolygon(poly2) });
+            CPPUNIT_ASSERT_EQUAL(expected, result);
+        }
+
+        { // Adjacent polygons merged to one, closed using setClosed().
+            B2DPolygon poly1{ { 0, 0 }, { 1, 0 }, { 1, 1 }, { 0, 1 } };
+            B2DPolygon poly2{ { 0, 1 }, { 1, 1 }, { 1, 2 }, { 0, 2 } };
+            poly1.setClosed(true);
+            poly2.setClosed(true);
+            B2DPolyPolygon expected(
+                B2DPolygon{ { 0, 0 }, { 1, 0 }, { 1, 1 }, { 1, 2 }, { 0, 2 }, { 0, 1 } });
+            expected.setClosed(true);
+            B2DPolyPolygon result
+                = utils::mergeToSinglePolyPolygon({ B2DPolyPolygon(poly1), B2DPolyPolygon(poly2) });
+            CPPUNIT_ASSERT_EQUAL(expected, result);
+        }
+
+        { // Non-adjacent polygons, no merge.
+            B2DPolygon poly1{ { 0, 0 }, { 1, 0 }, { 1, 1 }, { 0, 1 } };
+            B2DPolygon poly2{ { 0, 2 }, { 1, 3 }, { 1, 3 }, { 0, 3 } };
+            poly1.setClosed(true);
+            poly2.setClosed(true);
+            B2DPolyPolygon expected;
+            expected.append(poly1);
+            expected.append(poly2);
+            B2DPolyPolygon result
+                = utils::mergeToSinglePolyPolygon({ B2DPolyPolygon(poly1), B2DPolyPolygon(poly2) });
+            CPPUNIT_ASSERT_EQUAL(expected, result);
+        }
+
+        { // Horizontal and vertical rectangle that together form a cross.
+            B2DPolygon poly1{ { 1, 0 }, { 2, 0 }, { 2, 3 }, { 1, 3 } };
+            B2DPolygon poly2{ { 0, 1 }, { 3, 1 }, { 3, 2 }, { 0, 2 } };
+            poly1.setClosed(true);
+            poly2.setClosed(true);
+            B2DPolyPolygon expected(B2DPolygon{ { 1, 0 },
+                                                { 2, 0 },
+                                                { 2, 1 },
+                                                { 3, 1 },
+                                                { 3, 2 },
+                                                { 2, 2 },
+                                                { 2, 3 },
+                                                { 1, 3 },
+                                                { 1, 2 },
+                                                { 0, 2 },
+                                                { 0, 1 },
+                                                { 1, 1 } });
+            expected.setClosed(true);
+            B2DPolyPolygon result
+                = utils::mergeToSinglePolyPolygon({ B2DPolyPolygon(poly1), B2DPolyPolygon(poly2) });
+            CPPUNIT_ASSERT_EQUAL(expected, result);
+        }
+    }
+
+    CPPUNIT_TEST_SUITE(b2dpolypolygoncutter);
+    CPPUNIT_TEST(testMergeToSinglePolyPolygon);
+    CPPUNIT_TEST_SUITE_END();
+};
+
+} // namespace basegfx
+
+CPPUNIT_TEST_SUITE_REGISTRATION(basegfx::b2dpolypolygoncutter);
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/inc/skia/gdiimpl.hxx b/vcl/inc/skia/gdiimpl.hxx
index b8a5a179d1d0..ab12fd1b521d 100644
--- a/vcl/inc/skia/gdiimpl.hxx
+++ b/vcl/inc/skia/gdiimpl.hxx
@@ -247,6 +247,7 @@ protected:
 
     // Called by SkiaFlushIdle.
     virtual void performFlush() = 0;
+    void scheduleFlush();
     friend class SkiaFlushIdle;
 
     // get the width of the device
@@ -277,6 +278,14 @@ protected:
     // Value to add to be exactly in the middle of the pixel.
     static constexpr SkScalar toSkXYFix = SkScalar(0.005);
 
+    // Perform any pending drawing such as delayed merging of polygons. Called by preDraw()
+    // and anything that means the next operation cannot be another one in a series (e.g.
+    // changing colors).
+    void checkPendingDrawing();
+    bool mergePolyPolygonToPrevious(const basegfx::B2DPolyPolygon& polygon, double transparency);
+    void performDrawPolyPolygon(const basegfx::B2DPolyPolygon& polygon, double transparency,
+                                bool useAA);
+
     template <typename charT, typename traits>
     friend inline std::basic_ostream<charT, traits>&
     operator<<(std::basic_ostream<charT, traits>& stream, const SkiaSalGraphicsImpl* graphics)
@@ -302,6 +311,13 @@ protected:
     std::unique_ptr<SkCanvas> mXorCanvas;
     SkRegion mXorRegion; // the area that needs updating for the xor operation
     std::unique_ptr<SkiaFlushIdle> mFlush;
+    // Info about pending polygons to draw (we try to merge adjacent polygons into one).
+    struct LastPolyPolygonInfo
+    {
+        basegfx::B2DPolyPolygon polygon;
+        double transparency;
+    };
+    LastPolyPolygonInfo mLastPolyPolygonInfo;
 };
 
 #endif
diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx
index d9c5ba487237..980f0c688651 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -39,6 +39,7 @@
 #include <numeric>
 #include <basegfx/polygon/b2dpolygontools.hxx>
 #include <basegfx/polygon/b2dpolypolygontools.hxx>
+#include <basegfx/polygon/b2dpolypolygoncutter.hxx>
 
 namespace
 {
@@ -326,10 +327,17 @@ void SkiaSalGraphicsImpl::preDraw()
     assert(comphelper::SolarMutex::get()->IsCurrentThread());
     SkiaZone::enter(); // matched in postDraw()
     checkSurface();
+    checkPendingDrawing();
 }
 
 void SkiaSalGraphicsImpl::postDraw()
 {
+    scheduleFlush();
+    SkiaZone::leave(); // matched in preDraw()
+}
+
+void SkiaSalGraphicsImpl::scheduleFlush()
+{
     if (!isOffscreen())
     {
         if (!Application::IsInExecute())
@@ -337,7 +345,6 @@ void SkiaSalGraphicsImpl::postDraw()
         else if (!mFlush->IsActive())
             mFlush->Start();
     }
-    SkiaZone::leave(); // matched in preDraw()
 }
 
 // VCL can sometimes resize us without telling us, update the surface if needed.
@@ -385,6 +392,7 @@ void SkiaSalGraphicsImpl::checkSurface()
 
 void SkiaSalGraphicsImpl::flushDrawing()
 {
+    checkPendingDrawing();
     if (mXorMode)
         applyXor();
     mSurface->flush();
@@ -395,6 +403,7 @@ bool SkiaSalGraphicsImpl::setClipRegion(const vcl::Region& region)
     if (mClipRegion == region)
         return true;
     SkiaZone zone;
+    checkPendingDrawing();
     mClipRegion = region;
     checkSurface();
     SAL_INFO("vcl.skia.trace", "setclipregion(" << this << "): " << region);
@@ -438,18 +447,35 @@ sal_uInt16 SkiaSalGraphicsImpl::GetBitCount() const { return 32; }
 
 long SkiaSalGraphicsImpl::GetGraphicsWidth() const { return GetWidth(); }
 
-void SkiaSalGraphicsImpl::SetLineColor() { mLineColor = SALCOLOR_NONE; }
+void SkiaSalGraphicsImpl::SetLineColor()
+{
+    checkPendingDrawing();
+    mLineColor = SALCOLOR_NONE;
+}
 
-void SkiaSalGraphicsImpl::SetLineColor(Color nColor) { mLineColor = nColor; }
+void SkiaSalGraphicsImpl::SetLineColor(Color nColor)
+{
+    checkPendingDrawing();
+    mLineColor = nColor;
+}
 
-void SkiaSalGraphicsImpl::SetFillColor() { mFillColor = SALCOLOR_NONE; }
+void SkiaSalGraphicsImpl::SetFillColor()
+{
+    checkPendingDrawing();
+    mFillColor = SALCOLOR_NONE;
+}
 
-void SkiaSalGraphicsImpl::SetFillColor(Color nColor) { mFillColor = nColor; }
+void SkiaSalGraphicsImpl::SetFillColor(Color nColor)
+{
+    checkPendingDrawing();
+    mFillColor = nColor;
+}
 
 void SkiaSalGraphicsImpl::SetXORMode(bool set, bool)
 {
     if (mXorMode == set)
         return;
+    checkPendingDrawing();
     SAL_INFO("vcl.skia.trace", "setxormode(" << this << "): " << set);
     if (set)
         mXorRegion.setEmpty();
@@ -535,6 +561,7 @@ void SkiaSalGraphicsImpl::applyXor()
 
 void SkiaSalGraphicsImpl::SetROPLineColor(SalROPColor nROPColor)
 {
+    checkPendingDrawing();
     switch (nROPColor)
     {
         case SalROPColor::N0:
@@ -551,6 +578,7 @@ void SkiaSalGraphicsImpl::SetROPLineColor(SalROPColor nROPColor)
 
 void SkiaSalGraphicsImpl::SetROPFillColor(SalROPColor nROPColor)
 {
+    checkPendingDrawing();
     switch (nROPColor)
     {
         case SalROPColor::N0:
@@ -687,23 +715,38 @@ bool SkiaSalGraphicsImpl::drawPolyPolygon(const basegfx::B2DHomMatrix& rObjectTo
         || fTransparency >= 1.0)
         return true;
 
-    preDraw();
-
-    SkPath aPath;
     basegfx::B2DPolyPolygon aPolyPolygon(rPolyPolygon);
     aPolyPolygon.transform(rObjectToDevice);
+
     SAL_INFO("vcl.skia.trace", "drawpolypolygon(" << this << "): " << aPolyPolygon << ":"
                                                   << mLineColor << ":" << mFillColor);
+
+    if (mergePolyPolygonToPrevious(aPolyPolygon, fTransparency))
+    {
+        scheduleFlush();
+        return true;
+    }
+
+    performDrawPolyPolygon(aPolyPolygon, fTransparency, mParent.getAntiAliasB2DDraw());
+    return true;
+}
+
+void SkiaSalGraphicsImpl::performDrawPolyPolygon(const basegfx::B2DPolyPolygon& aPolyPolygon,
+                                                 double fTransparency, bool useAA)
+{
+    preDraw();
+
+    SkPath aPath;
     addPolyPolygonToPath(aPolyPolygon, aPath);
     aPath.setFillType(SkPathFillType::kEvenOdd);
 
     SkPaint aPaint;
-    aPaint.setAntiAlias(mParent.getAntiAliasB2DDraw());
+    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 = mParent.getAntiAliasB2DDraw() ? toSkXYFix : 0;
+    const SkScalar posFix = useAA ? toSkXYFix : 0;
     if (mFillColor != SALCOLOR_NONE)
     {
         aPaint.setColor(toSkColorWithTransparency(mFillColor, fTransparency));
@@ -724,12 +767,76 @@ bool SkiaSalGraphicsImpl::drawPolyPolygon(const basegfx::B2DHomMatrix& rObjectTo
     // WORKAROUND: The logo in the about dialog has drawing errors. This seems to happen
     // only on Linux (not Windows on the same machine), with both AMDGPU and Mesa,
     // and only when antialiasing is enabled. Flushing seems to avoid the problem.
-    if (mParent.getAntiAliasB2DDraw() && SkiaHelper::getVendor() == DriverBlocklist::VendorAMD)
+    if (useAA && SkiaHelper::getVendor() == DriverBlocklist::VendorAMD)
         mSurface->flush();
 #endif
+}
+
+bool SkiaSalGraphicsImpl::mergePolyPolygonToPrevious(const basegfx::B2DPolyPolygon& aPolyPolygon,
+                                                     double fTransparency)
+{
+    // There is some code that needlessly subdivides areas into adjacent rectangles,
+    // but Skia doesn't line them up perfectly if AA is enabled (e.g. Cairo, Qt5 do,
+    // but Skia devs claim it's working as intended
+    // https://groups.google.com/d/msg/skia-discuss/NlKpD2X_5uc/Vuwd-kyYBwAJ).
+    // An example is tdf#133016, which triggers SvgStyleAttributes::add_stroke()
+    // implementing a line stroke as a bunch of polygons instead of just one, and
+    // SvgLinearAtomPrimitive2D::create2DDecomposition() creates a gradient
+    // as a series of polygons of gradually changing color. Those places should be
+    // changed, but try to merge those split polygons back into the original one,
+    // where the needlessly created edges causing problems will not exist.
+    // This means drawing of such polygons needs to be delayed, so that they can
+    // be possibly merged with the next one.
+    // Merge only polygons of the same properties (color, etc.), so the gradient problem
+    // actually isn't handled here.
+
+    // Only AA polygons need merging, because they do not line up well because of the AA of the edges.
+    if (!mParent.getAntiAliasB2DDraw())
+        return false;
+    // Only filled polygons without an outline are problematic.
+    if (mFillColor == SALCOLOR_NONE || mLineColor != SALCOLOR_NONE)
+        return false;
+    // Merge only simple polygons, real polypolygons most likely aren't needlessly split,
+    // so they do not need joining.
+    if (aPolyPolygon.count() != 1)
+        return false;
+
+    if (mLastPolyPolygonInfo.polygon.count() != 0
+        && (mLastPolyPolygonInfo.transparency != fTransparency
+            || !mLastPolyPolygonInfo.polygon.getB2DRange().overlaps(aPolyPolygon.getB2DRange())))
+    {
+        checkPendingDrawing(); // Cannot be parts of the same larger polygon, draw the last and reset.
+    }
+    if (mLastPolyPolygonInfo.polygon.count() == 0)
+    {
+        mLastPolyPolygonInfo.polygon = aPolyPolygon;
+        mLastPolyPolygonInfo.transparency = fTransparency;
+        return true;
+    }
+    basegfx::B2DPolyPolygon merged
+        = basegfx::utils::mergeToSinglePolyPolygon({ mLastPolyPolygonInfo.polygon, aPolyPolygon });
+    if (merged.count() == 0) // it wasn't actually merged
+    {
+        checkPendingDrawing();
+        mLastPolyPolygonInfo.polygon = aPolyPolygon;
+        mLastPolyPolygonInfo.transparency = fTransparency;
+        return true;
+    }
+    mLastPolyPolygonInfo.polygon = std::move(merged);
     return true;
 }
 
+void SkiaSalGraphicsImpl::checkPendingDrawing()
+{
+    if (mLastPolyPolygonInfo.polygon.count() != 0)
+    { // Flush any pending polygon drawing.
+        basegfx::B2DPolyPolygon polygon;
+        std::swap(polygon, mLastPolyPolygonInfo.polygon);
+        double transparency = mLastPolyPolygonInfo.transparency;
+        performDrawPolyPolygon(polygon, transparency, true);
+    }
+}
+
 bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDevice,
                                        const basegfx::B2DPolygon& rPolyLine, double fTransparency,
                                        double fLineWidth, const std::vector<double>* pStroke,