Remove the special-casing in Bitmap::Invert (and fix)

and rather rely on the backends doing the right thing, which is
considerably faster.

Which uncovers a bug in the existing code - it is not legal
to simply invert the value when dealing with palette-based
images. Fix this by sharing some code with Bitmap::ReplaceMask.

Change-Id: I2ef340a9f251c8c7e27b68ab451ce85df07c1035
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/160332
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

5 files changed, 140 insertions, 72 deletions

diff --git a/include/vcl/BitmapInfoAccess.hxx b/include/vcl/BitmapInfoAccess.hxx
index c0ef7fb5be1e..6e255c97e7ec 100644
--- a/include/vcl/BitmapInfoAccess.hxx
+++ b/include/vcl/BitmapInfoAccess.hxx
@@ -77,6 +77,9 @@ public:
         return mpBuffer ? mpBuffer->mnBitCount : 0;
     }
 
+    /// Returns the BitmapColor (i.e. palette index) that is either an exact match
+    /// of the required color, or failing that, the entry that is the closest i.e. least error
+    /// as measured by Color::GetColorError.
     BitmapColor GetBestMatchingColor(const BitmapColor& rBitmapColor) const
     {
         if (HasPalette())
@@ -121,7 +124,13 @@ public:
         return pBuffer->maPalette[nColor];
     }
 
+    /// Returns the BitmapColor (i.e. palette index) that is either an exact match
+    /// of the required color, or failing that, the entry that is the closest i.e. least error
+    /// as measured by Color::GetColorError.
     sal_uInt16 GetBestPaletteIndex(const BitmapColor& rBitmapColor) const;
+    /// Returns the BitmapColor (i.e. palette index) that is an exact match
+    /// of the required color. Returns SAL_MAX_UINT16 if nothing found.
+    sal_uInt16 GetMatchingPaletteIndex(const BitmapColor& rBitmapColor) const;
 
     const ColorMask& GetColorMask() const
     {
diff --git a/include/vcl/BitmapPalette.hxx b/include/vcl/BitmapPalette.hxx
index 4f20970e15ec..1d5f79de5adf 100644
--- a/include/vcl/BitmapPalette.hxx
+++ b/include/vcl/BitmapPalette.hxx
@@ -65,7 +65,13 @@ public:
     const BitmapColor& operator[](sal_uInt16 nIndex) const;
     BitmapColor& operator[](sal_uInt16 nIndex);
 
+    /// Returns the BitmapColor (i.e. palette index) that is either an exact match
+    /// of the required color, or failing that, the entry that is the closest i.e. least error
+    /// as measured by Color::GetColorError.
     sal_uInt16 GetBestIndex(const BitmapColor& rCol) const;
+    /// Returns the BitmapColor (i.e. palette index) that is an exact match
+    /// of the required color. Returns SAL_MAX_UINT16 if nothing found.
+    sal_uInt16 GetMatchingIndex(const BitmapColor& rCol) const;
 
     /// Returns true if the palette is 8-bit grey palette.
     bool IsGreyPalette8Bit() const;
diff --git a/vcl/source/bitmap/BitmapInfoAccess.cxx b/vcl/source/bitmap/BitmapInfoAccess.cxx
index 50607e94dde3..318317519928 100644
--- a/vcl/source/bitmap/BitmapInfoAccess.cxx
+++ b/vcl/source/bitmap/BitmapInfoAccess.cxx
@@ -77,4 +77,10 @@ sal_uInt16 BitmapInfoAccess::GetBestPaletteIndex(const BitmapColor& rBitmapColor
     return (HasPalette() ? pBuffer->maPalette.GetBestIndex(rBitmapColor) : 0);
 }
 
+sal_uInt16 BitmapInfoAccess::GetMatchingPaletteIndex(const BitmapColor& rBitmapColor) const
+{
+    assert(HasPalette());
+    return mpBuffer->maPalette.GetMatchingIndex(rBitmapColor);
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/source/bitmap/bitmappaint.cxx b/vcl/source/bitmap/bitmappaint.cxx
index 5d405322e6ed..758f36bc1e93 100644
--- a/vcl/source/bitmap/bitmappaint.cxx
+++ b/vcl/source/bitmap/bitmappaint.cxx
@@ -31,6 +31,12 @@
 #include <algorithm>
 #include <memory>
 
+static BitmapColor UpdatePaletteForNewColor(BitmapScopedWriteAccess& pAcc,
+                                            const sal_uInt16 nActColors,
+                                            const sal_uInt16 nMaxColors, const tools::Long nHeight,
+                                            const tools::Long nWidth,
+                                            const BitmapColor& rWantedColor);
+
 bool Bitmap::Erase(const Color& rFillColor)
 {
     if (IsEmpty())
@@ -63,32 +69,6 @@ bool Bitmap::Invert()
     if (!mxSalBmp)
         return false;
 
-    // For alpha masks, we need to actually invert the underlying data
-    // or the optimisations elsewhere do not work right.
-    if (typeid(*this) != typeid(AlphaMask))
-    {
-        // We want to avoid using ScopedReadAccess until we really need
-        // it, because on Skia it triggers a GPU->RAM copy, which is very slow.
-        ScopedReadAccess pReadAcc(*this);
-        if (!pReadAcc)
-            return false;
-        if (pReadAcc->HasPalette())
-        {
-            BitmapScopedWriteAccess pWriteAcc(*this);
-            BitmapPalette aBmpPal(pWriteAcc->GetPalette());
-            const sal_uInt16 nCount = aBmpPal.GetEntryCount();
-
-            for (sal_uInt16 i = 0; i < nCount; i++)
-            {
-                aBmpPal[i].Invert();
-            }
-
-            pWriteAcc->SetPalette(aBmpPal);
-            mxSalBmp->InvalidateChecksum();
-            return true;
-        }
-    }
-
     // try optimised call, much faster on Skia
     if (mxSalBmp->Invert())
     {
@@ -100,17 +80,54 @@ bool Bitmap::Invert()
     const tools::Long nWidth = pWriteAcc->Width();
     const tools::Long nHeight = pWriteAcc->Height();
 
-    for (tools::Long nY = 0; nY < nHeight; nY++)
+    if (pWriteAcc->HasPalette())
     {
-        Scanline pScanline = pWriteAcc->GetScanline(nY);
-        for (tools::Long nX = 0; nX < nWidth; nX++)
+        const sal_uInt16 nActColors = pWriteAcc->GetPaletteEntryCount();
+        const sal_uInt16 nMaxColors = 1 << pWriteAcc->GetBitCount();
+
+        if (pWriteAcc->GetPalette().IsGreyPalette8Bit())
         {
-            BitmapColor aBmpColor = pWriteAcc->GetPixelFromData(pScanline, nX);
-            aBmpColor.Invert();
-            pWriteAcc->SetPixelOnData(pScanline, nX, aBmpColor);
+            for (tools::Long nY = 0; nY < nHeight; nY++)
+            {
+                Scanline pScanline = pWriteAcc->GetScanline(nY);
+                for (tools::Long nX = 0; nX < nWidth; nX++)
+                {
+                    BitmapColor aBmpColor = pWriteAcc->GetPixelFromData(pScanline, nX);
+                    aBmpColor.SetIndex(0xff - aBmpColor.GetIndex());
+                    pWriteAcc->SetPixelOnData(pScanline, nX, aBmpColor);
+                }
+            }
+        }
+        else
+        {
+            for (tools::Long nY = 0; nY < nHeight; nY++)
+            {
+                Scanline pScanline = pWriteAcc->GetScanline(nY);
+                for (tools::Long nX = 0; nX < nWidth; nX++)
+                {
+                    BitmapColor aBmpColor = pWriteAcc->GetPixelFromData(pScanline, nX);
+                    aBmpColor = pWriteAcc->GetPaletteColor(aBmpColor.GetIndex());
+                    aBmpColor.Invert();
+                    BitmapColor aReplace = UpdatePaletteForNewColor(
+                        pWriteAcc, nActColors, nMaxColors, nHeight, nWidth, aBmpColor);
+                    pWriteAcc->SetPixelOnData(pScanline, nX, aReplace);
+                }
+            }
+        }
+    }
+    else
+    {
+        for (tools::Long nY = 0; nY < nHeight; nY++)
+        {
+            Scanline pScanline = pWriteAcc->GetScanline(nY);
+            for (tools::Long nX = 0; nX < nWidth; nX++)
+            {
+                BitmapColor aBmpColor = pWriteAcc->GetPixelFromData(pScanline, nX);
+                aBmpColor.Invert();
+                pWriteAcc->SetPixelOnData(pScanline, nX, aBmpColor);
+            }
         }
     }
-
     mxSalBmp->InvalidateChecksum();
 
     return true;
@@ -902,45 +919,8 @@ bool Bitmap::ReplaceMask(const AlphaMask& rMask, const Color& rReplaceColor)
         const sal_uInt16 nActColors = pAcc->GetPaletteEntryCount();
         const sal_uInt16 nMaxColors = 1 << pAcc->GetBitCount();
 
-        // default to the nearest color
-        aReplace = pAcc->GetBestMatchingColor(rReplaceColor);
-
-        // for paletted images without a matching palette entry
-        // look for an unused palette entry (NOTE: expensive!)
-        if (pAcc->GetPaletteColor(aReplace.GetIndex()) != BitmapColor(rReplaceColor))
-        {
-            // if the palette has empty entries use the last one
-            if (nActColors < nMaxColors)
-            {
-                pAcc->SetPaletteEntryCount(nActColors + 1);
-                pAcc->SetPaletteColor(nActColors, rReplaceColor);
-                aReplace = BitmapColor(static_cast<sal_uInt8>(nActColors));
-            }
-            else
-            {
-                std::unique_ptr<bool[]> pFlags(new bool[nMaxColors]);
-
-                // Set all entries to false
-                std::fill(pFlags.get(), pFlags.get() + nMaxColors, false);
-
-                for (tools::Long nY = 0; nY < nHeight; nY++)
-                {
-                    Scanline pScanline = pAcc->GetScanline(nY);
-                    for (tools::Long nX = 0; nX < nWidth; nX++)
-                        pFlags[pAcc->GetIndexFromData(pScanline, nX)] = true;
-                }
-
-                for (sal_uInt16 i = 0; i < nMaxColors; i++)
-                {
-                    // Hurray, we do have an unused entry
-                    if (!pFlags[i])
-                    {
-                        pAcc->SetPaletteColor(i, rReplaceColor);
-                        aReplace = BitmapColor(static_cast<sal_uInt8>(i));
-                    }
-                }
-            }
-        }
+        aReplace
+            = UpdatePaletteForNewColor(pAcc, nActColors, nMaxColors, nHeight, nWidth, aReplace);
     }
     else
         aReplace = rReplaceColor;
@@ -1196,4 +1176,51 @@ bool Bitmap::Blend(const AlphaMask& rAlpha, const Color& rBackgroundColor)
     return true;
 }
 
+static BitmapColor UpdatePaletteForNewColor(BitmapScopedWriteAccess& pAcc,
+                                            const sal_uInt16 nActColors,
+                                            const sal_uInt16 nMaxColors, const tools::Long nHeight,
+                                            const tools::Long nWidth,
+                                            const BitmapColor& rWantedColor)
+{
+    // default to the nearest color
+    sal_uInt16 aReplacePalIndex = pAcc->GetMatchingPaletteIndex(rWantedColor);
+    if (aReplacePalIndex != SAL_MAX_UINT16)
+        return BitmapColor(static_cast<sal_uInt8>(aReplacePalIndex));
+
+    // for paletted images without a matching palette entry
+
+    // if the palette has empty entries use the last one
+    if (nActColors < nMaxColors)
+    {
+        pAcc->SetPaletteEntryCount(nActColors + 1);
+        pAcc->SetPaletteColor(nActColors, rWantedColor);
+        return BitmapColor(static_cast<sal_uInt8>(nActColors));
+    }
+
+    // look for an unused palette entry (NOTE: expensive!)
+    std::unique_ptr<bool[]> pFlags(new bool[nMaxColors]);
+
+    // Set all entries to false
+    std::fill(pFlags.get(), pFlags.get() + nMaxColors, false);
+
+    for (tools::Long nY = 0; nY < nHeight; nY++)
+    {
+        Scanline pScanline = pAcc->GetScanline(nY);
+        for (tools::Long nX = 0; nX < nWidth; nX++)
+            pFlags[pAcc->GetIndexFromData(pScanline, nX)] = true;
+    }
+
+    for (sal_uInt16 i = 0; i < nMaxColors; i++)
+    {
+        // Hurray, we do have an unused entry
+        if (!pFlags[i])
+        {
+            pAcc->SetPaletteColor(i, rWantedColor);
+            return BitmapColor(static_cast<sal_uInt8>(i));
+        }
+    }
+    assert(false && "found nothing");
+    return BitmapColor(0);
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/source/bitmap/bitmappalette.cxx b/vcl/source/bitmap/bitmappalette.cxx
index 97bfe4a62051..43eae3475463 100644
--- a/vcl/source/bitmap/bitmappalette.cxx
+++ b/vcl/source/bitmap/bitmappalette.cxx
@@ -147,6 +147,9 @@ BitmapColor& BitmapPalette::operator[](sal_uInt16 nIndex)
     return mpImpl->GetBitmapData()[nIndex];
 }
 
+/// Returns the BitmapColor (i.e. palette index) that is either an exact match
+/// of the required color, or failing that, the entry that is the closest i.e. least error
+/// as measured by Color::GetColorError.
 sal_uInt16 BitmapPalette::GetBestIndex(const BitmapColor& rCol) const
 {
     auto const& rBitmapColor = mpImpl->GetBitmapData();
@@ -177,6 +180,23 @@ sal_uInt16 BitmapPalette::GetBestIndex(const BitmapColor& rCol) const
     return nRetIndex;
 }
 
+/// Returns the BitmapColor (i.e. palette index) that is an exact match
+/// of the required color. Returns SAL_MAX_UINT16 if nothing found.
+sal_uInt16 BitmapPalette::GetMatchingIndex(const BitmapColor& rCol) const
+{
+    auto const& rBitmapColor = mpImpl->GetBitmapData();
+
+    for (size_t j = 0; j < rBitmapColor.size(); ++j)
+    {
+        if (rCol == rBitmapColor[j])
+        {
+            return j;
+        }
+    }
+
+    return SAL_MAX_UINT16;
+}
+
 bool BitmapPalette::IsGreyPaletteAny() const
 {
     auto const& rBitmapColor = mpImpl->GetBitmapData();