Revert[4] of add asserts around results from requestLock

This reverts commit 19663e54c0.

BUG=skia:
TBR=

Review URL: https://codereview.chromium.org/1159733006

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); }

src/core/SkBitmap.cpp

@@ -344,6 +344,9 @@ bool SkBitmap::installPixels(const SkImageInfo& requestedInfo, void* pixels, siz
         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();

src/core/SkPixelRef.cpp

@@ -160,8 +160,22 @@ 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;
@@ -172,38 +186,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(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 @@ 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(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 @@ 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(fInfo, result->fPixels, result->fCTable);
+    return true;
 }
 
 bool SkPixelRef::lockPixelsAreWritable() const {
@@ -358,16 +375,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;
 }

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 @@ DEF_TEST(PixelRef_GenIDChange, r) {
     REPORTER_ASSERT(r, 0 != pixelRef->getGenerationID());
     pixelRef->addGenIDChangeListener(NULL);
     pixelRef->notifyPixelsChanged();
+
+    test_install(r);
 }