captureVisibleTab Extension API producing garbled PNG results

ui/gfx/codec/png_codec_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/logging.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/libpng/png.h"
@@ skipping 952 matching lines @@
                                PNGCodec::FORMAT_RGB, &decoded,
                                &outw, &outh));
 
   // It should be the same as our non-alpha-channel reference.
   ASSERT_EQ(w, outw);
   ASSERT_EQ(h, outh);
   ASSERT_EQ(original_rgb.size(), decoded.size());
   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;
 
   SkBitmap original_bitmap;
   MakeTestSkBitmap(w, h, &original_bitmap);
 
   // Encode the bitmap.
   std::vector<unsigned char> encoded;
   PNGCodec::EncodeBGRASkBitmap(original_bitmap, false, &encoded);
 
@@ skipping 128 matching lines @@
   ASSERT_EQ(w, outw);
   ASSERT_EQ(h, outh);
   ASSERT_EQ(original.size(), decoded.size());
 
   // Images must be exactly equal
   ASSERT_TRUE(original == decoded);
 }
 
 
 }  // namespace gfx