From 19663e54c017499406036746e7689193aa6417e6 Mon Sep 17 00:00:00 2001 From: reed Date: Fri, 29 May 2015 18:36:07 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20of=20add=20asserts=20around=20results?= =?UTF-8?q?=20from=20requestLock=20(patchset=20#3=20id:40001=20of=20https:?= =?UTF-8?q?//codereview.chromium.or=E2=80=A6=20(patchset=20#1=20id:1=20of?= =?UTF-8?q?=20https://codereview.chromium.org/1165583005/)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reason for revert: failure in cc_unittests (need to diagnose) @@@STEP_LOG_LINE@ResourceProviderTests_ResourceProviderTest.TransferInvalidSoftware_1@ResourceProviderTests/ResourceProviderTest.TransferInvalidSoftware/1 (run #1):@@@ @@@STEP_LOG_LINE@ResourceProviderTests_ResourceProviderTest.TransferInvalidSoftware_1@[ RUN ] ResourceProviderTests/ResourceProviderTest.TransferInvalidSoftware/1@@@ @@@STEP_LOG_LINE@ResourceProviderTests_ResourceProviderTest.TransferInvalidSoftware_1@[17114:17114:0529/180822:15336467671:INFO:SkPixelRef.cpp(168)] ../../third_party/skia/src/core/SkPixelRef.cpp:168: failed assertion "pixels"@@@ Original issue's description: > add asserts around results from requestLock (patchset #3 id:40001 of https://codereview.chromium.org/1155403003/)" > > This reverts commit 3953d360417655b8000df0951699121383db45c3. > > BUG=skia: > TBR= > > Committed: https://skia.googlesource.com/skia/+/6980f5a96455c8062403c995a64b654a0e9a1319 TBR= NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia: Review URL: https://codereview.chromium.org/1151573009 --- include/core/SkPixelRef.h | 8 ++--- src/core/SkPixelRef.cpp | 68 +++++++++++++++------------------------ tests/PixelRefTest.cpp | 16 --------- 3 files changed, 28 insertions(+), 64 deletions(-) diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h index 3898269ef4..7c3156ee74 100644 --- a/include/core/SkPixelRef.h +++ b/include/core/SkPixelRef.h @@ -75,8 +75,6 @@ public: * Calling lockPixels returns a LockRec struct (on success). */ struct LockRec { - LockRec() : fPixels(NULL), fColorTable(NULL) {} - void* fPixels; SkColorTable* fColorTable; size_t fRowBytes; @@ -201,13 +199,11 @@ public: }; struct LockResult { - LockResult() : fPixels(NULL), fCTable(NULL) {} - void (*fUnlockProc)(void* ctx); void* fUnlockContext; - const void* fPixels; SkColorTable* fCTable; // should be NULL unless colortype is kIndex8 + const void* fPixels; size_t fRowBytes; SkISize fSize; @@ -349,7 +345,7 @@ private: LockRec fRec; int fLockCount; - bool lockPixelsInsideMutex(); + bool lockPixelsInsideMutex(LockRec* rec); // 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 c5aa3d130d..90e2a40ea4 100644 --- a/src/core/SkPixelRef.cpp +++ b/src/core/SkPixelRef.cpp @@ -160,22 +160,8 @@ void SkPixelRef::cloneGenID(const SkPixelRef& that) { SkASSERT(!that. genIDIsUnique()); } -static void validate_pixels_ctable(const SkImageInfo& info, const void* pixels, - const SkColorTable* ctable) { - if (info.isEmpty()) { - return; // can't require pixels if the dimensions are empty - } - SkASSERT(pixels); - if (kIndex_8_SkColorType == info.colorType()) { - SkASSERT(ctable); - } else { - SkASSERT(NULL == ctable); - } -} - void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctable) { #ifndef SK_IGNORE_PIXELREF_SETPRELOCKED - validate_pixels_ctable(fInfo, pixels, ctable); // only call me in your constructor, otherwise fLockCount tracking can get // out of sync. fRec.fPixels = pixels; @@ -186,33 +172,38 @@ void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctabl #endif } -// Increments fLockCount only on success -bool SkPixelRef::lockPixelsInsideMutex() { +bool SkPixelRef::lockPixelsInsideMutex(LockRec* rec) { 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()); - if (!this->onNewLockPixels(&fRec)) { - fRec.zero(); + + LockRec rec; + if (!this->onNewLockPixels(&rec)) { fLockCount -= 1; // we return fLockCount unchanged if we fail. return false; } + SkASSERT(!rec.isZero()); // else why did onNewLock return true? + fRec = rec; } - validate_pixels_ctable(fInfo, fRec.fPixels, fRec.fColorTable); + *rec = fRec; return true; } -// 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() { +bool SkPixelRef::lockPixels(LockRec* rec) { SkASSERT(!fPreLocked || SKPIXELREF_PRELOCKED_LOCKCOUNT == fLockCount); - if (!fPreLocked) { + if (fPreLocked) { + *rec = fRec; + return true; + } else { TRACE_EVENT_BEGIN0("skia", "SkPixelRef::lockPixelsMutex"); SkAutoMutexAcquire ac(*fMutex); TRACE_EVENT_END0("skia", "SkPixelRef::lockPixelsMutex"); SkDEBUGCODE(int oldCount = fLockCount;) - bool success = this->lockPixelsInsideMutex(); + bool success = this->lockPixelsInsideMutex(rec); // lockPixelsInsideMutex only increments the count if it succeeds. SkASSERT(oldCount + (int)success == fLockCount); @@ -220,19 +211,14 @@ bool SkPixelRef::lockPixels() { // 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(fInfo, fRec.fPixels, fRec.fColorTable); - return true; } -bool SkPixelRef::lockPixels(LockRec* rec) { - if (this->lockPixels()) { - *rec = fRec; - return true; - } - return false; +bool SkPixelRef::lockPixels() { + LockRec rec; + return this->lockPixels(&rec); } void SkPixelRef::unlockPixels() { @@ -267,14 +253,11 @@ 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); - if (!this->onRequestLock(request, result)) { - return false; - } + return this->onRequestLock(request, result); } - validate_pixels_ctable(fInfo, result->fPixels, result->fCTable); - return true; } bool SkPixelRef::lockPixelsAreWritable() const { @@ -375,15 +358,16 @@ static void unlock_legacy_result(void* ctx) { } bool SkPixelRef::onRequestLock(const LockRequest& request, LockResult* result) { - if (!this->lockPixelsInsideMutex()) { + LockRec rec; + if (!this->lockPixelsInsideMutex(&rec)) { 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->fCTable = rec.fColorTable; + result->fPixels = rec.fPixels; + result->fRowBytes = rec.fRowBytes; result->fSize.set(fInfo.width(), fInfo.height()); return true; } diff --git a/tests/PixelRefTest.cpp b/tests/PixelRefTest.cpp index 30575ba8d5..e13d0e07e5 100644 --- a/tests/PixelRefTest.cpp +++ b/tests/PixelRefTest.cpp @@ -1,22 +1,8 @@ -/* - * Copyright 2015 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - #include "Test.h" #include "SkMallocPixelRef.h" #include "SkPixelRef.h" -static void test_install(skiatest::Reporter* reporter) { - SkImageInfo info = SkImageInfo::MakeN32Premul(0, 0); - SkBitmap bm; - // make sure we don't assert on an empty install - bm.installPixels(info, NULL, 0); -} - class TestListener : public SkPixelRef::GenIDChangeListener { public: explicit TestListener(int* ptr) : fPtr(ptr) {} @@ -57,6 +43,4 @@ DEF_TEST(PixelRef_GenIDChange, r) { REPORTER_ASSERT(r, 0 != pixelRef->getGenerationID()); pixelRef->addGenIDChangeListener(NULL); pixelRef->notifyPixelsChanged(); - - test_install(r); }