SkResourceCache::GetAllocator() index8 and other color types handling

src/core/SkResourceCache.cpp

Index: src/core/SkResourceCache.cpp
diff --git a/src/core/SkResourceCache.cpp b/src/core/SkResourceCache.cpp
index 240d1977e53968d417de449d84ff42c8e34f405e..d51cf927d4dbce3741464a7b14459f194c1e7516 100644
--- a/src/core/SkResourceCache.cpp
+++ b/src/core/SkResourceCache.cpp
@@ -79,7 +79,7 @@ class SkOneShotDiscardablePixelRef : public SkPixelRef {
 public:
 
     // Ownership of the discardablememory is transfered to the pixelref
-    SkOneShotDiscardablePixelRef(const SkImageInfo&, SkDiscardableMemory*, size_t rowBytes);
+    SkOneShotDiscardablePixelRef(const SkImageInfo&, SkDiscardableMemory*, size_t rowBytes, SkColorTable* ctable);
     ~SkOneShotDiscardablePixelRef();
 
 protected:
@@ -93,22 +93,29 @@ private:
     SkDiscardableMemory* fDM;
     size_t               fRB;
     bool                 fFirstTime;
+    SkColorTable*        fCTable;
 
     typedef SkPixelRef INHERITED;
 };
 
 SkOneShotDiscardablePixelRef::SkOneShotDiscardablePixelRef(const SkImageInfo& info,
                                              SkDiscardableMemory* dm,
-                                             size_t rowBytes)
+                                             size_t rowBytes,
+                                             SkColorTable* ctable)
     : INHERITED(info)
     , fDM(dm)
     , fRB(rowBytes)
+    , fCTable(ctable)
 {
     SkASSERT(dm->data());
     fFirstTime = true;
+    SkSafeRef(ctable);
 }
 
-SkOneShotDiscardablePixelRef::~SkOneShotDiscardablePixelRef() { delete fDM; }
+SkOneShotDiscardablePixelRef::~SkOneShotDiscardablePixelRef() {
+    delete fDM;
+    SkSafeUnref(fCTable);
+}
 
 bool SkOneShotDiscardablePixelRef::onNewLockPixels(LockRec* rec) {
     if (fFirstTime) {
@@ -132,7 +139,7 @@ bool SkOneShotDiscardablePixelRef::onNewLockPixels(LockRec* rec) {
 
 SUCCESS:
     rec->fPixels = fDM->data();
-    rec->fColorTable = nullptr;
+    rec->fColorTable = fCTable;
     rec->fRowBytes = fRB;
     return true;
 }
@@ -171,13 +178,20 @@ bool SkResourceCacheDiscardableAllocator::allocPixelRef(SkBitmap* bitmap, SkColo
         return false;
     }
 
-    // can we relax this?
-    if (kN32_SkColorType != bitmap->colorType()) {
+    if (kIndex_8_SkColorType == bitmap->colorType()) {
+        if (ctable == nullptr) {

[reed1, 2015/11/09 14:14:19]

1. nit: skia convention is either

    if (!ctable) {
or
    if (nullptr == ctable) {

2. if we do return false, we will leak the discardable memory (allocated above).
I know the old code had a similar problem/leak. Can we perform this check before
we call the fFactory?

[aleksandar.stojiljkovic, 2015/11/09 15:28:00]

Good point. Fixing.

+            return false;
+        }
+    } else {
+        ctable = nullptr;
+    }
+
+    if ((unsigned)bitmap->colorType() > (unsigned)kLastEnum_SkColorType) {

[reed1, 2015/11/09 14:14:18]

What is this actually testing, now that we're not restricted to just 32bit
types?

[aleksandar.stojiljkovic, 2015/11/09 15:28:00]

Argh, I was some time ago, as customer, misusing color types to plug GPU texture
compression decoders. Similar check's there in SkMallocPixelRef.cpp is_valid().
This is internal code, not an API like SkMallocPixelRef and makes sense not to
have it. 

         return false;
     }
 
     SkImageInfo info = bitmap->info();
-    bitmap->setPixelRef(new SkOneShotDiscardablePixelRef(info, dm, bitmap->rowBytes()))->unref();
+    bitmap->setPixelRef(new SkOneShotDiscardablePixelRef(info, dm, bitmap->rowBytes(), ctable))->unref();
     bitmap->lockPixels();
     return bitmap->readyToDraw();
 }