PNGDecoder wasn't multiplying the alpha like PNGEncoder was. This lead to color

overflow.

This intermediate fix makes everything correct, but will make alphaed pixels
slightly darker until the user's cache is flushed (I have screenshots of the effect).

BUG=13360
TEST=none

Review URL: http://codereview.chromium.org/147049

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@19131 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 82 insertions, 9 deletions

diff --git a/base/gfx/png_codec_unittest.cc b/base/gfx/png_codec_unittest.cc
index 9184abe..c9734a7 100644
--- a/base/gfx/png_codec_unittest.cc
+++ b/base/gfx/png_codec_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/gfx/png_encoder.h"
 #include "base/gfx/png_decoder.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 static void MakeRGBImage(int w, int h, std::vector<unsigned char>* dat) {
   dat->resize(w * h * 3);
@@ -41,6 +42,25 @@ static void MakeRGBAImage(int w, int h, bool use_transparency,
   }
 }
 
+// Returns true if each channel of the given two colors are "close." This is
+// used for comparing colors where rounding errors may cause off-by-one.
+bool ColorsClose(uint32_t a, uint32_t b) {
+  return abs(static_cast<int>(SkColorGetB(a) - SkColorGetB(b))) < 2 &&
+         abs(static_cast<int>(SkColorGetG(a) - SkColorGetG(b))) < 2 &&
+         abs(static_cast<int>(SkColorGetR(a) - SkColorGetR(b))) < 2 &&
+         abs(static_cast<int>(SkColorGetA(a) - SkColorGetA(b))) < 2;
+}
+
+void MakeTestSkBitmap(int w, int h, SkBitmap* bmp) {
+  bmp->setConfig(SkBitmap::kARGB_8888_Config, w, h);
+  bmp->allocPixels();
+
+  uint32_t* src_data = bmp->getAddr32(0, 0);
+  for (int i = 0; i < w * h; i++) {
+    src_data[i] = SkPreMultiplyARGB(i % 255, i % 250, i % 245, i % 240);
+  }
+}
+
 TEST(PNGCodec, EncodeDecodeRGB) {
   const int w = 20, h = 20;
 
@@ -200,3 +220,30 @@ TEST(PNGCodec, StripAddAlpha) {
   ASSERT_EQ(original_rgb.size(), decoded.size());
   ASSERT_TRUE(original_rgb == decoded);
 }
+
+TEST(PNGCodec, EncodeBGRASkBitmap) {
+  const int w = 20, h = 20;
+
+  SkBitmap original_bitmap;
+  MakeTestSkBitmap(w, h, &original_bitmap);
+
+  // Encode the bitmap.
+  std::vector<unsigned char> encoded;
+  PNGEncoder::EncodeBGRASkBitmap(original_bitmap, false, &encoded);
+
+  // Decode the encoded string.
+  SkBitmap decoded_bitmap;
+  EXPECT_TRUE(PNGDecoder::Decode(&encoded, &decoded_bitmap));
+
+  // Compare the original bitmap and the output bitmap. We use ColorsClose
+  // as SkBitmaps are considered to be pre-multiplied, the unpremultiplication
+  // (in Encode) and repremultiplication (in Decode) can be lossy.
+  for (int x = 0; x < w; x++) {
+    for (int y = 0; y < h; y++) {
+      uint32_t original_pixel = original_bitmap.getAddr32(0, y)[x];
+      uint32_t decoded_pixel = decoded_bitmap.getAddr32(0, y)[x];
+      EXPECT_TRUE(ColorsClose(original_pixel, decoded_pixel));
+    }
+  }
+}
+
diff --git a/base/gfx/png_decoder.cc b/base/gfx/png_decoder.cc
index e493464..bd36c57 100644
--- a/base/gfx/png_decoder.cc
+++ b/base/gfx/png_decoder.cc
@@ -261,8 +261,8 @@ class PngReadStructDestroyer {
 
 // static
 bool PNGDecoder::Decode(const unsigned char* input, size_t input_size,
-                      ColorFormat format, std::vector<unsigned char>* output,
-                      int* w, int* h) {
+                        ColorFormat format, std::vector<unsigned char>* output,
+                        int* w, int* h) {
   if (input_size < 8)
     return false;  // Input data too small to be a png
 
@@ -324,7 +324,25 @@ bool PNGDecoder::Decode(const std::vector<unsigned char>* data,
                          &decoded_data, &width, &height)) {
     bitmap->setConfig(SkBitmap::kARGB_8888_Config, width, height);
     bitmap->allocPixels();
-    memcpy(bitmap->getPixels(), &decoded_data.front(), width * height * 4);
+    for (int y = 0; y < height; y++) {
+      for (int x = 0; x < width; x++) {
+        int pix = (y * height + x) * 4;
+        unsigned char alpha = decoded_data[pix + 3];
+        if (alpha != 0 && alpha != 255) {
+          bitmap->getAddr32(0, y)[x] = SkColorSetARGB(
+              alpha,
+              (decoded_data[pix + 2] * alpha) >> 8,
+              (decoded_data[pix + 1] * alpha) >> 8,
+              (decoded_data[pix] * alpha) >> 8);
+        } else {
+          bitmap->getAddr32(0, y)[x] = SkColorSetARGB(
+              alpha,
+              decoded_data[pix + 2],
+              decoded_data[pix + 1],
+              decoded_data[pix]);
+        }
+      }
+    }
     return true;
   }
   return false;
diff --git a/chrome/browser/webdata/web_database_unittest.cc b/chrome/browser/webdata/web_database_unittest.cc
index 5076fc2..a2ac50d 100644
--- a/chrome/browser/webdata/web_database_unittest.cc
+++ b/chrome/browser/webdata/web_database_unittest.cc
@@ -641,8 +641,16 @@ TEST_F(WebDatabaseTest, WebAppImages) {
   image.setConfig(SkBitmap::kARGB_8888_Config, 16, 16);
   image.allocPixels();
   image.eraseColor(SK_ColorBLACK);
-  // Some random pixels so that we can identify the image.
-  *(reinterpret_cast<unsigned char*>(image.getPixels())) = 0xAB;
+
+  // Set some random pixels so that we can identify the image. We don't use
+  // transparent images because of pre-multiplication rounding errors.
+  SkColor test_pixel_1 = 0xffccbbaa;
+  SkColor test_pixel_2 = 0x00aabbaa;
+  SkColor test_pixel_3 = 0xff339966;
+  image.getAddr32(0, 1)[0] = test_pixel_1;
+  image.getAddr32(0, 1)[1] = test_pixel_2;
+  image.getAddr32(0, 1)[2] = test_pixel_3;
+
   ASSERT_TRUE(db.SetWebAppImage(url, image));
   images.clear();
   ASSERT_TRUE(db.GetWebAppImages(url, &images));
@@ -650,10 +658,10 @@ TEST_F(WebDatabaseTest, WebAppImages) {
   ASSERT_EQ(16, images[0].width());
   ASSERT_EQ(16, images[0].height());
   images[0].lockPixels();
-  unsigned char* pixels =
-      reinterpret_cast<unsigned char*>(images[0].getPixels());
-  ASSERT_TRUE(pixels != NULL);
-  ASSERT_EQ(0xAB, *pixels);
+  ASSERT_TRUE(images[0].getPixels() != NULL);
+  ASSERT_EQ(test_pixel_1, images[0].getAddr32(0, 1)[0]);
+  ASSERT_EQ(test_pixel_2, images[0].getAddr32(0, 1)[1]);
+  ASSERT_EQ(test_pixel_3, images[0].getAddr32(0, 1)[2]);
   images[0].unlockPixels();
 
   // Add another image at a bigger size.