fixes for SkiaSalBitmap delayed scaling (tdf#140930)
The original idea for delayed scaling was that if a bitmap is to be
scaled, only the parameters will be saved and the pixel buffer
mBuffer will be resized only on-demand. But this gets complicated
by mImage sometimes not being just a cache of mBuffer, but sometimes
it is the only data. This is needed so that e.g.
OutputDevice::GetBitmap() can operate only on SkImage without
possibly ever needing a conversion to the pixel buffer, thus even
keeping the data only on the GPU in the Vulkan case.
Together with delayed scaling this means that the size of mImage
can be either the original size (if Scale() is called with mImage
already valid) or the final size (if mImage is created in
GetSkImage()). Thus relying on 'mPixelsSize != mSize' as
a detection of pending scaling does not always work for mImage.
Handle this by using mImage dimensions in cases where relevant.
Change-Id: Id9fad67b8936d2266c1f270d08023d15efee3987
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/112545
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
diff --git a/vcl/inc/skia/salbmp.hxx b/vcl/inc/skia/salbmp.hxx
index 0509ed3..0125941 100644
--- a/vcl/inc/skia/salbmp.hxx
+++ b/vcl/inc/skia/salbmp.hxx
@@ -105,8 +105,6 @@
void ResetToBuffer();
// Sets the data only as SkImage (will be converted as needed).
void ResetToSkImage(sk_sp<SkImage> image);
// Resets all data that does not match mSize.
void ResetCachedDataBySize();
// Resets all data (buffer and images).
void ResetAllData();
// Call to ensure mBuffer has data (will convert from mImage if necessary).
@@ -119,6 +117,8 @@
void CreateBitmapData();
// Should be called whenever mPixelsSize or mBitCount is set/changed.
bool ComputeScanlineSize();
// Resets information about pending scaling. To be called when mBuffer is resized or created.
void ResetPendingScaling();
// Sets bitmap to be erased on demand.
void EraseInternal(const Color& color);
// Sets pixels to the erase color.
diff --git a/vcl/qa/cppunit/skia/skia.cxx b/vcl/qa/cppunit/skia/skia.cxx
index 8f288d8..d8e103d1 100644
--- a/vcl/qa/cppunit/skia/skia.cxx
+++ b/vcl/qa/cppunit/skia/skia.cxx
@@ -20,6 +20,8 @@
#include <skia/utils.hxx>
#include <bitmap/BitmapWriteAccess.hxx>
using namespace SkiaHelper;
// This tests backends that use Skia (i.e. intentionally not the svp one, which is the default.)
// Note that you still may need to actually set for Skia to be used (see vcl/README.vars).
// If Skia is not enabled, all tests will be silently skipped.
@@ -39,6 +41,7 @@
void testAlphaBlendWith();
void testBitmapCopyOnWrite();
void testMatrixQuality();
void testDelayedScale();
void testTdf137329();
CPPUNIT_TEST_SUITE(SkiaTest);
@@ -48,6 +51,7 @@
CPPUNIT_TEST(testAlphaBlendWith);
CPPUNIT_TEST(testBitmapCopyOnWrite);
CPPUNIT_TEST(testMatrixQuality);
CPPUNIT_TEST(testDelayedScale);
CPPUNIT_TEST(testTdf137329);
CPPUNIT_TEST_SUITE_END();
@@ -329,6 +333,45 @@
#endif
}
void SkiaTest::testDelayedScale()
{
if (!SkiaHelper::isVCLSkiaEnabled())
return;
Bitmap bitmap1(Size(10, 10), vcl::PixelFormat::N24_BPP);
SkiaSalBitmap* skiaBitmap1 = dynamic_cast<SkiaSalBitmap*>(bitmap1.ImplGetSalBitmap().get());
CPPUNIT_ASSERT(skiaBitmap1);
// Do scaling based on mBuffer.
(void)BitmapReadAccess(bitmap1); // allocates mBuffer
CPPUNIT_ASSERT(skiaBitmap1->unittestHasBuffer());
CPPUNIT_ASSERT(!skiaBitmap1->unittestHasImage());
CPPUNIT_ASSERT(bitmap1.Scale(2, 2, BmpScaleFlag::Default));
skiaBitmap1 = dynamic_cast<SkiaSalBitmap*>(bitmap1.ImplGetSalBitmap().get());
CPPUNIT_ASSERT(skiaBitmap1);
CPPUNIT_ASSERT(skiaBitmap1->unittestHasBuffer());
CPPUNIT_ASSERT(!skiaBitmap1->unittestHasImage());
CPPUNIT_ASSERT_EQUAL(Size(20, 20), bitmap1.GetSizePixel());
CPPUNIT_ASSERT_EQUAL(Size(20, 20), imageSize(skiaBitmap1->GetSkImage()));
BitmapBuffer* buffer1 = skiaBitmap1->AcquireBuffer(BitmapAccessMode::Read);
CPPUNIT_ASSERT(buffer1);
CPPUNIT_ASSERT_EQUAL(tools::Long(20), buffer1->mnWidth);
CPPUNIT_ASSERT_EQUAL(tools::Long(20), buffer1->mnHeight);
skiaBitmap1->ReleaseBuffer(buffer1, BitmapAccessMode::Read);
// Do scaling based on mImage.
SkiaSalBitmap skiaBitmap2(skiaBitmap1->GetSkImage());
CPPUNIT_ASSERT(!skiaBitmap2.unittestHasBuffer());
CPPUNIT_ASSERT(skiaBitmap2.unittestHasImage());
CPPUNIT_ASSERT(skiaBitmap2.Scale(2, 3, BmpScaleFlag::Default));
CPPUNIT_ASSERT(!skiaBitmap2.unittestHasBuffer());
CPPUNIT_ASSERT(skiaBitmap2.unittestHasImage());
CPPUNIT_ASSERT_EQUAL(Size(40, 60), skiaBitmap2.GetSize());
CPPUNIT_ASSERT_EQUAL(Size(40, 60), imageSize(skiaBitmap2.GetSkImage()));
BitmapBuffer* buffer2 = skiaBitmap2.AcquireBuffer(BitmapAccessMode::Read);
CPPUNIT_ASSERT(buffer2);
CPPUNIT_ASSERT_EQUAL(tools::Long(40), buffer2->mnWidth);
CPPUNIT_ASSERT_EQUAL(tools::Long(60), buffer2->mnHeight);
skiaBitmap2.ReleaseBuffer(buffer2, BitmapAccessMode::Read);
}
void SkiaTest::testTdf137329()
{
if (!SkiaHelper::isVCLSkiaEnabled())
diff --git a/vcl/skia/salbmp.cxx b/vcl/skia/salbmp.cxx
index c8abfb6..3b4ec71 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -85,7 +85,8 @@
return false;
mPalette = rPal;
mBitCount = nBitCount;
mSize = mPixelsSize = rSize;
mSize = rSize;
ResetPendingScaling();
if (!ComputeScanlineSize())
{
mBitCount = 0;
@@ -387,8 +388,8 @@
if (mEraseColorSet)
{ // Simple.
mSize = mPixelsSize = newSize;
ComputeScanlineSize();
mSize = newSize;
ResetPendingScaling();
EraseInternal(mEraseColor);
return true;
}
@@ -408,7 +409,14 @@
// That means it can be GPU-accelerated, while done here directly it would need
// to be either done by CPU, or with the CPU->GPU->CPU roundtrip required
// by GPU-accelerated scaling.
// Pending scaling is detected by 'mSize != mPixelsSize'.
// Pending scaling is detected by 'mSize != mPixelsSize' for mBuffer,
// and 'imageSize(mImage) != mSize' for mImage. It is not intended to have 3 different
// sizes though, code below keeps only mBuffer or mImage. Note that imageSize(mImage)
// may or may not be equal to mPixelsSize, depending on whether mImage is set here
// (sizes will be equal) or whether it's set in GetSkImage() (will not be equal).
// Pending scaling is considered "done" by the time mBuffer is resized (or created).
// Resizing of mImage is somewhat independent of this, since mImage is primarily
// considered to be a cached object (although sometimes it's the only data available).
// If there is already one scale() pending, use the lowest quality of all requested.
switch (nScaleFlag)
@@ -427,10 +435,11 @@
SAL_INFO("vcl.skia.trace", "scale(" << this << "): unsupported scale algorithm");
return false;
}
// scaling will be actually done on-demand when needed, the need will be recognized
// by mSize != mPixelsSize
mSize = newSize;
// Do not reset cached data if mImage is possibly the only data we have.
// If we have both mBuffer and mImage, prefer mImage, since it likely will be drawn later.
// We could possibly try to keep the buffer as well, but that would complicate things
// with two different data structures to be scaled on-demand, and it's a question
// if that'd realistically help with anything.
if (mImage)
ResetToSkImage(mImage);
else
@@ -456,11 +465,12 @@
// Normally this would need to convert contents of mBuffer for all possible formats,
// so just let the VCL algorithm do it.
// Avoid the costly SkImage->buffer->SkImage conversion.
if (!mBuffer && mImage)
if (!mBuffer && mImage && !mEraseColorSet)
{
if (mBitCount == 8 && mPalette.IsGreyPalette8Bit())
return true;
sk_sp<SkSurface> surface = createSkSurface(mPixelsSize, mImage->imageInfo().alphaType());
sk_sp<SkSurface> surface
= createSkSurface(imageSize(mImage), mImage->imageInfo().alphaType());
SkPaint paint;
paint.setBlendMode(SkBlendMode::kSrc); // set as is, including alpha
// VCL uses different coefficients for conversion to gray than Skia, so use the VCL
@@ -763,7 +773,7 @@
}
if (mImage)
{
if (mImage->width() != mSize.Width() || mImage->height() != mSize.Height())
if (imageSize(mImage) != mSize)
{
assert(!mBuffer); // This code should be only called if only mImage holds data.
SkiaZone zone;
@@ -829,7 +839,7 @@
if (mImage)
{
SkiaZone zone;
bool scaling = mImage->width() != mSize.Width() || mImage->height() != mSize.Height();
const bool scaling = imageSize(mImage) != mSize;
SkPixmap pixmap;
// Note: We cannot do this when 'scaling' because SkCanvas::drawImageRect()
// with kAlpha_8_SkColorType as source and destination would act as SkBlendMode::kSrcOver
@@ -1026,7 +1036,6 @@
SkiaZone zone;
assert(mPixelsSize == mSize);
assert(!mBuffer);
mScaleQuality = BmpScaleFlag::BestQuality;
CreateBitmapData();
// Unset now, so that repeated call will return mBuffer.
mEraseColorSet = false;
@@ -1054,7 +1063,8 @@
}
// Convert from alpha image, if the conversion is simple.
if (mAlphaImage && mSize == mPixelsSize && mBitCount == 8 && mPalette.IsGreyPalette8Bit())
if (mAlphaImage && imageSize(mAlphaImage) == mSize && mBitCount == 8
&& mPalette.IsGreyPalette8Bit())
{
assert(mAlphaImage->colorType() == kAlpha_8_SkColorType);
SkiaZone zone;
@@ -1073,6 +1083,7 @@
canvas.flush();
}
bitmap.setImmutable();
ResetPendingScaling();
CreateBitmapData();
assert(mBuffer != nullptr);
assert(mPixelsSize == mSize);
@@ -1123,7 +1134,7 @@
#endif
SkBitmap bitmap;
SkPixmap pixmap;
if (mSize == mPixelsSize && mImage->imageInfo().alphaType() == alphaType
if (imageSize(mImage) == mSize && mImage->imageInfo().alphaType() == alphaType
&& mImage->peekPixels(&pixmap))
{
bitmap.installPixels(pixmap);
@@ -1135,27 +1146,20 @@
SkCanvas canvas(bitmap);
SkPaint paint;
paint.setBlendMode(SkBlendMode::kSrc); // set as is, including alpha
if (mSize != mPixelsSize) // pending scaling?
if (imageSize(mImage) != mSize) // pending scaling?
{
assert(mImage->width() == mPixelsSize.getWidth()
&& mImage->height() == mPixelsSize.getHeight());
canvas.drawImageRect(mImage, SkRect::MakeWH(mSize.getWidth(), mSize.getHeight()),
makeSamplingOptions(mScaleQuality), &paint);
SAL_INFO("vcl.skia.trace", "ensurebitmapdata(" << this << "): image scaled "
<< mPixelsSize << "->" << mSize << ":"
<< static_cast<int>(mScaleQuality));
mPixelsSize = mSize;
ComputeScanlineSize();
mScaleQuality = BmpScaleFlag::BestQuality;
// Information about the pending scaling has been discarded, so make sure we do not
// keep around any cached images that would still need scaling.
ResetCachedDataBySize();
SAL_INFO("vcl.skia.trace",
"ensurebitmapdata(" << this << "): image scaled " << imageSize(mImage) << "->"
<< mSize << ":" << static_cast<int>(mScaleQuality));
}
else
canvas.drawImage(mImage, 0, 0, SkSamplingOptions(), &paint);
canvas.flush();
}
bitmap.setImmutable();
ResetPendingScaling();
CreateBitmapData();
assert(mBuffer != nullptr);
assert(mPixelsSize == mSize);
@@ -1286,15 +1290,19 @@
mEraseColorSet = false;
}
void SkiaSalBitmap::ResetCachedDataBySize()
void SkiaSalBitmap::ResetPendingScaling()
{
if (mPixelsSize == mSize)
return;
SkiaZone zone;
assert(mSize == mPixelsSize);
assert(!mEraseColorSet);
if (mImage && (mImage->width() != mSize.getWidth() || mImage->height() != mSize.getHeight()))
mScaleQuality = BmpScaleFlag::BestQuality;
mPixelsSize = mSize;
ComputeScanlineSize();
// Information about the pending scaling has been discarded, so make sure we do not
// keep around any cached images that would still need scaling.
if (mImage && imageSize(mImage) != mSize)
mImage.reset();
if (mAlphaImage
&& (mAlphaImage->width() != mSize.getWidth() || mAlphaImage->height() != mSize.getHeight()))
if (mAlphaImage && imageSize(mAlphaImage) != mSize)
mAlphaImage.reset();
}
