Related: tdf#146842 Eliminate temporary copies of SkiaSalBitmap when printing

Commit 9eb732a32023e74c44ac8c3b5af9f5424273bb6c fixed crashing when
printing SkiaSalBitmaps to a non-Skia SalGraphics. However, the fix
almost always makes two copies of the SkiaSalBitmap's bitmap data: the
first copy is made in SkiaSalBitmap::AcquireBuffer() and then
QuartzSalBitmap makes a copy of the first copy.

By making QuartzSalBitmap's methods that return a CGImageRef pure
virtual, a non-Skia SalGraphics can now create a CGImageRef directly
from a SkiaSalBitmap's Skia bitmap data without copying to any
intermediate buffers.

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

3 files changed, 94 insertions, 263 deletions

diff --git a/vcl/quartz/AquaGraphicsBackend.cxx b/vcl/quartz/AquaGraphicsBackend.cxx
index 2dd1acc8d221..700c7dbe4304 100644
--- a/vcl/quartz/AquaGraphicsBackend.cxx
+++ b/vcl/quartz/AquaGraphicsBackend.cxx
@@ -44,11 +44,6 @@
 #include <svdata.hxx>
 #endif
 
-#if HAVE_FEATURE_SKIA
-#include <vcl/skia/SkiaHelper.hxx>
-#include <skia/salbmp.hxx>
-#endif
-
 using namespace vcl;
 
 namespace
@@ -192,48 +187,6 @@ void drawPattern50(void*, CGContextRef rContext)
     CGContextAddRects(rContext, aRects, 2);
     CGContextFillPath(rContext);
 }
-
-#if HAVE_FEATURE_SKIA
-
-// Related: tdf#146842 Convert SkiaSalBitmap to QuartzSalBitmap
-// Commit de3f13e2175564316eb5a62dee65e9ff8f31b460 disabled Skia for printing.
-// However, since all SalBitmaps created are either all QuartzSalBitmaps or all
-// SkiaSalBitmaps, a crash occurs whenever a SkiaSalBitmap is passed to a
-// printer's SalGraphics instance which is now always non-Skia.
-QuartzSalBitmap* checkAndConvertToQuartzSalBitmap(const SalTwoRect& rPosAry,
-                                                  const SalBitmap& rSalBitmap,
-                                                  SalTwoRect* pAdjustedSrcPosAry)
-{
-    QuartzSalBitmap* pRet = nullptr;
-
-    if (SkiaHelper::isVCLSkiaEnabled() && dynamic_cast<const SkiaSalBitmap*>(&rSalBitmap))
-    {
-        const SkiaSalBitmap& rSkiaBitmap = static_cast<const SkiaSalBitmap&>(rSalBitmap);
-
-        SalTwoRect aSrcPosAry(rPosAry);
-        aSrcPosAry.mnDestX = 0;
-        aSrcPosAry.mnDestY = 0;
-
-        pRet = new QuartzSalBitmap;
-        if (pRet)
-        {
-            // Ignore any failures as returning a nullptr will lead to a crash
-            pRet->Create(rSkiaBitmap, aSrcPosAry);
-
-            if (pAdjustedSrcPosAry)
-            {
-                pAdjustedSrcPosAry->mnSrcX = 0;
-                pAdjustedSrcPosAry->mnSrcY = 0;
-                pAdjustedSrcPosAry->mnSrcWidth = aSrcPosAry.mnDestWidth;
-                pAdjustedSrcPosAry->mnSrcHeight = aSrcPosAry.mnDestHeight;
-            }
-        }
-    }
-
-    return pRet;
-}
-
-#endif
 }
 
 AquaGraphicsBackend::AquaGraphicsBackend(AquaSharedAttributes& rShared)
@@ -949,23 +902,7 @@ void AquaGraphicsBackend::drawBitmap(const SalTwoRect& rPosAry, const SalBitmap&
     if (!mrShared.checkContext())
         return;
 
-#if HAVE_FEATURE_SKIA
-    if (mrShared.mbPrinter)
-    {
-        SAL_INFO("vcl.print", "Printing with Skia bitmaps: AquaGraphicsBackend::drawBitmap");
-        SalTwoRect aDestPosAry(rPosAry);
-        std::unique_ptr<QuartzSalBitmap> pSalBitmap(
-            checkAndConvertToQuartzSalBitmap(rPosAry, rSalBitmap, &aDestPosAry));
-        if (pSalBitmap)
-        {
-            drawBitmap(aDestPosAry, *pSalBitmap);
-            return;
-        }
-    }
-#endif
-
-    const QuartzSalBitmap& rBitmap = static_cast<const QuartzSalBitmap&>(rSalBitmap);
-    CGImageRef xImage = rBitmap.CreateCroppedImage(
+    CGImageRef xImage = rSalBitmap.CreateCroppedImage(
         static_cast<int>(rPosAry.mnSrcX), static_cast<int>(rPosAry.mnSrcY),
         static_cast<int>(rPosAry.mnSrcWidth), static_cast<int>(rPosAry.mnSrcHeight));
     if (!xImage)
@@ -985,36 +922,9 @@ void AquaGraphicsBackend::drawBitmap(const SalTwoRect& rPosAry, const SalBitmap&
     if (!mrShared.checkContext())
         return;
 
-#if HAVE_FEATURE_SKIA
-    if (mrShared.mbPrinter)
-    {
-        SAL_INFO(
-            "vcl.print",
-            "Printing with Skia bitmaps: AquaGraphicsBackend::drawBitmapWithTranspraentBitmap");
-        SalTwoRect aDestPosAry(rPosAry);
-        std::unique_ptr<QuartzSalBitmap> pSalBitmap(
-            checkAndConvertToQuartzSalBitmap(rPosAry, rSalBitmap, &aDestPosAry));
-        if (pSalBitmap)
-        {
-            drawBitmap(aDestPosAry, *pSalBitmap, rTransparentBitmap);
-            return;
-        }
-
-        pSalBitmap.reset(
-            checkAndConvertToQuartzSalBitmap(rPosAry, rTransparentBitmap, &aDestPosAry));
-        if (pSalBitmap)
-        {
-            drawBitmap(aDestPosAry, rSalBitmap, *pSalBitmap);
-            return;
-        }
-    }
-#endif
-
-    const QuartzSalBitmap& rBitmap = static_cast<const QuartzSalBitmap&>(rSalBitmap);
-    const QuartzSalBitmap& rMask = static_cast<const QuartzSalBitmap&>(rTransparentBitmap);
-
-    CGImageRef xMaskedImage(rBitmap.CreateWithMask(rMask, rPosAry.mnSrcX, rPosAry.mnSrcY,
-                                                   rPosAry.mnSrcWidth, rPosAry.mnSrcHeight));
+    CGImageRef xMaskedImage(rSalBitmap.CreateWithMask(rTransparentBitmap, rPosAry.mnSrcX,
+                                                      rPosAry.mnSrcY, rPosAry.mnSrcWidth,
+                                                      rPosAry.mnSrcHeight));
     if (!xMaskedImage)
         return;
 
@@ -1031,24 +941,8 @@ void AquaGraphicsBackend::drawMask(const SalTwoRect& rPosAry, const SalBitmap& r
     if (!mrShared.checkContext())
         return;
 
-#if HAVE_FEATURE_SKIA
-    if (mrShared.mbPrinter)
-    {
-        SAL_INFO("vcl.print", "Printing with Skia bitmaps: AquaGraphicsBackend::drawMask");
-        SalTwoRect aDestPosAry(rPosAry);
-        std::unique_ptr<QuartzSalBitmap> pSalBitmap(
-            checkAndConvertToQuartzSalBitmap(rPosAry, rSalBitmap, &aDestPosAry));
-        if (pSalBitmap)
-        {
-            drawMask(aDestPosAry, *pSalBitmap, nMaskColor);
-            return;
-        }
-    }
-#endif
-
-    const QuartzSalBitmap& rBitmap = static_cast<const QuartzSalBitmap&>(rSalBitmap);
-    CGImageRef xImage = rBitmap.CreateColorMask(rPosAry.mnSrcX, rPosAry.mnSrcY, rPosAry.mnSrcWidth,
-                                                rPosAry.mnSrcHeight, nMaskColor);
+    CGImageRef xImage = rSalBitmap.CreateColorMask(
+        rPosAry.mnSrcX, rPosAry.mnSrcY, rPosAry.mnSrcWidth, rPosAry.mnSrcHeight, nMaskColor);
     if (!xImage)
         return;
 
@@ -1324,28 +1218,7 @@ bool AquaGraphicsBackend::drawAlphaBitmap(const SalTwoRect& rTR, const SalBitmap
     if (rTR.mnDestWidth < 0 || rTR.mnDestHeight < 0)
         return false;
 
-#if HAVE_FEATURE_SKIA
-    if (!mrShared.checkContext())
-        return false;
-
-    if (mrShared.mbPrinter)
-    {
-        SAL_INFO("vcl.print", "Printing with Skia bitmaps: AquaGraphicsBackend::drawAlphaBitmap");
-        SalTwoRect aDestPosAry(rTR);
-        std::unique_ptr<QuartzSalBitmap> pSalBitmap(
-            checkAndConvertToQuartzSalBitmap(rTR, rSrcBitmap, &aDestPosAry));
-        if (pSalBitmap)
-            return drawAlphaBitmap(aDestPosAry, *pSalBitmap, rAlphaBmp);
-
-        pSalBitmap.reset(checkAndConvertToQuartzSalBitmap(rTR, rAlphaBmp, &aDestPosAry));
-        if (pSalBitmap)
-            return drawAlphaBitmap(aDestPosAry, rSrcBitmap, *pSalBitmap);
-    }
-#endif
-
-    const QuartzSalBitmap& rSrcSalBmp = static_cast<const QuartzSalBitmap&>(rSrcBitmap);
-    const QuartzSalBitmap& rMaskSalBmp = static_cast<const QuartzSalBitmap&>(rAlphaBmp);
-    CGImageRef xMaskedImage = rSrcSalBmp.CreateWithMask(rMaskSalBmp, rTR.mnSrcX, rTR.mnSrcY,
+    CGImageRef xMaskedImage = rSrcBitmap.CreateWithMask(rAlphaBmp, rTR.mnSrcX, rTR.mnSrcY,
                                                         rTR.mnSrcWidth, rTR.mnSrcHeight);
     if (!xMaskedImage)
         return false;
@@ -1375,35 +1248,15 @@ bool AquaGraphicsBackend::drawTransformedBitmap(const basegfx::B2DPoint& rNull,
     if (fAlpha != 1.0)
         return false;
 
-#if HAVE_FEATURE_SKIA
-    if (mrShared.mbPrinter)
-    {
-        SAL_INFO("vcl.print",
-                 "Printing with Skia bitmaps: AquaGraphicsBackend::drawTransformedBitmap");
-        const Size aSize = rSrcBitmap.GetSize();
-        SalTwoRect aTR(0, 0, aSize.Width(), aSize.Height(), 0, 0, aSize.Width(), aSize.Height());
-        std::unique_ptr<QuartzSalBitmap> pSalBitmap(
-            checkAndConvertToQuartzSalBitmap(aTR, rSrcBitmap, nullptr));
-        if (pSalBitmap)
-            return drawTransformedBitmap(rNull, rX, rY, *pSalBitmap, pAlphaBmp, fAlpha);
-
-        pSalBitmap.reset(checkAndConvertToQuartzSalBitmap(aTR, *pAlphaBmp, nullptr));
-        if (pSalBitmap)
-            return drawTransformedBitmap(rNull, rX, rY, rSrcBitmap, pSalBitmap.get(), fAlpha);
-    }
-#endif
-
     // get the Quartz image
     CGImageRef xImage = nullptr;
     const Size aSize = rSrcBitmap.GetSize();
-    const QuartzSalBitmap& rSrcSalBmp = static_cast<const QuartzSalBitmap&>(rSrcBitmap);
-    const QuartzSalBitmap* pMaskSalBmp = static_cast<const QuartzSalBitmap*>(pAlphaBmp);
 
-    if (!pMaskSalBmp)
-        xImage = rSrcSalBmp.CreateCroppedImage(0, 0, int(aSize.Width()), int(aSize.Height()));
+    if (!pAlphaBmp)
+        xImage = rSrcBitmap.CreateCroppedImage(0, 0, int(aSize.Width()), int(aSize.Height()));
     else
-        xImage = rSrcSalBmp.CreateWithMask(*pMaskSalBmp, 0, 0, int(aSize.Width()),
-                                           int(aSize.Height()));
+        xImage
+            = rSrcBitmap.CreateWithMask(*pAlphaBmp, 0, 0, int(aSize.Width()), int(aSize.Height()));
 
     if (!xImage)
         return false;
diff --git a/vcl/quartz/cgutils.mm b/vcl/quartz/cgutils.mm
new file mode 100644
index 000000000000..61935acc7fdd
--- /dev/null
+++ b/vcl/quartz/cgutils.mm
@@ -0,0 +1,73 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * This file incorporates work covered by the following license notice:
+ *
+ *   Licensed to the Apache Software Foundation (ASF) under one or more
+ *   contributor license agreements. See the NOTICE file distributed
+ *   with this work for additional information regarding copyright
+ *   ownership. The ASF licenses this file to you under the Apache
+ *   License, Version 2.0 (the "License"); you may not use this file
+ *   except in compliance with the License. You may obtain a copy of
+ *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
+ */
+
+#include <quartz/cgutils.h>
+
+#include <salbmp.hxx>
+#include <osx/saldata.hxx>
+
+static void CFRTLFree(void* /*info*/, const void* data, size_t /*size*/)
+{
+    std::free( const_cast<void*>(data) );
+}
+
+CGImageRef CreateWithSalBitmapAndMask( const SalBitmap& rBitmap, const SalBitmap& rMask, int nX, int nY, int nWidth, int nHeight )
+{
+    CGImageRef xImage( rBitmap.CreateCroppedImage( nX, nY, nWidth, nHeight ) );
+    if( !xImage )
+        return nullptr;
+
+    CGImageRef xMask = rMask.CreateCroppedImage( nX, nY, nWidth, nHeight );
+    if( !xMask )
+        return xImage;
+
+    // CGImageCreateWithMask() only likes masks or greyscale images => convert if needed
+    // TODO: isolate in an extra method?
+    if( !CGImageIsMask(xMask) || rMask.GetBitCount() != 8)//(CGImageGetColorSpace(xMask) != GetSalData()->mxGraySpace) )
+    {
+        const CGRect xImageRect=CGRectMake( 0, 0, nWidth, nHeight );//the rect has no offset
+
+        // create the alpha mask image fitting our image
+        // TODO: is caching the full mask or the subimage mask worth it?
+        int nMaskBytesPerRow = ((nWidth + 3) & ~3);
+        void* pMaskMem = std::malloc( nMaskBytesPerRow * nHeight );
+        CGContextRef xMaskContext = CGBitmapContextCreate( pMaskMem,
+            nWidth, nHeight, 8, nMaskBytesPerRow, GetSalData()->mxGraySpace, kCGImageAlphaNone );
+        CGContextDrawImage( xMaskContext, xImageRect, xMask );
+        CFRelease( xMask );
+        CGDataProviderRef xDataProvider( CGDataProviderCreateWithData( nullptr,
+        pMaskMem, nHeight * nMaskBytesPerRow, &CFRTLFree ) );
+
+        static const CGFloat* pDecode = nullptr;
+        xMask = CGImageMaskCreate( nWidth, nHeight, 8, 8, nMaskBytesPerRow, xDataProvider, pDecode, false );
+        CFRelease( xDataProvider );
+        CFRelease( xMaskContext );
+    }
+
+    if( !xMask )
+        return xImage;
+
+    // combine image and alpha mask
+    CGImageRef xMaskedImage = CGImageCreateWithMask( xImage, xMask );
+    CFRelease( xMask );
+    CFRelease( xImage );
+    return xMaskedImage;
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/quartz/salbmp.cxx b/vcl/quartz/salbmp.cxx
index 05b5093bafc1..e71c35612cb4 100644
--- a/vcl/quartz/salbmp.cxx
+++ b/vcl/quartz/salbmp.cxx
@@ -35,6 +35,7 @@
 #include <vcl/Scanline.hxx>
 
 #include <bitmap/bmpfast.hxx>
+#include <quartz/cgutils.h>
 #include <quartz/salbmp.h>
 #include <quartz/utils.h>
 #include <bitmap/ScanlineTools.hxx>
@@ -115,66 +116,6 @@ bool QuartzSalBitmap::Create( const SalBitmap& rSalBmp, vcl::PixelFormat eNewPix
     return false;
 }
 
-#if HAVE_FEATURE_SKIA
-
-bool QuartzSalBitmap::Create( const SkiaSalBitmap& rSalBmp, const SalTwoRect& rPosAry )
-{
-    bool bRet = false;
-
-    // Ugly but necessary to acquire the bitmap buffer because all of the
-    // SalBitmap instances that callers pass are already const. At least we
-    // only need to read, not write to the bitmap parameter.
-    SkiaSalBitmap& rSkiaSalBmp = const_cast<SkiaSalBitmap&>( rSalBmp );
-
-    BitmapBuffer *pSrcBuffer = rSkiaSalBmp.AcquireBuffer( BitmapAccessMode::Read );
-    if ( !pSrcBuffer )
-        return bRet;
-
-    if ( !pSrcBuffer->mpBits )
-    {
-        rSkiaSalBmp.ReleaseBuffer( pSrcBuffer, BitmapAccessMode::Read );
-        return bRet;
-    }
-
-    // Create only a 1 pixel buffer as it will always be discarded
-    mnBits = 32;
-    mnWidth = 1;
-    mnHeight = 1;
-    if( AllocateUserData() )
-    {
-        BitmapBuffer *pDestBuffer = AcquireBuffer( BitmapAccessMode::Read );
-        if ( pDestBuffer )
-        {
-            std::unique_ptr<BitmapBuffer> pConvertedBuffer = StretchAndConvert( *pSrcBuffer, rPosAry, pDestBuffer->mnFormat, pDestBuffer->maPalette, &pDestBuffer->maColorMask );
-            bool bUseDestBuffer = ( pConvertedBuffer &&
-                 pConvertedBuffer->mpBits &&
-                 pConvertedBuffer->mnFormat == pDestBuffer->mnFormat &&
-                 pConvertedBuffer->mnWidth == rPosAry.mnDestWidth &&
-                 pConvertedBuffer->mnHeight == rPosAry.mnDestHeight );
-
-            ReleaseBuffer( pDestBuffer, BitmapAccessMode::Read );
-
-            if ( bUseDestBuffer )
-            {
-                // Surprisingly, BitmapBuffer does not delete the bits so
-                // discard our 1 pixel buffer and take ownership of the bits
-                DestroyContext();
-                m_pUserBuffer.reset( pConvertedBuffer->mpBits );
-                mnWidth = pConvertedBuffer->mnWidth;
-                mnHeight = pConvertedBuffer->mnHeight;
-                mnBytesPerRow = pConvertedBuffer->mnScanlineSize;
-                bRet = true;
-            }
-        }
-    }
-
-    rSkiaSalBmp.ReleaseBuffer( pSrcBuffer, BitmapAccessMode::Read );
-
-    return bRet;
-}
-
-#endif
-
 bool QuartzSalBitmap::Create( const css::uno::Reference< css::rendering::XBitmapCanvas >& /*xBitmapCanvas*/,
                               Size& /*rSize*/, bool /*bMask*/ )
 {
@@ -576,48 +517,10 @@ static void CFRTLFree(void* /*info*/, const void* data, size_t /*size*/)
     std::free( const_cast<void*>(data) );
 }
 
-CGImageRef QuartzSalBitmap::CreateWithMask( const QuartzSalBitmap& rMask,
+CGImageRef QuartzSalBitmap::CreateWithMask( const SalBitmap& rMask,
     int nX, int nY, int nWidth, int nHeight ) const
 {
-    CGImageRef xImage( CreateCroppedImage( nX, nY, nWidth, nHeight ) );
-    if( !xImage )
-        return nullptr;
-
-    CGImageRef xMask = rMask.CreateCroppedImage( nX, nY, nWidth, nHeight );
-    if( !xMask )
-        return xImage;
-
-    // CGImageCreateWithMask() only likes masks or greyscale images => convert if needed
-    // TODO: isolate in an extra method?
-    if( !CGImageIsMask(xMask) || rMask.GetBitCount() != 8)//(CGImageGetColorSpace(xMask) != GetSalData()->mxGraySpace) )
-    {
-        const CGRect xImageRect=CGRectMake( 0, 0, nWidth, nHeight );//the rect has no offset
-
-        // create the alpha mask image fitting our image
-        // TODO: is caching the full mask or the subimage mask worth it?
-        int nMaskBytesPerRow = ((nWidth + 3) & ~3);
-        void* pMaskMem = std::malloc( nMaskBytesPerRow * nHeight );
-        CGContextRef xMaskContext = CGBitmapContextCreate( pMaskMem,
-            nWidth, nHeight, 8, nMaskBytesPerRow, GetSalData()->mxGraySpace, kCGImageAlphaNone );
-        CGContextDrawImage( xMaskContext, xImageRect, xMask );
-        CFRelease( xMask );
-        CGDataProviderRef xDataProvider( CGDataProviderCreateWithData( nullptr,
-        pMaskMem, nHeight * nMaskBytesPerRow, &CFRTLFree ) );
-
-        static const CGFloat* pDecode = nullptr;
-        xMask = CGImageMaskCreate( nWidth, nHeight, 8, 8, nMaskBytesPerRow, xDataProvider, pDecode, false );
-        CFRelease( xDataProvider );
-        CFRelease( xMaskContext );
-    }
-
-    if( !xMask )
-        return xImage;
-
-    // combine image and alpha mask
-    CGImageRef xMaskedImage = CGImageCreateWithMask( xImage, xMask );
-    CFRelease( xMask );
-    CFRelease( xImage );
-    return xMaskedImage;
+    return CreateWithSalBitmapAndMask( *this, rMask, nX, nY, nWidth, nHeight );
 }
 
 /** creates an image from the given rectangle, replacing all black pixels
@@ -628,13 +531,14 @@ CGImageRef QuartzSalBitmap::CreateColorMask( int nX, int nY, int nWidth,
     CGImageRef xMask = nullptr;
     if (m_pUserBuffer && (nX + nWidth <= mnWidth) && (nY + nHeight <= mnHeight))
     {
-        const sal_uInt32 nDestBytesPerRow = nWidth << 2;
-        std::unique_ptr<sal_uInt32[]> pMaskBuffer(new (std::nothrow) sal_uInt32[ nHeight * nDestBytesPerRow / 4] );
-        sal_uInt32* pDest = pMaskBuffer.get();
-
         auto pSourcePixels = vcl::bitmap::getScanlineTransformer(mnBits, maPalette);
+        // Don't allocate destination buffer if there is no scanline transformer
+        if( !pSourcePixels )
+            return xMask;
 
-        if( pMaskBuffer && pSourcePixels )
+        const sal_uInt32 nDestBytesPerRow = nWidth << 2;
+        std::unique_ptr<sal_uInt32[]> pMaskBuffer(new (std::nothrow) sal_uInt32[ nHeight * nDestBytesPerRow / 4] );
+        if( pMaskBuffer )
         {
             sal_uInt32 nColor;
             reinterpret_cast<sal_uInt8*>(&nColor)[0] = 0xff;
@@ -643,6 +547,7 @@ CGImageRef QuartzSalBitmap::CreateColorMask( int nX, int nY, int nWidth,
             reinterpret_cast<sal_uInt8*>(&nColor)[3] = nMaskColor.GetBlue();
 
             sal_uInt8* pSource = m_pUserBuffer.get();
+            sal_uInt32* pDest = pMaskBuffer.get();
             // First to nY on y-axis, as that is our starting point (sub-image)
             if( nY )
                 pSource += nY * mnBytesPerRow;