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

src/core/SkResourceCache.cpp

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkChecksum.h"
 #include "SkMessageBus.h"
 #include "SkMipMap.h"
@@ skipping 61 matching lines @@
     fTotalByteLimit = 0;
     fDiscardableFactory = nullptr;
 }
 
 #include "SkDiscardableMemory.h"
 
 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:
     bool onNewLockPixels(LockRec*) override;
     void onUnlockPixels() override;
     size_t getAllocatedSizeInBytes() const override;
 
     SkDiscardableMemory* diagnostic_only_getDiscardable() const override { return fDM; }
 
 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) {
         // we're already locked
         SkASSERT(fDM->data());
         fFirstTime = false;
         goto SUCCESS;
     }
 
     // A previous call to onUnlock may have deleted our DM, so check for that
     if (nullptr == fDM) {
         return false;
     }
 
     if (!fDM->lock()) {
         // since it failed, we delete it now, to free-up the resource
         delete fDM;
         fDM = nullptr;
         return false;
     }
 
 SUCCESS:
     rec->fPixels = fDM->data();
-    rec->fColorTable = nullptr;
+    rec->fColorTable = fCTable;
     rec->fRowBytes = fRB;
     return true;
 }
 
 void SkOneShotDiscardablePixelRef::onUnlockPixels() {
     SkASSERT(!fFirstTime);
     fDM->unlock();
 }
 
 size_t SkOneShotDiscardablePixelRef::getAllocatedSizeInBytes() const {
@@ skipping 18 matching lines @@
     uint64_t size64 = bitmap->computeSize64();
     if (0 == size || size64 > (uint64_t)size) {
         return false;
     }
 
     SkDiscardableMemory* dm = fFactory(size);
     if (nullptr == dm) {
         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();
 }
 
 SkResourceCache::SkResourceCache(DiscardableFactory factory) {
     this->init();
     fDiscardableFactory = factory;
 
     fAllocator = new SkResourceCacheDiscardableAllocator(factory);
 }
@@ skipping 496 matching lines @@
         dump->dumpNumericValue(dumpName.c_str(), "size", "bytes", rec.bytesUsed());
         dump->setMemoryBacking(dumpName.c_str(), "malloc", nullptr);
     }
 }
 
 void SkResourceCache::DumpMemoryStatistics(SkTraceMemoryDump* dump) {
     // Since resource could be backed by malloc or discardable, the cache always dumps detailed
     // stats to be accurate.
     VisitAll(sk_trace_dump_visitor, dump);
 }