Improve sorting of skdiff output, and make it consistent across platforms

BUG=https://code.google.com/p/skia/issues/detail?id=677
Review URL: https://codereview.appspot.com/6351045

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

tools/skdiff_main.cpp

@@ -40,14 +40,15 @@
 #endif
 
 // Result of comparison for each pair of files.
+// Listed from "better" to "worse", for sorting of results.
 enum Result {
     kEqualBits,      // both files in the pair contain exactly the same bits
     kEqualPixels,    // not bitwise equal, but their pixels are exactly the same
-    kDifferentSizes, // both are images we can parse, but of different sizes
     kDifferentPixels,// both are images we can parse, but with different pixels
+    kDifferentSizes, // both are images we can parse, but of different sizes
     kDifferentOther, // files have different bits but are not parsable images
-    kBaseMissing,      // missing from baseDir
     kComparisonMissing,// missing from comparisonDir
+    kBaseMissing,      // missing from baseDir
     kUnknown,        // not compared yet
     //
     kNumResultTypes  // NOT A VALID VALUE--used to set up arrays. Must be last.
@@ -177,7 +178,6 @@ struct DiffSummary {
             break;
           case kDifferentSizes:
             fNumMismatches++;
-            drp->fFractionDifference = 2;// sort as if 200% of pixels differed
             break;
           case kDifferentPixels:
             fNumMismatches++;
@@ -192,7 +192,6 @@ struct DiffSummary {
             break;
           case kDifferentOther:
             fNumMismatches++;
-            drp->fFractionDifference = 3;// sort as if 300% of pixels differed
             break;
           case kBaseMissing:
             fNumMismatches++;
@@ -212,63 +211,100 @@ struct DiffSummary {
 
 typedef SkTDArray<DiffRecord*> RecordArray;
 
-/// Comparison routine for qsort;  sorts by fFractionDifference
+/// A wrapper for any sortProc (comparison routine) which applies a first-order
-/// from largest to smallest.
+/// sort beforehand, and a tiebreaker if the sortProc returns 0.
-static int compare_diff_metrics (DiffRecord** lhs, DiffRecord** rhs) {
+template<typename T>
-    if ((*lhs)->fFractionDifference < (*rhs)->fFractionDifference) {
+static int compare(const void* untyped_lhs, const void* untyped_rhs) {
-        return 1;
+    const DiffRecord* lhs = *reinterpret_cast<DiffRecord* const*>(untyped_lhs);
+    const DiffRecord* rhs = *reinterpret_cast<DiffRecord* const*>(untyped_rhs);
+
+    // First-order sort... these comparisons should be applied before comparing
+    // pixel values, no matter what.
+    if (lhs->fResult != rhs->fResult) {
+        return (lhs->fResult < rhs->fResult) ? 1 : -1;
     }
-    if ((*rhs)->fFractionDifference < (*lhs)->fFractionDifference) {
+
-        return -1;
+    // Passed first-order sort, so call the pixel comparison routine.
+    int result = T::comparePixels(lhs, rhs);
+    if (result != 0) {
+        return result;
     }
-    return 0;
+
+    // Tiebreaker... if we got to this point, we don't really care
+    // which order they are sorted in, but let's at least be consistent.
+    return strcmp(lhs->fFilename.c_str(), rhs->fFilename.c_str());
 }
 
-static int compare_diff_weighted (DiffRecord** lhs, DiffRecord** rhs) {
+/// Comparison routine for qsort;  sorts by fFractionDifference
-    if ((*lhs)->fWeightedFraction < (*rhs)->fWeightedFraction) {
+/// from largest to smallest.
-        return 1;
+class CompareDiffMetrics {
+public:
+    static int comparePixels(const DiffRecord* lhs, const DiffRecord* rhs) {
+        if (lhs->fFractionDifference < rhs->fFractionDifference) {
+          return 1;
+        }
+        if (rhs->fFractionDifference < lhs->fFractionDifference) {
+          return -1;
+        }
+        return 0;
     }
-    if ((*lhs)->fWeightedFraction > (*rhs)->fWeightedFraction) {
+};
-        return -1;
+
+class CompareDiffWeighted {
+public:
+    static int comparePixels(const DiffRecord* lhs, const DiffRecord* rhs) {
+        if (lhs->fWeightedFraction < rhs->fWeightedFraction) {
+            return 1;
+        }
+        if (lhs->fWeightedFraction > rhs->fWeightedFraction) {
+            return -1;
+        }
+        return 0;
     }
-    return 0;
+};
-}
 
 /// Comparison routine for qsort;  sorts by max(fAverageMismatch{RGB})
 /// from largest to smallest.
-static int compare_diff_mean_mismatches (DiffRecord** lhs, DiffRecord** rhs) {
+class CompareDiffMeanMismatches {
-    float leftValue = MAX3((*lhs)->fAverageMismatchR,
+public:
-                           (*lhs)->fAverageMismatchG,
+    static int comparePixels(const DiffRecord* lhs, const DiffRecord* rhs) {
-                           (*lhs)->fAverageMismatchB);
+        float leftValue = MAX3(lhs->fAverageMismatchR,
-    float rightValue = MAX3((*rhs)->fAverageMismatchR,
+                               lhs->fAverageMismatchG,
-                            (*rhs)->fAverageMismatchG,
+                               lhs->fAverageMismatchB);
-                            (*rhs)->fAverageMismatchB);
+        float rightValue = MAX3(rhs->fAverageMismatchR,
-    if (leftValue < rightValue) {
+                                rhs->fAverageMismatchG,
-        return 1;
+                                rhs->fAverageMismatchB);
+        if (leftValue < rightValue) {
+            return 1;
+        }
+        if (rightValue < leftValue) {
+            return -1;
+        }
+        return 0;
     }
-    if (rightValue < leftValue) {
+};
-        return -1;
-    }
-    return 0;
-}
 
 /// Comparison routine for qsort;  sorts by max(fMaxMismatch{RGB})
 /// from largest to smallest.
-static int compare_diff_max_mismatches (DiffRecord** lhs, DiffRecord** rhs) {
+class CompareDiffMaxMismatches {
-    uint32_t leftValue = MAX3((*lhs)->fMaxMismatchR,
+public:
-                              (*lhs)->fMaxMismatchG,
+    static int comparePixels(const DiffRecord* lhs, const DiffRecord* rhs) {
-                              (*lhs)->fMaxMismatchB);
+        uint32_t leftValue = MAX3(lhs->fMaxMismatchR,
-    uint32_t rightValue = MAX3((*rhs)->fMaxMismatchR,
+                                  lhs->fMaxMismatchG,
-                               (*rhs)->fMaxMismatchG,
+                                  lhs->fMaxMismatchB);
-                               (*rhs)->fMaxMismatchB);
+        uint32_t rightValue = MAX3(rhs->fMaxMismatchR,
-    if (leftValue < rightValue) {
+                                   rhs->fMaxMismatchG,
-        return 1;
+                                   rhs->fMaxMismatchB);
+        if (leftValue < rightValue) {
+            return 1;
+        }
+        if (rightValue < leftValue) {
+            return -1;
+        }
+
+        return CompareDiffMeanMismatches::comparePixels(lhs, rhs);
     }
-    if (rightValue < leftValue) {
+};
-        return -1;
-    }
-    return compare_diff_mean_mismatches(lhs, rhs);
-}
 
 
 
@@ -1045,7 +1081,7 @@ static void usage (char * argv0) {
 
 int main (int argc, char ** argv) {
     DiffMetricProc diffProc = compute_diff_pmcolor;
-    int (*sortProc)(const void*, const void*) = SkCastForQSort(compare_diff_metrics);
+    int (*sortProc)(const void*, const void*) = compare<CompareDiffMetrics>;
 
     // Maximum error tolerated in any one color channel in any one pixel before
     // a difference is reported.
@@ -1085,15 +1121,15 @@ int main (int argc, char ** argv) {
             continue;
         }
         if (!strcmp(argv[i], "--sortbymismatch")) {
-            sortProc = SkCastForQSort(compare_diff_mean_mismatches);
+            sortProc = compare<CompareDiffMeanMismatches>;
             continue;
         }
         if (!strcmp(argv[i], "--sortbymaxmismatch")) {
-            sortProc = SkCastForQSort(compare_diff_max_mismatches);
+            sortProc = compare<CompareDiffMaxMismatches>;
             continue;
         }
         if (!strcmp(argv[i], "--weighted")) {
-            sortProc = SkCastForQSort(compare_diff_weighted);
+            sortProc = compare<CompareDiffWeighted>;
             continue;
         }
         if (argv[i][0] != '-') {

tools/tests/skdiff/test1/output-expected/index.html

@@ -8,6 +8,14 @@
 <th>tools/tests/skdiff/comparisonDir/</th>
 </tr>
 <tr>
+<td><b>missing-from-baseDir.png</b><br>Missing from baseDir</td><td>N/A</td><td>N/A</td><td>N/A</td><td><a href="../../../../../tools/tests/skdiff/comparisonDir/missing-from-baseDir.png"><img src="../../../../../tools/tests/skdiff/comparisonDir/missing-from-baseDir.png" height="240px"></a></td></tr>
+<tr>
+<td><b>missing-from-baseDir.xyz</b><br>Missing from baseDir</td><td>N/A</td><td>N/A</td><td>N/A</td><td>N/A</td></tr>
+<tr>
+<td><b>missing-from-comparisonDir.png</b><br>Missing from comparisonDir</td><td>N/A</td><td>N/A</td><td><a href="../../../../../tools/tests/skdiff/baseDir/missing-from-comparisonDir.png"><img src="../../../../../tools/tests/skdiff/baseDir/missing-from-comparisonDir.png" height="240px"></a></td><td>N/A</td></tr>
+<tr>
+<td><b>missing-from-comparisonDir.xyz</b><br>Missing from comparisonDir</td><td>N/A</td><td>N/A</td><td>N/A</td><td>N/A</td></tr>
+<tr>
 <td><b>different-bits-unknown-format.xyz</b><br>Files differ; unable to parse one or both files</td><td>N/A</td><td>N/A</td><td>N/A</td><td>N/A</td></tr>
 <tr>
 <td><b>slightly-different-sizes.png</b><br>Image sizes differ</td><td>N/A</td><td>N/A</td><td><a href="../../../../../tools/tests/skdiff/baseDir/slightly-different-sizes.png"><img src="../../../../../tools/tests/skdiff/baseDir/slightly-different-sizes.png" height="240px"></a></td><td><a href="../../../../../tools/tests/skdiff/comparisonDir/slightly-different-sizes.png"><img src="../../../../../tools/tests/skdiff/comparisonDir/slightly-different-sizes.png" height="240px"></a></td></tr>
@@ -19,14 +27,6 @@
 <tr>
 <td><b>slightly-different-pixels-same-size.png</b><br>      0.6630% of pixels differ
   (      0.1904% weighted)<br>(2164 pixels)<br>Average color mismatch 0<br>Max color mismatch 213</td><td><a href="slightly-different-pixels-same-size-white.png"><img src="slightly-different-pixels-same-size-white.png" height="240px"></a></td><td><a href="slightly-different-pixels-same-size-diff.png"><img src="slightly-different-pixels-same-size-diff.png" height="240px"></a></td><td><a href="../../../../../tools/tests/skdiff/baseDir/slightly-different-pixels-same-size.png"><img src="../../../../../tools/tests/skdiff/baseDir/slightly-different-pixels-same-size.png" height="240px"></a></td><td><a href="../../../../../tools/tests/skdiff/comparisonDir/slightly-different-pixels-same-size.png"><img src="../../../../../tools/tests/skdiff/comparisonDir/slightly-different-pixels-same-size.png" height="240px"></a></td></tr>
-<tr>
-<td><b>missing-from-baseDir.png</b><br>Missing from baseDir</td><td>N/A</td><td>N/A</td><td>N/A</td><td><a href="../../../../../tools/tests/skdiff/comparisonDir/missing-from-baseDir.png"><img src="../../../../../tools/tests/skdiff/comparisonDir/missing-from-baseDir.png" height="240px"></a></td></tr>
-<tr>
-<td><b>missing-from-baseDir.xyz</b><br>Missing from baseDir</td><td>N/A</td><td>N/A</td><td>N/A</td><td>N/A</td></tr>
-<tr>
-<td><b>missing-from-comparisonDir.png</b><br>Missing from comparisonDir</td><td>N/A</td><td>N/A</td><td><a href="../../../../../tools/tests/skdiff/baseDir/missing-from-comparisonDir.png"><img src="../../../../../tools/tests/skdiff/baseDir/missing-from-comparisonDir.png" height="240px"></a></td><td>N/A</td></tr>
-<tr>
-<td><b>missing-from-comparisonDir.xyz</b><br>Missing from comparisonDir</td><td>N/A</td><td>N/A</td><td>N/A</td><td>N/A</td></tr>
 </table>
 </body>
 </html>

tools/tests/skdiff/test1/output-expected/stdout

@@ -1,8 +1,8 @@
 ERROR: no codec found for basePath <tools/tests/skdiff/baseDir/different-bits-unknown-format.xyz>
-ERROR: no codec found for <tools/tests/skdiff/baseDir/different-bits-unknown-format.xyz>
-ERROR: no codec found for <tools/tests/skdiff/comparisonDir/different-bits-unknown-format.xyz>
 ERROR: no codec found for <tools/tests/skdiff/comparisonDir/missing-from-baseDir.xyz>
 ERROR: no codec found for <tools/tests/skdiff/baseDir/missing-from-comparisonDir.xyz>
+ERROR: no codec found for <tools/tests/skdiff/baseDir/different-bits-unknown-format.xyz>
+ERROR: no codec found for <tools/tests/skdiff/comparisonDir/different-bits-unknown-format.xyz>
 baseDir is [tools/tests/skdiff/baseDir/]
 comparisonDir is [tools/tests/skdiff/comparisonDir/]
 writing diffs to outputDir is [tools/tests/skdiff/test1/output-actual/]