[Libreoffice-commits] core.git: vcl/inc vcl/skia
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Tue Dec 15 15:04:02 UTC 2020
vcl/inc/skia/salbmp.hxx | 1 +
vcl/skia/salbmp.cxx | 35 +++++++++++++++++++++++++----------
2 files changed, 26 insertions(+), 10 deletions(-)
New commits:
commit 0e5b473a63409da2cdae4f4c60a91fcc93755ba5
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Tue Dec 15 12:59:25 2020 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Tue Dec 15 16:03:27 2020 +0100
do not free SkiaSalBitmap buffer if a read access points to it
When conserving memory in raster mode, SkiaSalBitmap may decide
to drop the pixel buffer if SkImage is created from it, since
having both wastes memory and converting between them is cheap.
But if there is still a Bitmap::ScopedReadAccess existing
for the bitmap (e.g. VclCanvasBitmap keeps it as a member),
then dropping the pixel buffer would make the data pointed to
by the read access invalid.
Technically this patch should distinguish between info and read
accesses, as info accesses do not point to pixels, but this is
simpler and hopefully doesn't make a difference in practice.
Change-Id: I307170ad4651b849feda0cf224976ca5a87e5207
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/107752
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 9edb1d620e3a..ec8d4f3c7b82 100644
--- a/vcl/inc/skia/salbmp.hxx
+++ b/vcl/inc/skia/salbmp.hxx
@@ -177,6 +177,7 @@ private:
// Erase() is delayed, just sets these two instead of filling the buffer.
bool mEraseColorSet = false;
Color mEraseColor;
+ int mAnyAccessCount = 0; // number of any kind of AcquireAccess() that have not been released
#ifdef DBG_UTIL
int mWriteAccessCount = 0; // number of write AcquireAccess() that have not been released
#endif
diff --git a/vcl/skia/salbmp.cxx b/vcl/skia/salbmp.cxx
index 096a667e9382..119f4acda526 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -68,6 +68,7 @@ SkiaSalBitmap::SkiaSalBitmap(const sk_sp<SkImage>& image)
mBitCount = 32;
mSize = mPixelsSize = Size(image->width(), image->height());
ComputeScanlineSize();
+ mAnyAccessCount = 0;
#ifdef DBG_UTIL
mWriteAccessCount = 0;
#endif
@@ -76,15 +77,13 @@ SkiaSalBitmap::SkiaSalBitmap(const sk_sp<SkImage>& image)
bool SkiaSalBitmap::Create(const Size& rSize, sal_uInt16 nBitCount, const BitmapPalette& rPal)
{
+ assert(mAnyAccessCount == 0);
ResetAllData();
if (!isValidBitCount(nBitCount))
return false;
mPalette = rPal;
mBitCount = nBitCount;
mSize = mPixelsSize = rSize;
-#ifdef DBG_UTIL
- mWriteAccessCount = 0;
-#endif
if (!ComputeScanlineSize())
{
mBitCount = 0;
@@ -150,6 +149,7 @@ bool SkiaSalBitmap::Create(const SalBitmap& rSalBmp, SalGraphics* pGraphics)
bool SkiaSalBitmap::Create(const SalBitmap& rSalBmp, sal_uInt16 nNewBitCount)
{
+ assert(mAnyAccessCount == 0);
const SkiaSalBitmap& src = static_cast<const SkiaSalBitmap&>(rSalBmp);
mImage = src.mImage;
mAlphaImage = src.mAlphaImage;
@@ -162,9 +162,6 @@ bool SkiaSalBitmap::Create(const SalBitmap& rSalBmp, sal_uInt16 nNewBitCount)
mScaleQuality = src.mScaleQuality;
mEraseColorSet = src.mEraseColorSet;
mEraseColor = src.mEraseColor;
-#ifdef DBG_UTIL
- mWriteAccessCount = 0;
-#endif
if (nNewBitCount != src.GetBitCount())
{
// This appears to be unused(?). Implement this just in case, but be lazy
@@ -188,6 +185,7 @@ void SkiaSalBitmap::Destroy()
#ifdef DBG_UTIL
assert(mWriteAccessCount == 0);
#endif
+ assert(mAnyAccessCount == 0);
ResetAllData();
}
@@ -226,7 +224,10 @@ BitmapBuffer* SkiaSalBitmap::AcquireBuffer(BitmapAccessMode nMode)
buffer->mnHeight = mSize.Height();
buffer->mnBitCount = mBitCount;
buffer->maPalette = mPalette;
- buffer->mpBits = mBuffer.get();
+ if (nMode != BitmapAccessMode::Info)
+ buffer->mpBits = mBuffer.get();
+ else
+ buffer->mpBits = nullptr;
if (mPixelsSize == mSize)
buffer->mnScanlineSize = mScanlineSize;
else
@@ -266,6 +267,7 @@ BitmapBuffer* SkiaSalBitmap::AcquireBuffer(BitmapAccessMode nMode)
abort();
}
buffer->mnFormat |= ScanlineFormat::TopDown;
+ ++mAnyAccessCount;
#ifdef DBG_UTIL
if (nMode == BitmapAccessMode::Write)
++mWriteAccessCount;
@@ -285,11 +287,13 @@ void SkiaSalBitmap::ReleaseBuffer(BitmapBuffer* pBuffer, BitmapAccessMode nMode)
ResetToBuffer();
InvalidateChecksum();
}
+ assert(mAnyAccessCount > 0);
+ --mAnyAccessCount;
// Are there any more ground movements underneath us ?
assert(pBuffer->mnWidth == mSize.Width());
assert(pBuffer->mnHeight == mSize.Height());
assert(pBuffer->mnBitCount == mBitCount);
- assert(pBuffer->mpBits == mBuffer.get());
+ assert(pBuffer->mpBits == mBuffer.get() || nMode == BitmapAccessMode::Info);
verify();
delete pBuffer;
}
@@ -456,6 +460,9 @@ bool SkiaSalBitmap::InterpretAs8Bit()
bool SkiaSalBitmap::Erase(const Color& color)
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
// Optimized variant, just remember the color and apply it when needed,
// which may save having to do format conversions (e.g. GetSkImage()
// may directly erase the SkImage).
@@ -473,6 +480,9 @@ void SkiaSalBitmap::EraseInternal(const Color& color)
bool SkiaSalBitmap::AlphaBlendWith(const SalBitmap& rSalBmp)
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
const SkiaSalBitmap* otherBitmap = dynamic_cast<const SkiaSalBitmap*>(&rSalBmp);
if (!otherBitmap)
return false;
@@ -720,7 +730,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage() const
thisPtr->mImage = image;
// The data is now stored both in the SkImage and in our mBuffer, so drop the buffer
// if conserving memory. It'll be converted back by EnsureBitmapData() if needed.
- if (ConserveMemory())
+ if (ConserveMemory() && mAnyAccessCount == 0)
{
SAL_INFO("vcl.skia.trace", "getskimage(" << this << "): dropping buffer");
thisPtr->ResetToSkImage(mImage);
@@ -869,7 +879,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const
// The data is now stored both in the SkImage and in our mBuffer, so drop the buffer
// if conserving memory and the conversion back would be simple (it'll be converted back
// by EnsureBitmapData() if needed).
- if (ConserveMemory() && mBitCount == 8 && mPalette.IsGreyPalette8Bit())
+ if (ConserveMemory() && mBitCount == 8 && mPalette.IsGreyPalette8Bit() && mAnyAccessCount == 0)
{
SAL_INFO("vcl.skia.trace", "getalphaskimage(" << this << "): dropping buffer");
SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this);
@@ -1155,6 +1165,9 @@ void SkiaSalBitmap::EnsureBitmapData()
void SkiaSalBitmap::EnsureBitmapUniqueData()
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
EnsureBitmapData();
assert(mPixelsSize == mSize);
if (mBuffer.use_count() > 1)
@@ -1182,6 +1195,7 @@ void SkiaSalBitmap::ResetToBuffer()
void SkiaSalBitmap::ResetToSkImage(sk_sp<SkImage> image)
{
+ assert(mAnyAccessCount == 0); // can't reset mBuffer if there's a read access pointing to it
SkiaZone zone;
mBuffer.reset();
mImage = image;
@@ -1191,6 +1205,7 @@ void SkiaSalBitmap::ResetToSkImage(sk_sp<SkImage> image)
void SkiaSalBitmap::ResetAllData()
{
+ assert(mAnyAccessCount == 0);
SkiaZone zone;
mBuffer.reset();
mImage.reset();
More information about the Libreoffice-commits
mailing list