[devtools] Support different encodings for Page.CaptureScreenshot.

content/browser/devtools/protocol/devtools_protocol_browsertest.cc

Index: content/browser/devtools/protocol/devtools_protocol_browsertest.cc
diff --git a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
index d2d04bebfe84e4c04fa38465dad61c91a542fe16..8d2758643d5d58f196b08fe62f9fb7c534f7962b 100644
--- a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
+++ b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
@@ -49,10 +49,12 @@
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/layout.h"
 #include "ui/compositor/compositor_switches.h"
+#include "ui/gfx/codec/jpeg_codec.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/skia_util.h"
+#include "ui/snapshot/snapshot.h"
 
 #define EXPECT_SIZE_EQ(expected, actual)               \
   do {                                                 \
@@ -464,10 +466,20 @@ bool DecodePNG(std::string base64_data, SkBitmap* bitmap) {
       bitmap);
 }
 
+std::unique_ptr<SkBitmap> DecodeJPEG(std::string base64_data) {
+  std::string jpeg_data;
+  if (!base::Base64Decode(base64_data, &jpeg_data))
+    return nullptr;
+  return gfx::JPEGCodec::Decode(
+      reinterpret_cast<unsigned const char*>(jpeg_data.data()),
+      jpeg_data.size());
+}
+
 // Adapted from cc::ExactPixelComparator.
 bool MatchesBitmap(const SkBitmap& expected_bmp,
                    const SkBitmap& actual_bmp,
-                   const gfx::Rect& matching_mask) {
+                   const gfx::Rect& matching_mask,
+                   int error_limit) {
   // Number of pixels with an error
   int error_pixels_count = 0;
 
@@ -490,7 +502,8 @@ bool MatchesBitmap(const SkBitmap& expected_bmp,
     for (int y = matching_mask.y(); y < matching_mask.height(); ++y) {
       SkColor actual_color = actual_bmp.getColor(x, y);
       SkColor expected_color = expected_bmp.getColor(x, y);
-      if (actual_color != expected_color) {
+      if (std::abs(static_cast<int32_t>(actual_color) -
+                   static_cast<int32_t>(expected_color)) > error_limit) {

[Avi (use Gerrit), 2017/01/09 16:57:33]

An SkColor is a set of four 8-bit values packed into a 32-bit integer. See
SkColorSetARGB():

(alpha) | (red) | (green) | (blue)

Using std::abs in this way allows for wiggle room in the blue channel (as that
channel is in the lowest bit positions), but in no other channel. I don't see
how this is a meaningful comparison.

You have to at least compare all the channels, so if you have to write a
comparison helper function we might as well go all the way.

bool ColorsMatchWithinLimit(SkColor color1, SkColor color2, int32_t error_limit)
{
  auto r_distance = std::abs(static_cast<int32_t>(SkColorGetR(color1)) -
      static_cast<int32_t>(SkColorGetR(color2)));
  auto g_distance = std::abs(static_cast<int32_t>(SkColorGetG(color1)) -
      static_cast<int32_t>(SkColorGetG(color2)));
  auto b_distance = std::abs(static_cast<int32_t>(SkColorGetB(color1)) -
      static_cast<int32_t>(SkColorGetB(color2)));

  return r_distance * r_distance + g_distance * g_distance + b_distance *
b_distance <
      error_limit * error_limit;
}

And then use that.

[Eric Seckler, 2017/01/11 15:58:44]

Uhh, yeah. Wasn't thinking here. Thank you for the helper function!

         if (error_pixels_count < 10) {
           LOG(ERROR) << "Pixel (" << x << "," << y << "): expected "
                      << expected_color << " actual " << actual_color;
@@ -513,19 +526,38 @@ bool MatchesBitmap(const SkBitmap& expected_bmp,
 
 class CaptureScreenshotTest : public DevToolsProtocolTest {
  protected:
-  void CaptureScreenshotAndCompareTo(const SkBitmap& expected_bitmap) {
-    SendCommand("Page.captureScreenshot", nullptr);
+  enum ScreenshotEncoding { ENCODING_PNG, ENCODING_JPEG };
+  void CaptureScreenshotAndCompareTo(const SkBitmap& expected_bitmap,
+                                     ScreenshotEncoding encoding) {
+    std::unique_ptr<base::DictionaryValue> params(new base::DictionaryValue());
+    params->SetString("format", encoding == ENCODING_PNG ? "png" : "jpeg");
+    params->SetInteger("quality", 100);
+    SendCommand("Page.captureScreenshot", std::move(params));
+
     std::string base64;
     EXPECT_TRUE(result_->GetString("data", &base64));
-    SkBitmap result_bitmap;
-    EXPECT_TRUE(DecodePNG(base64, &result_bitmap));
+    std::unique_ptr<SkBitmap> result_bitmap;
+    int error_limit = 0;
+    if (encoding == ENCODING_PNG) {
+      result_bitmap.reset(new SkBitmap());
+      EXPECT_TRUE(DecodePNG(base64, result_bitmap.get()));
+    } else {
+      result_bitmap = DecodeJPEG(base64);
+      // Even with quality 100, jpeg isn't lossless. So, we allow some skew in
+      // pixel values. Not that this assumes that there is no skew in pixel
+      // positions, so will only work reliably if all pixels have equal values.
+      error_limit = 3;
+    }
+    EXPECT_TRUE(result_bitmap);
+
     gfx::Rect matching_mask(gfx::SkIRectToRect(expected_bitmap.bounds()));
 #if defined(OS_MACOSX)
     // Mask out the corners, which may be drawn differently on Mac because of
     // rounded corners.
     matching_mask.Inset(4, 4, 4, 4);
 #endif
-    EXPECT_TRUE(MatchesBitmap(expected_bitmap, result_bitmap, matching_mask));
+    EXPECT_TRUE(MatchesBitmap(expected_bitmap, *result_bitmap, matching_mask,
+                              error_limit));
   }
 
   // Takes a screenshot of a colored box that is positioned inside the frame.
@@ -584,7 +616,7 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
     expected_bitmap.allocN32Pixels(scaled_box_size.width(),
                                    scaled_box_size.height());
     expected_bitmap.eraseColor(SkColorSetRGB(0x00, 0x00, 0xff));
-    CaptureScreenshotAndCompareTo(expected_bitmap);
+    CaptureScreenshotAndCompareTo(expected_bitmap, ENCODING_PNG);
 
     // Reset for next screenshot.
     SendCommand("Emulation.resetViewport", nullptr);
@@ -604,11 +636,32 @@ IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest, CaptureScreenshot) {
   // See crbug.com/653637.
   if (base::SysInfo::IsLowEndDevice()) return;
 
-  shell()->LoadURL(GURL("about:blank"));
+  shell()->LoadURL(
+      GURL("data:text/html,<body style='background:#123456'></body>"));
+  WaitForLoadStop(shell()->web_contents());
+  Attach();
+  SkBitmap expected_bitmap;
+  // We compare against the actual physical backing size rather than the
+  // view size, because the view size is stored adjusted for DPI and only in
+  // integer precision.
+  gfx::Size view_size = static_cast<RenderWidgetHostViewBase*>(
+                            shell()->web_contents()->GetRenderWidgetHostView())
+                            ->GetPhysicalBackingSize();
+  expected_bitmap.allocN32Pixels(view_size.width(), view_size.height());
+  expected_bitmap.eraseColor(SkColorSetRGB(0x12, 0x34, 0x56));
+  CaptureScreenshotAndCompareTo(expected_bitmap, ENCODING_PNG);
+}
+
+IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest, CaptureScreenshotJpeg) {
+  // This test fails consistently on low-end Android devices.
+  // See crbug.com/653637.

[Avi (use Gerrit), 2017/01/09 16:57:33]

This bug is about mismatches in color. Do we want to re-enable the tests, but
for low-end devices crank up the error limit now that we have that ability? At
least we can get some basic coverage, and stop things from getting worse :)

[Eric Seckler, 2017/01/11 15:58:44]

I'm actually not sure if this would still be happening on the bots, since the
original bot that was flaky doesn't seem to exist anymore. Thus, I'd be happy to
re-enable and (if necessary) use a higher error limit on low-end devices - but
let's do that separately :)

Added a todo for this here.

[Avi (use Gerrit), 2017/01/11 17:09:20]

SGTM!

+  if (base::SysInfo::IsLowEndDevice())
+    return;
+
+  shell()->LoadURL(
+      GURL("data:text/html,<body style='background:#123456'></body>"));
+  WaitForLoadStop(shell()->web_contents());
   Attach();
-  EXPECT_TRUE(
-      content::ExecuteScript(shell()->web_contents()->GetRenderViewHost(),
-                             "document.body.style.background = '#123456'"));
   SkBitmap expected_bitmap;
   // We compare against the actual physical backing size rather than the
   // view size, because the view size is stored adjusted for DPI and only in
@@ -618,7 +671,7 @@ IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest, CaptureScreenshot) {
                             ->GetPhysicalBackingSize();
   expected_bitmap.allocN32Pixels(view_size.width(), view_size.height());
   expected_bitmap.eraseColor(SkColorSetRGB(0x12, 0x34, 0x56));
-  CaptureScreenshotAndCompareTo(expected_bitmap);
+  CaptureScreenshotAndCompareTo(expected_bitmap, ENCODING_JPEG);
 }
 
 // Setting frame size (through RWHV) is not supported on Android.