From 818580d81b4a80071b5150e0febdf2fbe3826413 Mon Sep 17 00:00:00 2001 From: Mike Reed Date: Thu, 13 Apr 2017 16:02:22 -0400 Subject: [PATCH] remove code directly involved in lock/unlock in SkPixelRef Bug: skia:6481 Change-Id: I3c301ad42b082e04e233aa32d77862036fe998fa Reviewed-on: https://skia-review.googlesource.com/13463 Reviewed-by: Matt Sarett Commit-Queue: Mike Reed --- gn/android_framework_defines.gni | 2 - include/core/SkMallocPixelRef.h | 4 - include/core/SkPixelRef.h | 72 +---------------- src/core/SkBitmap.cpp | 3 - src/core/SkMallocPixelRef.cpp | 11 --- src/core/SkPixelRef.cpp | 163 +++------------------------------------ 6 files changed, 12 insertions(+), 243 deletions(-) diff --git a/gn/android_framework_defines.gni b/gn/android_framework_defines.gni index 3447efe..67c6a2d 100644 --- a/gn/android_framework_defines.gni +++ b/gn/android_framework_defines.gni @@ -4,8 +4,6 @@ # found in the LICENSE file. android_framework_defines = [ - "SK_SUPPORT_LEGACY_UNBALANCED_PIXELREF_LOCKCOUNT", - # Needed until we fix https://bug.skia.org/2440 . "SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG", "SK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS", diff --git a/include/core/SkMallocPixelRef.h b/include/core/SkMallocPixelRef.h index a62da0e..e378da7 100644 --- a/include/core/SkMallocPixelRef.h +++ b/include/core/SkMallocPixelRef.h @@ -101,10 +101,6 @@ public: protected: ~SkMallocPixelRef() override; -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF - bool onNewLockPixels(LockRec*) override; - void onUnlockPixels() override; -#endif size_t getAllocatedSizeInBytes() const override; private: diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h index 97aca19..3a3d8b5 100644 --- a/include/core/SkPixelRef.h +++ b/include/core/SkPixelRef.h @@ -27,17 +27,11 @@ class SkDiscardableMemory; /** \class SkPixelRef - This class is the smart container for pixel memory, and is used with - SkBitmap. A pixelref is installed into a bitmap, and then the bitmap can - access the actual pixel memory by calling lockPixels/unlockPixels. - + This class is the smart container for pixel memory, and is used with SkBitmap. This class can be shared/accessed between multiple threads. */ class SK_API SkPixelRef : public SkRefCnt { public: -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF - explicit SkPixelRef(const SkImageInfo&); -#endif explicit SkPixelRef(const SkImageInfo&, void* addr, size_t rowBytes, sk_sp = nullptr); virtual ~SkPixelRef(); @@ -46,15 +40,8 @@ public: return fInfo; } - /** Return the pixel memory returned from lockPixels, or null if the - lockCount is 0. - */ void* pixels() const { return fRec.fPixels; } - - /** Return the current colorTable (if any) if pixels are locked, or null. - */ SkColorTable* colorTable() const { return fRec.fColorTable; } - size_t rowBytes() const { return fRec.fRowBytes; } /** @@ -75,14 +62,8 @@ public: } }; - SkDEBUGCODE(bool isLocked() const { return fLockCount > 0; }) - SkDEBUGCODE(int getLockCount() const { return fLockCount; }) - - /** - * Call to access the pixel memory. Return true on success. Balance this - * with a call to unlockPixels(). - */ - bool lockPixels(); + bool lockPixels() { return true; } + void unlockPixels() {} /** * Call to access the pixel memory. On success, return true and fill out @@ -91,12 +72,6 @@ public: */ bool lockPixels(LockRec* rec); - /** Call to balanace a previous call to lockPixels(). Returns the pixels - (or null) after the unlock. NOTE: lock calls can be nested, but the - matching number of unlock calls must be made in order to free the - memory (if the subclass implements caching/deferred-decoding.) - */ - void unlockPixels(); /** Returns a non-zero, unique value corresponding to the pixels in this pixelref. Each time the pixels are changed (and notifyPixelsChanged is @@ -192,36 +167,6 @@ public: virtual SkDiscardableMemory* diagnostic_only_getDiscardable() const { return NULL; } protected: -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF - virtual -#endif - /** - * On success, returns true and fills out the LockRec for the pixels. On - * failure returns false and ignores the LockRec parameter. - * - * The caller will have already acquired a mutex for thread safety, so this - * method need not do that. - */ - bool onNewLockPixels(LockRec*) { - SkASSERT(false); // should never be called - return true; - } - -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF - virtual -#endif - /** - * Balancing the previous successful call to onNewLockPixels. The locked - * pixel address will no longer be referenced, so the subclass is free to - * move or discard that memory. - * - * The caller will have already acquired a mutex for thread safety, so this - * method need not do that. - */ - void onUnlockPixels() { - SkASSERT(false); // should never be called - } - // default impl does nothing. virtual void onNotifyPixelsChanged(); @@ -245,13 +190,6 @@ protected: */ SkBaseMutex* mutex() const { return &fMutex; } -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF - // only call from constructor. Flags this to always be locked, removing - // the need to grab the mutex and call onLockPixels/onUnlockPixels. - // Performance tweak to avoid those calls (esp. in multi-thread use case). - void setPreLocked(void*, size_t rowBytes, SkColorTable*); -#endif - private: mutable SkMutex fMutex; @@ -261,10 +199,6 @@ private: // LockRec is only valid if we're in a locked state (isLocked()) LockRec fRec; - int fLockCount; - - bool lockPixelsInsideMutex(); - bool internalRequestLock(const LockRequest&, LockResult*); // 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 a06d133..e18f429 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -197,8 +197,6 @@ bool SkBitmap::setAlphaType(SkAlphaType newAlphaType) { void SkBitmap::updatePixelsFromRef() const { if (fPixelRef) { if (fPixelLockCount > 0) { - SkASSERT(fPixelRef->isLocked()); - void* p = fPixelRef->pixels(); if (p) { p = (char*)p @@ -1018,7 +1016,6 @@ void SkBitmap::validate() const { if (fPixels) { SkASSERT(fPixelRef); SkASSERT(fPixelLockCount > 0); - SkASSERT(fPixelRef->isLocked()); SkASSERT(fPixelRef->rowBytes() == fRowBytes); SkASSERT(fPixelRefOrigin.fX >= 0); SkASSERT(fPixelRefOrigin.fY >= 0); diff --git a/src/core/SkMallocPixelRef.cpp b/src/core/SkMallocPixelRef.cpp index ed3a97b..f49c1ca 100644 --- a/src/core/SkMallocPixelRef.cpp +++ b/src/core/SkMallocPixelRef.cpp @@ -168,17 +168,6 @@ SkMallocPixelRef::~SkMallocPixelRef() { } } -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF -bool SkMallocPixelRef::onNewLockPixels(LockRec* rec) { - sk_throw(); // should never get here - return true; -} - -void SkMallocPixelRef::onUnlockPixels() { - // nothing to do -} -#endif - size_t SkMallocPixelRef::getAllocatedSizeInBytes() const { return this->info().getSafeSize(this->rowBytes()); } diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp index 1b14e26..37a450d 100644 --- a/src/core/SkPixelRef.cpp +++ b/src/core/SkPixelRef.cpp @@ -10,7 +10,6 @@ #include "SkPixelRef.h" #include "SkTraceEvent.h" -//#define SK_SUPPORT_LEGACY_UNBALANCED_PIXELREF_LOCKCOUNT //#define SK_TRACE_PIXELREF_LIFETIME #include "SkNextID.h" @@ -51,26 +50,6 @@ static void validate_pixels_ctable(const SkImageInfo& info, const SkColorTable* static int32_t gInstCounter; #endif -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF -SkPixelRef::SkPixelRef(const SkImageInfo& info) - : fInfo(validate_info(info)) -#ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK - , fStableID(SkNextID::ImageID()) -#endif - -{ -#ifdef SK_TRACE_PIXELREF_LIFETIME - SkDebugf(" pixelref %d\n", sk_atomic_inc(&gInstCounter)); -#endif - fRec.zero(); - fLockCount = 0; - this->needsNewGenID(); - fMutability = kMutable; - fPreLocked = false; - fAddedToCache.store(false); -} -#endif - SkPixelRef::SkPixelRef(const SkImageInfo& info, void* pixels, size_t rowBytes, sk_sp ctable) : fInfo(validate_info(info)) @@ -88,7 +67,6 @@ SkPixelRef::SkPixelRef(const SkImageInfo& info, void* pixels, size_t rowBytes, fRec.fRowBytes = rowBytes; fRec.fColorTable = fCTable.get(); - fLockCount = SKPIXELREF_PRELOCKED_LOCKCOUNT; this->needsNewGenID(); fMutability = kMutable; fPreLocked = true; @@ -96,10 +74,6 @@ SkPixelRef::SkPixelRef(const SkImageInfo& info, void* pixels, size_t rowBytes, } SkPixelRef::~SkPixelRef() { -#ifndef SK_SUPPORT_LEGACY_UNBALANCED_PIXELREF_LOCKCOUNT - SkASSERT(SKPIXELREF_PRELOCKED_LOCKCOUNT == fLockCount || 0 == fLockCount); -#endif - #ifdef SK_TRACE_PIXELREF_LIFETIME SkDebugf("~pixelref %d\n", sk_atomic_dec(&gInstCounter) - 1); #endif @@ -142,94 +116,9 @@ void SkPixelRef::cloneGenID(const SkPixelRef& that) { SkASSERT(!that. genIDIsUnique()); } -#ifdef SK_SUPPORT_LEGACY_NO_ADDR_PIXELREF -void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctable) { - SkASSERT(pixels); - validate_pixels_ctable(fInfo, ctable); - // only call me in your constructor, otherwise fLockCount tracking can get - // out of sync. - fRec.fPixels = pixels; - fRec.fColorTable = ctable; - fRec.fRowBytes = rowBytes; - fLockCount = SKPIXELREF_PRELOCKED_LOCKCOUNT; - fPreLocked = true; -} -#endif - -// Increments fLockCount only on success -bool SkPixelRef::lockPixelsInsideMutex() { - fMutex.assertHeld(); - - if (1 == ++fLockCount) { - SkASSERT(fRec.isZero()); - if (!this->onNewLockPixels(&fRec)) { - fRec.zero(); - fLockCount -= 1; // we return fLockCount unchanged if we fail. - return false; - } - } - if (fRec.fPixels) { - validate_pixels_ctable(fInfo, fRec.fColorTable); - return true; - } - // no pixels, so we failed (somehow) - --fLockCount; - return false; -} - -// For historical reasons, we always inc fLockCount, even if we return false. -// It would be nice to change this (it seems), and only inc if we actually succeed... -bool SkPixelRef::lockPixels() { - SkASSERT(!fPreLocked || SKPIXELREF_PRELOCKED_LOCKCOUNT == fLockCount); - - if (!fPreLocked) { - TRACE_EVENT_BEGIN0("skia", "SkPixelRef::lockPixelsMutex"); - SkAutoMutexAcquire ac(fMutex); - TRACE_EVENT_END0("skia", "SkPixelRef::lockPixelsMutex"); - SkDEBUGCODE(int oldCount = fLockCount;) - bool success = this->lockPixelsInsideMutex(); - // lockPixelsInsideMutex only increments the count if it succeeds. - SkASSERT(oldCount + (int)success == fLockCount); - - if (!success) { - // For compatibility with SkBitmap calling lockPixels, we still want to increment - // fLockCount even if we failed. If we updated SkBitmap we could remove this oddity. - fLockCount += 1; - return false; - } - } - if (fRec.fPixels) { - validate_pixels_ctable(fInfo, fRec.fColorTable); - return true; - } - return false; -} - bool SkPixelRef::lockPixels(LockRec* rec) { - if (this->lockPixels()) { - *rec = fRec; - return true; - } - return false; -} - -void SkPixelRef::unlockPixels() { - SkASSERT(!fPreLocked || SKPIXELREF_PRELOCKED_LOCKCOUNT == fLockCount); - - if (!fPreLocked) { - SkAutoMutexAcquire ac(fMutex); - - SkASSERT(fLockCount > 0); - if (0 == --fLockCount) { - // don't call onUnlockPixels unless onLockPixels succeeded - if (fRec.fPixels) { - this->onUnlockPixels(); - fRec.zero(); - } else { - SkASSERT(fRec.isZero()); - } - } - } + *rec = fRec; + return true; } bool SkPixelRef::requestLock(const LockRequest& request, LockResult* result) { @@ -242,24 +131,13 @@ bool SkPixelRef::requestLock(const LockRequest& request, LockResult* result) { return false; } - if (fPreLocked) { - result->fUnlockProc = nullptr; - result->fUnlockContext = nullptr; - result->fCTable = fRec.fColorTable; - result->fPixels = fRec.fPixels; - result->fRowBytes = fRec.fRowBytes; - result->fSize.set(fInfo.width(), fInfo.height()); - } else { - SkAutoMutexAcquire ac(fMutex); - if (!this->internalRequestLock(request, result)) { - return false; - } - } - if (result->fPixels) { - validate_pixels_ctable(fInfo, result->fCTable); - return true; - } - return false; + result->fUnlockProc = nullptr; + result->fUnlockContext = nullptr; + result->fCTable = fRec.fColorTable; + result->fPixels = fRec.fPixels; + result->fRowBytes = fRec.fRowBytes; + result->fSize.set(fInfo.width(), fInfo.height()); + return true; } uint32_t SkPixelRef::getGenerationID() const { @@ -344,31 +222,8 @@ void SkPixelRef::restoreMutability() { fMutability = kMutable; } -/////////////////////////////////////////////////////////////////////////////////////////////////// - - void SkPixelRef::onNotifyPixelsChanged() { } size_t SkPixelRef::getAllocatedSizeInBytes() const { return 0; } - -static void unlock_legacy_result(void* ctx) { - SkPixelRef* pr = (SkPixelRef*)ctx; - pr->unlockPixels(); - pr->unref(); // balancing the Ref in onRequestLoc -} - -bool SkPixelRef::internalRequestLock(const LockRequest& request, LockResult* result) { - if (!this->lockPixelsInsideMutex()) { - return false; - } - - result->fUnlockProc = unlock_legacy_result; - result->fUnlockContext = SkRef(this); // this is balanced in our fUnlockProc - result->fCTable = fRec.fColorTable; - result->fPixels = fRec.fPixels; - result->fRowBytes = fRec.fRowBytes; - result->fSize.set(fInfo.width(), fInfo.height()); - return true; -} -- 2.7.4