xor drawing done twice in the same place should be a no-op

This extends the VCL backend unittest to check for this, and also
fixes Skia to handle that properly.
This makes tdf#132241 slow again. The problem there is that it
does drawGradient() with xor enabled (for whatever strange reason),
and since Skia does not implement drawGradient(), it gets drawn
using polygons and their bounds overlap, causing applyXor() after
each operation again. Implementing drawGradient() will handle that.

Change-Id: Ibea433ad95f8c6d53049f4a49295e57a5aec184f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103280
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
(cherry picked from commit 787bc48c883b70b1721805b5d6a93bd731983410)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103337
Reviewed-by: Caolán McNamara <caolanm@redhat.com>

3 files changed, 47 insertions, 25 deletions

diff --git a/vcl/backendtest/outputdevice/outputdevice.cxx b/vcl/backendtest/outputdevice/outputdevice.cxx
index c05c03e06a3a..07d66ffb413c 100644
--- a/vcl/backendtest/outputdevice/outputdevice.cxx
+++ b/vcl/backendtest/outputdevice/outputdevice.cxx
@@ -49,6 +49,19 @@ Bitmap OutputDeviceTestAnotherOutDev::setupXOR()
     mpVirtualDevice->SetFillColor(constFillColor);
     mpVirtualDevice->DrawRect(aDrawRectangle);
 
+    mpVirtualDevice->SetRasterOp(RasterOp::Xor);
+    mpVirtualDevice->SetLineColor(constFillColor);
+    mpVirtualDevice->SetFillColor();
+    // Rectangle drawn twice is a no-op.
+    aDrawRectangle = maVDRectangle;
+    mpVirtualDevice->DrawRect(aDrawRectangle);
+    mpVirtualDevice->DrawRect(aDrawRectangle);
+    // Rectangle drawn three times is like drawing once.
+    aDrawRectangle.shrink(1);
+    mpVirtualDevice->DrawRect(aDrawRectangle);
+    mpVirtualDevice->DrawRect(aDrawRectangle);
+    mpVirtualDevice->DrawRect(aDrawRectangle);
+
     return mpVirtualDevice->GetBitmap(maVDRectangle.TopLeft(), maVDRectangle.GetSize());
 }
 
@@ -64,9 +77,12 @@ TestResult OutputDeviceTestAnotherOutDev::checkDrawOutDev(Bitmap& rBitmap)
 
 TestResult OutputDeviceTestAnotherOutDev::checkXOR(Bitmap& rBitmap)
 {
+    Color xorColor( constBackgroundColor.GetRed() ^ constFillColor.GetRed(),
+                    constBackgroundColor.GetGreen() ^ constFillColor.GetGreen(),
+                    constBackgroundColor.GetBlue() ^ constFillColor.GetBlue());
     std::vector<Color> aExpected
     {
-        constBackgroundColor, constBackgroundColor,
+        constBackgroundColor, xorColor,
         constBackgroundColor, constBackgroundColor,
         constFillColor, constFillColor,
         constFillColor
diff --git a/vcl/inc/skia/gdiimpl.hxx b/vcl/inc/skia/gdiimpl.hxx
index 5b72e4b4e249..e84c251cdde1 100644
--- a/vcl/inc/skia/gdiimpl.hxx
+++ b/vcl/inc/skia/gdiimpl.hxx
@@ -257,12 +257,19 @@ protected:
 
     SkCanvas* getXorCanvas();
     void applyXor();
+    // NOTE: This must be called before the operation does any drawing.
     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);
+            SkIRect addedRect = rect.makeOutset(2, 2).round();
+            // Two xor operations should cancel each other out. We batch xor operations,
+            // but if they can overlap, apply xor now, since applyXor() does the operation
+            // just once.
+            if (mXorRegion.intersects(addedRect))
+                applyXor();
+            mXorRegion.op(addedRect, SkRegion::kUnion_Op);
         }
     }
     static void setCanvasClipRegion(SkCanvas* canvas, const vcl::Region& region);
diff --git a/vcl/skia/gdiimpl.cxx b/vcl/skia/gdiimpl.cxx
index 743eea5d891a..2f308ad8aba8 100644
--- a/vcl/skia/gdiimpl.cxx
+++ b/vcl/skia/gdiimpl.cxx
@@ -559,7 +559,7 @@ void SkiaSalGraphicsImpl::applyXor()
     // in each operation by extending mXorRegion with the area that should be
     // updated.
     assert(mXorMode);
-    if (!mSurface
+    if (!mSurface || !mXorCanvas
         || !mXorRegion.op(SkIRect::MakeXYWH(0, 0, mSurface->width(), mSurface->height()),
                           SkRegion::kIntersect_Op))
     {
@@ -650,12 +650,12 @@ void SkiaSalGraphicsImpl::drawPixel(long nX, long nY, Color nColor)
         return;
     preDraw();
     SAL_INFO("vcl.skia.trace", "drawpixel(" << this << "): " << Point(nX, nY) << ":" << nColor);
+    addXorRegion(SkRect::MakeXYWH(nX, nY, 1, 1));
     SkPaint paint;
     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
     getDrawCanvas()->drawPoint(toSkX(nX), toSkY(nY), paint);
-    addXorRegion(SkRect::MakeXYWH(nX, nY, 1, 1));
     postDraw();
 }
 
@@ -666,11 +666,11 @@ void SkiaSalGraphicsImpl::drawLine(long nX1, long nY1, long nX2, long nY2)
     preDraw();
     SAL_INFO("vcl.skia.trace", "drawline(" << this << "): " << Point(nX1, nY1) << "->"
                                            << Point(nX2, nY2) << ":" << mLineColor);
+    addXorRegion(SkRect::MakeLTRB(nX1, nY1, nX2, nY2).makeSorted());
     SkPaint paint;
     paint.setColor(toSkColor(mLineColor));
     paint.setAntiAlias(mParent.getAntiAliasB2DDraw());
     getDrawCanvas()->drawLine(toSkX(nX1), toSkY(nY1), toSkX(nX2), toSkY(nY2), paint);
-    addXorRegion(SkRect::MakeLTRB(nX1, nY1, nX2 + 1, nY2 + 1));
     postDraw();
 }
 
@@ -681,6 +681,7 @@ void SkiaSalGraphicsImpl::privateDrawAlphaRect(long nX, long nY, long nWidth, lo
     SAL_INFO("vcl.skia.trace",
              "privatedrawrect(" << this << "): " << SkIRect::MakeXYWH(nX, nY, nWidth, nHeight)
                                 << ":" << mLineColor << ":" << mFillColor << ":" << fTransparency);
+    addXorRegion(SkRect::MakeXYWH(nX, nY, nWidth, nHeight));
     SkCanvas* canvas = getDrawCanvas();
     SkPaint paint;
     paint.setAntiAlias(!blockAA && mParent.getAntiAliasB2DDraw());
@@ -704,7 +705,6 @@ void SkiaSalGraphicsImpl::privateDrawAlphaRect(long nX, long nY, long nWidth, lo
         canvas->drawIRect(
             SkIRect::MakeXYWH(nX, nY, std::max(1L, nWidth - 1), std::max(1L, nHeight - 1)), paint);
     }
-    addXorRegion(SkRect::MakeXYWH(nX, nY, nWidth, nHeight));
     postDraw();
 }
 
@@ -789,9 +789,10 @@ void SkiaSalGraphicsImpl::performDrawPolyPolygon(const basegfx::B2DPolyPolygon&
 {
     preDraw();
 
-    SkPath aPath;
-    addPolyPolygonToPath(aPolyPolygon, aPath);
-    aPath.setFillType(SkPathFillType::kEvenOdd);
+    SkPath polygonPath;
+    addPolyPolygonToPath(aPolyPolygon, polygonPath);
+    polygonPath.setFillType(SkPathFillType::kEvenOdd);
+    addXorRegion(polygonPath.getBounds());
 
     SkPaint aPaint;
     aPaint.setAntiAlias(useAA);
@@ -802,23 +803,24 @@ void SkiaSalGraphicsImpl::performDrawPolyPolygon(const basegfx::B2DPolyPolygon&
     const SkScalar posFix = useAA ? toSkXYFix : 0;
     if (mFillColor != SALCOLOR_NONE)
     {
-        aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
+        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 && aPath.getBounds().isEmpty())
+        if (mLineColor == SALCOLOR_NONE && path.getBounds().isEmpty())
             aPaint.setStyle(SkPaint::kStroke_Style);
-        getDrawCanvas()->drawPath(aPath, aPaint);
+        getDrawCanvas()->drawPath(path, aPaint);
     }
     if (mLineColor != SALCOLOR_NONE)
     {
-        aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
+        SkPath path;
+        polygonPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, &path);
         aPaint.setColor(toSkColorWithTransparency(mLineColor, fTransparency));
         aPaint.setStyle(SkPaint::kStroke_Style);
-        getDrawCanvas()->drawPath(aPath, aPaint);
+        getDrawCanvas()->drawPath(path, aPaint);
     }
-    addXorRegion(aPath.getBounds());
     postDraw();
 #if defined LINUX
     // WORKAROUND: The logo in the about dialog has drawing errors. This seems to happen
@@ -1028,8 +1030,8 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
         for (sal_uInt32 a(0); a < aPolyPolygonLine.count(); a++)
             addPolygonToPath(aPolyPolygonLine.getB2DPolygon(a), aPath);
         aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
-        getDrawCanvas()->drawPath(aPath, aPaint);
         addXorRegion(aPath.getBounds());
+        getDrawCanvas()->drawPath(aPath, aPaint);
     }
     else
     {
@@ -1049,8 +1051,8 @@ bool SkiaSalGraphicsImpl::drawPolyLine(const basegfx::B2DHomMatrix& rObjectToDev
                              rPolygon.getB2DPoint(index2).getY());
 
                 aPath.offset(toSkX(0) + posFix, toSkY(0) + posFix, nullptr);
-                getDrawCanvas()->drawPath(aPath, aPaint);
                 addXorRegion(aPath.getBounds());
+                getDrawCanvas()->drawPath(aPath, aPaint);
             }
         }
     }
@@ -1120,7 +1122,6 @@ void SkiaSalGraphicsImpl::copyArea(long nDestX, long nDestY, long nSrcX, long nS
     assert(!mXorMode);
     ::copyArea(getDrawCanvas(), mSurface, nDestX, nDestY, nSrcX, nSrcY, nSrcWidth, nSrcHeight,
                !isGPU(), !isGPU());
-    addXorRegion(SkRect::MakeXYWH(nDestX, nDestY, nSrcWidth, nSrcHeight));
     postDraw();
 }
 
@@ -1175,8 +1176,7 @@ void SkiaSalGraphicsImpl::copyBits(const SalTwoRect& rPosAry, SalGraphics* pSrcG
                                                         rPosAry.mnDestWidth, rPosAry.mnDestHeight),
                                        &paint);
     }
-    addXorRegion(SkRect::MakeXYWH(rPosAry.mnDestX, rPosAry.mnDestY, rPosAry.mnDestWidth,
-                                  rPosAry.mnDestHeight));
+    assert(!mXorMode);
     postDraw();
 }
 
@@ -1392,7 +1392,6 @@ void SkiaSalGraphicsImpl::invert(basegfx::B2DPolygon const& rPoly, SalInvert eFl
             getDrawCanvas()->drawImageRect(SkiaHelper::makeCheckedImageSnapshot(surface), size,
                                            area, &copy);
         }
-        addXorRegion(aPath.getBounds());
     }
     postDraw();
 }
@@ -1588,8 +1587,8 @@ void SkiaSalGraphicsImpl::drawImage(const SalTwoRect& rPosAry, const sk_sp<SkIma
     preDraw();
     SAL_INFO("vcl.skia.trace",
              "drawimage(" << this << "): " << rPosAry << ":" << SkBlendMode_Name(eBlendMode));
-    getDrawCanvas()->drawImageRect(aImage, aSourceRect, aDestinationRect, &aPaint);
     addXorRegion(aDestinationRect);
+    getDrawCanvas()->drawImageRect(aImage, aSourceRect, aDestinationRect, &aPaint);
     postDraw();
 }
 
@@ -1601,6 +1600,7 @@ void SkiaSalGraphicsImpl::drawShader(const SalTwoRect& rPosAry, const sk_sp<SkSh
     SAL_INFO("vcl.skia.trace", "drawshader(" << this << "): " << rPosAry);
     SkRect destinationRect = SkRect::MakeXYWH(rPosAry.mnDestX, rPosAry.mnDestY, rPosAry.mnDestWidth,
                                               rPosAry.mnDestHeight);
+    addXorRegion(destinationRect);
     SkPaint paint;
     paint.setShader(shader);
     if (rPosAry.mnSrcWidth != rPosAry.mnDestWidth || rPosAry.mnSrcHeight != rPosAry.mnDestHeight)
@@ -1617,7 +1617,6 @@ void SkiaSalGraphicsImpl::drawShader(const SalTwoRect& rPosAry, const sk_sp<SkSh
     matrix.set(SkMatrix::kMTransY, rPosAry.mnDestY - rPosAry.mnSrcY);
     canvas->concat(matrix);
     canvas->drawPaint(paint);
-    addXorRegion(destinationRect);
     postDraw();
 }
 
@@ -1642,6 +1641,7 @@ bool SkiaSalGraphicsImpl::drawTransformedBitmap(const basegfx::B2DPoint& rNull,
     SAL_INFO("vcl.skia.trace", "drawtransformedbitmap(" << this << "): " << rSourceBitmap.GetSize()
                                                         << " " << rNull << ":" << rX << ":" << rY);
 
+    addXorRegion(SkRect::MakeWH(GetWidth(), GetHeight())); // can't tell, use whole area
     // In raster mode scaling and alpha blending is still somewhat expensive if done repeatedly,
     // so use mergeCacheBitmaps(), which will cache the result if useful.
     // It is better to use SkShader if in GPU mode, if the operation is simple or if the temporary
@@ -1697,7 +1697,6 @@ bool SkiaSalGraphicsImpl::drawTransformedBitmap(const basegfx::B2DPoint& rNull,
             canvas->drawImage(rSkiaBitmap.GetSkImage(), 0, 0, &paint);
         }
     }
-    addXorRegion(SkRect::MakeWH(GetWidth(), GetHeight())); // can't tell, use whole area
     postDraw();
     return true;
 }
@@ -1751,10 +1750,10 @@ void SkiaSalGraphicsImpl::drawGenericLayout(const GenericSalLayout& layout, Colo
     preDraw();
     SAL_INFO("vcl.skia.trace",
              "drawtextblob(" << this << "): " << textBlob->bounds() << ":" << textColor);
+    addXorRegion(textBlob->bounds());
     SkPaint paint;
     paint.setColor(toSkColor(textColor));
     getDrawCanvas()->drawTextBlob(textBlob, 0, 0, paint);
-    addXorRegion(textBlob->bounds());
     postDraw();
 }