vcl/inc/skia/salbmp.hxx | 2 +-
vcl/skia/salbmp.cxx | 30 +++++++++++++++++++-----------
2 files changed, 20 insertions(+), 12 deletions(-)
New commits:
commit 20de4e3ca38e177ea61e818b32d82008758b8caa
Author: Luboš Luňák <l.lu...@collabora.com>
AuthorDate: Tue Sep 13 18:15:17 2022 +0200
Commit: Luboš Luňák <l.lu...@collabora.com>
CommitDate: Wed Sep 14 08:05:18 2022 +0200
do not check and refcount Info access to Skia bitmaps (tdf#150817)
VclCanvasBitmap keeps one around for no good reason, and I don't
feel like digging into it. Since there's no pixel data involved
in that case, let's assume the reader knows that the info about
the bitmap will not change. Making this difference is actually
what I suggested in 0e5b473a63409da2cdae4f4c60a91fcc93755ba5.
Change-Id: I302a9c2c63c8b9474a541a963928933e223f4b45
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/139873
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lu...@collabora.com>
diff --git a/vcl/inc/skia/salbmp.hxx b/vcl/inc/skia/salbmp.hxx
index 05e489643f88..e79fb1cc101d 100644
--- a/vcl/inc/skia/salbmp.hxx
+++ b/vcl/inc/skia/salbmp.hxx
@@ -211,7 +211,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
+ int mReadAccessCount = 0; // number of read 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 df3536b4de91..57cea14316c5 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -68,7 +68,7 @@ SkiaSalBitmap::SkiaSalBitmap(const sk_sp<SkImage>& image)
#endif
mSize = mPixelsSize = Size(image->width(), image->height());
ComputeScanlineSize();
- mAnyAccessCount = 0;
+ mReadAccessCount = 0;
#ifdef DBG_UTIL
mWriteAccessCount = 0;
#endif
@@ -78,7 +78,7 @@ SkiaSalBitmap::SkiaSalBitmap(const sk_sp<SkImage>& image)
bool SkiaSalBitmap::Create(const Size& rSize, vcl::PixelFormat ePixelFormat,
const BitmapPalette& rPal)
{
- assert(mAnyAccessCount == 0);
+ assert(mReadAccessCount == 0);
ResetAllData();
if (ePixelFormat == vcl::PixelFormat::INVALID)
return false;
@@ -157,7 +157,7 @@ bool SkiaSalBitmap::Create(const SalBitmap& rSalBmp,
SalGraphics* pGraphics)
bool SkiaSalBitmap::Create(const SalBitmap& rSalBmp, vcl::PixelFormat
eNewPixelFormat)
{
- assert(mAnyAccessCount == 0);
+ assert(mReadAccessCount == 0);
assert(&rSalBmp != this);
ResetAllData();
const SkiaSalBitmap& src = static_cast<const SkiaSalBitmap&>(rSalBmp);
@@ -195,7 +195,7 @@ void SkiaSalBitmap::Destroy()
#ifdef DBG_UTIL
assert(mWriteAccessCount == 0);
#endif
- assert(mAnyAccessCount == 0);
+ assert(mReadAccessCount == 0);
ResetAllData();
}
@@ -274,7 +274,12 @@ BitmapBuffer*
SkiaSalBitmap::AcquireBuffer(BitmapAccessMode nMode)
abort();
}
buffer->mnFormat |= ScanlineFormat::TopDown;
- ++mAnyAccessCount;
+ // Refcount all read/write accesses, to catch problems with existing
accesses while
+ // a bitmap changes, and also to detect when we can free mBuffer if wanted.
+ // Write mode implies also reading. It would be probably a good idea to
count even
+ // Info accesses, but VclCanvasBitmap keeps one around pointlessly,
causing tdf#150817.
+ if (nMode == BitmapAccessMode::Read || nMode == BitmapAccessMode::Write)
+ ++mReadAccessCount;
#ifdef DBG_UTIL
if (nMode == BitmapAccessMode::Write)
++mWriteAccessCount;
@@ -300,8 +305,11 @@ void SkiaSalBitmap::ReleaseBuffer(BitmapBuffer* pBuffer,
BitmapAccessMode nMode,
ResetToBuffer();
DataChanged();
}
- assert(mAnyAccessCount > 0);
- --mAnyAccessCount;
+ if (nMode == BitmapAccessMode::Read || nMode == BitmapAccessMode::Write)
+ {
+ assert(mReadAccessCount > 0);
+ --mReadAccessCount;
+ }
// Are there any more ground movements underneath us ?
assert(pBuffer->mnWidth == mSize.Width());
assert(pBuffer->mnHeight == mSize.Height());
@@ -812,7 +820,7 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage(DirectImage
direct) 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() && mAnyAccessCount == 0)
+ if (ConserveMemory() && mReadAccessCount == 0)
{
SAL_INFO("vcl.skia.trace", "getskimage(" << this << "): dropping
buffer");
thisPtr->ResetToSkImage(mImage);
@@ -968,7 +976,7 @@ const sk_sp<SkImage>&
SkiaSalBitmap::GetAlphaSkImage(DirectImage direct) 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() &&
mAnyAccessCount == 0)
+ if (ConserveMemory() && mBitCount == 8 && mPalette.IsGreyPalette8Bit() &&
mReadAccessCount == 0)
{
SAL_INFO("vcl.skia.trace", "getalphaskimage(" << this << "): dropping
buffer");
SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this);
@@ -1308,7 +1316,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
+ assert(mReadAccessCount == 0); // can't reset mBuffer if there's a read
access pointing to it
SkiaZone zone;
mBuffer.reset();
mImage = image;
@@ -1318,7 +1326,7 @@ void SkiaSalBitmap::ResetToSkImage(sk_sp<SkImage> image)
void SkiaSalBitmap::ResetAllData()
{
- assert(mAnyAccessCount == 0);
+ assert(mReadAccessCount == 0);
SkiaZone zone;
mBuffer.reset();
mImage.reset();
