diff options
author | reed <reed@google.com> | 2015-05-29 11:39:14 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-05-29 11:39:14 -0700 |
commit | f941a68126d8fe647eaea902c244c466568b7809 (patch) | |
tree | cc3888f1178caeb1a7672d2ee190a5f85f9f5f1d | |
parent | 993a4216a6014b9de8f4d8120360c94550dc6761 (diff) |
add asserts around results from requestLock and lockPixels, ensuring that true always means we have non-null pixels (and non-null colortable if that matches the colortype)
BUG= 491975
TBR=
Review URL: https://codereview.chromium.org/1155403003
-rw-r--r-- | include/core/SkPixelRef.h | 8 | ||||
-rw-r--r-- | src/core/SkPixelRef.cpp | 64 |
2 files changed, 44 insertions, 28 deletions
diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h index 7c3156ee74..3898269ef4 100644 --- a/include/core/SkPixelRef.h +++ b/include/core/SkPixelRef.h @@ -75,6 +75,8 @@ public: * Calling lockPixels returns a LockRec struct (on success). */ struct LockRec { + LockRec() : fPixels(NULL), fColorTable(NULL) {} + void* fPixels; SkColorTable* fColorTable; size_t fRowBytes; @@ -199,11 +201,13 @@ public: }; struct LockResult { + LockResult() : fPixels(NULL), fCTable(NULL) {} + void (*fUnlockProc)(void* ctx); void* fUnlockContext; - SkColorTable* fCTable; // should be NULL unless colortype is kIndex8 const void* fPixels; + SkColorTable* fCTable; // should be NULL unless colortype is kIndex8 size_t fRowBytes; SkISize fSize; @@ -345,7 +349,7 @@ private: LockRec fRec; int fLockCount; - bool lockPixelsInsideMutex(LockRec* rec); + bool lockPixelsInsideMutex(); // Bottom bit indicates the Gen ID is unique. bool genIDIsUnique() const { return SkToBool(fTaggedGenID.load() & 1); } diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp index 90e2a40ea4..1fda14b025 100644 --- a/src/core/SkPixelRef.cpp +++ b/src/core/SkPixelRef.cpp @@ -160,8 +160,18 @@ void SkPixelRef::cloneGenID(const SkPixelRef& that) { SkASSERT(!that. genIDIsUnique()); } +static void validate_pixels_ctable(const void* pixels, const SkColorTable* ctable, SkColorType ct) { + SkASSERT(pixels); + if (kIndex_8_SkColorType == ct) { + SkASSERT(ctable); + } else { + SkASSERT(NULL == ctable); + } +} + void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctable) { #ifndef SK_IGNORE_PIXELREF_SETPRELOCKED + validate_pixels_ctable(pixels, ctable, fInfo.colorType()); // only call me in your constructor, otherwise fLockCount tracking can get // out of sync. fRec.fPixels = pixels; @@ -172,38 +182,33 @@ void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctabl #endif } -bool SkPixelRef::lockPixelsInsideMutex(LockRec* rec) { +// Increments fLockCount only on success +bool SkPixelRef::lockPixelsInsideMutex() { fMutex->assertHeld(); - // 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... if (1 == ++fLockCount) { SkASSERT(fRec.isZero()); - - LockRec rec; - if (!this->onNewLockPixels(&rec)) { + if (!this->onNewLockPixels(&fRec)) { + fRec.zero(); fLockCount -= 1; // we return fLockCount unchanged if we fail. return false; } - SkASSERT(!rec.isZero()); // else why did onNewLock return true? - fRec = rec; } - *rec = fRec; + validate_pixels_ctable(fRec.fPixels, fRec.fColorTable, fInfo.colorType()); return true; } -bool SkPixelRef::lockPixels(LockRec* rec) { +// 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) { - *rec = fRec; - return true; - } else { + 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(rec); + bool success = this->lockPixelsInsideMutex(); // lockPixelsInsideMutex only increments the count if it succeeds. SkASSERT(oldCount + (int)success == fLockCount); @@ -211,14 +216,19 @@ bool SkPixelRef::lockPixels(LockRec* rec) { // 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; } - return success; } + validate_pixels_ctable(fRec.fPixels, fRec.fColorTable, fInfo.colorType()); + return true; } -bool SkPixelRef::lockPixels() { - LockRec rec; - return this->lockPixels(&rec); +bool SkPixelRef::lockPixels(LockRec* rec) { + if (this->lockPixels()) { + *rec = fRec; + return true; + } + return false; } void SkPixelRef::unlockPixels() { @@ -253,11 +263,14 @@ bool SkPixelRef::requestLock(const LockRequest& request, LockResult* result) { result->fPixels = fRec.fPixels; result->fRowBytes = fRec.fRowBytes; result->fSize.set(fInfo.width(), fInfo.height()); - return true; } else { SkAutoMutexAcquire ac(*fMutex); - return this->onRequestLock(request, result); + if (!this->onRequestLock(request, result)) { + return false; + } } + validate_pixels_ctable(result->fPixels, result->fCTable, fInfo.colorType()); + return true; } bool SkPixelRef::lockPixelsAreWritable() const { @@ -358,16 +371,15 @@ static void unlock_legacy_result(void* ctx) { } bool SkPixelRef::onRequestLock(const LockRequest& request, LockResult* result) { - LockRec rec; - if (!this->lockPixelsInsideMutex(&rec)) { + if (!this->lockPixelsInsideMutex()) { return false; } result->fUnlockProc = unlock_legacy_result; result->fUnlockContext = SkRef(this); // this is balanced in our fUnlockProc - result->fCTable = rec.fColorTable; - result->fPixels = rec.fPixels; - result->fRowBytes = rec.fRowBytes; + result->fCTable = fRec.fColorTable; + result->fPixels = fRec.fPixels; + result->fRowBytes = fRec.fRowBytes; result->fSize.set(fInfo.width(), fInfo.height()); return true; } |