Allow SkLazyPixelRef to use SkScaledImageCache

src/lazy/SkLazyPixelRef.cpp

Index: src/lazy/SkLazyPixelRef.cpp
diff --git a/src/lazy/SkLazyPixelRef.cpp b/src/lazy/SkLazyPixelRef.cpp
index 0454362b942f4e5ceb1aa5ece04270caa3e8f710..27285f137cb46f2486440b96e956112755993e59 100644
--- a/src/lazy/SkLazyPixelRef.cpp
+++ b/src/lazy/SkLazyPixelRef.cpp
@@ -11,6 +11,7 @@
 #include "SkData.h"
 #include "SkImageCache.h"
 #include "SkImagePriv.h"
+#include "SkScaledImageCache.h"
 
 #if LAZY_CACHE_STATS
 #include "SkThread.h"
@@ -22,10 +23,12 @@ int32_t SkLazyPixelRef::gCacheMisses;
 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();
@@ -35,8 +38,9 @@ SkLazyPixelRef::SkLazyPixelRef(SkData* data, SkBitmapFactory::DecodeProc proc, S
         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));
@@ -48,6 +52,12 @@ SkLazyPixelRef::SkLazyPixelRef(SkData* data, SkBitmapFactory::DecodeProc proc, S
 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);
@@ -79,10 +89,66 @@ const SkImage::Info* SkLazyPixelRef::getCachedInfo() {
     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) {
@@ -144,10 +210,14 @@ void* SkLazyPixelRef::onLockPixels(SkColorTable**) {
 }
 
 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);
     }
 }