captureVisibleTab Extension API producing garbled PNG results

ui/gfx/codec/png_codec_unittest.cc

Index: ui/gfx/codec/png_codec_unittest.cc
diff --git a/ui/gfx/codec/png_codec_unittest.cc b/ui/gfx/codec/png_codec_unittest.cc
index c22e7b12b4e7df65e6c8701aca73358758f560db..27b776283cb80558f98c8a139a7a3ea2d16d6050 100644
--- a/ui/gfx/codec/png_codec_unittest.cc
+++ b/ui/gfx/codec/png_codec_unittest.cc
@@ -970,6 +970,42 @@ TEST(PNGCodec, StripAddAlpha) {
   ASSERT_EQ(original_rgb, decoded);
 }
 
+TEST(PNGCodec, EncodeBGRASkBitmapStridePadded) {
+  const int w = 20, h = 20, padded_w = 32;

[Alexei Svitkine (slow), 2013/03/05 18:29:26]

Separate lines please.

I suggest using constant naming convention for these, i.e. kWidth, kPaddedWidth,
etc.

[Sam Larison, 2013/03/05 20:30:41]

I have changed the naming conventions... except I only separated my new variable
kPaddedWidth onto it's own line, shall kWidth and kHeight each be on their own
lines too?

[Alexei Svitkine (slow), 2013/03/05 21:35:23]

Each on a separate line, please.

+
+  SkBitmap original_bitmap;
+  original_bitmap.setConfig(SkBitmap::kARGB_8888_Config, w, h, padded_w * 4);
+  original_bitmap.allocPixels();
+
+  // Write data over the source bitmap.
+  // We write on the pad area here too.
+  // The encoder should ignore the pad area.
+  uint32_t* src_data = original_bitmap.getAddr32(0, 0);
+  for (int i = 0; i < padded_w * h; i++) {

[Alexei Svitkine (slow), 2013/03/05 18:29:26]

Extract padded_w * h into a variable, e.g.

const int kBufferSize = kHeight * kPaddedWidth;

+    src_data[i] = SkPreMultiplyARGB(i % 255, i % 250, i % 245, i % 240);
+  }
+
+  // Encode the bitmap.
+  std::vector<unsigned char> encoded;
+  PNGCodec::EncodeBGRASkBitmap(original_bitmap, false, &encoded);
+
+  // Decode the encoded string.
+  SkBitmap decoded_bitmap;
+  EXPECT_TRUE(PNGCodec::Decode(&encoded.front(), encoded.size(),
+                               &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];

[Alexei Svitkine (slow), 2013/03/05 18:29:26]

Can you do *original_bitmap.getAddr32(x, y) here?

[Sam Larison, 2013/03/05 20:30:41]

I attempted doing so however this produced unsatisfactory results and the test
fails (making this change to other tests causes those to fail too), not exactly
sure why.  All the other tests do it this way.  Do you think that's okay?

I see unfortunately I missed at least one of the other changes... I will submit
another patch-set after I hear back about this.

[Alexei Svitkine (slow), 2013/03/05 21:35:23]

Okay, that's fine then.

+      uint32_t decoded_pixel = decoded_bitmap.getAddr32(0, y)[x];

[Alexei Svitkine (slow), 2013/03/05 18:29:26]

ditto

+      EXPECT_TRUE(ColorsClose(original_pixel, decoded_pixel));
+    }
+  }
+}
+
 TEST(PNGCodec, EncodeBGRASkBitmap) {
   const int w = 20, h = 20;