update skpdiff visualization (image magnification with alpha mask)

tools/skpdiff/SkDiffContext.cpp

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmap.h"
 #include "SkImageDecoder.h"
 #include "SkOSFile.h"
@@ skipping 23 matching lines @@
         DiffRecord* nextRecord = currentRecord->fNext;
         SkDELETE(currentRecord);
         currentRecord = nextRecord;
     }
 
     if (NULL != fDiffers) {
         SkDELETE_ARRAY(fDiffers);
     }
 }
 
+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) {
         SkDELETE_ARRAY(fDiffers);
         fDiffers = NULL;
         fDifferCount = 0;
     }
 
     // Copy over the new differs
     fDifferCount = differs.count();
@@ skipping 14 matching lines @@
         return;
     }
 
     // Setup a record for this diff
     DiffRecord* newRecord = SkNEW(DiffRecord);
     newRecord->fBaselinePath = baselinePath;
     newRecord->fTestPath = 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) {
 
             // 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 (!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);
         }
     }
 
     // if we get a difference and we want the alpha mask then compute it here.
 }
 
 class SkThreadedDiff : public SkRunnable {
@@ skipping 90 matching lines @@
         stream.writeText("{\n");
     }
     stream.writeText("    \"records\": [\n");
     while (NULL != currentRecord) {
         stream.writeText("        {\n");
 
             SkString differenceAbsPath = get_absolute_path(currentRecord->fDifferencePath);
             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\": \"");
             stream.writeText(differenceAbsPath.c_str());
             stream.writeText("\",\n");
 
             stream.writeText("            \"baselinePath\": \"");
             stream.writeText(baselineAbsPath.c_str());
             stream.writeText("\",\n");
 
@@ skipping 110 matching lines @@
         for (int i = 0; i < cntColumns; i++) {
             SkString str;
             str.printf(", %f", values[i]);
             stream.writeText(str.c_str());
         }
         stream.writeText("\n");
 
         currentRecord = currentRecord->fNext;
     }
 }