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

tests/CachedDecodingPixelRefTest.cpp

Index: tests/CachedDecodingPixelRefTest.cpp
diff --git a/tests/CachedDecodingPixelRefTest.cpp b/tests/CachedDecodingPixelRefTest.cpp
index 2813b42bf8b40ce3638266a19c8d8b3d317dc09b..4f5ff23253bc60969c903cdf3afa81aa7bd7af08 100644
--- a/tests/CachedDecodingPixelRefTest.cpp
+++ b/tests/CachedDecodingPixelRefTest.cpp
@@ -163,16 +163,16 @@ public:
     };
     static int Width() { return 10; }
     static int Height() { return 10; }
-    static uint32_t Color() { return 0xff123456; }
-    TestImageGenerator(TestType type, skiatest::Reporter* reporter)
-    : INHERITED(GetMyInfo()), fType(type), fReporter(reporter) {
+    static uint32_t Color() { return 0xff10345a; } // value choosen so that there is no loss when converting to to RGB565 and back

[reed1, 2015/11/09 14:14:19]

I know the old code used uint32_t for this type, but w/ your extensions to the
test, it is getting very confusion what this type is, specifically, is it
SkColor or SkPMColor.

Since your modifications at the bottom of this file now always call
bm.getColor(x,y), I think we should declare this to be SkColor. To do that
cleanly, we'll need to convert it to SkPMColor when we use it to write into our
image buffer (both for kN32 and for the colortable entry.

[reed1, 2015/11/09 14:14:19]

nits: skia limits itself to 100 columns

[aleksandar.stojiljkovic, 2015/11/09 15:28:00]

Acknowledged.

[aleksandar.stojiljkovic, 2015/11/09 15:28:00]

Acknowledged.

+    TestImageGenerator(TestType type, skiatest::Reporter* reporter, SkColorType colorType = kN32_SkColorType)
+    : INHERITED(GetMyInfo(colorType)), fType(type), fReporter(reporter) {
         SkASSERT((fType <= kLast_TestType) && (fType >= 0));
     }
     virtual ~TestImageGenerator() { }
 
 protected:
-    static SkImageInfo GetMyInfo() {
-        return SkImageInfo::MakeN32(TestImageGenerator::Width(), TestImageGenerator::Height(),
+    static SkImageInfo GetMyInfo(SkColorType colorType) {
+        return SkImageInfo::Make(TestImageGenerator::Width(), TestImageGenerator::Height(), colorType,

[reed1, 2015/11/09 14:14:19]

100 cols

[aleksandar.stojiljkovic, 2015/11/09 15:28:00]

Acknowledged.

                                     kOpaque_SkAlphaType);
     }
 
@@ -183,14 +183,35 @@ protected:
         if (fType != kSucceedGetPixels_TestType) {
             return false;
         }
-        if (info.colorType() != kN32_SkColorType) {
+        if (info.colorType() != kN32_SkColorType && info.colorType() != getInfo().colorType()) {
             return false;
         }
         char* bytePtr = static_cast<char*>(pixels);
-        for (int y = 0; y < info.height(); ++y) {
-            sk_memset32(reinterpret_cast<SkColor*>(bytePtr),
-                        TestImageGenerator::Color(), info.width());
-            bytePtr += rowBytes;
+        switch (info.colorType()) {
+        case kN32_SkColorType:
+            for (int y = 0; y < info.height(); ++y) {
+                sk_memset32(reinterpret_cast<SkColor*>(bytePtr),
+                    TestImageGenerator::Color(), info.width());
+                bytePtr += rowBytes;
+            }
+            break;
+        case kIndex_8_SkColorType:
+            *ctableCount = 1;
+            ctable[0] = TestImageGenerator::Color();
+            for (int y = 0; y < info.height(); ++y) {
+                memset(bytePtr, 0, info.width());
+                bytePtr += rowBytes;
+            }
+            break;
+        case kRGB_565_SkColorType:
+            for (int y = 0; y < info.height(); ++y) {
+                sk_memset16((uint16_t*)bytePtr,
+                    SkDitherPixel32ToPixel16(TestImageGenerator::Color()), info.width());

[reed1, 2015/11/09 14:14:19]

why the dither conversion? Since you're repeating the same value for all pixels,
it is a little confusing/unnecessary it seems.

[aleksandar.stojiljkovic, 2015/11/09 15:28:00]

I just didn't know better:
SkDitherPixel32ToPixel16 (though called diether) doesn't take surrounding pixels
into account, but only does RGB888-RGB565 quantization and packing into 16 bits:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...

I wanted to call directly SkPackRGB16, but there is assert if components don't
fit 5/6 bits. 

[reed1, 2015/11/09 15:32:47]

That dither call is the compliment to SkPixel32ToPixel16(). and *always* packs
with a different bias applied, so it can pair up with SkPixel32ToPixel16
(calling each alternatively).

I think your code will read simpler if you just call SkPixel32ToPixel16().

+                bytePtr += rowBytes;
+            }
+            break;
+        default:
+            return false;
         }
         return true;
     }
@@ -214,7 +235,7 @@ static void check_test_image_generator_bitmap(skiatest::Reporter* reporter,
     int errors = 0;
     for (int y = 0; y < bm.height(); ++y) {
         for (int x = 0; x < bm.width(); ++x) {
-            if (TestImageGenerator::Color() != *bm.getAddr32(x, y)) {
+            if (TestImageGenerator::Color() != bm.getColor(x, y)) {
                 ++errors;
             }
         }
@@ -224,8 +245,9 @@ static void check_test_image_generator_bitmap(skiatest::Reporter* reporter,
 
 static void check_pixelref(TestImageGenerator::TestType type,
                            skiatest::Reporter* reporter,
-                           SkDiscardableMemory::Factory* factory) {
-    SkAutoTDelete<SkImageGenerator> gen(new TestImageGenerator(type, reporter));
+                           SkDiscardableMemory::Factory* factory,
+                           SkColorType colorType) {
+    SkAutoTDelete<SkImageGenerator> gen(new TestImageGenerator(type, reporter, colorType));
     REPORTER_ASSERT(reporter, gen.get() != nullptr);
     SkBitmap lazy;
     bool success = SkDEPRECATED_InstallDiscardablePixelRef(gen.detach(), nullptr, &lazy, factory);
@@ -245,23 +267,30 @@ static void check_pixelref(TestImageGenerator::TestType type,
  *  SkDiscardableMemory::Factory choices.
  */
 DEF_TEST(DiscardableAndCachingPixelRef, reporter) {
-    check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, nullptr);
-    check_pixelref(TestImageGenerator::kSucceedGetPixels_TestType, reporter, nullptr);
+    SkColorType testColorTypes[] = {

[reed1, 2015/11/09 14:14:19]

nit: const

+        kN32_SkColorType,
+        kIndex_8_SkColorType,
+        kRGB_565_SkColorType
+    };
+    for (size_t i = 0; i < SK_ARRAY_COUNT(testColorTypes); ++i) {
+        check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, nullptr, testColorTypes[i]);

[reed1, 2015/11/09 14:14:19]

100 cols

+        check_pixelref(TestImageGenerator::kSucceedGetPixels_TestType, reporter, nullptr, testColorTypes[i]);
 
-    SkAutoTUnref<SkDiscardableMemoryPool> pool(
-        SkDiscardableMemoryPool::Create(1, nullptr));
-    REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
-    check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, pool);
-    REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
-    check_pixelref(TestImageGenerator::kSucceedGetPixels_TestType, reporter, pool);
-    REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
+        SkAutoTUnref<SkDiscardableMemoryPool> pool(
+            SkDiscardableMemoryPool::Create(1, nullptr));
+        REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
+        check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, pool, testColorTypes[i]);
+        REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
+        check_pixelref(TestImageGenerator::kSucceedGetPixels_TestType, reporter, pool, testColorTypes[i]);
+        REPORTER_ASSERT(reporter, 0 == pool->getRAMUsed());
 
-    SkDiscardableMemoryPool* globalPool = SkGetGlobalDiscardableMemoryPool();
-    // Only acts differently from nullptr on a platform that has a
-    // default discardable memory implementation that differs from the
-    // global DM pool.
-    check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, globalPool);
-    check_pixelref(TestImageGenerator::kSucceedGetPixels_TestType, reporter, globalPool);
+        SkDiscardableMemoryPool* globalPool = SkGetGlobalDiscardableMemoryPool();
+        // Only acts differently from nullptr on a platform that has a
+        // default discardable memory implementation that differs from the
+        // global DM pool.
+        check_pixelref(TestImageGenerator::kFailGetPixels_TestType, reporter, globalPool, testColorTypes[i]);
+        check_pixelref(TestImageGenerator::kSucceedGetPixels_TestType, reporter, globalPool, testColorTypes[i]);
+    }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -271,30 +300,38 @@ DEF_TEST(Image_NewFromGenerator, r) {
         TestImageGenerator::kFailGetPixels_TestType,
         TestImageGenerator::kSucceedGetPixels_TestType,
     };
+    SkColorType testColorTypes[] = {

[reed1, 2015/11/09 14:14:19]

const

+        kN32_SkColorType,
+        kIndex_8_SkColorType,
+        kRGB_565_SkColorType
+    };
     for (size_t i = 0; i < SK_ARRAY_COUNT(testTypes); ++i) {
         TestImageGenerator::TestType test = testTypes[i];
-        SkImageGenerator* gen = new TestImageGenerator(test, r);
-        SkAutoTUnref<SkImage> image(SkImage::NewFromGenerator(gen));
-        if (nullptr == image.get()) {
-            ERRORF(r, "SkImage::NewFromGenerator unexpecedly failed ["
-                   SK_SIZE_T_SPECIFIER "]", i);
-            continue;
-        }
-        REPORTER_ASSERT(r, TestImageGenerator::Width() == image->width());
-        REPORTER_ASSERT(r, TestImageGenerator::Height() == image->height());
-        REPORTER_ASSERT(r, image->isLazyGenerated());
+        for (size_t j = 0; j < SK_ARRAY_COUNT(testColorTypes); ++j) {
+            SkImageGenerator* gen = new TestImageGenerator(test, r, testColorTypes[j]);
+            SkAutoTUnref<SkImage> image(SkImage::NewFromGenerator(gen));
+            if (nullptr == image.get()) {
+                ERRORF(r, "SkImage::NewFromGenerator unexpecedly failed ["
+                    SK_SIZE_T_SPECIFIER "]", i);
+                continue;
+            }
+            REPORTER_ASSERT(r, TestImageGenerator::Width() == image->width());
+            REPORTER_ASSERT(r, TestImageGenerator::Height() == image->height());
+            REPORTER_ASSERT(r, image->isLazyGenerated());
 
-        SkBitmap bitmap;
-        bitmap.allocN32Pixels(TestImageGenerator::Width(), TestImageGenerator::Height());
-        SkCanvas canvas(bitmap);
-        const SkColor kDefaultColor = 0xffabcdef;
-        canvas.clear(kDefaultColor);
-        canvas.drawImage(image, 0, 0, nullptr);
-        if (TestImageGenerator::kSucceedGetPixels_TestType == test) {
-            REPORTER_ASSERT(
-                    r, TestImageGenerator::Color() == *bitmap.getAddr32(0, 0));
-        } else {
-            REPORTER_ASSERT(r, kDefaultColor == bitmap.getColor(0,0));
+            SkBitmap bitmap;
+            bitmap.allocN32Pixels(TestImageGenerator::Width(), TestImageGenerator::Height());
+            SkCanvas canvas(bitmap);
+            const SkColor kDefaultColor = 0xffabcdef;
+            canvas.clear(kDefaultColor);
+            canvas.drawImage(image, 0, 0, nullptr);
+            if (TestImageGenerator::kSucceedGetPixels_TestType == test) {
+                REPORTER_ASSERT(
+                    r, TestImageGenerator::Color() == bitmap.getColor(0, 0));
+            }
+            else {
+                REPORTER_ASSERT(r, kDefaultColor == bitmap.getColor(0, 0));
+            }
         }
     }
 }