From 2fcd4a480deecb23e9fdde07e63d18d2735c9287 Mon Sep 17 00:00:00 2001 From: Ben Wagner Date: Fri, 28 Apr 2017 13:06:02 -0400 Subject: [PATCH] Revert "Only store width and height on SkPixelRef" This reverts commit 2cbb6662e329981840f90ef4edd62f70f69e6030. Reason for revert: Likely cause of Chromium DEPS roll failure; speculative revert. Original change's description: > Only store width and height on SkPixelRef > > Bug: skia:6535 > Change-Id: Id91e8d1e82f593be7d4b23ca5abde752f2666a77 > Reviewed-on: https://skia-review.googlesource.com/14105 > Commit-Queue: Matt Sarett > Reviewed-by: Mike Reed > TBR=djsollen@google.com,msarett@google.com,reed@google.com,stani@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: I12a024a71833f33432d5ea8cffdfc642b8b4240a Bug: skia: Reviewed-on: https://skia-review.googlesource.com/14644 Reviewed-by: Ben Wagner Commit-Queue: Ben Wagner --- gn/android_framework_defines.gni | 1 - include/core/SkMallocPixelRef.h | 2 ++ include/core/SkPixelRef.h | 59 ++++++++++++++++------------------------ public.bzl | 2 -- src/core/SkBitmap.cpp | 32 ++++++++++++++++++---- src/core/SkMallocPixelRef.cpp | 6 +++- src/core/SkPixelRef.cpp | 55 +++++++------------------------------ src/core/SkSpecialSurface.cpp | 11 ++++---- src/image/SkImage_Raster.cpp | 2 +- src/image/SkSurface_Raster.cpp | 11 ++++---- src/pdf/SkPDFDevice.cpp | 4 +-- 11 files changed, 80 insertions(+), 105 deletions(-) diff --git a/gn/android_framework_defines.gni b/gn/android_framework_defines.gni index bc49f4b..67c6a2d 100644 --- a/gn/android_framework_defines.gni +++ b/gn/android_framework_defines.gni @@ -9,7 +9,6 @@ android_framework_defines = [ "SK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS", "SK_SUPPORT_LEGACY_GRADIENT_DITHERING", "SK_SUPPORT_LEGACY_DRAWFILTER", - "SK_SUPPORT_LEGACY_PIXELREF_API", "SK_IGNORE_GPU_DITHER", "SK_SUPPORT_LEGACY_SHADER_ISABITMAP", "SK_SUPPORT_LEGACY_EMBOSSMASKFILTER", diff --git a/include/core/SkMallocPixelRef.h b/include/core/SkMallocPixelRef.h index e6d9727..45ab654 100644 --- a/include/core/SkMallocPixelRef.h +++ b/include/core/SkMallocPixelRef.h @@ -78,6 +78,8 @@ 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 535c6e1..6a86e59 100644 --- a/include/core/SkPixelRef.h +++ b/include/core/SkPixelRef.h @@ -32,42 +32,13 @@ class SkDiscardableMemory; */ class SK_API SkPixelRef : public SkRefCnt { public: -#ifdef SK_SUPPORT_LEGACY_PIXELREF_API 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; } - -#endif - 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; } SkColorTable* colorTable() const { return fCTable.get(); } size_t rowBytes() const { return fRowBytes; } @@ -97,6 +68,13 @@ 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. */ @@ -136,18 +114,27 @@ 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(int width, int height, size_t rowBytes, sk_sp); + void android_only_reset(const SkImageInfo&, size_t rowBytes, sk_sp); #endif private: - // TODO (msarett): After we remove legacy APIs, we should replace |fInfo| with just a width - // and height. - const SkImageInfo fInfo; + // mostly const. fInfo.fAlpahType can be changed at runtime. + 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/public.bzl b/public.bzl index f0ac995..9c41cdb 100644 --- a/public.bzl +++ b/public.bzl @@ -655,10 +655,8 @@ DEFINES_ALL = [ # Turn on a few Google3-specific build fixes. "GOOGLE3", # Staging flags for API changes - "SK_SUPPORT_LEGACY_PIXELREF_API", # Temporarily Disable analytic AA for Google3 "SK_NO_ANALYTIC_AA", - ] ################################################################################ diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index 8bd2684..1ba2968 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -171,6 +171,9 @@ bool SkBitmap::setAlphaType(SkAlphaType newAlphaType) { } if (fInfo.alphaType() != newAlphaType) { fInfo = fInfo.makeAlphaType(newAlphaType); + if (fPixelRef) { + fPixelRef->changeAlphaType(newAlphaType); + } } SkDEBUGCODE(this->validate();) return true; @@ -195,15 +198,32 @@ void SkBitmap::setPixelRef(sk_sp pr, int dx, int dy) { #ifdef SK_DEBUG if (pr) { if (kUnknown_SkColorType != fInfo.colorType()) { - SkASSERT(fInfo.width() <= pr->width()); - SkASSERT(fInfo.height() <= pr->height()); + 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; + } } } #endif fPixelRef = std::move(pr); if (fPixelRef) { - fPixelRefOrigin.set(SkTPin(dx, 0, fPixelRef->width()), SkTPin(dy, 0, fPixelRef->height())); + const SkImageInfo& info = fPixelRef->info(); + fPixelRefOrigin.set(SkTPin(dx, 0, info.width()), SkTPin(dy, 0, info.height())); this->updatePixelsFromRef(); } else { // ignore dx,dy if there is no pixelref @@ -823,7 +843,7 @@ bool SkBitmap::ReadRawPixels(SkReadBuffer* buffer, SkBitmap* bitmap) { if (!pr) { return false; } - bitmap->setInfo(info); + bitmap->setInfo(pr->info()); bitmap->setPixelRef(std::move(pr), 0, 0); return true; } @@ -863,8 +883,8 @@ void SkBitmap::validate() const { SkASSERT(fPixelRef->rowBytes() == fRowBytes); SkASSERT(fPixelRefOrigin.fX >= 0); SkASSERT(fPixelRefOrigin.fY >= 0); - SkASSERT(fPixelRef->width() >= (int)this->width() + fPixelRefOrigin.fX); - SkASSERT(fPixelRef->height() >= (int)this->height() + fPixelRefOrigin.fY); + SkASSERT(fPixelRef->info().width() >= (int)this->width() + fPixelRefOrigin.fX); + SkASSERT(fPixelRef->info().height() >= (int)this->height() + fPixelRefOrigin.fY); SkASSERT(fPixelRef->rowBytes() >= fInfo.minRowBytes()); } } diff --git a/src/core/SkMallocPixelRef.cpp b/src/core/SkMallocPixelRef.cpp index cf4fac7..cdc555b 100644 --- a/src/core/SkMallocPixelRef.cpp +++ b/src/core/SkMallocPixelRef.cpp @@ -156,7 +156,7 @@ SkMallocPixelRef::SkMallocPixelRef(const SkImageInfo& info, void* storage, size_t rowBytes, sk_sp ctable, SkMallocPixelRef::ReleaseProc proc, void* context) - : INHERITED(info.width(), info.height(), storage, rowBytes, sanitize(info, std::move(ctable))) + : INHERITED(info, storage, rowBytes, sanitize(info, std::move(ctable))) , fReleaseProc(proc) , fReleaseProcContext(context) {} @@ -167,3 +167,7 @@ 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 487779c..c6d59ea 100644 --- a/src/core/SkPixelRef.cpp +++ b/src/core/SkPixelRef.cpp @@ -26,12 +26,6 @@ uint32_t SkNextID::ImageID() { /////////////////////////////////////////////////////////////////////////////// -#ifdef SK_TRACE_PIXELREF_LIFETIME - static int32_t gInstCounter; -#endif - -#ifdef SK_SUPPORT_LEGACY_PIXELREF_API - static SkImageInfo validate_info(const SkImageInfo& info) { SkAlphaType newAlphaType = info.alphaType(); SkAssertResult(SkColorTypeValidateAlphaType(info.colorType(), info.alphaType(), &newAlphaType)); @@ -49,6 +43,10 @@ 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)) @@ -70,27 +68,6 @@ SkPixelRef::SkPixelRef(const SkImageInfo& info, void* pixels, size_t rowBytes, fAddedToCache.store(false); } -#endif - -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); @@ -99,25 +76,12 @@ SkPixelRef::~SkPixelRef() { } #ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK - -#ifdef SK_SUPPORT_LEGACY_PIXELREF_API // 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) { - *const_cast(&fInfo) = info; - 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 + validate_pixels_ctable(info, ctable.get()); -// 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); + *const_cast(&fInfo) = info; fRowBytes = rowBytes; fCTable = std::move(ctable); // note: we do not change fPixels @@ -125,7 +89,6 @@ void SkPixelRef::android_only_reset(int width, int height, size_t rowBytes, // conservative, since its possible the "new" settings are the same as the old. this->notifyPixelsChanged(); } - #endif void SkPixelRef::needsNewGenID() { @@ -186,11 +149,9 @@ void SkPixelRef::notifyPixelsChanged() { this->onNotifyPixelsChanged(); } -#ifdef SK_SUPPORT_LEGACY_PIXELREF_API void SkPixelRef::changeAlphaType(SkAlphaType at) { *const_cast(&fInfo) = fInfo.makeAlphaType(at); } -#endif void SkPixelRef::setImmutable() { fMutability = kImmutable; @@ -218,3 +179,7 @@ void SkPixelRef::restoreMutability() { } void SkPixelRef::onNotifyPixelsChanged() { } + +size_t SkPixelRef::getAllocatedSizeInBytes() const { + return 0; +} diff --git a/src/core/SkSpecialSurface.cpp b/src/core/SkSpecialSurface.cpp index 9564a14..33ef97e 100644 --- a/src/core/SkSpecialSurface.cpp +++ b/src/core/SkSpecialSurface.cpp @@ -63,11 +63,12 @@ sk_sp SkSpecialSurface::makeImageSnapshot() { class SkSpecialSurface_Raster : public SkSpecialSurface_Base { public: - SkSpecialSurface_Raster(const SkImageInfo& info, - sk_sp pr, + SkSpecialSurface_Raster(sk_sp pr, const SkIRect& subset, const SkSurfaceProps* props) : INHERITED(subset, props) { + const SkImageInfo& info = pr->info(); + fBitmap.setInfo(info, info.minRowBytes()); fBitmap.setPixelRef(std::move(pr), 0, 0); @@ -92,7 +93,7 @@ private: sk_sp SkSpecialSurface::MakeFromBitmap(const SkIRect& subset, SkBitmap& bm, const SkSurfaceProps* props) { - return sk_make_sp(bm.info(), sk_ref_sp(bm.pixelRef()), subset, props); + return sk_make_sp(sk_ref_sp(bm.pixelRef()), subset, props); } sk_sp SkSpecialSurface::MakeRaster(const SkImageInfo& info, @@ -102,9 +103,9 @@ sk_sp SkSpecialSurface::MakeRaster(const SkImageInfo& info, return nullptr; } - const SkIRect subset = SkIRect::MakeWH(info.width(), info.height()); + const SkIRect subset = SkIRect::MakeWH(pr->info().width(), pr->info().height()); - return sk_make_sp(info, std::move(pr), subset, props); + return sk_make_sp(std::move(pr), subset, props); } #if SK_SUPPORT_GPU diff --git a/src/image/SkImage_Raster.cpp b/src/image/SkImage_Raster.cpp index 8884755..91de4c5 100644 --- a/src/image/SkImage_Raster.cpp +++ b/src/image/SkImage_Raster.cpp @@ -28,7 +28,7 @@ // fixes https://bug.skia.org/5096 static bool is_not_subset(const SkBitmap& bm) { SkASSERT(bm.pixelRef()); - SkISize dim = SkISize::Make(bm.pixelRef()->width(), bm.pixelRef()->height()); + SkISize dim = bm.pixelRef()->info().dimensions(); SkASSERT(dim != bm.dimensions() || bm.pixelRefOrigin().isZero()); return dim == bm.dimensions(); } diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp index c4796c4..2de1ed7 100644 --- a/src/image/SkSurface_Raster.cpp +++ b/src/image/SkSurface_Raster.cpp @@ -20,7 +20,7 @@ public: SkSurface_Raster(const SkImageInfo&, void*, size_t rb, void (*releaseProc)(void* pixels, void* context), void* context, const SkSurfaceProps*); - SkSurface_Raster(const SkImageInfo& info, sk_sp, const SkSurfaceProps*); + SkSurface_Raster(sk_sp, const SkSurfaceProps*); SkCanvas* onNewCanvas() override; sk_sp onNewSurface(const SkImageInfo&) override; @@ -108,10 +108,11 @@ SkSurface_Raster::SkSurface_Raster(const SkImageInfo& info, void* pixels, size_t fWeOwnThePixels = false; // We are "Direct" } -SkSurface_Raster::SkSurface_Raster(const SkImageInfo& info, sk_sp pr, - const SkSurfaceProps* props) - : INHERITED(pr->width(), pr->height(), props) +SkSurface_Raster::SkSurface_Raster(sk_sp pr, const SkSurfaceProps* props) + : INHERITED(pr->info().width(), pr->info().height(), props) { + const SkImageInfo& info = pr->info(); + fBitmap.setInfo(info, pr->rowBytes()); fRowBytes = pr->rowBytes(); // we track this, so that subsequent re-allocs will match fBitmap.setPixelRef(std::move(pr), 0, 0); @@ -214,5 +215,5 @@ sk_sp SkSurface::MakeRaster(const SkImageInfo& info, size_t rowBytes, if (rowBytes) { SkASSERT(pr->rowBytes() == rowBytes); } - return sk_make_sp(info, std::move(pr), props); + return sk_make_sp(std::move(pr), props); } diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp index 768277a..358ba16 100644 --- a/src/pdf/SkPDFDevice.cpp +++ b/src/pdf/SkPDFDevice.cpp @@ -106,9 +106,7 @@ static SkImageSubset make_image_subset(const SkBitmap& bitmap) { SkIRect subset = bitmap.getSubset(); SkASSERT(bitmap.pixelRef()); SkBitmap tmp; - SkImageInfo pixelRefInfo = - bitmap.info().makeWH(bitmap.pixelRef()->width(), bitmap.pixelRef()->height()); - tmp.setInfo(pixelRefInfo, bitmap.rowBytes()); + tmp.setInfo(bitmap.pixelRef()->info(), bitmap.rowBytes()); tmp.setPixelRef(sk_ref_sp(bitmap.pixelRef()), 0, 0); auto img = SkImage::MakeFromBitmap(tmp); if (img) { -- 2.7.4