Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder

tests/CodecTest.cpp

Index: tests/CodecTest.cpp
diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp
index b01750d41074a42b7b4d4e9139f9bc42b741e1b5..2f23222b509171fcbfd59043527b3cada642185e 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp
@@ -1074,22 +1074,49 @@ DEF_TEST(Codec_ColorXform, r) {
     check_color_xform(r, "mandrill_512.png");
 }
 
-static void check_round_trip(skiatest::Reporter* r, const SkBitmap& bm1) {
-    SkColorType origColorType = bm1.colorType();
-    SkAlphaType origAlphaType = bm1.alphaType();
+static bool color_type_match(SkColorType origColorType, SkColorType codecColorType) {
+    switch (origColorType) {
+        case kRGBA_8888_SkColorType:
+        case kBGRA_8888_SkColorType:
+            return kRGBA_8888_SkColorType == codecColorType ||
+                   kBGRA_8888_SkColorType == codecColorType;
+        default:
+            return origColorType == codecColorType;
+    }
+}
+
+static bool alpha_type_match(SkAlphaType origAlphaType, SkAlphaType codecAlphaType) {
+    switch (origAlphaType) {
+        case kUnpremul_SkAlphaType:
+        case kPremul_SkAlphaType:
+            return kUnpremul_SkAlphaType == codecAlphaType ||
+                    kPremul_SkAlphaType == codecAlphaType;
+        default:
+            return origAlphaType == codecAlphaType;
+    }
+}
+
+static void check_round_trip(skiatest::Reporter* r, SkCodec* origCodec, const SkImageInfo& info) {
+    SkBitmap bm1;
+    SkPMColor colors[256];
+    SkAutoTUnref<SkColorTable> colorTable(new SkColorTable(colors, 256));
+    bm1.allocPixels(info, nullptr, colorTable.get());
+    int numColors;
+    SkCodec::Result result = origCodec->getPixels(info, bm1.getPixels(), bm1.rowBytes(), nullptr,

[scroggo, 2016/09/12 17:16:50]

Won't this leave colorTable (which the bitmap points to) uninitialized?

Is this tested on an index8 bitmap?

[msarett, 2016/09/12 19:39:20]

Yes, it's just irrelevant because the digest comparison will still work.

+                                                  colors, &numColors);
+    REPORTER_ASSERT(r, SkCodec::kSuccess == result);
 
     // Encode the image to png.
     sk_sp<SkData> data =
             sk_sp<SkData>(SkImageEncoder::EncodeData(bm1, SkImageEncoder::kPNG_Type, 100));
 
-    // Prepare to decode.  The codec should recognize that the PNG is 565.
     SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(data.get()));
-    REPORTER_ASSERT(r, origColorType == codec->getInfo().colorType());
-    REPORTER_ASSERT(r, origAlphaType == codec->getInfo().alphaType());
+    REPORTER_ASSERT(r, color_type_match(info.colorType(), codec->getInfo().colorType()));
+    REPORTER_ASSERT(r, alpha_type_match(info.alphaType(), codec->getInfo().alphaType()));
 
     SkBitmap bm2;
-    bm2.allocPixels(codec->getInfo());
-    SkCodec::Result result = codec->getPixels(codec->getInfo(), bm2.getPixels(), bm2.rowBytes());
+    bm2.allocPixels(info, nullptr, colorTable.get());
+    result = codec->getPixels(info, bm2.getPixels(), bm2.rowBytes(), nullptr, colors, &numColors);

[scroggo, 2016/09/12 17:16:50]

Once again, bm2's colorTable will remain uninitialized. I'm guessing the only
reason the md5s match down below is because they are using the same
uninitialized colorTable. (But I would hope that one of our *SAN bots would
catch it?)

[msarett, 2016/09/12 19:39:20]

I had thought that the md5 would be comparing the indices (and the not the color
table itself).  Still, I'll fix this anyway.

     REPORTER_ASSERT(r, SkCodec::kSuccess == result);
 
     SkMD5::Digest d1, d2;
@@ -1099,26 +1126,61 @@ static void check_round_trip(skiatest::Reporter* r, const SkBitmap& bm1) {
 }
 
 DEF_TEST(Codec_PngRoundTrip, r) {
-    // Create an arbitrary 565 bitmap.
     const char* path = "mandrill_512_q075.jpg";
     SkAutoTDelete<SkStream> stream(resource(path));
     SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream.release()));
+
+    // Test 565
     SkImageInfo info565 = codec->getInfo().makeColorType(kRGB_565_SkColorType);

[scroggo, 2016/09/12 17:16:50]

nit: Can you fold these three into one for loop? Something like:

for (ct : { kRGB_565_SkColorType, kRGBA_8888_SkColorType, kBGRA_8888_SkColorType
}) {
  SkImageInfo info = codec->getInfo().makeColorType(ct);
  check_round_trip(r, codec.get(), info);
}

[msarett, 2016/09/12 19:39:20]

Done.

-    SkBitmap bm1;
-    bm1.allocPixels(info565);
-    SkCodec::Result result = codec->getPixels(info565, bm1.getPixels(), bm1.rowBytes());
-    REPORTER_ASSERT(r, SkCodec::kSuccess == result);
-    check_round_trip(r, bm1);
+    check_round_trip(r, codec.get(), info565);
+
+    // Test RGBA opaque
+    SkImageInfo infoRGBA = codec->getInfo().makeColorType(kRGBA_8888_SkColorType);
+    check_round_trip(r, codec.get(), infoRGBA);
+
+    // Test BGRA opaque
+    SkImageInfo infoBGRA = codec->getInfo().makeColorType(kBGRA_8888_SkColorType);
+    check_round_trip(r, codec.get(), infoBGRA);
 
-    // Create an arbitrary gray bitmap.
+    // Test Gray
     path = "grayscale.jpg";
     stream.reset(resource(path));
     codec.reset(SkCodec::NewFromStream(stream.release()));
-    SkBitmap bm2;
-    bm2.allocPixels(codec->getInfo());
-    result = codec->getPixels(codec->getInfo(), bm2.getPixels(), bm2.rowBytes());
-    REPORTER_ASSERT(r, SkCodec::kSuccess == result);
-    check_round_trip(r, bm2);
+    check_round_trip(r, codec.get(), codec->getInfo());
+
+    path = "yellow_rose.png";
+    stream.reset(resource(path));
+    codec.reset(SkCodec::NewFromStream(stream.release()));
+
+    // Test RGBA/BGRA with alpha

[scroggo, 2016/09/12 17:16:50]

Can this be another for loop?

[msarett, 2016/09/12 19:39:20]

Done.

+    SkImageInfo infoRGBAPremul = codec->getInfo().makeColorType(kRGBA_8888_SkColorType)
+                                                 .makeAlphaType(kPremul_SkAlphaType)
+                                                 .makeColorSpace(nullptr);

[scroggo, 2016/09/12 17:16:50]

I'm guessing the color space needs to be removed because of premultiplication?

[msarett, 2016/09/12 19:39:20]

Yes, uggh...

+    check_round_trip(r, codec.get(), infoRGBAPremul);
+    SkImageInfo infoBGRAPremul = codec->getInfo().makeColorType(kBGRA_8888_SkColorType)
+                                                 .makeAlphaType(kPremul_SkAlphaType)
+                                                 .makeColorSpace(nullptr);
+    check_round_trip(r, codec.get(), infoBGRAPremul);
+    SkImageInfo infoRGBAUnpremul = codec->getInfo().makeColorType(kRGBA_8888_SkColorType)
+                                                   .makeAlphaType(kUnpremul_SkAlphaType)
+                                                   .makeColorSpace(nullptr);
+    check_round_trip(r, codec.get(), infoRGBAUnpremul);
+    SkImageInfo infoBGRAUnpremul = codec->getInfo().makeColorType(kBGRA_8888_SkColorType)
+                                                   .makeAlphaType(kUnpremul_SkAlphaType)
+                                                   .makeColorSpace(nullptr);
+    check_round_trip(r, codec.get(), infoBGRAUnpremul);
+
+    path = "index8.png";
+    stream.reset(resource(path));
+    codec.reset(SkCodec::NewFromStream(stream.release()));
+
+    // Text Index8
+    SkImageInfo infoIndex8Unpremul = codec->getInfo().makeAlphaType(kUnpremul_SkAlphaType)
+                                                     .makeColorSpace(nullptr);
+    check_round_trip(r, codec.get(), infoIndex8Unpremul);
+    //SkImageInfo infoIndex8Premul = codec->getInfo().makeAlphaType(kPremul_SkAlphaType)

[scroggo, 2016/09/12 17:16:50]

Comment explaining why this one is commented out? (Also, this could be another
for loop?)

[msarett, 2016/09/12 19:39:20]

Because I forgot to comment it back in :).

+    //                                               .makeColorSpace(nullptr);
+    //check_round_trip(r, codec.get(), infoIndex8Premul);
 }
 
 static void test_conversion_possible(skiatest::Reporter* r, const char* path,