[Libreoffice-commits] core.git: vcl/inc vcl/qa vcl/skia
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Tue Mar 16 11:57:12 UTC 2021
vcl/inc/skia/salbmp.hxx | 4 +-
vcl/qa/cppunit/skia/skia.cxx | 43 +++++++++++++++++++++++++
vcl/skia/salbmp.cxx | 72 +++++++++++++++++++++++--------------------
3 files changed, 85 insertions(+), 34 deletions(-)
New commits:
commit 1e9b97dc1f795da63c92b0169415a4f455d9d014
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Mon Mar 15 22:15:15 2021 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Tue Mar 16 12:56:17 2021 +0100
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 at collabora.com>
diff --git a/vcl/inc/skia/salbmp.hxx b/vcl/inc/skia/salbmp.hxx
index 0509ed3381b6..012594169132 100644
--- a/vcl/inc/skia/salbmp.hxx
+++ b/vcl/inc/skia/salbmp.hxx
@@ -105,8 +105,6 @@ private:
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 @@ private:
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 8f288d813404..d8e103d1a431 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 @@ public:
void testAlphaBlendWith();
void testBitmapCopyOnWrite();
void testMatrixQuality();
+ void testDelayedScale();
void testTdf137329();
CPPUNIT_TEST_SUITE(SkiaTest);
@@ -48,6 +51,7 @@ public:
CPPUNIT_TEST(testAlphaBlendWith);
CPPUNIT_TEST(testBitmapCopyOnWrite);
CPPUNIT_TEST(testMatrixQuality);
+ CPPUNIT_TEST(testDelayedScale);
CPPUNIT_TEST(testTdf137329);
CPPUNIT_TEST_SUITE_END();
@@ -329,6 +333,45 @@ void SkiaTest::testMatrixQuality()
#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 c8abfb64252b..3b4ec7104af4 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -85,7 +85,8 @@ bool SkiaSalBitmap::Create(const Size& rSize, sal_uInt16 nBitCount, const Bitmap
return false;
mPalette = rPal;
mBitCount = nBitCount;
- mSize = mPixelsSize = rSize;
+ mSize = rSize;
+ ResetPendingScaling();
if (!ComputeScanlineSize())
{
mBitCount = 0;
@@ -387,8 +388,8 @@ bool SkiaSalBitmap::Scale(const double& rScaleX, const double& rScaleY, BmpScale
if (mEraseColorSet)
{ // Simple.
- mSize = mPixelsSize = newSize;
- ComputeScanlineSize();
+ mSize = newSize;
+ ResetPendingScaling();
EraseInternal(mEraseColor);
return true;
}
@@ -408,7 +409,14 @@ bool SkiaSalBitmap::Scale(const double& rScaleX, const double& rScaleY, BmpScale
// 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 @@ bool SkiaSalBitmap::Scale(const double& rScaleX, const double& rScaleY, BmpScale
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 @@ bool SkiaSalBitmap::ConvertToGreyscale()
// 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 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage() const
}
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 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const
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 @@ void SkiaSalBitmap::EnsureBitmapData()
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 @@ void SkiaSalBitmap::EnsureBitmapData()
}
// 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 @@ void SkiaSalBitmap::EnsureBitmapData()
canvas.flush();
}
bitmap.setImmutable();
+ ResetPendingScaling();
CreateBitmapData();
assert(mBuffer != nullptr);
assert(mPixelsSize == mSize);
@@ -1123,7 +1134,7 @@ void SkiaSalBitmap::EnsureBitmapData()
#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 @@ void SkiaSalBitmap::EnsureBitmapData()
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 @@ void SkiaSalBitmap::ResetAllData()
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();
}
More information about the Libreoffice-commits
mailing list