Big Cleanup: SkBitmapFactory, SkLazyPixelRef, SkImageCache

tests/CachedDecodingPixelRefTest.cpp

Index: tests/CachedDecodingPixelRefTest.cpp
diff --git a/tests/CachedDecodingPixelRefTest.cpp b/tests/CachedDecodingPixelRefTest.cpp
index 2d8a0e70737a66c376987caf9c2adb055026f054..9957c4afadde1e371eafa8d44478a788ea02bb90 100644
--- a/tests/CachedDecodingPixelRefTest.cpp
+++ b/tests/CachedDecodingPixelRefTest.cpp
@@ -9,13 +9,15 @@
 #include "SkCanvas.h"
 #include "SkData.h"
 #include "SkDecodingImageGenerator.h"
+#include "SkDiscardableMemoryPool.h"
+#include "SkDiscardablePixelRef.h"
 #include "SkForceLinking.h"
 #include "SkImageDecoder.h"
 #include "SkImagePriv.h"
 #include "SkLazyCachingPixelRef.h"
-#include "SkLazyPixelRef.h"
 #include "SkScaledImageCache.h"
 #include "SkStream.h"
+#include "SkUtils.h"
 
 #include "Test.h"
 #include "TestClassDef.h"
@@ -53,24 +55,6 @@ static SkData* create_data_from_bitmap(const SkBitmap& bm,
     return NULL;
 }
 
-/**
- *  A simplified version of SkBitmapFactory
- */
-static bool simple_bitmap_factory(SkBitmapFactory::DecodeProc proc,
-                                  SkData* data,
-                                  SkBitmap* dst) {
-    SkImageInfo info;
-    if (!proc(data->data(), data->size(), &info, NULL)) {
-        return false;
-    }
-    dst->setConfig(SkImageInfoToBitmapConfig(info), info.fWidth,
-                    info.fHeight, 0, info.fAlphaType);
-    SkAutoTUnref<SkLazyPixelRef> ref(SkNEW_ARGS(SkLazyPixelRef,
-                                                (data, proc, NULL)));
-    dst->setPixelRef(ref);
-    return true;
-}
-
 static void compare_bitmaps(skiatest::Reporter* reporter,
                             const SkBitmap& b1, const SkBitmap& b2,
                             bool pixelPerfect = true) {
@@ -142,38 +126,6 @@ static void test_three_encodings(skiatest::Reporter* reporter,
 }
 
 /**
- *  This checks to see that a SkLazyPixelRef works as advertised.
- */
-static void compare_with_skLazyPixelRef(skiatest::Reporter* reporter,
-                                        SkData* encoded,
-                                        const SkBitmap& original,
-                                        bool comparePixels) {
-    SkBitmap lazy;
-    static const SkBitmapFactory::DecodeProc decoder =
-        &(SkImageDecoder::DecodeMemoryToTarget);
-    bool success = simple_bitmap_factory(decoder, encoded, &lazy);
-    REPORTER_ASSERT(reporter, success);
-
-    REPORTER_ASSERT(reporter, NULL == lazy.getPixels());
-    {
-        SkAutoLockPixels autoLockPixels(lazy);  // now pixels are good.
-        REPORTER_ASSERT(reporter, NULL != lazy.getPixels());
-    }
-    // pixels should be gone!
-    REPORTER_ASSERT(reporter, NULL == lazy.getPixels());
-    {
-        SkAutoLockPixels autoLockPixels(lazy);  // now pixels are good.
-        REPORTER_ASSERT(reporter, NULL != lazy.getPixels());
-    }
-    compare_bitmaps(reporter, original, lazy, comparePixels);
-}
-DEF_TEST(LazyPixelRef, reporter) {
-    test_three_encodings(reporter, compare_with_skLazyPixelRef);
-}
-
-
-
-/**
  *  This checks to see that a SkLazyCachedPixelRef works as advertised.
  */
 
@@ -182,7 +134,7 @@ static void compare_with_skLazyCachedPixelRef(skiatest::Reporter* reporter,
                                               const SkBitmap& original,
                                               bool comparePixels) {
     SkBitmap lazy;
-    static const SkBitmapFactory::DecodeProc decoder =
+    static const SkLazyCachingPixelRef::DecodeProc decoder =
         &(SkImageDecoder::DecodeMemoryToTarget);
     bool success = SkLazyCachingPixelRef::Install(decoder, encoded, &lazy);
     REPORTER_ASSERT(reporter, success);
@@ -206,13 +158,14 @@ DEF_TEST(LazyCachedPixelRef, reporter) {
 
 class TestPixelRef : public SkCachingPixelRef {
 public:
-    TestPixelRef(int x) : fX(x) { }
+    explicit TestPixelRef(int x) : fX(x) { }
     virtual ~TestPixelRef() { }
     static bool Install(SkBitmap* destination, int x) {
         SkAutoTUnref<TestPixelRef> ref(SkNEW_ARGS(TestPixelRef, (x)));
         return ref->configure(destination) && destination->setPixelRef(ref);
     }
     SK_DECLARE_UNFLATTENABLE_OBJECT()
+
 protected:
     virtual bool onDecodeInfo(SkImageInfo* info) SK_OVERRIDE {
         if (fX == 0) {
@@ -221,7 +174,7 @@ protected:
         SkASSERT(info);
         info->fWidth = 10;
         info->fHeight = 10;
-        info->fColorType = kRGBA_8888_SkColorType;
+        info->fColorType = kPMColor_SkColorType;
         info->fAlphaType = kOpaque_SkAlphaType;
         return true;
     }
@@ -230,6 +183,7 @@ protected:
                                 size_t rowBytes) SK_OVERRIDE {
         return false;
     }
+
 private:
     int fX;  // controls where the failure happens
     typedef SkCachingPixelRef INHERITED;
@@ -250,9 +204,12 @@ static void compare_with_SkDecodingImageGenerator(skiatest::Reporter* reporter,
                                                   SkData* encoded,
                                                   const SkBitmap& original,
                                                   bool comparePixels) {
+    SkAutoTUnref<SkDiscardableMemoryPool> dMFactory(
+        SkNEW_ARGS(SkDiscardableMemoryPool, (1)));
+    // Set the pool's budget abnormally low to force re-decoding
 
     SkBitmap lazy;
-    bool success = SkDecodingImageGenerator::Install(encoded, &lazy);
+    bool success = SkDecodingImageGenerator::Install(encoded, &lazy, dMFactory);
     REPORTER_ASSERT(reporter, success);
     if (!success) {
         return;
@@ -277,3 +234,113 @@ static void compare_with_SkDecodingImageGenerator(skiatest::Reporter* reporter,
 DEF_TEST(DecodingImageGenerator, reporter) {
     test_three_encodings(reporter, compare_with_SkDecodingImageGenerator);
 }
+
+////////////////////////////////////////////////////////////////////////////////
+namespace {
+/**
+ *  if x == 0, will fail at getPixels()

[scroggo, 2013/12/04 19:09:25]

This seems like a good candidate for an enum.

Also, the comments aren't quite right. If 0 == x, it will fail getInfo.

[hal.canary, 2013/12/04 19:39:40]

Done.

+ *  if x <= 1, will fail at getInfo()
+ *  if x >= 2, will fill the given pixels with SK_ColorCYAN.
+ */
+class TestImageGenerator : public SkImageGenerator {
+public:
+    static int Width() { return 10; }
+    static int Height() { return 10; }
+    static SkColor Color() { return SK_ColorCYAN; }
+    TestImageGenerator(int x, skiatest::Reporter* reporter)
+        : fX(x), fReporter(reporter) { }
+    ~TestImageGenerator() { }
+    bool getInfo(SkImageInfo* info) SK_OVERRIDE {
+        REPORTER_ASSERT(fReporter, NULL != info);
+        if ((info == NULL) || (fX == 0)) {
+            return false;
+        }
+        info->fWidth = TestImageGenerator::Width();
+        info->fHeight = TestImageGenerator::Height();
+        info->fColorType = kPMColor_SkColorType;
+        info->fAlphaType = kOpaque_SkAlphaType;
+        return true;
+    }
+    bool getPixels(const SkImageInfo& info,
+                   void* pixels,
+                   size_t rowBytes) SK_OVERRIDE {
+        REPORTER_ASSERT(fReporter, pixels != NULL);
+        REPORTER_ASSERT(fReporter,
+                        rowBytes >= (info.fWidth * info.bytesPerPixel()));
+        if ((pixels == NULL)

[scroggo, 2013/12/04 19:09:25]

For your == checks (here and above), NULL or 0 should go on the left.

[hal.canary, 2013/12/04 19:39:40]

Done.

+            || (fX < 2)
+            || (info.fColorType != kPMColor_SkColorType)) {
+            return false;
+        }
+        char* bytePtr = static_cast<char*>(pixels);
+        for (int y = 0; y < info.fHeight; ++y) {
+            sk_memset32(reinterpret_cast<SkPMColor*>(bytePtr),
+                        TestImageGenerator::Color(), info.fWidth);
+            bytePtr += rowBytes;
+        }
+        return true;
+    }
+private:
+    int fX;
+    skiatest::Reporter* fReporter;
+};
+void CheckTestImageGeneratorBitmap(skiatest::Reporter* reporter,
+                                   const SkBitmap& bm) {
+    REPORTER_ASSERT(reporter, TestImageGenerator::Width() == bm.width());
+    REPORTER_ASSERT(reporter, TestImageGenerator::Height() == bm.height());
+    SkAutoLockPixels autoLockPixels(bm);
+    REPORTER_ASSERT(reporter, NULL != bm.getPixels());
+    if (NULL == bm.getPixels()) {
+        return;
+    }
+    int errors = 0;
+    for (int y = 0; y < bm.height(); ++y) {
+        for (int x = 0; x < bm.width(); ++x) {
+            if (TestImageGenerator::Color() != bm.getColor(x, y)) {
+                ++errors;
+            }
+        }
+    }
+    REPORTER_ASSERT(reporter, 0 == errors);
+}
+void CheckDiscardablePixelRef(int x, skiatest::Reporter* reporter,
+                              SkDiscardableMemory::Factory* factory) {
+    SkAutoTDelete<SkImageGenerator> gen(SkNEW_ARGS(TestImageGenerator,
+                                                   (x, reporter)));
+    REPORTER_ASSERT(reporter, gen.get() != NULL);
+    SkBitmap lazy;
+    bool success = SkDiscardablePixelRef::Install(gen.detach(),
+                                                  &lazy, factory);
+    REPORTER_ASSERT(reporter, success == (0 != x));  // since x==2

[scroggo, 2013/12/04 19:09:25]

Does this comment go with the next line? I think the next line would be clearer
if x was an enum, and you did a comparison like

if (kSuccessfulImageGenerator == x)

That doesn't have to be the exact name, but something conveying that information
would make this test easier to read.

The current line would also read better using enums. I'm also not convinced the
speed increase of comparing the boolean to a comparison is worth the sacrifice
in readability.

[hal.canary, 2013/12/04 19:39:40]

Done.

+    if (x > 1) {
+        CheckTestImageGeneratorBitmap(reporter, lazy);
+    }
+}
+}  // namespace
+/**
+ *  This tests the basic functionality of SkDiscardablePixelRef with a
+ *  basic SkImageGenerator implementation and several
+ *  SkDiscardableMemory::Factory choices.
+ */
+DEF_TEST(DiscardablePixelRef, reporter) {
+    CheckDiscardablePixelRef(0, reporter, NULL);
+    CheckDiscardablePixelRef(1, reporter, NULL);
+    CheckDiscardablePixelRef(2, reporter, NULL);
+
+    SkAutoTUnref<SkDiscardableMemoryPool> pool(
+        SkNEW_ARGS(SkDiscardableMemoryPool, (1, NULL)));
+    REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
+    CheckDiscardablePixelRef(1, reporter, pool);
+    REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
+    CheckDiscardablePixelRef(2, reporter, pool);
+    REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
+
+    SkDiscardableMemoryPool* globalPool = SkGetGlobalDiscardableMemoryPool();
+    CheckDiscardablePixelRef(1, reporter, globalPool);
+    CheckDiscardablePixelRef(2, reporter, globalPool);
+
+    // TODO(halcanary): When ashmem-backed SkDiscardableMemory lands,
+    // test that here (on platforms where it is availible).
+}
+////////////////////////////////////////////////////////////////////////////////
+