diff options
author | Luboš Luňák <l.lunak@collabora.com> | 2019-11-07 11:59:13 +0100 |
---|---|---|
committer | Luboš Luňák <l.lunak@collabora.com> | 2019-11-27 09:55:16 +0100 |
commit | 36d0e875c72341ee0305c2840556e3839f136034 (patch) | |
tree | 4f78598e369dc72885730188787ea3f6b0c1f21c | |
parent | 54be644c092eebb8be57a251556fe3f0e5f99e57 (diff) |
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
-rw-r--r-- | vcl/inc/skia/gdiimpl.hxx | 11 | ||||
-rw-r--r-- | vcl/qa/cppunit/BackendTest.cxx | 4 | ||||
-rw-r--r-- | vcl/skia/gdiimpl.cxx | 46 | ||||
-rw-r--r-- | vcl/skia/win/gdiimpl.cxx | 3 | ||||
-rw-r--r-- | vcl/skia/x11/gdiimpl.cxx | 3 |
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 |