Revert[4] of add asserts around results from requestLock
This reverts commit 19663e54c017499406036746e7689193aa6417e6.
BUG=skia:
TBR=
Review URL: https://codereview.chromium.org/1159733006
diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h
index 7c3156e..3898269 100644
--- a/include/core/SkPixelRef.h
+++ b/include/core/SkPixelRef.h
@@ -75,6 +75,8 @@
* 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 @@
};
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 @@
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/SkBitmap.cpp b/src/core/SkBitmap.cpp
index 563fc28..36e31f5 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -344,6 +344,9 @@
this->reset();
return false;
}
+ if (NULL == pixels) {
+ return true; // we behaved as if they called setInfo()
+ }
// setInfo may have corrected info (e.g. 565 is always opaque).
const SkImageInfo& correctedInfo = this->info();
diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp
index 90e2a40..c5aa3d1 100644
--- a/src/core/SkPixelRef.cpp
+++ b/src/core/SkPixelRef.cpp
@@ -160,8 +160,22 @@
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;
@@ -172,38 +186,33 @@
#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(fInfo, fRec.fPixels, fRec.fColorTable);
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 +220,19 @@
// 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;
- return this->lockPixels(&rec);
+bool SkPixelRef::lockPixels(LockRec* rec) {
+ if (this->lockPixels()) {
+ *rec = fRec;
+ return true;
+ }
+ return false;
}
void SkPixelRef::unlockPixels() {
@@ -253,11 +267,14 @@
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(fInfo, result->fPixels, result->fCTable);
+ return true;
}
bool SkPixelRef::lockPixelsAreWritable() const {
@@ -358,16 +375,15 @@
}
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;
}
diff --git a/tests/PixelRefTest.cpp b/tests/PixelRefTest.cpp
index e13d0e0..ed9ea87 100644
--- a/tests/PixelRefTest.cpp
+++ b/tests/PixelRefTest.cpp
@@ -1,8 +1,29 @@
+/*
+ * 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) {
+ bool success;
+ SkImageInfo info = SkImageInfo::MakeN32Premul(0, 0);
+ SkBitmap bm;
+ // make sure we don't assert on an empty install
+ success = bm.installPixels(info, NULL, 0);
+ REPORTER_ASSERT(reporter, success);
+
+ // no pixels should be the same as setInfo()
+ info = SkImageInfo::MakeN32Premul(10, 10);
+ success = bm.installPixels(info, NULL, 0);
+ REPORTER_ASSERT(reporter, success);
+}
+
class TestListener : public SkPixelRef::GenIDChangeListener {
public:
explicit TestListener(int* ptr) : fPtr(ptr) {}
@@ -43,4 +64,6 @@
REPORTER_ASSERT(r, 0 != pixelRef->getGenerationID());
pixelRef->addGenIDChangeListener(NULL);
pixelRef->notifyPixelsChanged();
+
+ test_install(r);
}