Allow SkLazyPixelRef to use SkScaledImageCache

src/lazy/SkLazyPixelRef.cpp

 /*
  * Copyright 2012 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "Sk64.h"
 #include "SkLazyPixelRef.h"
 #include "SkColorTable.h"
 #include "SkData.h"
 #include "SkImageCache.h"
 #include "SkImagePriv.h"
+#include "SkScaledImageCache.h"
 
 #if LAZY_CACHE_STATS
 #include "SkThread.h"
 
 int32_t SkLazyPixelRef::gCacheHits;
 int32_t SkLazyPixelRef::gCacheMisses;
 #endif
 
 SkLazyPixelRef::SkLazyPixelRef(SkData* data, SkBitmapFactory::DecodeProc proc, SkImageCache* cache)
     // Pass NULL for the Mutex so that the default (ring buffer) will be used.
     : INHERITED(NULL)
+    , fErrorInDecoding(false)
     , fDecodeProc(proc)
     , fImageCache(cache)
-    , fCacheId(SkImageCache::UNINITIALIZED_ID)
-    , fRowBytes(0) {
+    , fCacheId(0)

[scroggo, 2013/10/23 15:27:49]

Could you add a comment or an assert that 0 is an appropriate number whether
we're using SkImageCache or SkScaledImageCache?

It might be nice to even define a constant local to this file so we're not
comparing against 0.

[hal.canary, 2013/10/23 16:11:52]

Done.

+    , fRowBytes(0)
+    , fAllocator(NULL) {
     SkASSERT(fDecodeProc != NULL);
     if (NULL == data) {
         fData = SkData::NewEmpty();
         fErrorInDecoding = true;
     } else {
         fData = data;
         fData->ref();
         fErrorInDecoding = data->size() == 0;
     }
-    SkASSERT(cache != NULL);
-    cache->ref();
+    if (cache != NULL) {

[mtklein, 2013/10/23 18:50:14]

I'd slightly prefer it if you started calling it fImageCache once you
initialized it.  It's easier to see that it's the same thing we talk about below
when it's got the same name.

Given that this NULL value has special meaning, I kind of like the explicit if. 
It calls it out as special rather than hiding it in
fImageCache(SkSafeRef(cache)) or something.

[hal.canary, 2013/10/23 22:57:41]

Done.

+      cache->ref();

[scroggo, 2013/10/23 15:27:49]

nit: 4 spaces

You could also use SkSafeRef, which checks against NULL. Not sure whether that's
preferred, but it means you could use it in the initialization.

[hal.canary, 2013/10/23 16:11:52]

Done.

+    }  // else use global SkScaledImageCache
 
     // mark as uninitialized -- all fields are -1
     memset(&fLazilyCachedInfo, 0xFF, sizeof(fLazilyCachedInfo));
 
     // Since this pixel ref bases its data on encoded data, it should never change.
     this->setImmutable();
 }
 
 SkLazyPixelRef::~SkLazyPixelRef() {
     SkASSERT(fData != NULL);
     fData->unref();
+    if (fImageCache == NULL) {

[scroggo, 2013/10/23 15:27:49]

nit: NULL == fImageCache

[hal.canary, 2013/10/23 16:11:52]

Done.

+        if (fCacheId != 0) {
+            SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId));

[mtklein, 2013/10/23 18:50:14]

This sort of cast makes me nervous.  Can we make fCacheId a union to make it
clear which types of pointers we allow?  I guess, if (fImageCache == NULL),
SkScaledImageCache::ID, otherwise SkImageCache::ID?

[hal.canary, 2013/10/23 22:57:41]

Can you show me what that would look like?  I don't want to put too many
implementation details in the header.

[mtklein, 2013/10/24 16:52:12]

In the header, replace this:
  // fCacheId is a (SkScaledImageCache::ID*) or a (SkImageCache::ID).
  intptr_t                    fCacheId;
with something like this:
  union {
    SkImageCache::ID        fCacheID;        // fImageCache != NULL
    SkScaledImageCache::ID* fScaledCacheID;  // fImageCache == NULL
  };

(I hope that spacing looks right :P )

+        }
+        return;
+    }
     SkASSERT(fImageCache);
     if (fCacheId != SkImageCache::UNINITIALIZED_ID) {
         fImageCache->throwAwayCache(fCacheId);
     }
     fImageCache->unref();
 }
 
 static size_t ComputeMinRowBytesAndSize(const SkImage::Info& info, size_t* rowBytes) {
     *rowBytes = SkImageMinRowBytes(info);
 
@@ skipping 11 matching lines @@
         SkImage::Info info;
         fErrorInDecoding = !fDecodeProc(fData->data(), fData->size(), &info, NULL);
         if (fErrorInDecoding) {
             return NULL;
         }
         fLazilyCachedInfo = info;
     }
     return &fLazilyCachedInfo;
 }
 
+/**
+   Returns bitmap->getPixels() on success; NULL on failure */
+static void * decode_into_bitmap(SkImage::Info * info,

[mtklein, 2013/10/23 18:50:14]

These *'s look wishy washy!  Pick a side!  (Choose the correct side, the side by
the TYPE.)

[hal.canary, 2013/10/23 22:57:41]

Done.

+                                 SkBitmapFactory::DecodeProc decodeProc,
+                                 size_t * rowBytes,
+                                 SkData * data,
+                                 SkBitmap::Allocator * allocator,
+                                 SkBitmap * bm) {

[scroggo, 2013/10/23 15:27:49]

Maybe assert that none of these parameters are NULL?

[hal.canary, 2013/10/23 16:11:52]

Done.

+    if (!(bm->setConfig(SkImageInfoToBitmapConfig(*info), info->fWidth,
+                        info->fHeight, *rowBytes, info->fAlphaType)
+          && bm->allocPixels(allocator, NULL))) {
+        return NULL;
+    }
+    SkBitmapFactory::Target target;
+    target.fAddr = bm->getPixels();
+    target.fRowBytes = bm->rowBytes();
+    *rowBytes = target.fRowBytes;
+    if (!decodeProc(data->data(), data->size(), info, &target)) {
+        return NULL;
+    }
+    return target.fAddr;
+}
+
 void* SkLazyPixelRef::onLockPixels(SkColorTable**) {
     if (fErrorInDecoding) {
         return NULL;
     }
+    if (NULL == fImageCache) {

[mtklein, 2013/10/23 18:50:14]

Usually when the nesting gets this deep I think it means we want more functions
that do things less conditionally.

E.g. 

if (NULL == fImageCache) {
  return lockScaledImageCachePixels(...);
} else {
  ...
}

I trust myself to understand a single if statement, two nested make me think too
hard, and three practically guarantees I'm going to get lost.
Trust the inliner.  Use small functions.

+        SkBitmap bitmap;
+        if (fLazilyCachedInfo.fWidth > 0) {  // fInfo already populated

[mtklein, 2013/10/23 18:50:14]

Sometimes a little private inline method can be helpful for readability, e.g.

private:
  inline bool imageInfoPopulated() const { return fLazilyCachedInfo.fWidth > 0;
}

I often find that when I write a comment like this it's a sign I want better
names.

[mtklein, 2013/10/23 18:50:14]

We've got about three levels of code jammed into one block here.  I'd like it if
this hypothetical lockScaledImageCachePixels() method, corresponding to this
block, looked like this:

if (!this->imageInfoPopulated()) {
  this->populateImageInfo(...);
}

void* pixels = this->findInScaledImageCache(...);
if (pixels) {
  return pixels;
}

pixels = this->decodeImage(...);
if (!pixels) {
  return NULL;
}

this->addToScaledImageCache(...);
return pixels;

+            fCacheId = (intptr_t)(SkScaledImageCache::FindAndLock(
+                                           this->getGenerationID(),
+                                           fLazilyCachedInfo.fWidth,
+                                           fLazilyCachedInfo.fHeight,
+                                           SK_Scalar1, SK_Scalar1, &bitmap));
+            if (fCacheId != 0) {
+                return bitmap.getPixels();
+            }  // else cache has been purged, must re-decode.
+        } else {  // first time through this code, must populate info
+            fErrorInDecoding = !fDecodeProc(fData->data(), fData->size(),
+                                            &fLazilyCachedInfo, NULL);
+            if (fErrorInDecoding) {
+                return NULL;
+            }
+        }
+        SkASSERT(fLazilyCachedInfo.fWidth > 0);
+        void * ptr = decode_into_bitmap(&fLazilyCachedInfo, fDecodeProc,
+                                        &fRowBytes, fData, fAllocator, &bitmap);
+        if (NULL == ptr) {
+            fErrorInDecoding = true;
+            return NULL;
+        }
+        fCacheId = (intptr_t)(SkScaledImageCache::AddAndLock(
+                                           this->getGenerationID(),
+                                           fLazilyCachedInfo.fWidth,
+                                           fLazilyCachedInfo.fHeight,
+                                           SK_Scalar1, SK_Scalar1, bitmap));
+        return ptr;
+    }
+    // else use fImageCache
     SkBitmapFactory::Target target;
     // Check to see if the pixels still exist in the cache.
     if (SkImageCache::UNINITIALIZED_ID == fCacheId) {
         target.fAddr = NULL;
     } else {
         SkImageCache::DataStatus status;
         target.fAddr = fImageCache->pinCache(fCacheId, &status);
         if (target.fAddr == NULL) {
             fCacheId = SkImageCache::UNINITIALIZED_ID;
         } else {
@@ skipping 41 matching lines @@
         fImageCache->throwAwayCache(fCacheId);
         fCacheId = SkImageCache::UNINITIALIZED_ID;
         return NULL;
     }
     // Upon success, store fRowBytes so it can be used in case pinCache later returns purged memory.
     fRowBytes = target.fRowBytes;
     return target.fAddr;
 }
 
 void SkLazyPixelRef::onUnlockPixels() {
-    if (fErrorInDecoding) {
+    if (fErrorInDecoding || (0 == fCacheId)) {
         return;
     }
-    if (fCacheId != SkImageCache::UNINITIALIZED_ID) {
+    if (NULL == fImageCache) {

[mtklein, 2013/10/23 18:50:14]

This guy is small enough that even I'd probably leave it all in one place.  You
can't really get lost in here like you can in the jungle of lockPixels.

+        SkScaledImageCache::Unlock((SkScaledImageCache::ID *)(fCacheId));
+        fCacheId = 0;
+        return;
+    } else {  // use fImageCache
         fImageCache->releaseCache(fCacheId);
     }
 }
 
 SkData* SkLazyPixelRef::onRefEncodedData() {
     fData->ref();
     return fData;
 }
 
 #include "SkImagePriv.h"
@@ skipping 38 matching lines @@
 
     target.fAddr = tmp.getPixels();
     fErrorInDecoding = !fDecodeProc(fData->data(), fData->size(), &info, &target);
     if (fErrorInDecoding) {
         return false;
     }
 
     *bitmap = tmp;
     return true;
 }