use center of pixels when doing GPU drawing using Skia

According to https://bugs.chromium.org/p/skia/issues/detail?id=9611
rounding errors may cause off-by-one errors, so compensate when
converting int->SkScalar in relevant cases.

Change-Id: I72a579064206c216c9f99adc7d7c2c57bbe567d6

5 files changed, 37 insertions, 30 deletions

diff --git a/vcl/inc/skia/gdiimpl.hxx b/vcl/inc/skia/gdiimpl.hxx
index f45b29abb07e..8225e76d27ff 100644
--- a/vcl/inc/skia/gdiimpl.hxx
+++ b/vcl/inc/skia/gdiimpl.hxx
@@ -215,8 +215,7 @@ protected:
     void setProvider(SalGeometryProvider* provider) { mProvider = provider; }
 
     bool isOffscreen() const { return mProvider == nullptr || mProvider->IsOffScreen(); }
-    // TODO mainly for debugging purposes
-    bool isGPU() const;
+    bool isGPU() const { return mIsGPU; }
 
     void invert(basegfx::B2DPolygon const& rPoly, SalInvert eFlags);
 
@@ -231,6 +230,13 @@ protected:
 
     void drawMask(const SalTwoRect& rPosAry, const SkBitmap& rBitmap, Color nMaskColor);
 
+    // When drawing using GPU, rounding errors may result in off-by-one errors,
+    // 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; }
+
     // Which Skia backend to use.
     enum RenderMethod
     {
@@ -256,6 +262,7 @@ protected:
     SalGeometryProvider* mProvider;
     // The Skia surface that is target of all the rendering.
     sk_sp<SkSurface> mSurface;
+    bool mIsGPU; // whether the surface is GPU-backed
     vcl::Region mClipRegion;
     Color mLineColor;
     Color mFillColor;
diff --git a/vcl/qa/cppunit/BackendTest.cxx b/vcl/qa/cppunit/BackendTest.cxx
index a36c0faa5568..fd9bf10eaff5 100644
--- a/vcl/qa/cppunit/BackendTest.cxx
+++ b/vcl/qa/cppunit/BackendTest.cxx
@@ -438,8 +438,8 @@ public:
     //    CPPUNIT_TEST(testDrawFilledRectWithPolyPolygon); TODO SKIA
     //    CPPUNIT_TEST(testDrawFilledRectWithPolyPolygon2D); TODO SKIA
 
-    //    CPPUNIT_TEST(testDrawDiamondWithPolygon); TODO SKIA
-    //    CPPUNIT_TEST(testDrawDiamondWithLine); TODO SKIA
+    CPPUNIT_TEST(testDrawDiamondWithPolygon);
+    CPPUNIT_TEST(testDrawDiamondWithLine);
     CPPUNIT_TEST(testDrawDiamondWithPolyline);
     CPPUNIT_TEST(testDrawDiamondWithPolylineB2D);
 
diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx
index 612e097b6e12..861c3bceb591 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -40,7 +40,7 @@ namespace
 {
 // Create Skia Path from B2DPolygon
 // TODO - use this for all Polygon / PolyPolygon needs
-void lclPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
+void addPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
 {
     const sal_uInt32 nPointCount(rPolygon.count());
 
@@ -105,7 +105,7 @@ void lclPolygonToPath(const basegfx::B2DPolygon& rPolygon, SkPath& rPath)
     }
 }
 
-void lclPolyPolygonToPath(const basegfx::B2DPolyPolygon& rPolyPolygon, SkPath& rPath)
+void addPolyPolygonToPath(const basegfx::B2DPolyPolygon& rPolyPolygon, SkPath& rPath)
 {
     const sal_uInt32 nPolygonCount(rPolyPolygon.count());
 
@@ -114,7 +114,7 @@ void lclPolyPolygonToPath(const basegfx::B2DPolyPolygon& rPolyPolygon, SkPath& r
 
     for (const auto& rPolygon : rPolyPolygon)
     {
-        lclPolygonToPath(rPolygon, rPath);
+        addPolygonToPath(rPolygon, rPath);
     }
 }
 
@@ -183,6 +183,7 @@ SkiaSalGraphicsImpl::RenderMethod SkiaSalGraphicsImpl::renderMethodToUse()
 SkiaSalGraphicsImpl::SkiaSalGraphicsImpl(SalGraphics& rParent, SalGeometryProvider* pProvider)
     : mParent(rParent)
     , mProvider(pProvider)
+    , mIsGPU(false)
     , mLineColor(SALCOLOR_NONE)
     , mFillColor(SALCOLOR_NONE)
     , mFlush(new SkiaFlushIdle(this))
@@ -210,6 +211,7 @@ void SkiaSalGraphicsImpl::createSurface()
     // Create surface for offscreen graphics. Subclasses will create GPU-backed
     // surfaces as appropriate.
     mSurface = SkSurface::MakeRasterN32Premul(GetWidth(), GetHeight());
+    mIsGPU = false;
 #ifdef DBG_UTIL
     prefillSurface();
 #endif
@@ -234,6 +236,7 @@ void SkiaSalGraphicsImpl::destroySurface()
     if (mSurface)
         mSurface->flush();
     mSurface.reset();
+    mIsGPU = false;
 }
 
 void SkiaSalGraphicsImpl::DeInit() { destroySurface(); }
@@ -287,7 +290,7 @@ static SkRegion toSkRegion(const vcl::Region& region)
     if (region.HasPolyPolygonOrB2DPolyPolygon())
     {
         SkPath path;
-        lclPolyPolygonToPath(region.GetAsB2DPolyPolygon(), path);
+        addPolyPolygonToPath(region.GetAsB2DPolyPolygon(), path);
         path.setFillType(SkPath::kEvenOdd_FillType);
         SkRegion skRegion;
         skRegion.setPath(path, SkRegion(path.getBounds().roundOut()));
@@ -327,7 +330,7 @@ bool SkiaSalGraphicsImpl::setClipRegion(const vcl::Region& region)
     if (!region.IsEmpty() && !region.IsRectangle())
     {
         SkPath path;
-        lclPolyPolygonToPath(region.GetAsB2DPolyPolygon(), path);
+        addPolyPolygonToPath(region.GetAsB2DPolyPolygon(), path);
         path.setFillType(SkPath::kEvenOdd_FillType);
         canvas->clipPath(path);
     }
@@ -407,9 +410,7 @@ void SkiaSalGraphicsImpl::drawPixel(long nX, long nY, Color nColor)
     paint.setColor(toSkColor(nColor));
     // Apparently drawPixel() is actually expected to set the pixel and not draw it.
     paint.setBlendMode(SkBlendMode::kSrc); // set as is, including alpha
-    if (isGPU()) // TODO this may need caching?
-        ++nX; // https://bugs.chromium.org/p/skia/issues/detail?id=9611
-    canvas->drawPoint(nX, nY, paint);
+    canvas->drawPoint(toSkX(nX), toSkY(nY), paint);
     postDraw();
 }
 
@@ -424,7 +425,7 @@ void SkiaSalGraphicsImpl::drawLine(long nX1, long nY1, long nX2, long nY2)
     SkPaint paint;
     paint.setColor(toSkColor(mLineColor));
     paint.setAntiAlias(mParent.getAntiAliasB2DDraw());
-    canvas->drawLine(nX1, nY1, nX2, nY2, paint);
+    canvas->drawLine(toSkX(nX1), toSkY(nY1), toSkX(nX2), toSkY(nY2), paint);
     postDraw();
 }
 
@@ -521,7 +522,7 @@ bool SkiaSalGraphicsImpl::drawPolyPolygon(const basegfx::B2DHomMatrix& rObjectTo
     aPolyPolygon.transform(rObjectToDevice);
     SAL_INFO("vcl.skia", "drawpolypolygon(" << this << "): " << aPolyPolygon << ":" << mLineColor
                                             << ":" << mFillColor);
-    lclPolyPolygonToPath(aPolyPolygon, aPath);
+    addPolyPolygonToPath(aPolyPolygon, aPath);
     aPath.setFillType(SkPath::kEvenOdd_FillType);
 
     SkPaint aPaint;
@@ -534,6 +535,8 @@ bool SkiaSalGraphicsImpl::drawPolyPolygon(const basegfx::B2DHomMatrix& rObjectTo
     }
     if (mLineColor != SALCOLOR_NONE)
     {
+        if (isGPU()) // Apply the same adjustment as toSkX()/toSkY() do.
+            aPath.offset(0.5, 0.5, nullptr);
         aPaint.setColor(toSkColorWithTransparency(mLineColor, fTransparency));
         aPaint.setStyle(SkPaint::kStroke_Style);
         mSurface->getCanvas()->drawPath(aPath, aPaint);
@@ -626,14 +629,12 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
     aPaint.setAntiAlias(mParent.getAntiAliasB2DDraw());
 
     SkPath aPath;
-    lclPolygonToPath(rPolyLine, aPath);
+    addPolygonToPath(rPolyLine, aPath);
     aPath.setFillType(SkPath::kEvenOdd_FillType);
-    SkMatrix matrix = SkMatrix::MakeTrans(0.5, 0.5);
-    {
-        SkAutoCanvasRestore autoRestore(mSurface->getCanvas(), true);
-        mSurface->getCanvas()->concat(matrix);
-        mSurface->getCanvas()->drawPath(aPath, aPaint);
-    }
+    // 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);
+    mSurface->getCanvas()->drawPath(aPath, aPaint);
     postDraw();
 
     return true;
@@ -838,7 +839,7 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl
     if (eFlags == SalInvert::TrackFrame)
     {
         SkPath aPath;
-        lclPolygonToPath(rPoly, aPath);
+        addPolygonToPath(rPoly, aPath);
         aPath.setFillType(SkPath::kEvenOdd_FillType);
         SkPaint aPaint;
         aPaint.setStrokeWidth(2);
@@ -853,7 +854,7 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl
     else
     {
         SkPath aPath;
-        lclPolygonToPath(rPoly, aPath);
+        addPolygonToPath(rPoly, aPath);
         aPath.setFillType(SkPath::kEvenOdd_FillType);
         SkPaint aPaint;
         aPaint.setColor(SkColorSetARGB(255, 255, 255, 255));
@@ -1057,13 +1058,6 @@ bool SkiaSalGraphicsImpl::supportsOperation(OutDevSupportType eType) const
     }
 }
 
-bool SkiaSalGraphicsImpl::isGPU() const
-{
-    return mSurface.get()
-           && mSurface->getBackendRenderTarget(SkSurface::kFlushWrite_BackendHandleAccess)
-                  .isValid();
-}
-
 #ifdef DBG_UTIL
 void SkiaSalGraphicsImpl::dump(const char* file) const
 {
diff --git a/vcl/skia/win/gdiimpl.cxx b/vcl/skia/win/gdiimpl.cxx
index f908ace261a3..e9db555c25c0 100644
--- a/vcl/skia/win/gdiimpl.cxx
+++ b/vcl/skia/win/gdiimpl.cxx
@@ -54,6 +54,7 @@ void WinSkiaSalGraphicsImpl::createSurface()
                 mSurface = SkSurface::MakeRenderTarget(
                     SkiaVulkanGrContext::getGrContext(), SkBudgeted::kNo,
                     SkImageInfo::MakeN32Premul(GetWidth(), GetHeight()));
+                mIsGPU = true;
                 assert(mSurface.get());
 #ifdef DBG_UTIL
                 prefillSurface();
@@ -74,10 +75,12 @@ void WinSkiaSalGraphicsImpl::createSurface()
         case RenderRaster:
             mWindowContext = sk_app::window_context_factory::MakeRasterForWin(mWinParent.gethWnd(),
                                                                               displayParams);
+            mIsGPU = false;
             break;
         case RenderVulkan:
             mWindowContext = sk_app::window_context_factory::MakeVulkanForWin(mWinParent.gethWnd(),
                                                                               displayParams);
+            mIsGPU = true;
             break;
     }
     assert(SkToBool(mWindowContext)); // TODO
diff --git a/vcl/skia/x11/gdiimpl.cxx b/vcl/skia/x11/gdiimpl.cxx
index af602d35bede..72fc311f7aaa 100644
--- a/vcl/skia/x11/gdiimpl.cxx
+++ b/vcl/skia/x11/gdiimpl.cxx
@@ -49,6 +49,7 @@ void X11SkiaSalGraphicsImpl::createSurface()
                 mSurface = SkSurface::MakeRenderTarget(
                     SkiaVulkanGrContext::getGrContext(), SkBudgeted::kNo,
                     SkImageInfo::MakeN32Premul(GetWidth(), GetHeight()));
+                mIsGPU = true;
                 assert(mSurface.get());
 #ifdef DBG_UTIL
                 prefillSurface();
@@ -83,10 +84,12 @@ void X11SkiaSalGraphicsImpl::createSurface()
         case RenderRaster:
             mWindowContext
                 = sk_app::window_context_factory::MakeRasterForXlib(winInfo, displayParams);
+            mIsGPU = false;
             break;
         case RenderVulkan:
             mWindowContext
                 = sk_app::window_context_factory::MakeVulkanForXlib(winInfo, displayParams);
+            mIsGPU = true;
             break;
     }
     assert(SkToBool(mWindowContext)); // TODO