This eliminates the need to copy the generated images from a temporary directory to the directory that is served by the rebaseline_server.
BUG=skia:2815, skia:2818
R=epoger@google.com
Author: stephana@google.com
Review URL: https://codereview.chromium.org/457203003
diff --git a/gm/rebaseline_server/imagediffdb.py b/gm/rebaseline_server/imagediffdb.py
index 167134a..3c5f45f 100644
--- a/gm/rebaseline_server/imagediffdb.py
+++ b/gm/rebaseline_server/imagediffdb.py
@@ -125,8 +125,10 @@
try:
skpdiff_summary_file = os.path.join(skpdiff_output_dir,
'skpdiff-output.json')
- skpdiff_rgbdiff_dir = os.path.join(skpdiff_output_dir, 'rgbDiff')
- skpdiff_whitediff_dir = os.path.join(skpdiff_output_dir, 'whiteDiff')
+ skpdiff_rgbdiff_dir = os.path.join(storage_root, RGBDIFFS_SUBDIR)
+ skpdiff_whitediff_dir = os.path.join(storage_root, WHITEDIFFS_SUBDIR)
+ _mkdir_unless_exists(skpdiff_rgbdiff_dir)
+ _mkdir_unless_exists(skpdiff_rgbdiff_dir)
# TODO(epoger): Consider calling skpdiff ONCE for all image pairs,
# instead of calling it separately for each image pair.
@@ -134,9 +136,13 @@
# spinning up the skpdiff binary, etc.
# Con: we would have to wait until all image pairs were loaded before
# generating any of the diffs?
+ # Note(stephana): '--longnames' was added to allow for this
+ # case (multiple files at once) versus specifying output diffs
+ # directly.
find_run_binary.run_command(
[SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
'--jsonp', 'false',
+ '--longnames', 'true',
'--output', skpdiff_summary_file,
'--differs', 'perceptual', 'different_pixels',
'--rgbDiffDir', skpdiff_rgbdiff_dir,
@@ -157,8 +163,6 @@
# See http://stackoverflow.com/a/626871
self._max_diff_per_channel = [
record['maxRedDiff'], record['maxGreenDiff'], record['maxBlueDiff']]
- rgb_diff_path = record['rgbDiffPath']
- white_diff_path = record['whiteDiffPath']
per_differ_stats = record['diffs']
for stats in per_differ_stats:
differ_name = stats['differName']
@@ -175,22 +179,6 @@
if not 0 <= perceptual_similarity <= 1:
perceptual_similarity = 0
self._perceptual_difference = 100 - (perceptual_similarity * 100)
-
- # Store the rgbdiff and whitediff images generated above.
- diff_image_locator = _get_difference_locator(
- expected_image_locator=expected_image_locator,
- actual_image_locator=actual_image_locator)
- basename = str(diff_image_locator) + image_suffix
- _mkdir_unless_exists(os.path.join(storage_root, RGBDIFFS_SUBDIR))
- _mkdir_unless_exists(os.path.join(storage_root, WHITEDIFFS_SUBDIR))
- # TODO: Modify skpdiff's behavior so we can tell it exactly where to
- # write the image files into, rather than having to move them around
- # after skpdiff writes them out.
- shutil.copyfile(rgb_diff_path,
- os.path.join(storage_root, RGBDIFFS_SUBDIR, basename))
- shutil.copyfile(white_diff_path,
- os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename))
-
finally:
shutil.rmtree(skpdiff_output_dir)
@@ -479,20 +467,3 @@
return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator))
else:
return locator
-
-def _get_difference_locator(expected_image_locator, actual_image_locator):
- """Returns the locator string used to look up the diffs between expected_image
- and actual_image.
-
- We must keep this function in sync with getImageDiffRelativeUrl() in
- static/loader.js
-
- Args:
- expected_image_locator: locator string pointing at expected image
- actual_image_locator: locator string pointing at actual image
-
- Returns: already-sanitized locator where the diffs between expected and
- actual images can be found
- """
- return "%s-vs-%s" % (_sanitize_locator(expected_image_locator),
- _sanitize_locator(actual_image_locator))
diff --git a/gm/rebaseline_server/static/live-loader.js b/gm/rebaseline_server/static/live-loader.js
index 16a19aa..cd4560f 100644
--- a/gm/rebaseline_server/static/live-loader.js
+++ b/gm/rebaseline_server/static/live-loader.js
@@ -666,12 +666,35 @@
* @param key (string): sort by value associated with this key in subdict
*/
$scope.sortResultsBy = function(subdict, key) {
- $scope.sortColumnSubdict = subdict;
- $scope.sortColumnKey = key;
+ // if we are already sorting by this column then toggle between asc/desc
+ if ((subdict === $scope.sortColumnSubdict) && ($scope.sortColumnKey === key)) {
+ currSortAsc = !currSortAsc;
+ } else {
+ $scope.sortColumnSubdict = subdict;
+ $scope.sortColumnKey = key;
+ currSortAsc = true;
+ }
$scope.updateResults();
}
/**
+ * Returns ASC or DESC (from constants) if currently the data
+ * is sorted by the provided column.
+ *
+ * @param colName: name of the column for which we need to get the class.
+ */
+
+ $scope.sortedByColumnsCls = function (colName) {
+ if ($scope.sortColumnKey !== colName) {
+ return '';
+ }
+
+ var result = (currSortAsc) ? constants.ASC : constants.DESC;
+ console.log("sort class:", result);
+ return result;
+ };
+
+ /**
* For a particular ImagePair, return the value of the column we are
* sorting on (according to $scope.sortColumnSubdict and
* $scope.sortColumnKey).
@@ -686,7 +709,7 @@
} else {
return undefined;
}
- }
+ };
/**
* For a particular ImagePair, return the value we use for the
@@ -701,7 +724,7 @@
$scope.getSecondOrderSortValue = function(imagePair) {
return imagePair[constants.KEY__IMAGEPAIRS__IMAGE_A_URL] + "-vs-" +
imagePair[constants.KEY__IMAGEPAIRS__IMAGE_B_URL];
- }
+ };
/**
* Set $scope.columnStringMatch[name] = value, and update results.
@@ -712,7 +735,7 @@
$scope.setColumnStringMatch = function(name, value) {
$scope.columnStringMatch[name] = value;
$scope.updateResults();
- }
+ };
/**
* Update $scope.showingColumnValues[columnName] and $scope.columnStringMatch[columnName]
@@ -727,7 +750,7 @@
$scope.showingColumnValues[columnName] = {};
$scope.toggleValueInSet(columnValue, $scope.showingColumnValues[columnName]);
$scope.updateResults();
- }
+ };
/**
* Update $scope.showingColumnValues[columnName] and $scope.columnStringMatch[columnName]
@@ -742,7 +765,7 @@
$scope.toggleValuesInSet($scope.allColumnValues[columnName],
$scope.showingColumnValues[columnName]);
$scope.updateResults();
- }
+ };
//
@@ -852,7 +875,7 @@
"Please see server-side log for details.");
$scope.submitPending = false;
});
- }
+ };
//
@@ -870,7 +893,7 @@
*/
$scope.setSize = function(set) {
return Object.keys(set).length;
- }
+ };
/**
* Returns true if value "value" is present within set "set".
@@ -881,7 +904,7 @@
*/
$scope.isValueInSet = function(value, set) {
return (true == set[value]);
- }
+ };
/**
* If value "value" is already in set "set", remove it; otherwise, add it.
@@ -895,7 +918,7 @@
} else {
set[value] = true;
}
- }
+ };
/**
* For each value in valueArray, call toggleValueInSet(value, set).
@@ -908,7 +931,7 @@
for (var i = 0; i < arrayLength; i++) {
$scope.toggleValueInSet(valueArray[i], set);
}
- }
+ };
//
@@ -925,7 +948,7 @@
*/
$scope.isValueInArray = function(value, array) {
return (-1 != array.indexOf(value));
- }
+ };
/**
* If value "value" is already in array "array", remove it; otherwise,
@@ -941,7 +964,7 @@
} else {
array.splice(i, 1);
}
- }
+ };
//
@@ -969,7 +992,7 @@
slice.push(array[row][column]);
}
return slice;
- }
+ };
/**
* Returns a human-readable (in local time zone) time string for a
@@ -980,7 +1003,7 @@
$scope.localTimeString = function(secondsPastEpoch) {
var d = new Date(secondsPastEpoch * 1000);
return d.toString();
- }
+ };
/**
* Returns a hex color string (such as "#aabbcc") for the given RGB values.
@@ -1003,7 +1026,7 @@
bString = "0" + bString;
}
return '#' + rString + gString + bString;
- }
+ };
/**
* Returns a hex color string (such as "#aabbcc") for the given brightness.
@@ -1016,7 +1039,7 @@
$scope.brightnessStringToHexColor = function(brightnessString) {
var v = parseInt(brightnessString);
return $scope.hexColorString(v, v, v);
- }
+ };
/**
* Returns the last path component of image diff URL for a given ImagePair.
@@ -1034,7 +1057,7 @@
imagePair[constants.KEY__IMAGEPAIRS__IMAGE_A_URL] + "-vs-" +
imagePair[constants.KEY__IMAGEPAIRS__IMAGE_B_URL];
return before.replace(/[^\w\-]/g, "_") + ".png";
- }
+ };
}
);
diff --git a/gm/rebaseline_server/static/live-view.html b/gm/rebaseline_server/static/live-view.html
index cb50378..8593adc 100644
--- a/gm/rebaseline_server/static/live-view.html
+++ b/gm/rebaseline_server/static/live-view.html
@@ -261,7 +261,6 @@
class="sortable-header">
bugs
</a>
- bugs
</th>
<th width="{{imageSize}}">
<a ng-class="'sort-' + sortedByColumnsCls(constants.KEY__IMAGEPAIRS__IMAGE_A_URL)"
diff --git a/gyp/tools.gyp b/gyp/tools.gyp
index 46d668b..1766a09 100644
--- a/gyp/tools.gyp
+++ b/gyp/tools.gyp
@@ -170,10 +170,12 @@
],
'include_dirs': [
'../src/core/', # needed for SkTLList.h
+ '../tools/', # needed for picture_utils::replace_char
],
'dependencies': [
'flags.gyp:flags',
'skia_lib.gyp:skia_lib',
+ 'tools.gyp:picture_utils',
],
'cflags': [
'-O3',
diff --git a/tools/skpdiff/SkDiffContext.cpp b/tools/skpdiff/SkDiffContext.cpp
index ea44c90..42d20de 100644
--- a/tools/skpdiff/SkDiffContext.cpp
+++ b/tools/skpdiff/SkDiffContext.cpp
@@ -14,6 +14,9 @@
#include "SkTDict.h"
#include "SkThreadPool.h"
+// from the tools directory for replace_char(...)
+#include "picture_utils.h"
+
#include "SkDiffContext.h"
#include "SkImageDiffer.h"
#include "skpdiff_util.h"
@@ -48,6 +51,10 @@
}
}
+void SkDiffContext::setLongNames(const bool useLongNames) {
+ longNames = useLongNames;
+}
+
void SkDiffContext::setDiffers(const SkTDArray<SkImageDiffer*>& differs) {
// Delete whatever the last array of differs was
if (NULL != fDiffers) {
@@ -79,6 +86,16 @@
}
}
+static SkString get_combined_name(const SkString& a, const SkString& b) {
+ // Note (stephana): We must keep this function in sync with
+ // getImageDiffRelativeUrl() in static/loader.js (under rebaseline_server).
+ SkString result = a;
+ result.append("-vs-");
+ result.append(b);
+ sk_tools::replace_char(&result, '.', '_');
+ return result;
+}
+
void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
// Load the images at the paths
SkBitmap baselineBitmap;
@@ -100,7 +117,13 @@
// compute the common name
SkString baseName = SkOSPath::Basename(baselinePath);
SkString testName = SkOSPath::Basename(testPath);
- newRecord->fCommonName = get_common_prefix(baseName, testName);
+
+ if (longNames) {
+ newRecord->fCommonName = get_combined_name(baseName, testName);
+ } else {
+ newRecord->fCommonName = get_common_prefix(baseName, testName);
+ }
+ newRecord->fCommonName.append(".png");
newRecord->fBaselinePath = baselinePath;
newRecord->fTestPath = testPath;
diff --git a/tools/skpdiff/SkDiffContext.h b/tools/skpdiff/SkDiffContext.h
index 996737f..8f4789f 100644
--- a/tools/skpdiff/SkDiffContext.h
+++ b/tools/skpdiff/SkDiffContext.h
@@ -52,6 +52,25 @@
void setWhiteDiffDir(const SkString& directory);
/**
+ * Modify the pattern used to generate commonName (= the
+ * basename of rgb/white diff files).
+ *
+ * - true: basename is a combination of the input file names.
+ * - false: basename is the common prefix of the input file names.
+ *
+ * For example, for:
+ * baselinePath=/tmp/dir/image-before.png
+ * testPath=/tmp/dir/image-after.png
+ *
+ * If setLongNames(true), commonName would be:
+ * image-before-png-vs-image-after-png.png
+ *
+ * If setLongNames(false), commonName would be:
+ * image-.png
+ */
+ void setLongNames(const bool useLongNames);
+
+ /**
* Sets the differs to be used in each diff. Already started diffs will not retroactively use
* these.
* @param differs An array of differs to use. The array is copied, but not the differs
@@ -85,8 +104,9 @@
*
* The format of the JSON document is one top level array named "records".
* Each record in the array is an object with the following values:
- * "commonName" : string containing the common prefix of the baselinePath
- * and testPath filenames
+ * "commonName" : string containing the output filename (basename)
+ * depending on the value of 'longNames'.
+ * (see 'setLongNames' for an explanation and example).
* "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
@@ -177,6 +197,7 @@
SkString fAlphaMaskDir;
SkString fRgbDiffDir;
SkString fWhiteDiffDir;
+ bool longNames;
};
#endif
diff --git a/tools/skpdiff/skpdiff_main.cpp b/tools/skpdiff/skpdiff_main.cpp
index 3d1bcda..6c40552 100644
--- a/tools/skpdiff/skpdiff_main.cpp
+++ b/tools/skpdiff/skpdiff_main.cpp
@@ -49,6 +49,7 @@
DEFINE_bool(jsonp, true, "Output JSON with padding");
DEFINE_string(csv, "", "Writes the output of these diffs to a csv file: <filepath>");
DEFINE_int32(threads, -1, "run N threads in parallel [default is derived from CPUs available]");
+DEFINE_bool(longnames, false, "Output image names are a combination of baseline and test names");
#if SK_SUPPORT_OPENCL
/// A callback for any OpenCL errors
@@ -206,6 +207,7 @@
return 1;
}
}
+
if (!FLAGS_whiteDiffDir.isEmpty()) {
if (1 != FLAGS_whiteDiffDir.count()) {
SkDebugf("whiteDiffDir flag expects one argument: <directory>\n");
@@ -215,6 +217,7 @@
SkDiffContext ctx;
ctx.setDiffers(chosenDiffers);
+ ctx.setLongNames(FLAGS_longnames);
if (!FLAGS_alphaDir.isEmpty()) {
ctx.setAlphaMaskDir(SkString(FLAGS_alphaDir[0]));