update skpdiff visualization (image magnification with alpha mask)

tools/skpdiff/SkDiffContext.cpp

Index: tools/skpdiff/SkDiffContext.cpp
diff --git a/tools/skpdiff/SkDiffContext.cpp b/tools/skpdiff/SkDiffContext.cpp
index 07d304b64cb2e0fac003848c8cfa1e7d2bb80fed..d25c595bbc2894447f693edef177230891767f26 100644
--- a/tools/skpdiff/SkDiffContext.cpp
+++ b/tools/skpdiff/SkDiffContext.cpp
@@ -41,6 +41,12 @@ SkDiffContext::~SkDiffContext() {
     }
 }
 
+void SkDiffContext::setDifferenceDir(const SkString& path) {
+    if (!path.isEmpty() && sk_mkdir(path.c_str())) {
+        fDifferenceDir = path;
+    }
+}
+
 void SkDiffContext::setDiffers(const SkTDArray<SkImageDiffer*>& differs) {
     // Delete whatever the last array of differs was
     if (NULL != fDiffers) {
@@ -75,9 +81,22 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     newRecord->fNext = fRecords;
     fRecords = newRecord;
 
+    // compute the common name

[epoger, 2013/10/21 15:02:53]

What does "common name" mean exactly?  Does it mean the name of the test with
the config stripped off?  Or just "whatever characters baselinePath and testPath
have in common"?

[djsollen, 2013/11/06 16:02:30]

Since the input can be two arbitrary files it means whatever characters they
have in common (prefix only).  In practice when used with GM output this is the
test name with the config stripped off.

+    SkString baseName = SkOSPath::SkBasename(baselinePath);
+    SkString testName = SkOSPath::SkBasename(testPath);
+    for (size_t x = 0; x < baseName.size(); ++x) {
+        if (baseName[x] != testName[x]) {

[epoger, 2013/10/21 15:02:53]

Is there the possibility for out-of-bounds reference if testName is shorter than
baseName?

[djsollen, 2013/11/06 16:02:30]

Done.

+            baseName.insertUnichar(x, '\n');

[epoger, 2013/10/21 15:02:53]

Why do you insert a newline rather than just truncating it?

[djsollen, 2013/11/06 16:02:30]

SkString doesn't support realloc so if we were to try to truncate it we would
need to alloc new memory, do a copy, and free the old memory just to save a few
bytes.  That seemed like more overhead than it was worth.  

[epoger, 2013/11/06 18:35:14]

I don't understand how inserting a newline actually truncates the string.  Won't
someone who prints the string get the original string with a carriage return in
the middle?

+            break;
+        }
+    }
+    newRecord->fCommonName = baseName.c_str();
+
     // Perform each diff
     for (int differIndex = 0; differIndex < fDifferCount; differIndex++) {
         SkImageDiffer* differ = fDiffers[differIndex];
+        // TODO only enable for one differ
+        differ->enablePOIAlphaMask();
         int diffID = differ->queueDiff(&baselineBitmap, &testBitmap);
         if (diffID >= 0) {
 
@@ -91,6 +110,20 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
             SkIPoint* poi = differ->getPointsOfInterest(diffID);
             diffData.fPointsOfInterest.append(poiCount, poi);
 
+            if (!fDifferenceDir.isEmpty()
+                    && 1.0f != diffData.fResult

[epoger, 2013/10/21 15:02:53]

Is 1.0 a "magic number" that fResult is assigned in a particular case?

If so... please use a constant

If not... are you concerned about float comparisons not matching up exactly?

[djsollen, 2013/11/06 16:02:30]

Done.

+                    && newRecord->fDifferencePath.isEmpty()) {
+                newRecord->fDifferencePath = SkOSPath::SkPathJoin(fDifferenceDir.c_str(),
+                                                                  newRecord->fCommonName.c_str());

[epoger, 2013/10/21 15:02:53]

Is fCommonName an SkString or a const char *? 

Line 93: newRecord->fCommonName = baseName.c_str();
Line 117: reference to newRecord->fCommonName.c_str()

[djsollen, 2013/11/06 16:02:30]

It is an SkString.  I updated line 93 for clarity.

+
+                // 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);
+            }
+
             // Because we are doing everything synchronously for now, we are done with the diff
             // after reading it.
             differ->deleteDiff(diffID);
@@ -201,18 +234,8 @@ void SkDiffContext::outputRecords(SkWStream& stream, bool useJSONP) {
             SkString baselineAbsPath = get_absolute_path(currentRecord->fBaselinePath);
             SkString testAbsPath = get_absolute_path(currentRecord->fTestPath);
 
-            // strip off directory structure and find the common part of the filename
-            SkString baseName = SkOSPath::SkBasename(baselineAbsPath.c_str());
-            SkString testName = SkOSPath::SkBasename(testAbsPath.c_str());
-            for (size_t x = 0; x < baseName.size(); ++x) {
-                if (baseName[x] != testName[x]) {
-                    baseName.insertUnichar(x, '\n');
-                    break;
-                }
-            }
-
             stream.writeText("            \"commonName\": \"");
-            stream.writeText(baseName.c_str());
+            stream.writeText(currentRecord->fCommonName.c_str());

[epoger, 2013/10/21 15:02:53]

ditto (const char * vs SkString)

             stream.writeText("\",\n");
 
             stream.writeText("            \"differencePath\": \"");