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 <[email protected]> AuthorDate: Mon Mar 15 22:15:15 2021 +0100 Commit: Luboš Luňák <[email protected]> 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 <[email protected]> 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(); } _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
