From f758311c7362d9232b98b6519ec0af1f71ad02f8 Mon Sep 17 00:00:00 2001 From: Matt Sarett Date: Mon, 1 May 2017 10:22:31 -0400 Subject: [PATCH] Only store width and height on SkPixelRef (part 2) Relanding https://skia-review.googlesource.com/c/14105/ in pieces to try to diagnose problems with the Chrome roll. Bug: skia:6535 Change-Id: Iada034fc41ef315f7f00984d8de9d9cc2f361ad2 Reviewed-on: https://skia-review.googlesource.com/14657 Commit-Queue: Matt Sarett Reviewed-by: Mike Reed --- include/core/SkMallocPixelRef.h | 2 -- include/core/SkPixelRef.h | 55 +++++++++++++++++++-------------- src/core/SkBitmap.cpp | 29 ++++------------- src/core/SkMallocPixelRef.cpp | 4 --- src/core/SkPixelRef.cpp | 47 ++++++++++++++++++++++------ 5 files changed, 75 insertions(+), 62 deletions(-) diff --git a/include/core/SkMallocPixelRef.h b/include/core/SkMallocPixelRef.h index 45ab6547f4..e6d9727952 100644 --- a/include/core/SkMallocPixelRef.h +++ b/include/core/SkMallocPixelRef.h @@ -78,8 +78,6 @@ public: protected: ~SkMallocPixelRef() override; - size_t getAllocatedSizeInBytes() const override; - private: // Uses alloc to implement NewAllocate or NewZeroed. static sk_sp MakeUsing(void*(*alloc)(size_t), diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h index 05b0fbcf9c..3c002dca70 100644 --- a/include/core/SkPixelRef.h +++ b/include/core/SkPixelRef.h @@ -33,12 +33,37 @@ class SkDiscardableMemory; class SK_API SkPixelRef : public SkRefCnt { public: SkPixelRef(const SkImageInfo&, void* addr, size_t rowBytes, sk_sp = nullptr); - ~SkPixelRef() override; const SkImageInfo& info() const { return fInfo; } +#ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK + // This is undefined if there are clients in-flight trying to use us + void android_only_reset(const SkImageInfo&, size_t rowBytes, sk_sp); +#endif + + /** + * Change the info's AlphaType. Note that this does not automatically + * invalidate the generation ID. If the pixel values themselves have + * changed, then you must explicitly call notifyPixelsChanged() as well. + */ + void changeAlphaType(SkAlphaType at); + + /** + * Returns the size (in bytes) of the internally allocated memory. + * This should be implemented in all serializable SkPixelRef derived classes. + * SkBitmap::fPixelRefOffset + SkBitmap::getSafeSize() should never overflow this value, + * otherwise the rendering code may attempt to read memory out of bounds. + * + * @return default impl returns 0. + */ + virtual size_t getAllocatedSizeInBytes() const { return 0; } + + SkPixelRef(int width, int height, void* addr, size_t rowBytes, sk_sp = nullptr); + + ~SkPixelRef() override; + int width() const { return fInfo.width(); } int height() const { return fInfo.height(); } void* pixels() const { return fPixels; } @@ -70,13 +95,6 @@ public: */ void notifyPixelsChanged(); - /** - * Change the info's AlphaType. Note that this does not automatically - * invalidate the generation ID. If the pixel values themselves have - * changed, then you must explicitly call notifyPixelsChanged() as well. - */ - void changeAlphaType(SkAlphaType at); - /** Returns true if this pixelref is marked as immutable, meaning that the contents of its pixels will not change for the lifetime of the pixelref. */ @@ -116,27 +134,18 @@ protected: // default impl does nothing. virtual void onNotifyPixelsChanged(); - /** - * Returns the size (in bytes) of the internally allocated memory. - * This should be implemented in all serializable SkPixelRef derived classes. - * SkBitmap::fPixelRefOffset + SkBitmap::getSafeSize() should never overflow this value, - * otherwise the rendering code may attempt to read memory out of bounds. - * - * @return default impl returns 0. - */ - virtual size_t getAllocatedSizeInBytes() const; - #ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK // This is undefined if there are clients in-flight trying to use us - void android_only_reset(const SkImageInfo&, size_t rowBytes, sk_sp); + void android_only_reset(int width, int height, size_t rowBytes, sk_sp); #endif private: - // mostly const. fInfo.fAlpahType can be changed at runtime. - const SkImageInfo fInfo; + // TODO (msarett): After we remove legacy APIs, we should replace |fInfo| with just a width + // and height. + const SkImageInfo fInfo; sk_sp fCTable; - void* fPixels; - size_t fRowBytes; + void* fPixels; + size_t fRowBytes; // Bottom bit indicates the Gen ID is unique. bool genIDIsUnique() const { return SkToBool(fTaggedGenID.load() & 1); } diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index 1ba2968466..5969a73a05 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -198,32 +198,15 @@ void SkBitmap::setPixelRef(sk_sp pr, int dx, int dy) { #ifdef SK_DEBUG if (pr) { if (kUnknown_SkColorType != fInfo.colorType()) { - const SkImageInfo& prInfo = pr->info(); - SkASSERT(fInfo.width() <= prInfo.width()); - SkASSERT(fInfo.height() <= prInfo.height()); - SkASSERT(fInfo.colorType() == prInfo.colorType()); - switch (prInfo.alphaType()) { - case kUnknown_SkAlphaType: - SkASSERT(fInfo.alphaType() == kUnknown_SkAlphaType); - break; - case kOpaque_SkAlphaType: - case kPremul_SkAlphaType: - SkASSERT(fInfo.alphaType() == kOpaque_SkAlphaType || - fInfo.alphaType() == kPremul_SkAlphaType); - break; - case kUnpremul_SkAlphaType: - SkASSERT(fInfo.alphaType() == kOpaque_SkAlphaType || - fInfo.alphaType() == kUnpremul_SkAlphaType); - break; - } + SkASSERT(fInfo.width() + dx <= pr->width()); + SkASSERT(fInfo.height() + dy <= pr->height()); } } #endif fPixelRef = std::move(pr); if (fPixelRef) { - const SkImageInfo& info = fPixelRef->info(); - fPixelRefOrigin.set(SkTPin(dx, 0, info.width()), SkTPin(dy, 0, info.height())); + fPixelRefOrigin.set(SkTPin(dx, 0, fPixelRef->width()), SkTPin(dy, 0, fPixelRef->height())); this->updatePixelsFromRef(); } else { // ignore dx,dy if there is no pixelref @@ -843,7 +826,7 @@ bool SkBitmap::ReadRawPixels(SkReadBuffer* buffer, SkBitmap* bitmap) { if (!pr) { return false; } - bitmap->setInfo(pr->info()); + bitmap->setInfo(info); bitmap->setPixelRef(std::move(pr), 0, 0); return true; } @@ -883,8 +866,8 @@ void SkBitmap::validate() const { SkASSERT(fPixelRef->rowBytes() == fRowBytes); SkASSERT(fPixelRefOrigin.fX >= 0); SkASSERT(fPixelRefOrigin.fY >= 0); - SkASSERT(fPixelRef->info().width() >= (int)this->width() + fPixelRefOrigin.fX); - SkASSERT(fPixelRef->info().height() >= (int)this->height() + fPixelRefOrigin.fY); + SkASSERT(fPixelRef->width() >= (int)this->width() + fPixelRefOrigin.fX); + SkASSERT(fPixelRef->height() >= (int)this->height() + fPixelRefOrigin.fY); SkASSERT(fPixelRef->rowBytes() >= fInfo.minRowBytes()); } } diff --git a/src/core/SkMallocPixelRef.cpp b/src/core/SkMallocPixelRef.cpp index cdc555b748..36b790f8c6 100644 --- a/src/core/SkMallocPixelRef.cpp +++ b/src/core/SkMallocPixelRef.cpp @@ -167,7 +167,3 @@ SkMallocPixelRef::~SkMallocPixelRef() { fReleaseProc(this->pixels(), fReleaseProcContext); } } - -size_t SkMallocPixelRef::getAllocatedSizeInBytes() const { - return this->info().getSafeSize(this->rowBytes()); -} diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp index c6d59eab19..143abbc5cf 100644 --- a/src/core/SkPixelRef.cpp +++ b/src/core/SkPixelRef.cpp @@ -26,6 +26,10 @@ uint32_t SkNextID::ImageID() { /////////////////////////////////////////////////////////////////////////////// +#ifdef SK_TRACE_PIXELREF_LIFETIME + static int32_t gInstCounter; +#endif + static SkImageInfo validate_info(const SkImageInfo& info) { SkAlphaType newAlphaType = info.alphaType(); SkAssertResult(SkColorTypeValidateAlphaType(info.colorType(), info.alphaType(), &newAlphaType)); @@ -43,10 +47,6 @@ static void validate_pixels_ctable(const SkImageInfo& info, const SkColorTable* } } -#ifdef SK_TRACE_PIXELREF_LIFETIME - static int32_t gInstCounter; -#endif - SkPixelRef::SkPixelRef(const SkImageInfo& info, void* pixels, size_t rowBytes, sk_sp ctable) : fInfo(validate_info(info)) @@ -68,6 +68,25 @@ SkPixelRef::SkPixelRef(const SkImageInfo& info, void* pixels, size_t rowBytes, fAddedToCache.store(false); } +SkPixelRef::SkPixelRef(int width, int height, void* pixels, size_t rowBytes, + sk_sp ctable) + : fInfo(SkImageInfo::MakeUnknown(width, height)) + , fCTable(std::move(ctable)) + , fPixels(pixels) + , fRowBytes(rowBytes) +#ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK + , fStableID(SkNextID::ImageID()) +#endif +{ +#ifdef SK_TRACE_PIXELREF_LIFETIME + SkDebugf(" pixelref %d\n", sk_atomic_inc(&gInstCounter)); +#endif + + this->needsNewGenID(); + fMutability = kMutable; + fAddedToCache.store(false); +} + SkPixelRef::~SkPixelRef() { #ifdef SK_TRACE_PIXELREF_LIFETIME SkDebugf("~pixelref %d\n", sk_atomic_dec(&gInstCounter) - 1); @@ -76,11 +95,10 @@ SkPixelRef::~SkPixelRef() { } #ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK + // This is undefined if there are clients in-flight trying to use us void SkPixelRef::android_only_reset(const SkImageInfo& info, size_t rowBytes, sk_sp ctable) { - validate_pixels_ctable(info, ctable.get()); - *const_cast(&fInfo) = info; fRowBytes = rowBytes; fCTable = std::move(ctable); @@ -89,6 +107,19 @@ void SkPixelRef::android_only_reset(const SkImageInfo& info, size_t rowBytes, // conservative, since its possible the "new" settings are the same as the old. this->notifyPixelsChanged(); } + +// This is undefined if there are clients in-flight trying to use us +void SkPixelRef::android_only_reset(int width, int height, size_t rowBytes, + sk_sp ctable) { + *const_cast(&fInfo) = fInfo.makeWH(width, height); + fRowBytes = rowBytes; + fCTable = std::move(ctable); + // note: we do not change fPixels + + // conservative, since its possible the "new" settings are the same as the old. + this->notifyPixelsChanged(); +} + #endif void SkPixelRef::needsNewGenID() { @@ -179,7 +210,3 @@ void SkPixelRef::restoreMutability() { } void SkPixelRef::onNotifyPixelsChanged() { } - -size_t SkPixelRef::getAllocatedSizeInBytes() const { - return 0; -}