fix multithread related crashes in skpdiff

BUG=skia:1798
R=mtklein@google.com, scroggo@google.com

Review URL: https://codereview.chromium.org/60833002

git-svn-id: http://skia.googlecode.com/svn/trunk@12252 2bbb7eff-a529-9590-31e7-b0007b416f81

12 files changed, 187 insertions, 424 deletions

diff --git a/gyp/tools.gyp b/gyp/tools.gyp
index afd4934be8..f462c50e5e 100644
--- a/gyp/tools.gyp
+++ b/gyp/tools.gyp
@@ -65,7 +65,8 @@
         '../tools/flags/SkCommandLineFlags.cpp',
       ],
       'include_dirs': [
-        '../tools/flags'
+        '../tools/flags',
+        '../src/core/', # needed for SkTLList.h
       ],
       'dependencies': [
         'skia_lib.gyp:skia_lib',
diff --git a/tools/skpdiff/SkCLImageDiffer.cpp b/tools/skpdiff/SkCLImageDiffer.cpp
index d1e6958ab9..b99ef40d70 100644
--- a/tools/skpdiff/SkCLImageDiffer.cpp
+++ b/tools/skpdiff/SkCLImageDiffer.cpp
@@ -84,7 +84,7 @@ bool SkCLImageDiffer::loadKernelSource(const char source[], const char name[], c
     return true;
 }
 
-bool SkCLImageDiffer::makeImage2D(SkBitmap* bitmap, cl_mem* image) {
+bool SkCLImageDiffer::makeImage2D(SkBitmap* bitmap, cl_mem* image) const {
     cl_int imageErr;
     cl_image_format bitmapFormat;
     switch (bitmap->config()) {
diff --git a/tools/skpdiff/SkCLImageDiffer.h b/tools/skpdiff/SkCLImageDiffer.h
index 032ee6f990..6e9c2dc0cf 100644
--- a/tools/skpdiff/SkCLImageDiffer.h
+++ b/tools/skpdiff/SkCLImageDiffer.h
@@ -26,7 +26,7 @@ class SkCLImageDiffer : public SkImageDiffer {
 public:
     SkCLImageDiffer();
 
-    virtual bool requiresOpenCL() SK_OVERRIDE { return true; }
+    virtual bool requiresOpenCL() const SK_OVERRIDE { return true; }
 
     /**
      * Initializes the OpenCL resources this differ needs to work
@@ -80,12 +80,15 @@ protected:
      * @param  image  A pointer to return the allocated image to
      * @return        True on success, false otherwise
      */
-    bool makeImage2D(SkBitmap* bitmap, cl_mem* image);
+    bool makeImage2D(SkBitmap* bitmap, cl_mem* image) const;
 
     cl_device_id     fDevice;
     cl_context       fContext;
     cl_command_queue fCommandQueue;
 
+protected:
+    bool fIsGood;
+
 private:
 
     typedef SkImageDiffer INHERITED;
diff --git a/tools/skpdiff/SkDiffContext.cpp b/tools/skpdiff/SkDiffContext.cpp
index ce1fad9d58..f64aeac2ac 100644
--- a/tools/skpdiff/SkDiffContext.cpp
+++ b/tools/skpdiff/SkDiffContext.cpp
@@ -14,28 +14,18 @@
 #include "SkThreadPool.h"
 
 #include "SkDiffContext.h"
-#include "SkImageDiffer.h"
 #include "skpdiff_util.h"
 
 // Truncates the number of points of interests in JSON output to not freeze the parser
 static const int kMaxPOI = 100;
 
 SkDiffContext::SkDiffContext() {
-    fRecords = NULL;
     fDiffers = NULL;
     fDifferCount = 0;
     fThreadCount = SkThreadPool::kThreadPerCore;
 }
 
 SkDiffContext::~SkDiffContext() {
-    // Delete the record linked list
-    DiffRecord* currentRecord = fRecords;
-    while (NULL != currentRecord) {
-        DiffRecord* nextRecord = currentRecord->fNext;
-        SkDELETE(currentRecord);
-        currentRecord = nextRecord;
-    }
-
     if (NULL != fDiffers) {
         SkDELETE_ARRAY(fDiffers);
     }
@@ -82,6 +72,7 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     // Load the images at the paths
     SkBitmap baselineBitmap;
     SkBitmap testBitmap;
+    SkAutoMutexAcquire imageLock(fImageMutex);
     if (!SkImageDecoder::DecodeFile(baselinePath, &baselineBitmap)) {
         SkDebugf("Failed to load bitmap \"%s\"\n", baselinePath);
         return;
@@ -90,68 +81,62 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
         SkDebugf("Failed to load bitmap \"%s\"\n", testPath);
         return;
     }
+    imageLock.release();
 
     // Setup a record for this diff
-    DiffRecord* newRecord = SkNEW(DiffRecord);
-    newRecord->fBaselinePath = baselinePath;
-    newRecord->fTestPath = testPath;
-    newRecord->fNext = fRecords;
-    fRecords = newRecord;
+    fRecordMutex.acquire();
+    DiffRecord* newRecord = fRecords.addToHead(DiffRecord());
+    fRecordMutex.release();
 
     // compute the common name
     SkString baseName = SkOSPath::SkBasename(baselinePath);
     SkString testName = SkOSPath::SkBasename(testPath);
     newRecord->fCommonName = get_common_prefix(baseName, testName);
 
+    newRecord->fBaselinePath = baselinePath;
+    newRecord->fTestPath = testPath;
+
     bool alphaMaskPending = false;
-    bool alphaMaskCreated = false;
+
+    // only enable alpha masks if a difference dir has been provided
+    if (!fDifferenceDir.isEmpty()) {
+        alphaMaskPending = true;
+    }
 
     // Perform each diff
     for (int differIndex = 0; differIndex < fDifferCount; differIndex++) {
         SkImageDiffer* differ = fDiffers[differIndex];
-        // TODO only enable for one differ
-        if (!alphaMaskCreated && !fDifferenceDir.isEmpty()) {
-            alphaMaskPending = differ->enablePOIAlphaMask();
+
+        // Copy the results into data for this record
+        DiffData& diffData = newRecord->fDiffs.push_back();
+        diffData.fDiffName = differ->getName();
+
+        if (!differ->diff(&baselineBitmap, &testBitmap, alphaMaskPending, &diffData.fResult)) {
+            // if the diff failed the remove its entry from the list
+            newRecord->fDiffs.pop_back();
+            continue;
         }
-        int diffID = differ->queueDiff(&baselineBitmap, &testBitmap);
-        if (diffID >= 0) {
-
-            // Copy the results into data for this record
-            DiffData& diffData = newRecord->fDiffs.push_back();
-
-            diffData.fDiffName = differ->getName();
-            diffData.fResult = differ->getResult(diffID);
-
-            int poiCount = differ->getPointsOfInterestCount(diffID);
-            SkIPoint* poi = differ->getPointsOfInterest(diffID);
-            diffData.fPointsOfInterest.append(poiCount, poi);
-
-            if (alphaMaskPending
-                    && SkImageDiffer::RESULT_CORRECT != diffData.fResult
-                    && newRecord->fDifferencePath.isEmpty()) {
-                newRecord->fDifferencePath = SkOSPath::SkPathJoin(fDifferenceDir.c_str(),
-                                                                  newRecord->fCommonName.c_str());
-
-                // compute the image diff and output it
-                SkBitmap* alphaMask = differ->getPointsOfInterestAlphaMask(diffID);
-                SkBitmap copy;
-                alphaMask->copyTo(&copy, SkBitmap::kARGB_8888_Config);
-                SkImageEncoder::EncodeFile(newRecord->fDifferencePath.c_str(), copy,
-                                           SkImageEncoder::kPNG_Type, 100);
-            }
 
-            if (alphaMaskPending) {
-                alphaMaskPending = false;
-                alphaMaskCreated = true;
-            }
+        if (alphaMaskPending
+                && SkImageDiffer::RESULT_CORRECT != diffData.fResult.result
+                && !diffData.fResult.poiAlphaMask.empty()
+                && !newRecord->fCommonName.isEmpty()) {
+
+            newRecord->fDifferencePath = SkOSPath::SkPathJoin(fDifferenceDir.c_str(),
+                                                              newRecord->fCommonName.c_str());
+
+            // compute the image diff and output it
+            SkBitmap copy;
+            diffData.fResult.poiAlphaMask.copyTo(&copy, SkBitmap::kARGB_8888_Config);
+            SkImageEncoder::EncodeFile(newRecord->fDifferencePath.c_str(), copy,
+                                       SkImageEncoder::kPNG_Type, 100);
 
-            // Because we are doing everything synchronously for now, we are done with the diff
-            // after reading it.
-            differ->deleteDiff(diffID);
+            // cleanup the existing bitmap to free up resources;
+            diffData.fResult.poiAlphaMask.reset();
+
+            alphaMaskPending = false;
         }
     }
-
-    // if we get a difference and we want the alpha mask then compute it here.
 }
 
 class SkThreadedDiff : public SkRunnable {
@@ -241,7 +226,9 @@ void SkDiffContext::diffPatterns(const char baselinePattern[], const char testPa
 }
 
 void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
-    DiffRecord* currentRecord = fRecords;
+    SkTLList<DiffRecord>::Iter iter(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    DiffRecord* currentRecord = iter.get();
+
     if (useJSONP) {
         stream.writeText("var SkPDiffRecords = {\n");
     } else {
@@ -281,27 +268,13 @@ void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
                     stream.writeText("\",\n");
 
                     stream.writeText("                    \"result\": ");
-                    stream.writeScalarAsText((SkScalar)data.fResult);
+                    stream.writeScalarAsText((SkScalar)data.fResult.result);
                     stream.writeText(",\n");
 
-                    stream.writeText("                    \"pointsOfInterest\": [\n");
-                    for (int poiIndex = 0; poiIndex < data.fPointsOfInterest.count() &&
-                                           poiIndex < kMaxPOI; poiIndex++) {
-                        SkIPoint poi = data.fPointsOfInterest[poiIndex];
-                        stream.writeText("                        [");
-                        stream.writeDecAsText(poi.x());
-                        stream.writeText(",");
-                        stream.writeDecAsText(poi.y());
-                        stream.writeText("]");
-
-                        // JSON does not allow trailing commas
-                        if (poiIndex + 1 < data.fPointsOfInterest.count() &&
-                            poiIndex + 1 < kMaxPOI) {
-                            stream.writeText(",");
-                        }
-                        stream.writeText("\n");
-                    }
-                    stream.writeText("                    ]\n");
+                    stream.writeText("                    \"pointsOfInterest\": ");
+                    stream.writeDecAsText(data.fResult.poiCount);
+                    stream.writeText("\n");
+
                 stream.writeText("                }");
 
                 // JSON does not allow trailing commas
@@ -314,12 +287,13 @@ void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
 
         stream.writeText("        }");
 
+        currentRecord = iter.next();
+
         // JSON does not allow trailing commas
-        if (NULL != currentRecord->fNext) {
+        if (NULL != currentRecord) {
             stream.writeText(",");
         }
         stream.writeText("\n");
-        currentRecord = currentRecord->fNext;
     }
     stream.writeText("    ]\n");
     if (useJSONP) {
@@ -335,7 +309,8 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
 
     stream.writeText("key");
 
-    DiffRecord* currentRecord = fRecords;
+    SkTLList<DiffRecord>::Iter iter(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    DiffRecord* currentRecord = iter.get();
 
     // Write CSV header and create a dictionary of all columns.
     while (NULL != currentRecord) {
@@ -348,14 +323,15 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
                 cntColumns++;
             }
         }
-        currentRecord = currentRecord->fNext;
+        currentRecord = iter.next();
     }
     stream.writeText("\n");
 
     double values[100];
     SkASSERT(cntColumns < 100);  // Make the array larger, if we ever have so many diff types.
 
-    currentRecord = fRecords;
+    SkTLList<DiffRecord>::Iter iter2(fRecords, SkTLList<DiffRecord>::Iter::kHead_IterStart);
+    currentRecord = iter2.get();
     while (NULL != currentRecord) {
         for (int i = 0; i < cntColumns; i++) {
             values[i] = -1;
@@ -366,7 +342,7 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
             int index = -1;
             SkAssertResult(columns.find(data.fDiffName, &index));
             SkASSERT(index >= 0 && index < cntColumns);
-            values[index] = data.fResult;
+            values[index] = data.fResult.result;
         }
 
         const char* filename = currentRecord->fBaselinePath.c_str() +
@@ -384,6 +360,6 @@ void SkDiffContext::outputCsv(SkWStream& stream) {
         }
         stream.writeText("\n");
 
-        currentRecord = currentRecord->fNext;
+        currentRecord = iter2.next();
     }
 }
diff --git a/tools/skpdiff/SkDiffContext.h b/tools/skpdiff/SkDiffContext.h
index 9669ae0ad3..2d971059c2 100644
--- a/tools/skpdiff/SkDiffContext.h
+++ b/tools/skpdiff/SkDiffContext.h
@@ -8,12 +8,14 @@
 #ifndef SkDiffContext_DEFINED
 #define SkDiffContext_DEFINED
 
+#include "SkImageDiffer.h"
 #include "SkString.h"
 #include "SkTArray.h"
 #include "SkTDArray.h"
+#include "SkTLList.h"
+#include "SkThread.h"
 
 class SkWStream;
-class SkImageDiffer;
 
 /**
  * Collects records of diffs and outputs them as JSON.
@@ -64,29 +66,33 @@ public:
      * Output the records of each diff in JSON.
      *
      * The format of the JSON document is one top level array named "records".
-     * Each record in the array is an object with both a "baselinePath" and "testPath" string field.
+     * Each record in the array is an object with the following values:
+     *    "commonName"     : string containing the common prefix of the baselinePath
+     *                       and testPath filenames
+     *    "baselinePath"   : string containing the path to the baseline image
+     *    "testPath"       : string containing the path to the test image
+     *    "differencePath" : (optional) string containing the path to an alpha
+     *                       mask of the pixel difference between the baseline
+     *                       and test images
+     *
      * They also have an array named "diffs" with each element being one diff record for the two
      * images indicated in the above field.
      * A diff record includes:
      *    "differName"       : string name of the diff metric used
      *    "result"           : numerical result of the diff
-     *    "pointsOfInterest" : an array of coordinates (stored as a 2-array of ints) of interesting
-     *                         points
      *
      * Here is an example:
      *
      * {
      *     "records": [
      *         {
-     *             "baselinePath": "queue.png",
-     *             "testPath": "queue.png",
+     *             "commonName": "queue.png",
+     *             "baselinePath": "/a/queue.png",
+     *             "testPath": "/b/queue.png",
      *             "diffs": [
      *                 {
      *                     "differName": "different_pixels",
      *                     "result": 1,
-     *                     "pointsOfInterest": [
-     *                         [285,279],
-     *                     ]
      *                 }
      *             ]
      *         }
@@ -107,8 +113,7 @@ public:
 private:
     struct DiffData {
         const char* fDiffName;
-        double fResult;
-        SkTDArray<SkIPoint> fPointsOfInterest;
+        SkImageDiffer::Result fResult;
     };
 
     struct DiffRecord {
@@ -117,13 +122,22 @@ private:
         SkString           fBaselinePath;
         SkString               fTestPath;
         SkTArray<DiffData>        fDiffs;
-        DiffRecord*                fNext;
     };
 
+    // This is needed to work around a bug in the multithreaded case where the
+    // image decoders are crashing when large numbers of threads are invoking
+    // the decoder at the same time.
+    // see https://code.google.com/p/skia/issues/detail?id=1803
+    SkMutex fImageMutex;
+
+    // Used to protect access to fRecords and ensure only one thread is
+    // adding new entries at a time.
+    SkMutex fRecordMutex;
+
     // We use linked list for the records so that their pointers remain stable. A resizable array
     // might change its pointers, which would make it harder for async diffs to record their
     // results.
-    DiffRecord * fRecords;
+    SkTLList<DiffRecord> fRecords;
 
     SkImageDiffer** fDiffers;
     int fDifferCount;
diff --git a/tools/skpdiff/SkDifferentPixelsMetric.h b/tools/skpdiff/SkDifferentPixelsMetric.h
index 614f920356..06c56b1cca 100644
--- a/tools/skpdiff/SkDifferentPixelsMetric.h
+++ b/tools/skpdiff/SkDifferentPixelsMetric.h
@@ -27,17 +27,9 @@ class SkDifferentPixelsMetric :
     public SkImageDiffer {
 #endif
 public:
-    SkDifferentPixelsMetric() : fPOIAlphaMask(false) {}
-
-    virtual const char* getName() SK_OVERRIDE;
-    virtual bool enablePOIAlphaMask() SK_OVERRIDE;
-    virtual int queueDiff(SkBitmap* baseline, SkBitmap* test) SK_OVERRIDE;
-    virtual void deleteDiff(int id) SK_OVERRIDE;
-    virtual bool isFinished(int id) SK_OVERRIDE;
-    virtual double getResult(int id) SK_OVERRIDE;
-    virtual int getPointsOfInterestCount(int id) SK_OVERRIDE;
-    virtual SkIPoint* getPointsOfInterest(int id) SK_OVERRIDE;
-    virtual SkBitmap* getPointsOfInterestAlphaMask(int id) SK_OVERRIDE;
+    virtual const char* getName() const SK_OVERRIDE;
+    virtual bool diff(SkBitmap* baseline, SkBitmap* test, bool computeMask,
+                      Result* result) const SK_OVERRIDE;
 
 protected:
 #if SK_SUPPORT_OPENCL
@@ -45,11 +37,6 @@ protected:
 #endif
 
 private:
-    bool fPOIAlphaMask;
-
-    struct QueuedDiff;
-    SkTDArray<QueuedDiff> fQueuedDiffs;
-
 #if SK_SUPPORT_OPENCL
     cl_kernel fKernel;
 
diff --git a/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp b/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
index a3e4a383f2..27c7a135d7 100644
--- a/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
+++ b/tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp
@@ -5,60 +5,39 @@
  * found in the LICENSE file.
  */
 
-#include <cstring>
+#include "SkDifferentPixelsMetric.h"
 
 #include "SkBitmap.h"
-
-#include "SkDifferentPixelsMetric.h"
 #include "skpdiff_util.h"
 
-struct SkDifferentPixelsMetric::QueuedDiff {
-    bool finished;
-    double result;
-    SkTDArray<SkIPoint>* poi;
-    SkBitmap poiAlphaMask;
-};
-
-const char* SkDifferentPixelsMetric::getName() {
+const char* SkDifferentPixelsMetric::getName() const {
     return "different_pixels";
 }
 
-bool SkDifferentPixelsMetric::enablePOIAlphaMask() {
-    fPOIAlphaMask = true;
-    return true;
-}
-
-int SkDifferentPixelsMetric::queueDiff(SkBitmap* baseline, SkBitmap* test) {
+bool SkDifferentPixelsMetric::diff(SkBitmap* baseline, SkBitmap* test, bool computeMask,
+                                   Result* result) const {
     double startTime = get_seconds();
-    int diffID = fQueuedDiffs.count();
-    QueuedDiff* diff = fQueuedDiffs.push();
-    SkTDArray<SkIPoint>* poi = diff->poi = new SkTDArray<SkIPoint>();
-
-    // If we never end up running the kernel, include some safe defaults in the result.
-    diff->finished = false;
-    diff->result = -1;
 
     // Ensure the images are comparable
     if (baseline->width() != test->width() || baseline->height() != test->height() ||
         baseline->width() <= 0 || baseline->height() <= 0 ||
         baseline->config() != test->config()) {
-        diff->finished = true;
-        return diffID;
+        return false;
     }
 
     int width = baseline->width();
     int height = baseline->height();
-    int differentPixelsCount = 0;
 
     // Prepare the POI alpha mask if needed
-    if (fPOIAlphaMask) {
-        diff->poiAlphaMask.setConfig(SkBitmap::kA8_Config, width, height);
-        diff->poiAlphaMask.allocPixels();
-        diff->poiAlphaMask.lockPixels();
-        diff->poiAlphaMask.eraseARGB(SK_AlphaOPAQUE, 0, 0, 0);
+    if (computeMask) {
+        result->poiAlphaMask.setConfig(SkBitmap::kA8_Config, width, height);
+        result->poiAlphaMask.allocPixels();
+        result->poiAlphaMask.lockPixels();
+        result->poiAlphaMask.eraseARGB(SK_AlphaOPAQUE, 0, 0, 0);
     }
 
     // Prepare the pixels for comparison
+    result->poiCount = 0;
     baseline->lockPixels();
     test->lockPixels();
     for (int y = 0; y < height; y++) {
@@ -67,11 +46,10 @@ int SkDifferentPixelsMetric::queueDiff(SkBitmap* baseline, SkBitmap* test) {
         unsigned char* testRow = (unsigned char*)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 (std::memcmp(&baselineRow[x * 4], &testRow[x * 4], 4) != 0) {
-                poi->push()->set(x, y);
-                differentPixelsCount++;
-                if (fPOIAlphaMask) {
-                    *diff->poiAlphaMask.getAddr8(x,y) = SK_AlphaTRANSPARENT;
+            if (memcmp(&baselineRow[x * 4], &testRow[x * 4], 4) != 0) {
+                result->poiCount++;
+                if (computeMask) {
+                    *result->poiAlphaMask.getAddr8(x,y) = SK_AlphaTRANSPARENT;
                 }
             }
         }
@@ -79,41 +57,13 @@ int SkDifferentPixelsMetric::queueDiff(SkBitmap* baseline, SkBitmap* test) {
     test->unlockPixels();
     baseline->unlockPixels();
 
-    // Calculates the percentage of identical pixels
-    diff->result = 1.0 - ((double)differentPixelsCount / (width * height));
-
-    SkDebugf("Time: %f\n", (get_seconds() - startTime));
-
-    return diffID;
-}
-
-void SkDifferentPixelsMetric::deleteDiff(int id) {
-    if (NULL != fQueuedDiffs[id].poi)
-    {
-        delete fQueuedDiffs[id].poi;
-        fQueuedDiffs[id].poi = NULL;
+    if (computeMask) {
+        result->poiAlphaMask.unlockPixels();
     }
-}
-
-bool SkDifferentPixelsMetric::isFinished(int id) {
-    return fQueuedDiffs[id].finished;
-}
-
-double SkDifferentPixelsMetric::getResult(int id) {
-    return fQueuedDiffs[id].result;
-}
 
-int SkDifferentPixelsMetric::getPointsOfInterestCount(int id) {
-    return fQueuedDiffs[id].poi->count();
-}
-
-SkIPoint* SkDifferentPixelsMetric::getPointsOfInterest(int id) {
-    return fQueuedDiffs[id].poi->begin();
-}
+    // Calculates the percentage of identical pixels
+    result->result = 1.0 - ((double)result->poiCount / (width * height));
+    result->timeElapsed = get_seconds() - startTime;
 
-SkBitmap* SkDifferentPixelsMetric::getPointsOfInterestAlphaMask(int id) {
-    if (fQueuedDiffs[id].poiAlphaMask.empty()) {
-        return NULL;
-    }
-    return &fQueuedDiffs[id].poiAlphaMask;
+    return true;
 }
diff --git a/tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp b/tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp
index b3f5d2d7e0..14225055f1 100644
--- a/tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp
+++ b/tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp
@@ -18,7 +18,7 @@ static const char kDifferentPixelsKernelSource[] =
     "                             CLK_FILTER_NEAREST;                           \n"
     "                                                                           \n"
     "__kernel void diff(read_only image2d_t baseline, read_only image2d_t test, \n"
-    "                   __global int* result, __global int2* poi) {             \n"
+    "                   __global int* result) {                                 \n"
     "    int2 coord = (int2)(get_global_id(0), get_global_id(1));               \n"
     "    uint4 baselinePixel = read_imageui(baseline, gInSampler, coord);       \n"
     "    uint4 testPixel = read_imageui(test, gInSampler, coord);               \n"
@@ -27,78 +27,59 @@ static const char kDifferentPixelsKernelSource[] =
     "        baselinePixel.z != testPixel.z ||                                  \n"
     "        baselinePixel.w != testPixel.w) {                                  \n"
     "                                                                           \n"
-    "        int poiIndex = atomic_inc(result);                                 \n"
-    "        poi[poiIndex] = coord;                                             \n"
+    "        atomic_inc(result);                                                \n"
+    "        // TODO: generate alpha mask                                       \n"
     "    }                                                                      \n"
     "}                                                                          \n";
 
-struct SkDifferentPixelsMetric::QueuedDiff {
-    bool finished;
-    double result;
-    int numDiffPixels;
-    SkIPoint* poi;
-    cl_mem baseline;
-    cl_mem test;
-    cl_mem resultsBuffer;
-    cl_mem poiBuffer;
-};
-
-const char* SkDifferentPixelsMetric::getName() {
+const char* SkDifferentPixelsMetric::getName() const {
     return "different_pixels";
 }
 
-bool SkDifferentPixelsMetric::enablePOIAlphaMask() {
-    return false;
-}
-
-int SkDifferentPixelsMetric::queueDiff(SkBitmap* baseline, SkBitmap* test) {
-    int diffID = fQueuedDiffs.count();
+bool SkDifferentPixelsMetric::diff(SkBitmap* baseline, SkBitmap* test, bool computeMask,
+                                   Result* result) const {
     double startTime = get_seconds();
-    QueuedDiff* diff = fQueuedDiffs.push();
+
+    if (!fIsGood) {
+        return false;
+    }
 
     // If we never end up running the kernel, include some safe defaults in the result.
-    diff->finished = false;
-    diff->result = -1.0;
-    diff->numDiffPixels = 0;
-    diff->poi = NULL;
+    result->poiCount = 0;
 
     // Ensure the images are comparable
     if (baseline->width() != test->width() || baseline->height() != test->height() ||
         baseline->width() <= 0 || baseline->height() <= 0 ||
         baseline->config() != test->config()) {
-        diff->finished = true;
-        return diffID;
+        return false;
     }
 
+    cl_mem baselineImage;
+    cl_mem testImage;
+    cl_mem resultsBuffer;
+
     // Upload images to the CL device
-    if (!this->makeImage2D(baseline, &diff->baseline) || !this->makeImage2D(test, &diff->test)) {
-        diff->finished = true;
-        fIsGood = false;
-        return -1;
+    if (!this->makeImage2D(baseline, &baselineImage) || !this->makeImage2D(test, &testImage)) {
+        SkDebugf("creation of openCL images failed");
+        return false;
     }
 
     // A small hack that makes calculating percentage difference easier later on.
-    diff->result = 1.0 / ((double)baseline->width() * baseline->height());
+    result->result = 1.0 / ((double)baseline->width() * baseline->height());
 
     // Make a buffer to store results into. It must be initialized with pointers to memory.
     static const int kZero = 0;
     // We know OpenCL won't write to it because we use CL_MEM_COPY_HOST_PTR
-    diff->resultsBuffer = clCreateBuffer(fContext, CL_MEM_READ_WRITE | CL_MEM_COPY_HOST_PTR,
-                                         sizeof(int), (int*)&kZero, NULL);
-
-    diff->poiBuffer = clCreateBuffer(fContext, CL_MEM_WRITE_ONLY,
-                                     sizeof(int) * 2 * baseline->width() * baseline->height(),
-                                     NULL, NULL);
+    resultsBuffer = clCreateBuffer(fContext, CL_MEM_READ_WRITE | CL_MEM_COPY_HOST_PTR,
+                                   sizeof(int), (int*)&kZero, NULL);
 
     // Set all kernel arguments
-    cl_int setArgErr = clSetKernelArg(fKernel, 0, sizeof(cl_mem), &diff->baseline);
-    setArgErr       |= clSetKernelArg(fKernel, 1, sizeof(cl_mem), &diff->test);
-    setArgErr       |= clSetKernelArg(fKernel, 2, sizeof(cl_mem), &diff->resultsBuffer);
-    setArgErr       |= clSetKernelArg(fKernel, 3, sizeof(cl_mem), &diff->poiBuffer);
+    cl_int setArgErr = clSetKernelArg(fKernel, 0, sizeof(cl_mem), &baselineImage);
+    setArgErr       |= clSetKernelArg(fKernel, 1, sizeof(cl_mem), &testImage);
+    setArgErr       |= clSetKernelArg(fKernel, 2, sizeof(cl_mem), &resultsBuffer);
     if (CL_SUCCESS != setArgErr) {
         SkDebugf("Set arg failed: %s\n", cl_error_to_string(setArgErr));
-        fIsGood = false;
-        return -1;
+        return false;
     }
 
     // Queue this diff on the CL device
@@ -109,60 +90,25 @@ int SkDifferentPixelsMetric::queueDiff(SkBitmap* baseline, SkBitmap* test) {
                                         NULL, 0, NULL, &event);
     if (CL_SUCCESS != enqueueErr) {
         SkDebugf("Enqueue failed: %s\n", cl_error_to_string(enqueueErr));
-        fIsGood = false;
-        return -1;
+        return false;
     }
 
     // This makes things totally synchronous. Actual queue is not ready yet
     clWaitForEvents(1, &event);
-    diff->finished = true;
 
     // Immediate read back the results
-    clEnqueueReadBuffer(fCommandQueue, diff->resultsBuffer, CL_TRUE, 0,
-                        sizeof(int), &diff->numDiffPixels, 0, NULL, NULL);
-    diff->result *= (double)diff->numDiffPixels;
-    diff->result = (1.0 - diff->result);
-
-    // Reading a buffer of size zero can cause issues on some (Mac) OpenCL platforms.
-    if (diff->numDiffPixels > 0) {
-        diff->poi = SkNEW_ARRAY(SkIPoint, diff->numDiffPixels);
-        clEnqueueReadBuffer(fCommandQueue, diff->poiBuffer, CL_TRUE, 0,
-                        sizeof(SkIPoint) * diff->numDiffPixels, diff->poi, 0, NULL, NULL);
-    }
+    clEnqueueReadBuffer(fCommandQueue, resultsBuffer, CL_TRUE, 0,
+                        sizeof(int), &result->poiCount, 0, NULL, NULL);
+    result->result *= (double)result->poiCount;
+    result->result = (1.0 - result->result);
 
     // Release all the buffers created
-    clReleaseMemObject(diff->poiBuffer);
-    clReleaseMemObject(diff->resultsBuffer);
-    clReleaseMemObject(diff->baseline);
-    clReleaseMemObject(diff->test);
-
-    SkDebugf("Time: %f\n", (get_seconds() - startTime));
-
-    return diffID;
-}
-
-void SkDifferentPixelsMetric::deleteDiff(int id) {
-    QueuedDiff* diff = &fQueuedDiffs[id];
-    if (NULL != diff->poi) {
-        SkDELETE_ARRAY(diff->poi);
-        diff->poi = NULL;
-    }
-}
+    clReleaseMemObject(resultsBuffer);
+    clReleaseMemObject(baselineImage);
+    clReleaseMemObject(testImage);
 
-bool SkDifferentPixelsMetric::isFinished(int id) {
-    return fQueuedDiffs[id].finished;
-}
-
-double SkDifferentPixelsMetric::getResult(int id) {
-    return fQueuedDiffs[id].result;
-}
-
-int SkDifferentPixelsMetric::getPointsOfInterestCount(int id) {
-    return fQueuedDiffs[id].numDiffPixels;
-}
-
-SkIPoint* SkDifferentPixelsMetric::getPointsOfInterest(int id) {
-    return fQueuedDiffs[id].poi;
+    result->timeElapsed = get_seconds() - startTime;
+    return true;
 }
 
 bool SkDifferentPixelsMetric::onInit() {
diff --git a/tools/skpdiff/SkImageDiffer.cpp b/tools/skpdiff/SkImageDiffer.cpp
index 40a34da6b1..215a799895 100644
--- a/tools/skpdiff/SkImageDiffer.cpp
+++ b/tools/skpdiff/SkImageDiffer.cpp
@@ -14,25 +14,10 @@
 const double SkImageDiffer::RESULT_CORRECT = 1.0f;
 const double SkImageDiffer::RESULT_INCORRECT = 0.0f;
 
-SkImageDiffer::SkImageDiffer()
-    : fIsGood(true) {
+SkImageDiffer::SkImageDiffer() {
 
 }
 
 SkImageDiffer::~SkImageDiffer() {
 
 }
-
-int SkImageDiffer::queueDiffOfFile(const char baseline[], const char test[]) {
-    SkBitmap baselineBitmap;
-    SkBitmap testBitmap;
-    if (!SkImageDecoder::DecodeFile(baseline, &baselineBitmap)) {
-        SkDebugf("Failed to load bitmap \"%s\"\n", baseline);
-        return -1;
-    }
-    if (!SkImageDecoder::DecodeFile(test, &testBitmap)) {
-        SkDebugf("Failed to load bitmap \"%s\"\n", test);
-        return -1;
-    }
-    return this->queueDiff(&baselineBitmap, &testBitmap);
-}
diff --git a/tools/skpdiff/SkImageDiffer.h b/tools/skpdiff/SkImageDiffer.h
index 2c1fa7e788..641bbe8f8f 100644
--- a/tools/skpdiff/SkImageDiffer.h
+++ b/tools/skpdiff/SkImageDiffer.h
@@ -8,8 +8,7 @@
 #ifndef SkImageDiffer_DEFINED
 #define SkImageDiffer_DEFINED
 
-class SkBitmap;
-struct SkIPoint;
+#include "SkBitmap.h"
 
 /**
  * Encapsulates an image difference metric algorithm that can be potentially run asynchronously.
@@ -22,92 +21,33 @@ public:
     static const double RESULT_CORRECT;
     static const double RESULT_INCORRECT;
 
+    struct Result {
+        double result;
+        int poiCount;
+        SkBitmap poiAlphaMask; // optional
+        double timeElapsed; // optional
+    };
+
     /**
      * Gets a unique and descriptive name of this differ
      * @return A statically allocated null terminated string that is the name of this differ
      */
-    virtual const char* getName() = 0;
-
-    /**
-     * Gets if this differ is in a usable state
-     * @return True if this differ can be used, false otherwise
-     */
-    bool isGood() { return fIsGood; }
+    virtual const char* getName() const = 0;
 
     /**
      * Gets if this differ needs to be initialized with and OpenCL device and context.
      */
-    virtual bool requiresOpenCL() { return false; }
-
-    /**
-     * Enables the generation of an alpha mask for all points of interest.
-     * @return True if the differ supports generating an alpha mask and false otherwise.
-     */
-    virtual bool enablePOIAlphaMask() { return false; }
-
-    /**
-     * Wraps a call to queueDiff by loading the given filenames into SkBitmaps
-     * @param  baseline The file path of the baseline image
-     * @param  test     The file path of the test image
-     * @return          The results of queueDiff with the loaded bitmaps
-     */
-    int queueDiffOfFile(const char baseline[], const char test[]);
-
-    /**
-     * Queues a diff on a pair of bitmaps to be done at some future time.
-     * @param  baseline The correct bitmap
-     * @param  test     The bitmap whose difference is being tested
-     * @return          An non-negative diff ID on success, a negative integer on failure.
-     */
-    virtual int queueDiff(SkBitmap* baseline, SkBitmap* test) = 0;
-
-    /**
-     * Gets whether a queued diff of the given id has finished
-     * @param  id The id of the queued diff to query
-     * @return    True if the queued diff is finished and has results, false otherwise
-     */
-    virtual bool isFinished(int id) = 0;
-
-    /**
-     * Deletes memory associated with a diff and its results. This may block execution until the
-     * diff is finished,
-     * @param id The id of the diff to query
-     */
-    virtual void deleteDiff(int id) = 0;
+    virtual bool requiresOpenCL() const { return false; }
 
     /**
-     * Gets the results of the queued diff of the given id. The results are only meaningful after
-     * the queued diff has finished.
-     * @param  id The id of the queued diff to query
+     * diff on a pair of bitmaps.
+     * @param  baseline    The correct bitmap
+     * @param  test        The bitmap whose difference is being tested
+     * @param  computeMask true if the differ is to attempt to create poiAlphaMask
+     * @return             true on success, and false in the case of failure
      */
-    virtual double getResult(int id) = 0;
-
-    /**
-     * Gets the number of points of interest for the diff of the given id. The results are only
-     * meaningful after the queued diff has finished.
-     * @param  id The id of the queued diff to query
-     */
-    virtual int getPointsOfInterestCount(int id) = 0;
-
-    /**
-     * Gets an array of the points of interest for the diff of the given id. The results are only
-     * meaningful after the queued diff has finished.
-     * @param  id The id of the queued diff to query
-     */
-    virtual SkIPoint* getPointsOfInterest(int id) = 0;
-
-    /*
-     * Gets a bitmap containing an alpha mask containing transparent pixels at the points of
-     * interest for the diff of the given id. The results are only meaningful after the
-     * queued diff has finished.
-     * @param  id The id of the queued diff to query
-     */
-    virtual SkBitmap* getPointsOfInterestAlphaMask(int id) { return NULL; }
-
-
-protected:
-    bool fIsGood;
+    virtual bool diff(SkBitmap* baseline, SkBitmap* test, bool computeMask,
+                      Result* result) const = 0;
 };
 
-
 #endif
diff --git a/tools/skpdiff/SkPMetric.cpp b/tools/skpdiff/SkPMetric.cpp
index 5120e129b9..22337d854d 100644
--- a/tools/skpdiff/SkPMetric.cpp
+++ b/tools/skpdiff/SkPMetric.cpp
@@ -265,7 +265,11 @@ static void convolve(const ImageL* imageL, bool vertical, ImageL* outImageL) {
     }
 }
 
-static double pmetric(const ImageLAB* baselineLAB, const ImageLAB* testLAB, SkTDArray<SkIPoint>* poi) {
+static double pmetric(const ImageLAB* baselineLAB, const ImageLAB* testLAB, int* poiCount) {
+    SkASSERT(baselineLAB);
+    SkASSERT(testLAB);
+    SkASSERT(poiCount);
+
     int width = baselineLAB->width;
     int height = baselineLAB->height;
     int maxLevels = 0;
@@ -329,7 +333,6 @@ static double pmetric(const ImageLAB* baselineLAB, const ImageLAB* testLAB, SkTD
                                                contrast_sensitivity(cpd, 100.0f);
     }
 
-    int failures = 0;
     // Calculate F
     for (int y = 0; y < height; y++) {
         for (int x = 0; x < width; x++) {
@@ -424,8 +427,7 @@ static double pmetric(const ImageLAB* baselineLAB, const ImageLAB* testLAB, SkTD
             }
 
             if (isFailure) {
-                failures++;
-                poi->push()->set(x, y);
+                (*poiCount)++;
             }
         }
     }
@@ -434,57 +436,28 @@ static double pmetric(const ImageLAB* baselineLAB, const ImageLAB* testLAB, SkTD
     SkDELETE_ARRAY(contrast);
     SkDELETE_ARRAY(thresholdFactorFrequency);
     SkDELETE_ARRAY(contrastSensitivityTable);
-    return 1.0 - (double)failures / (width * height);
-}
-
-const char* SkPMetric::getName() {
-    return "perceptual";
+    return 1.0 - (double)(*poiCount) / (width * height);
 }
 
-int SkPMetric::queueDiff(SkBitmap* baseline, SkBitmap* test) {
+bool SkPMetric::diff(SkBitmap* baseline, SkBitmap* test, bool computeMask, Result* result) const {
     double startTime = get_seconds();
-    int diffID = fQueuedDiffs.count();
-    QueuedDiff& diff = fQueuedDiffs.push_back();
-    diff.result = 0.0;
 
     // Ensure the images are comparable
     if (baseline->width() != test->width() || baseline->height() != test->height() ||
                     baseline->width() <= 0 || baseline->height() <= 0) {
-        diff.finished = true;
-        return diffID;
+        return false;
     }
 
     ImageLAB baselineLAB(baseline->width(), baseline->height());
     ImageLAB testLAB(baseline->width(), baseline->height());
 
     if (!bitmap_to_cielab(baseline, &baselineLAB) || !bitmap_to_cielab(test, &testLAB)) {
-        return diffID;
+        return true;
     }
 
-    diff.result = pmetric(&baselineLAB, &testLAB, &diff.poi);
+    result->poiCount = 0;
+    result->result = pmetric(&baselineLAB, &testLAB, &result->poiCount);
+    result->timeElapsed = get_seconds() - startTime;
 
-    SkDebugf("Time: %f\n", (get_seconds() - startTime));
-
-    return diffID;
-}
-
-
-void SkPMetric::deleteDiff(int id) {
-
-}
-
-bool SkPMetric::isFinished(int id) {
-    return fQueuedDiffs[id].finished;
-}
-
-double SkPMetric::getResult(int id) {
-    return fQueuedDiffs[id].result;
-}
-
-int SkPMetric::getPointsOfInterestCount(int id) {
-    return fQueuedDiffs[id].poi.count();
-}
-
-SkIPoint* SkPMetric::getPointsOfInterest(int id) {
-    return fQueuedDiffs[id].poi.begin();
+    return true;
 }
diff --git a/tools/skpdiff/SkPMetric.h b/tools/skpdiff/SkPMetric.h
index 5963c2f33c..b60858a659 100644
--- a/tools/skpdiff/SkPMetric.h
+++ b/tools/skpdiff/SkPMetric.h
@@ -18,23 +18,11 @@
  */
 class SkPMetric : public SkImageDiffer {
 public:
-    virtual const char* getName() SK_OVERRIDE;
-    virtual int queueDiff(SkBitmap* baseline, SkBitmap* test) SK_OVERRIDE;
-    virtual void deleteDiff(int id) SK_OVERRIDE;
-    virtual bool isFinished(int id) SK_OVERRIDE;
-    virtual double getResult(int id) SK_OVERRIDE;
-    virtual int getPointsOfInterestCount(int id) SK_OVERRIDE;
-    virtual SkIPoint* getPointsOfInterest(int id) SK_OVERRIDE;
+    virtual const char* getName() const SK_OVERRIDE { return "perceptual"; }
+    virtual bool diff(SkBitmap* baseline, SkBitmap* test, bool computeMask,
+                      Result* result) const SK_OVERRIDE;
 
 private:
-    struct QueuedDiff {
-        bool finished;
-        double result;
-        SkTDArray<SkIPoint> poi;
-    };
-
-    SkTArray<QueuedDiff> fQueuedDiffs;
-
     typedef SkImageDiffer INHERITED;
 };