Use SkBitmapCache to optimize readPixels on a texture-backed bitmap

src/gpu/SkGrPixelRef.cpp

Index: src/gpu/SkGrPixelRef.cpp
diff --git a/src/gpu/SkGrPixelRef.cpp b/src/gpu/SkGrPixelRef.cpp
index f56c3b63907d202f4dd3f3b500f340c643439e92..9184e9b473e5677f5026027494b959027bb7c89c 100644
--- a/src/gpu/SkGrPixelRef.cpp
+++ b/src/gpu/SkGrPixelRef.cpp
@@ -9,8 +9,10 @@
 
 
 #include "SkGrPixelRef.h"
+
 #include "GrContext.h"
 #include "GrTexture.h"
+#include "SkBitmapCache.h"
 #include "SkGr.h"
 #include "SkRect.h"
 
@@ -143,7 +145,7 @@ SkPixelRef* SkGrPixelRef::deepCopy(SkColorType dstCT, const SkIRect* subset) {
     if (NULL == fSurface) {
         return NULL;
     }
-    
+
     // Note that when copying a render-target-backed pixel ref, we
     // return a texture-backed pixel ref instead.  This is because
     // render-target pixel refs are usually created in conjunction with
@@ -153,30 +155,65 @@ SkPixelRef* SkGrPixelRef::deepCopy(SkColorType dstCT, const SkIRect* subset) {
     return copyToTexturePixelRef(fSurface->asTexture(), dstCT, subset);
 }
 
+static bool tryAllocBitmapPixels(SkBitmap& bitmap) {

[reed1, 2014/09/04 18:49:59]

style: skia passes mutable args by pointer, not ref (except SkReadBuffer for
some reason).

[Rémi Piotaix, 2014/09/05 21:37:36]

Done.

+    SkBitmap::Allocator* allocator = SkBitmapCache::GetAllocator();
+    if (NULL != allocator) {
+        return allocator->allocPixelRef(&bitmap, 0);
+    } else {
+        // DiscardableMemory is not available, fallback to default allocator
+        return bitmap.tryAllocPixels();
+    }
+}
+
 bool SkGrPixelRef::onReadPixels(SkBitmap* dst, const SkIRect* subset) {
     if (NULL == fSurface || fSurface->wasDestroyed()) {
         return false;
     }
 
-    int left, top, width, height;
+    SkIRect bounds;
     if (NULL != subset) {
-        left = subset->fLeft;
-        width = subset->width();
-        top = subset->fTop;
-        height = subset->height();
+        bounds = *subset;
     } else {
-        left = 0;
-        width = this->info().width();
-        top = 0;
-        height = this->info().height();
+        bounds = SkIRect::MakeWH(this->info().width(), this->info().height());
     }
-    if (!dst->tryAllocN32Pixels(width, height)) {
-        SkDebugf("SkGrPixelRef::onReadPixels failed to alloc bitmap for result!\n");
-        return false;
+
+    //Check the cache
+    if(!SkBitmapCache::Find(this->getGenerationID(), bounds, dst)) {
+        //Cache miss
+
+        SkBitmap cachedBitmap;

[reed1, 2014/09/04 18:49:59]

nit: could use

this->info().makeWH(bounds.width(), bounds.height())

to make the desired info.

[Rémi Piotaix, 2014/09/05 21:37:36]

Done.

+        cachedBitmap.setInfo(SkImageInfo::Make(bounds.width(), bounds.height(),
+                                               this->info().colorType(),
+                                               this->info().alphaType()));
+        // If we can't alloc the pixels, then fail
+        if (!tryAllocBitmapPixels(cachedBitmap))

[reed1, 2014/09/04 18:49:59]

style: Skia *always* use { } with "if" blocks.

[Rémi Piotaix, 2014/09/05 21:37:36]

Done.

+            return false;
+
+        { // Try to read the pixels from the surface
+            SkAutoLockPixels alp(cachedBitmap);

[reed1, 2014/09/04 18:49:59]

allocPixels always returns a locked bitmap, so this autolock is unnecessary.

[Rémi Piotaix, 2014/09/05 21:37:36]

Done.

+            void* buffer = cachedBitmap.getPixels();
+            bool readPixelsOk = fSurface->readPixels(bounds.fLeft, bounds.fTop, bounds.width(), bounds.width(),
+                                    kSkia8888_GrPixelConfig,
+                                    buffer, cachedBitmap.rowBytes());
+
+            if (!readPixelsOk)
+                return false;
+        }
+
+        // If we are here, pixels were read correctly from the surface.
+        cachedBitmap.setImmutable();
+        //Add to the cache
+        SkBitmapCache::Add(this->getGenerationID(), bounds, cachedBitmap);
+
+        dst->swap(cachedBitmap);
     }
-    SkAutoLockPixels al(*dst);
-    void* buffer = dst->getPixels();
-    return fSurface->readPixels(left, top, width, height,
-                                kSkia8888_GrPixelConfig,
-                                buffer, dst->rowBytes());
+
+    // Unlock the pixels (lock comes from Find or allocPixelRef)

[reed1, 2014/09/04 18:49:59]

I see that the old code did this, but I think we don't think we need to do this.
readPixels can (and probably should) return a locked bitmap, since by definition
its pixels are direct allocated, and can never be "lazy".

[Rémi Piotaix, 2014/09/05 21:37:36]

Done.

+    dst->unlockPixels();
+
+    // The pixels of the bitmap should not be locked
+    SkASSERT(NULL == dst->getPixels());
+
+    return true;
+
 }