batch Skia xor drawing (tdf#132241)

The code previously applied the xor operation after each drawing,
but the bugdoc draws a large number of polygons in xor mode,
so the xor drawing was done repeatedly. Do the xor drawing just
once when leaving the xor mode.

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

3 files changed, 146 insertions, 94 deletions

diff --git a/vcl/inc/skia/gdiimpl.hxx b/vcl/inc/skia/gdiimpl.hxx
index a99bb9ae4ce1..904053200d0a 100644
--- a/vcl/inc/skia/gdiimpl.hxx
+++ b/vcl/inc/skia/gdiimpl.hxx
@@ -26,6 +26,7 @@
 #include <salgeom.hxx>
 
 #include <SkSurface.h>
+#include <SkRegion.h>
 
 #include <prewin.h>
 #include <tools/sk_app/WindowContext.h>
@@ -219,6 +220,8 @@ protected:
     void postDraw();
     // The canvas to draw to. Will be diverted to a temporary for Xor mode.
     SkCanvas* getDrawCanvas() { return mXorMode ? getXorCanvas() : mSurface->getCanvas(); }
+    // Call before makeImageSnapshot(), ensures the content is up to date.
+    void flushDrawing();
 
     virtual void createSurface();
     // Call to ensure that mSurface is valid. If mSurface is going to be modified,
@@ -254,6 +257,15 @@ protected:
     void drawMask(const SalTwoRect& rPosAry, const sk_sp<SkImage>& rImage, Color nMaskColor);
 
     SkCanvas* getXorCanvas();
+    void applyXor();
+    void addXorRegion(const SkRect& rect)
+    {
+        if (mXorMode)
+        {
+            // Make slightly larger, just in case (rounding, antialiasing,...).
+            mXorRegion.op(rect.makeOutset(2, 2).round(), SkRegion::kUnion_Op);
+        }
+    }
     static void setCanvasClipRegion(SkCanvas* canvas, const vcl::Region& region);
 
     // When drawing using GPU, rounding errors may result in off-by-one errors,
@@ -286,7 +298,7 @@ protected:
     bool mXorMode;
     SkBitmap mXorBitmap;
     std::unique_ptr<SkCanvas> mXorCanvas;
-    SkRect mXorExtents; // the area that needs updating for the xor operation (or empty for all)
+    SkRegion mXorRegion; // the area that needs updating for the xor operation
     std::unique_ptr<SkiaFlushIdle> mFlush;
     int mPendingPixelsToFlush;
 };
diff --git a/vcl/inc/skia/utils.hxx b/vcl/inc/skia/utils.hxx
index 16e5addd6909..942b5c3b88ef 100644
--- a/vcl/inc/skia/utils.hxx
+++ b/vcl/inc/skia/utils.hxx
@@ -25,6 +25,7 @@
 #include <tools/gen.hxx>
 #include <driverblocklist.hxx>
 
+#include <SkRegion.h>
 #include <tools/sk_app/VulkanWindowContext.h>
 
 namespace SkiaHelper
@@ -67,6 +68,42 @@ inline DriverBlocklist::DeviceVendor getVendor()
 
 } // namespace
 
+template <typename charT, typename traits>
+inline std::basic_ostream<charT, traits>& operator<<(std::basic_ostream<charT, traits>& stream,
+                                                     const SkRect& rectangle)
+{
+    if (rectangle.isEmpty())
+        return stream << "EMPTY";
+    else
+        return stream << rectangle.width() << 'x' << rectangle.height() << "@(" << rectangle.x()
+                      << ',' << rectangle.y() << ")";
+}
+
+template <typename charT, typename traits>
+inline std::basic_ostream<charT, traits>& operator<<(std::basic_ostream<charT, traits>& stream,
+                                                     const SkIRect& rectangle)
+{
+    if (rectangle.isEmpty())
+        return stream << "EMPTY";
+    else
+        return stream << rectangle.width() << 'x' << rectangle.height() << "@(" << rectangle.x()
+                      << ',' << rectangle.y() << ")";
+}
+
+template <typename charT, typename traits>
+inline std::basic_ostream<charT, traits>& operator<<(std::basic_ostream<charT, traits>& stream,
+                                                     const SkRegion& region)
+{
+    if (region.isEmpty())
+        return stream << "EMPTY";
+    stream << "(";
+    SkRegion::Iterator it(region);
+    for (int i = 0; !it.done(); it.next(), ++i)
+        stream << "[" << i << "] " << it.rect();
+    stream << ")";
+    return stream;
+}
+
 #endif // INCLUDED_VCL_INC_SKIA_UTILS_H
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx
index b4bda610e1b6..1937115e1c23 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -327,71 +327,10 @@ void SkiaSalGraphicsImpl::preDraw()
     assert(comphelper::SolarMutex::get()->IsCurrentThread());
     SkiaZone::enter(); // matched in postDraw()
     checkSurface();
-    assert(!mXorMode || mXorExtents.isEmpty()); // must be reset in postDraw()
 }
 
 void SkiaSalGraphicsImpl::postDraw()
 {
-    if (mXorMode)
-    {
-        // Apply the result from the temporary bitmap manually. This is indeed
-        // slow, but it doesn't seem to be needed often and can be optimized
-        // in each operation by setting mXorExtents to the area that should be
-        // updated.
-        if (mXorExtents.isEmpty())
-            mXorExtents = SkRect::MakeXYWH(0, 0, mSurface->width(), mSurface->height());
-        else
-        {
-            // Make slightly larger, just in case (rounding, antialiasing,...).
-            mXorExtents.outset(2, 2);
-            if (!mXorExtents.intersect(
-                    SkRect::MakeXYWH(0, 0, mSurface->width(), mSurface->height())))
-                mXorExtents.setEmpty();
-        }
-        SAL_INFO("vcl.skia.trace",
-                 "applyxor(" << this << "): "
-                             << tools::Rectangle(mXorExtents.left(), mXorExtents.top(),
-                                                 mXorExtents.right(), mXorExtents.bottom()));
-        if (!mXorExtents.isEmpty()) // the intersection above may be empty
-        {
-            // Copy the surface contents to another pixmap.
-            SkBitmap surfaceBitmap;
-            // Use unpremultiplied alpha format, so that we do not have to do the conversions to get
-            // the RGB and back (Skia will do it when converting, but it'll be presumably faster at it).
-            if (!surfaceBitmap.tryAllocPixels(
-                    mSurface->imageInfo().makeAlphaType(kUnpremul_SkAlphaType)))
-                abort();
-            SkPaint paint;
-            paint.setBlendMode(SkBlendMode::kSrc); // copy as is
-            SkCanvas canvas(surfaceBitmap);
-            canvas.drawImageRect(mSurface->makeImageSnapshot(), mXorExtents, mXorExtents, &paint);
-            // xor to surfaceBitmap
-            assert(surfaceBitmap.info().alphaType() == kUnpremul_SkAlphaType);
-            assert(mXorBitmap.info().alphaType() == kUnpremul_SkAlphaType);
-            assert(surfaceBitmap.bytesPerPixel() == 4);
-            assert(mXorBitmap.bytesPerPixel() == 4);
-            for (int y = mXorExtents.top(); y < mXorExtents.bottom(); ++y)
-            {
-                uint8_t* data = static_cast<uint8_t*>(surfaceBitmap.getAddr(mXorExtents.x(), y));
-                const uint8_t* xordata
-                    = static_cast<uint8_t*>(mXorBitmap.getAddr(mXorExtents.x(), y));
-                for (int x = 0; x < mXorExtents.width(); ++x)
-                {
-                    *data++ ^= *xordata++;
-                    *data++ ^= *xordata++;
-                    *data++ ^= *xordata++;
-                    // alpha is not xor-ed
-                    data++;
-                    xordata++;
-                }
-            }
-            surfaceBitmap.notifyPixelsChanged();
-            mSurface->getCanvas()->drawBitmapRect(surfaceBitmap, mXorExtents, mXorExtents, &paint);
-        }
-        mXorCanvas.reset();
-        mXorBitmap.reset();
-        mXorExtents.setEmpty();
-    }
     if (!isOffscreen())
     {
         if (!Application::IsInExecute())
@@ -440,7 +379,10 @@ void SkiaSalGraphicsImpl::checkSurface()
             // will be usually repainted anyway.
             sk_sp<SkImage> snapshot;
             if (!isOffscreen())
+            {
+                flushDrawing();
                 snapshot = mSurface->makeImageSnapshot();
+            }
             recreateSurface();
             if (snapshot)
             {
@@ -456,6 +398,13 @@ void SkiaSalGraphicsImpl::checkSurface()
     }
 }
 
+void SkiaSalGraphicsImpl::flushDrawing()
+{
+    if (mXorMode)
+        applyXor();
+    mSurface->flush();
+}
+
 bool SkiaSalGraphicsImpl::setClipRegion(const vcl::Region& region)
 {
     if (mClipRegion == region)
@@ -520,9 +469,14 @@ void SkiaSalGraphicsImpl::SetFillColor(Color nColor) { mFillColor = nColor; }
 
 void SkiaSalGraphicsImpl::SetXORMode(bool set, bool)
 {
+    if (mXorMode == set)
+        return;
+    SAL_INFO("vcl.skia.trace", "setxormode(" << this << "): " << set);
+    if (set)
+        mXorRegion.setEmpty();
+    else
+        applyXor();
     mXorMode = set;
-    if (mXorMode)
-        mXorExtents.setEmpty();
 }
 
 SkCanvas* SkiaSalGraphicsImpl::getXorCanvas()
@@ -534,7 +488,7 @@ SkCanvas* SkiaSalGraphicsImpl::getXorCanvas()
     // There's no point in using SkSurface for GPU, we'd immediately need to get the pixels back.
     if (!mXorCanvas)
     {
-        // Use unpremultiplied alpha (see xor applying in PostDraw()).
+        // Use unpremultiplied alpha (see xor applying in applyXor()).
         if (!mXorBitmap.tryAllocPixels(mSurface->imageInfo().makeAlphaType(kUnpremul_SkAlphaType)))
             abort();
         mXorBitmap.eraseARGB(0, 0, 0, 0);
@@ -544,6 +498,61 @@ SkCanvas* SkiaSalGraphicsImpl::getXorCanvas()
     return mXorCanvas.get();
 }
 
+void SkiaSalGraphicsImpl::applyXor()
+{
+    // Apply the result from the temporary bitmap manually. This is indeed
+    // slow, but it doesn't seem to be needed often and is optimized
+    // in each operation by extending mXorRegion with the area that should be
+    // updated.
+    assert(mXorMode);
+    if (!mXorRegion.op(SkIRect::MakeXYWH(0, 0, mSurface->width(), mSurface->height()),
+                       SkRegion::kIntersect_Op))
+    {
+        mXorRegion.setEmpty();
+        return;
+    }
+    SAL_INFO("vcl.skia.trace", "applyxor(" << this << "): " << mXorRegion);
+    // Copy the surface contents to another pixmap.
+    SkBitmap surfaceBitmap;
+    // Use unpremultiplied alpha format, so that we do not have to do the conversions to get
+    // the RGB and back (Skia will do it when converting, but it'll be presumably faster at it).
+    if (!surfaceBitmap.tryAllocPixels(mSurface->imageInfo().makeAlphaType(kUnpremul_SkAlphaType)))
+        abort();
+    SkPaint paint;
+    paint.setBlendMode(SkBlendMode::kSrc); // copy as is
+    SkCanvas canvas(surfaceBitmap);
+    canvas.drawImageRect(mSurface->makeImageSnapshot(), mXorRegion.getBounds(),
+                         SkRect::Make(mXorRegion.getBounds()), &paint);
+    // xor to surfaceBitmap
+    assert(surfaceBitmap.info().alphaType() == kUnpremul_SkAlphaType);
+    assert(mXorBitmap.info().alphaType() == kUnpremul_SkAlphaType);
+    assert(surfaceBitmap.bytesPerPixel() == 4);
+    assert(mXorBitmap.bytesPerPixel() == 4);
+    for (SkRegion::Iterator it(mXorRegion); !it.done(); it.next())
+    {
+        for (int y = it.rect().top(); y < it.rect().bottom(); ++y)
+        {
+            uint8_t* data = static_cast<uint8_t*>(surfaceBitmap.getAddr(it.rect().x(), y));
+            const uint8_t* xordata = static_cast<uint8_t*>(mXorBitmap.getAddr(it.rect().x(), y));
+            for (int x = 0; x < it.rect().width(); ++x)
+            {
+                *data++ ^= *xordata++;
+                *data++ ^= *xordata++;
+                *data++ ^= *xordata++;
+                // alpha is not xor-ed
+                data++;
+                xordata++;
+            }
+        }
+    }
+    surfaceBitmap.notifyPixelsChanged();
+    mSurface->getCanvas()->drawBitmapRect(surfaceBitmap, mXorRegion.getBounds(),
+                                          SkRect::Make(mXorRegion.getBounds()), &paint);
+    mXorCanvas.reset();
+    mXorBitmap.reset();
+    mXorRegion.setEmpty();
+}
+
 void SkiaSalGraphicsImpl::SetROPLineColor(SalROPColor nROPColor)
 {
     switch (nROPColor)
@@ -589,8 +598,7 @@ void SkiaSalGraphicsImpl::drawPixel(long nX, long nY, Color nColor)
     // Apparently drawPixel() is actually expected to set the pixel and not draw it.
     paint.setBlendMode(SkBlendMode::kSrc); // set as is, including alpha
     getDrawCanvas()->drawPoint(toSkX(nX), toSkY(nY), paint);
-    if (mXorMode) // limit xor area update
-        mXorExtents = SkRect::MakeXYWH(nX, nY, 1, 1);
+    addXorRegion(SkRect::MakeXYWH(nX, nY, 1, 1));
     postDraw();
 }
 
@@ -609,8 +617,7 @@ void SkiaSalGraphicsImpl::drawLine(long nX1, long nY1, long nX2, long nY2)
         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);
-    if (mXorMode) // limit xor area update
-        mXorExtents = SkRect::MakeLTRB(nX1, nY1, nX2 + 1, nY2 + 1);
+    addXorRegion(SkRect::MakeLTRB(nX1, nY1, nX2 + 1, nY2 + 1));
     postDraw();
 }
 
@@ -636,8 +643,7 @@ void SkiaSalGraphicsImpl::privateDrawAlphaRect(long nX, long nY, long nWidth, lo
         paint.setStyle(SkPaint::kStroke_Style);
         canvas->drawIRect(SkIRect::MakeXYWH(nX, nY, nWidth - 1, nHeight - 1), paint);
     }
-    if (mXorMode) // limit xor area update
-        mXorExtents = SkRect::MakeXYWH(nX, nY, nWidth, nHeight);
+    addXorRegion(SkRect::MakeXYWH(nX, nY, nWidth, nHeight));
     postDraw();
 }
 
@@ -732,8 +738,7 @@ bool SkiaSalGraphicsImpl::drawPolyPolygon(const basegfx::B2DHomMatrix& rObjectTo
         aPaint.setStyle(SkPaint::kStroke_Style);
         getDrawCanvas()->drawPath(aPath, aPaint);
     }
-    if (mXorMode) // limit xor area update
-        mXorExtents = aPath.getBounds();
+    addXorRegion(aPath.getBounds());
     postDraw();
 #if defined LINUX
     // WORKAROUND: The logo in the about dialog has drawing errors. This seems to happen
@@ -861,8 +866,7 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
     // case as it seems to produce better results.
     aPath.offset(0.5, 0.5, nullptr);
     getDrawCanvas()->drawPath(aPath, aPaint);
-    if (mXorMode) // limit xor area update
-        mXorExtents = aPath.getBounds();
+    addXorRegion(aPath.getBounds());
     postDraw();
 
     return true;
@@ -910,9 +914,9 @@ void SkiaSalGraphicsImpl::copyArea(long nDestX, long nDestY, long nSrcX, long nS
     SAL_INFO("vcl.skia.trace", "copyarea(" << this << "): " << Point(nSrcX, nSrcY) << "->"
                                            << Point(nDestX, nDestY) << "/"
                                            << Size(nSrcWidth, nSrcHeight));
+    assert(!mXorMode);
     ::copyArea(getDrawCanvas(), mSurface, nDestX, nDestY, nSrcX, nSrcY, nSrcWidth, nSrcHeight);
-    if (mXorMode) // limit xor area update
-        mXorExtents = SkRect::MakeXYWH(nDestX, nDestY, nSrcWidth, nSrcHeight);
+    addXorRegion(SkRect::MakeXYWH(nDestX, nDestY, nSrcWidth, nSrcHeight));
     postDraw();
 }
 
@@ -925,9 +929,13 @@ void SkiaSalGraphicsImpl::copyBits(const SalTwoRect& rPosAry, SalGraphics* pSrcG
         assert(dynamic_cast<SkiaSalGraphicsImpl*>(pSrcGraphics->GetImpl()));
         src = static_cast<SkiaSalGraphicsImpl*>(pSrcGraphics->GetImpl());
         src->checkSurface();
+        src->flushDrawing();
     }
     else
+    {
         src = this;
+        assert(!mXorMode);
+    }
     if (rPosAry.mnSrcWidth == rPosAry.mnDestWidth && rPosAry.mnSrcHeight == rPosAry.mnDestHeight)
     {
         auto srcDebug = [&]() -> std::string {
@@ -959,9 +967,8 @@ void SkiaSalGraphicsImpl::copyBits(const SalTwoRect& rPosAry, SalGraphics* pSrcG
                                                         rPosAry.mnDestWidth, rPosAry.mnDestHeight),
                                        &paint);
     }
-    if (mXorMode) // limit xor area update
-        mXorExtents = SkRect::MakeXYWH(rPosAry.mnDestX, rPosAry.mnDestY, rPosAry.mnDestWidth,
-                                       rPosAry.mnDestHeight);
+    addXorRegion(SkRect::MakeXYWH(rPosAry.mnDestX, rPosAry.mnDestY, rPosAry.mnDestWidth,
+                                  rPosAry.mnDestHeight));
     postDraw();
 }
 
@@ -1078,7 +1085,7 @@ std::shared_ptr<SalBitmap> SkiaSalGraphicsImpl::getBitmap(long nX, long nY, long
     checkSurface();
     SAL_INFO("vcl.skia.trace",
              "getbitmap(" << this << "): " << Point(nX, nY) << "/" << Size(nWidth, nHeight));
-    mSurface->getCanvas()->flush();
+    flushDrawing();
     // TODO makeImageSnapshot(rect) may copy the data, which may be a waste if this is used
     // e.g. for VirtualDevice's lame alpha blending, in which case the image will eventually end up
     // in blendAlphaBitmap(), where we could simply use the proper rect of the image.
@@ -1105,6 +1112,7 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl
 {
     preDraw();
     SAL_INFO("vcl.skia.trace", "invert(" << this << "): " << rPoly << ":" << int(eFlags));
+    assert(!mXorMode);
     // Intel Vulkan drivers (up to current 0.401.3889) have a problem
     // with SkBlendMode::kDifference(?) and surfaces wider than 1024 pixels, resulting
     // in drawing errors. Work that around by fetching the relevant part of the surface
@@ -1139,13 +1147,12 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl
             sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(area.width(), area.height());
             SkPaint copy;
             copy.setBlendMode(SkBlendMode::kSrc);
+            flushDrawing();
             surface->getCanvas()->drawImageRect(mSurface->makeImageSnapshot(), area, size, &copy);
             aPath.offset(-area.x(), -area.y());
             surface->getCanvas()->drawPath(aPath, aPaint);
             getDrawCanvas()->drawImageRect(surface->makeImageSnapshot(), size, area, &copy);
         }
-        if (mXorMode) // limit xor area update
-            mXorExtents = aPath.getBounds();
     }
     else
     {
@@ -1188,13 +1195,13 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl
             sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(area.width(), area.height());
             SkPaint copy;
             copy.setBlendMode(SkBlendMode::kSrc);
+            flushDrawing();
             surface->getCanvas()->drawImageRect(mSurface->makeImageSnapshot(), area, size, &copy);
             aPath.offset(-area.x(), -area.y());
             surface->getCanvas()->drawPath(aPath, aPaint);
             getDrawCanvas()->drawImageRect(surface->makeImageSnapshot(), size, area, &copy);
         }
-        if (mXorMode) // limit xor area update
-            mXorExtents = aPath.getBounds();
+        addXorRegion(aPath.getBounds());
     }
     postDraw();
 }
@@ -1258,8 +1265,7 @@ void SkiaSalGraphicsImpl::drawImage(const SalTwoRect& rPosAry, const sk_sp<SkIma
     preDraw();
     SAL_INFO("vcl.skia.trace", "drawimage(" << this << "): " << rPosAry << ":" << int(eBlendMode));
     getDrawCanvas()->drawImageRect(aImage, aSourceRect, aDestinationRect, &aPaint);
-    if (mXorMode) // limit xor area update
-        mXorExtents = aDestinationRect;
+    addXorRegion(aDestinationRect);
     postDraw();
 }
 
@@ -1277,8 +1283,7 @@ void SkiaSalGraphicsImpl::drawBitmap(const SalTwoRect& rPosAry, const SkBitmap&
     preDraw();
     SAL_INFO("vcl.skia.trace", "drawbitmap(" << this << "): " << rPosAry << ":" << int(eBlendMode));
     getDrawCanvas()->drawBitmapRect(aBitmap, aSourceRect, aDestinationRect, &aPaint);
-    if (mXorMode) // limit xor area update
-        mXorExtents = aDestinationRect;
+    addXorRegion(aDestinationRect);
     mPendingPixelsToFlush += aBitmap.width() * aBitmap.height();
     postDraw();
 }
@@ -1332,6 +1337,7 @@ bool SkiaSalGraphicsImpl::drawTransformedBitmap(const basegfx::B2DPoint& rNull,
         getDrawCanvas()->concat(aMatrix);
         getDrawCanvas()->drawImage(tmpSurface->makeImageSnapshot(), 0, 0);
     }
+    assert(!mXorMode);
     postDraw();
 
     return true;
@@ -1385,14 +1391,11 @@ void SkiaSalGraphicsImpl::drawGenericLayout(const GenericSalLayout& layout, Colo
                                       glyphForms.data(), font, SkTextEncoding::kGlyphID);
     preDraw();
     SAL_INFO("vcl.skia.trace",
-             "drawtextblob(" << this << "): "
-                             << tools::Rectangle(
-                                    Point(textBlob->bounds().x(), textBlob->bounds().y()),
-                                    Size(textBlob->bounds().width(), textBlob->bounds().height()))
-                             << ":" << textColor);
+             "drawtextblob(" << this << "): " << textBlob->bounds() << ":" << textColor);
     SkPaint paint;
     paint.setColor(toSkColor(textColor));
     getDrawCanvas()->drawTextBlob(textBlob, 0, 0, paint);
+    addXorRegion(textBlob->bounds());
     postDraw();
 }