[Libreoffice-commits] core.git: 2 commits - vcl/inc vcl/skia vcl/source
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Wed Dec 11 14:17:59 UTC 2019
vcl/inc/skia/salbmp.hxx | 7 +-
vcl/skia/salbmp.cxx | 73 ++++++++++++++++++++++++---
vcl/source/bitmap/BitmapScaleSuperFilter.cxx | 1
vcl/source/gdi/bitmap3.cxx | 1
vcl/source/gdi/bitmapex.cxx | 1
5 files changed, 72 insertions(+), 11 deletions(-)
New commits:
commit 818d04478ddddb9d775a638062f19ea0d26a4054
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Mon Dec 9 17:19:10 2019 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Wed Dec 11 15:17:05 2019 +0100
do not allow both read and write bitmap access to the same bitmap
Since BitmapBuffer stores a copy of the bitmap's palette and the bitmap
can update its state only when SalBitmap::ReleaseBuffer() is called,
setting a palette using write access and then reading the bitmap
before the write access is completed could result in inconsistencies.
I wrote this while debugging tdf#129077, which eventually turned out
to be a different problem, but it still makes sense to check this.
Change-Id: I56e9e36c02f2da652cedb3bbc6eb5b630ebaf3ae
Reviewed-on: https://gerrit.libreoffice.org/84771
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 95eb338114cb..4e4aebf4f978 100644
--- a/vcl/inc/skia/salbmp.hxx
+++ b/vcl/inc/skia/salbmp.hxx
@@ -99,6 +99,9 @@ private:
int mScanlineSize; // size of one row in mBuffer
sk_sp<SkImage> mImage; // cached contents, possibly GPU-backed
sk_sp<SkImage> mAlphaImage; // cached contents as alpha image, possibly GPU-backed
+#ifdef DBG_UTIL
+ int mWriteAccessCount; // number of write AcquireAccess() that have not been released
+#endif
};
#endif // INCLUDED_VCL_INC_SKIA_SALBMP_H
diff --git a/vcl/skia/salbmp.cxx b/vcl/skia/salbmp.cxx
index 00dd47c85cbb..b2edfb509c44 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -136,6 +136,9 @@ bool SkiaSalBitmap::Create(const Size& rSize, sal_uInt16 nBitCount, const Bitmap
mPalette = rPal;
mBitCount = nBitCount;
mSize = rSize;
+#ifdef DBG_UTIL
+ mWriteAccessCount = 0;
+#endif
SAL_INFO("vcl.skia", "create(" << this << ")");
return true;
}
@@ -171,6 +174,9 @@ bool SkiaSalBitmap::Create(const SalBitmap& rSalBmp, sal_uInt16 nNewBitCount)
mBuffer.reset(newBuffer);
mScanlineSize = src.mScanlineSize;
}
+#ifdef DBG_UTIL
+ mWriteAccessCount = 0;
+#endif
SAL_INFO("vcl.skia", "create(" << this << "): (" << &src << ")");
return true;
}
@@ -190,6 +196,9 @@ bool SkiaSalBitmap::Create(const css::uno::Reference<css::rendering::XBitmapCanv
void SkiaSalBitmap::Destroy()
{
SAL_INFO("vcl.skia", "destroy(" << this << ")");
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
mBitmap.reset();
mBuffer.reset();
}
@@ -198,11 +207,18 @@ Size SkiaSalBitmap::GetSize() const { return mSize; }
sal_uInt16 SkiaSalBitmap::GetBitCount() const { return mBitCount; }
-BitmapBuffer* SkiaSalBitmap::AcquireBuffer(BitmapAccessMode /*nMode*/)
+BitmapBuffer* SkiaSalBitmap::AcquireBuffer(BitmapAccessMode nMode)
{
- //(void)nMode; // TODO
+#ifndef DBG_UTIL
+ (void)nMode; // TODO
+#endif
if (mBitmap.drawsNothing() && !mBuffer)
return nullptr;
+#ifdef DBG_UTIL
+ // BitmapWriteAccess stores also a copy of the palette and it can
+ // be modified, so concurrent reading of it might result in inconsistencies.
+ assert(mWriteAccessCount == 0 || nMode == BitmapAccessMode::Write);
+#endif
BitmapBuffer* buffer = new BitmapBuffer;
buffer->mnWidth = mSize.Width();
buffer->mnHeight = mSize.Height();
@@ -250,6 +266,10 @@ BitmapBuffer* SkiaSalBitmap::AcquireBuffer(BitmapAccessMode /*nMode*/)
abort();
}
buffer->mnFormat |= ScanlineFormat::TopDown;
+#ifdef DBG_UTIL
+ if (nMode == BitmapAccessMode::Write)
+ ++mWriteAccessCount;
+#endif
return buffer;
}
@@ -257,6 +277,10 @@ void SkiaSalBitmap::ReleaseBuffer(BitmapBuffer* pBuffer, BitmapAccessMode nMode)
{
if (nMode == BitmapAccessMode::Write) // TODO something more?
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount > 0);
+ --mWriteAccessCount;
+#endif
mPalette = pBuffer->maPalette;
ResetCachedBitmap();
}
@@ -270,6 +294,9 @@ void SkiaSalBitmap::ReleaseBuffer(BitmapBuffer* pBuffer, BitmapAccessMode nMode)
bool SkiaSalBitmap::GetSystemData(BitmapSystemData&)
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
// TODO?
return false;
}
@@ -278,18 +305,27 @@ bool SkiaSalBitmap::ScalingSupported() const { return false; }
bool SkiaSalBitmap::Scale(const double&, const double&, BmpScaleFlag)
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
// TODO?
return false;
}
bool SkiaSalBitmap::Replace(const Color&, const Color&, sal_uInt8)
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
// TODO?
return false;
}
bool SkiaSalBitmap::ConvertToGreyscale()
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
// Skia can convert color SkBitmap to a greyscale one (draw using SkCanvas),
// but it uses different coefficients for the color->grey conversion than VCL.
// So just let VCL do it.
@@ -298,6 +334,9 @@ bool SkiaSalBitmap::ConvertToGreyscale()
SkBitmap SkiaSalBitmap::GetAsSkBitmap() const
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
if (!mBitmap.drawsNothing())
return mBitmap;
SkBitmap bitmap;
@@ -348,6 +387,9 @@ SkBitmap SkiaSalBitmap::GetAsSkBitmap() const
const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage() const
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
if (mImage)
return mImage;
sk_sp<SkSurface> surface = SkiaHelper::createSkSurface(mSize);
@@ -362,6 +404,9 @@ const sk_sp<SkImage>& SkiaSalBitmap::GetSkImage() const
const sk_sp<SkImage>& SkiaSalBitmap::GetAlphaSkImage() const
{
+#ifdef DBG_UTIL
+ assert(mWriteAccessCount == 0);
+#endif
if (mAlphaImage)
return mAlphaImage;
SkBitmap alphaBitmap;
@@ -433,7 +478,20 @@ void SkiaSalBitmap::ResetCachedBitmap()
}
#ifdef DBG_UTIL
-void SkiaSalBitmap::dump(const char* file) const { SkiaHelper::dump(GetSkImage(), file); }
+void SkiaSalBitmap::dump(const char* file) const
+{
+ sk_sp<SkImage> saveImage = mImage;
+ sk_sp<SkImage> saveAlphaImage = mAlphaImage;
+ int saveWriteAccessCount = mWriteAccessCount;
+ SkiaSalBitmap* thisPtr = const_cast<SkiaSalBitmap*>(this);
+ // avoid possible assert
+ thisPtr->mWriteAccessCount = 0;
+ SkiaHelper::dump(GetSkImage(), file);
+ // restore old state, so that debugging doesn't affect it
+ thisPtr->mImage = saveImage;
+ thisPtr->mAlphaImage = saveAlphaImage;
+ thisPtr->mWriteAccessCount = saveWriteAccessCount;
+}
void SkiaSalBitmap::verify() const
{
diff --git a/vcl/source/bitmap/BitmapScaleSuperFilter.cxx b/vcl/source/bitmap/BitmapScaleSuperFilter.cxx
index 9ee6e80c7b40..263b6fdb7f0d 100644
--- a/vcl/source/bitmap/BitmapScaleSuperFilter.cxx
+++ b/vcl/source/bitmap/BitmapScaleSuperFilter.cxx
@@ -1176,6 +1176,7 @@ BitmapEx BitmapScaleSuperFilter::execute(BitmapEx const& rBitmap) const
if (!bUseThreads)
pScaleRangeFn( aContext, nStartY, nEndY );
+ pWriteAccess.reset();
bRet = true;
aBitmap.AdaptBitCount(aOutBmp);
aBitmap = aOutBmp;
diff --git a/vcl/source/gdi/bitmap3.cxx b/vcl/source/gdi/bitmap3.cxx
index 3cf09f40e529..87f858de7732 100644
--- a/vcl/source/gdi/bitmap3.cxx
+++ b/vcl/source/gdi/bitmap3.cxx
@@ -634,6 +634,7 @@ bool Bitmap::ImplConvertDown(sal_uInt16 nBitCount, Color const * pExtColor)
bRet = true;
}
+ pWriteAcc.reset();
if(bRet)
{
diff --git a/vcl/source/gdi/bitmapex.cxx b/vcl/source/gdi/bitmapex.cxx
index adb14f014ddf..b8a20fcefe61 100644
--- a/vcl/source/gdi/bitmapex.cxx
+++ b/vcl/source/gdi/bitmapex.cxx
@@ -864,6 +864,7 @@ namespace
}
}
}
+ xWrite.reset();
rSource.AdaptBitCount(aDestination);
commit 852bba5478f38edc68bec8d38fba7262474e82ea
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Mon Dec 9 17:15:25 2019 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Wed Dec 11 15:16:53 2019 +0100
do not store 8bpp bitmaps as Skia's SkBitmap (tdf#129077)
Skia does not support paletted images, so it only supports grayscale 8bpp.
But whether a bitmap is grayscale depends on the palette, and that can
change merely by assigning a different one.
Change-Id: If55d31e46e79ef32f08ecd196ba7a6091d05a13f
Reviewed-on: https://gerrit.libreoffice.org/84770
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 ea25d477c985..95eb338114cb 100644
--- a/vcl/inc/skia/salbmp.hxx
+++ b/vcl/inc/skia/salbmp.hxx
@@ -79,7 +79,7 @@ private:
template <typename charT, typename traits>
friend inline std::basic_ostream<charT, traits>&
operator<<(std::basic_ostream<charT, traits>& stream, const SkiaSalBitmap* bitmap)
- { // TODO GPU-based, once it's done
+ {
// B - has SkBitmap, D - has data buffer, I/i - has SkImage (on GPU/CPU),
// A/a - has alpha SkImage (on GPU/CPU)
return stream << static_cast<const void*>(bitmap) << " " << bitmap->GetSize() << "/"
@@ -95,8 +95,6 @@ private:
BitmapPalette mPalette;
int mBitCount; // bpp
Size mSize;
- // Skia does not natively support 1bpp and 4bpp, so such bitmaps are stored
- // in a buffer (and converted to 32bpp SkBitmap on-demand using GetSkBitmap()).
std::unique_ptr<sal_uInt8[]> mBuffer;
int mScanlineSize; // size of one row in mBuffer
sk_sp<SkImage> mImage; // cached contents, possibly GPU-backed
diff --git a/vcl/skia/salbmp.cxx b/vcl/skia/salbmp.cxx
index d7fc3f6905c6..00dd47c85cbb 100644
--- a/vcl/skia/salbmp.cxx
+++ b/vcl/skia/salbmp.cxx
@@ -69,15 +69,14 @@ bool SkiaSalBitmap::Create(const Size& rSize, sal_uInt16 nBitCount, const Bitmap
return false;
// Skia only supports 8bit gray, 16bit and 32bit formats (e.g. 24bpp is actually stored as 32bpp).
// But some of our code accessing the bitmap assumes that when it asked for 24bpp, the format
- // really will be 24bpp (e.g. the png loader).
+ // really will be 24bpp (e.g. the png loader), so we cannot use SkBitmap to store the data.
+ // And even 8bpp is problematic, since Skia does not support palettes and a VCL bitmap can change
+ // its grayscale status simply by changing the palette.
+ // So basically use Skia only for 32bpp bitmaps.
// TODO what is the performance impact of handling 24bpp ourselves instead of in Skia?
SkColorType colorType = kUnknown_SkColorType;
switch (nBitCount)
{
- case 8:
- if (rPal.IsGreyPalette()) // see GetAlphaSkBitmap()
- colorType = kGray_8_SkColorType;
- break;
case 32:
colorType = kN32_SkColorType;
break;
More information about the Libreoffice-commits
mailing list