rebaseline_server: use just skpdiff, not Python Image Library

tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp

Index: tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
diff --git a/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp b/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
index 768bfc7d21aea34811db02baf291ba2e39d6f68d..345baccd0f50c8a5b8e6cb3f023f24cc56a074ad 100644
--- a/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
+++ b/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
@@ -10,11 +10,25 @@
 #include "SkBitmap.h"
 #include "skpdiff_util.h"
 
+/*
+ * TODO: It would be more flexible, and probably more efficient, if a diffing
+ * module could return an arbitrary number of results, each one as a name/value
+ * pair.

[djsollen, 2014/06/12 13:27:00]

isn't that the same as adding them explicitly to the Results struct with a way
to indicate they are optional?  Seems like that would be more efficient than
maintaining an arbitrary list while ensuring that the diff tools only set values
that the reader knows how to parse.

[epoger, 2014/06/12 14:02:34]

Yeah, that seems quite reasonable, thanks for the idea.

I look forward to discussing with you the pros and cons of the
various-differs-running-in-parallel model that is currently used in skpdiff.  It
bugs me that multiple threads are simultaneously iterating over the same image
pairs... seems like they are wasting throughput to improve latency, and it makes
the process as a whole take longer.

+ *
+ * So, this one could return:
+ *   "num_differing_pixels": 1111
+ *   "ratio_differing_pixels": 0.3984
+ *   "max_red_diff": 32
+ *   "max_green_diff": 121
+ *   "max_blue_diff": 84
+ */
+
 const char* SkDifferentPixelsMetric::getName() const {
     return "different_pixels";
 }
 
-bool SkDifferentPixelsMetric::diff(SkBitmap* baseline, SkBitmap* test, bool computeMask,
+bool SkDifferentPixelsMetric::diff(SkBitmap* baseline, SkBitmap* test, bool computeAlphaMask,
+                                   bool computeRgbDiff, bool computeWhiteDiff,
                                    Result* result) const {
     double startTime = get_seconds();
 
@@ -22,17 +36,34 @@ bool SkDifferentPixelsMetric::diff(SkBitmap* baseline, SkBitmap* test, bool comp
     if (baseline->width() != test->width() || baseline->height() != test->height() ||
         baseline->width() <= 0 || baseline->height() <= 0 ||
         baseline->config() != test->config()) {
+        SkASSERT(baseline->width() == test->width());
+        SkASSERT(baseline->height() == test->height());
+        SkASSERT(baseline->width() > 0);
+        SkASSERT(baseline->height() > 0);
+        SkASSERT(baseline->config() == test->config());
         return false;
     }
 
     int width = baseline->width();
     int height = baseline->height();
+    uint32_t maxRedDiff = 0;
+    uint32_t maxGreenDiff = 0;
+    uint32_t maxBlueDiff = 0;
 
-    // Prepare the POI alpha mask if needed
-    if (computeMask) {
+    // Prepare any bitmaps we will be filling in
+    if (computeAlphaMask) {
         result->poiAlphaMask.allocPixels(SkImageInfo::MakeA8(width, height));
         result->poiAlphaMask.eraseARGB(SK_AlphaOPAQUE, 0, 0, 0);
     }
+    if (computeRgbDiff) {
+        result->rgbDiffBitmap.allocPixels(SkImageInfo::Make(width, height, baseline->colorType(),
+                                                            kPremul_SkAlphaType));
+        result->rgbDiffBitmap.eraseARGB(SK_AlphaTRANSPARENT, 0, 0, 0);
+    }
+    if (computeWhiteDiff) {
+        result->whiteDiffBitmap.allocPixels(SkImageInfo::MakeN32Premul(width, height));
+        result->whiteDiffBitmap.eraseARGB(SK_AlphaOPAQUE, 0, 0, 0);
+    }
 
     // Prepare the pixels for comparison
     result->poiCount = 0;
@@ -40,24 +71,55 @@ bool SkDifferentPixelsMetric::diff(SkBitmap* baseline, SkBitmap* test, bool comp
     test->lockPixels();
     for (int y = 0; y < height; y++) {
         // Grab a row from each image for easy comparison
-        unsigned char* baselineRow = (unsigned char*)baseline->getAddr(0, y);
-        unsigned char* testRow = (unsigned char*)test->getAddr(0, y);
+        // TODO: The code below already assumes 4 bytes per pixel, so I think
+        // we could just call getAddr32() to save a little time.
+        // OR, if we want to play it safe, call ComputeBytesPerPixel instead
+        // of assuming 4 bytes per pixel.
+        uint32_t* baselineRow = static_cast<uint32_t *>(baseline->getAddr(0, y));
+        uint32_t* testRow = static_cast<uint32_t *>(test->getAddr(0, y));
         for (int x = 0; x < width; x++) {
             // Compare one pixel at a time so each differing pixel can be noted
-            if (memcmp(&baselineRow[x * 4], &testRow[x * 4], 4) != 0) {
+            uint32_t baselinePixel = baselineRow[x];
+            uint32_t testPixel = testRow[x];
+            if (baselinePixel != testPixel) {
                 result->poiCount++;
-                if (computeMask) {
+
+                uint32_t redDiff = abs(SkColorGetR(baselinePixel) - SkColorGetR(testPixel));

[epoger, 2014/06/12 07:02:07]

This section of the code seems like a good place to look for performance
killers.

+                if (redDiff > maxRedDiff) {maxRedDiff = redDiff;}
+                uint32_t greenDiff = abs(SkColorGetG(baselinePixel) - SkColorGetG(testPixel));
+                if (greenDiff > maxGreenDiff) {maxGreenDiff = greenDiff;}
+                uint32_t blueDiff = abs(SkColorGetB(baselinePixel) - SkColorGetB(testPixel));
+                if (blueDiff > maxBlueDiff) {maxBlueDiff = blueDiff;}
+
+                if (computeAlphaMask) {
                     *result->poiAlphaMask.getAddr8(x,y) = SK_AlphaTRANSPARENT;
                 }
+                if (computeRgbDiff) {
+                    *result->rgbDiffBitmap.getAddr32(x,y) =
+                        SkColorSetRGB(redDiff, greenDiff, blueDiff);
+                }
+                if (computeWhiteDiff) {
+                    *result->whiteDiffBitmap.getAddr32(x,y) = SK_ColorWHITE;
+                }
             }
         }
     }
     test->unlockPixels();
     baseline->unlockPixels();
 
-    if (computeMask) {
+    result->maxRedDiff = maxRedDiff;
+    result->maxGreenDiff = maxGreenDiff;
+    result->maxBlueDiff = maxBlueDiff;
+
+    if (computeAlphaMask) {
         result->poiAlphaMask.unlockPixels();
     }
+    if (computeRgbDiff) {
+        result->rgbDiffBitmap.unlockPixels();
+    }
+    if (computeWhiteDiff) {
+        result->whiteDiffBitmap.unlockPixels();
+    }
 
     // Calculates the percentage of identical pixels
     result->result = 1.0 - ((double)result->poiCount / (width * height));