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 <[email protected]> AuthorDate: Mon Dec 9 17:19:10 2019 +0100 Commit: Luboš Luňák <[email protected]> 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 <[email protected]> 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 <[email protected]> AuthorDate: Mon Dec 9 17:15:25 2019 +0100 Commit: Luboš Luňák <[email protected]> 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 <[email protected]> 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; _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
