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
This commit is contained in:
stephana 2014-08-13 10:36:06 -07:00 committed by Commit bot
parent 97a0d43771
commit 21b342d19c
7 changed files with 102 additions and 60 deletions

View File

@ -125,8 +125,10 @@ class DiffRecord(object):
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 @@ class DiffRecord(object):
# 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 @@ class DiffRecord(object):
# 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 @@ class DiffRecord(object):
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 @@ def _sanitize_locator(locator):
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))

View File

@ -666,11 +666,34 @@ Loader.controller(
* @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
@ -686,7 +709,7 @@ Loader.controller(
} else {
return undefined;
}
}
};
/**
* For a particular ImagePair, return the value we use for the
@ -701,7 +724,7 @@ Loader.controller(
$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 @@ Loader.controller(
$scope.setColumnStringMatch = function(name, value) {
$scope.columnStringMatch[name] = value;
$scope.updateResults();
}
};
/**
* Update $scope.showingColumnValues[columnName] and $scope.columnStringMatch[columnName]
@ -727,7 +750,7 @@ Loader.controller(
$scope.showingColumnValues[columnName] = {};
$scope.toggleValueInSet(columnValue, $scope.showingColumnValues[columnName]);
$scope.updateResults();
}
};
/**
* Update $scope.showingColumnValues[columnName] and $scope.columnStringMatch[columnName]
@ -742,7 +765,7 @@ Loader.controller(
$scope.toggleValuesInSet($scope.allColumnValues[columnName],
$scope.showingColumnValues[columnName]);
$scope.updateResults();
}
};
//
@ -852,7 +875,7 @@ Loader.controller(
"Please see server-side log for details.");
$scope.submitPending = false;
});
}
};
//
@ -870,7 +893,7 @@ Loader.controller(
*/
$scope.setSize = function(set) {
return Object.keys(set).length;
}
};
/**
* Returns true if value "value" is present within set "set".
@ -881,7 +904,7 @@ Loader.controller(
*/
$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 @@ Loader.controller(
} else {
set[value] = true;
}
}
};
/**
* For each value in valueArray, call toggleValueInSet(value, set).
@ -908,7 +931,7 @@ Loader.controller(
for (var i = 0; i < arrayLength; i++) {
$scope.toggleValueInSet(valueArray[i], set);
}
}
};
//
@ -925,7 +948,7 @@ Loader.controller(
*/
$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 @@ Loader.controller(
} else {
array.splice(i, 1);
}
}
};
//
@ -969,7 +992,7 @@ Loader.controller(
slice.push(array[row][column]);
}
return slice;
}
};
/**
* Returns a human-readable (in local time zone) time string for a
@ -980,7 +1003,7 @@ Loader.controller(
$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 @@ Loader.controller(
bString = "0" + bString;
}
return '#' + rString + gString + bString;
}
};
/**
* Returns a hex color string (such as "#aabbcc") for the given brightness.
@ -1016,7 +1039,7 @@ Loader.controller(
$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 @@ Loader.controller(
imagePair[constants.KEY__IMAGEPAIRS__IMAGE_A_URL] + "-vs-" +
imagePair[constants.KEY__IMAGEPAIRS__IMAGE_B_URL];
return before.replace(/[^\w\-]/g, "_") + ".png";
}
};
}
);

View File

@ -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)"

View File

@ -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',

View File

@ -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::setWhiteDiffDir(const SkString& path) {
}
}
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_common_prefix(const SkString& a, const SkString& b) {
}
}
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 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
// 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;

View File

@ -51,6 +51,25 @@ public:
*/
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.
@ -85,8 +104,9 @@ public:
*
* 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 @@ private:
SkString fAlphaMaskDir;
SkString fRgbDiffDir;
SkString fWhiteDiffDir;
bool longNames;
};
#endif

View File

@ -49,6 +49,7 @@ DEFINE_string(whiteDiffDir, "", "If the differ can generate an image showing eve
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 @@ int tool_main(int argc, char * argv[]) {
return 1;
}
}
if (!FLAGS_whiteDiffDir.isEmpty()) {
if (1 != FLAGS_whiteDiffDir.count()) {
SkDebugf("whiteDiffDir flag expects one argument: <directory>\n");
@ -215,6 +217,7 @@ int tool_main(int argc, char * argv[]) {
SkDiffContext ctx;
ctx.setDiffers(chosenDiffers);
ctx.setLongNames(FLAGS_longnames);
if (!FLAGS_alphaDir.isEmpty()) {
ctx.setAlphaMaskDir(SkString(FLAGS_alphaDir[0]));