create sk_tools::Expectation class, similar to skiagm::Expectations class
The overarching goal here is for our "gm" and "render_pictures" tools to handle
image expectations/actuals in the same way, sharing the same code, so their
results can be processed through a single pipeline.
By adding an Expectation class within tools/image_expectations.h, similar to
the Expectations class in gm/gm_expectations.h, we get one step closer to
that goal.
R=stephana@google.com
TBR=stephana
Author: epoger@google.com
Review URL: https://codereview.chromium.org/493363002
diff --git a/tools/PictureRenderer.cpp b/tools/PictureRenderer.cpp
index 7325c20..ac639e5 100644
--- a/tools/PictureRenderer.cpp
+++ b/tools/PictureRenderer.cpp
@@ -297,7 +297,7 @@
SkString outputFilename;
const char *outputSubdirPtr = NULL;
if (useChecksumBasedFilenames) {
- const ImageDigest *imageDigestPtr = bitmapAndDigest.getImageDigestPtr();
+ ImageDigest *imageDigestPtr = bitmapAndDigest.getImageDigestPtr();
outputSubdirPtr = escapedInputFilename.c_str();
outputFilename.set(imageDigestPtr->getHashType());
outputFilename.append("_");
@@ -312,7 +312,7 @@
outputFilename.append(".png");
if (NULL != jsonSummaryPtr) {
- const ImageDigest *imageDigestPtr = bitmapAndDigest.getImageDigestPtr();
+ ImageDigest *imageDigestPtr = bitmapAndDigest.getImageDigestPtr();
SkString outputRelativePath;
if (outputSubdirPtr) {
outputRelativePath.set(outputSubdirPtr);
@@ -325,8 +325,8 @@
jsonSummaryPtr->add(inputFilename.c_str(), outputRelativePath.c_str(),
*imageDigestPtr, tileNumberPtr);
if (!mismatchPath.isEmpty() &&
- !jsonSummaryPtr->matchesExpectation(inputFilename.c_str(), *imageDigestPtr,
- tileNumberPtr)) {
+ !jsonSummaryPtr->getExpectation(inputFilename.c_str(),
+ tileNumberPtr).matches(*imageDigestPtr)) {
if (!write_bitmap_to_disk(bitmap, mismatchPath, outputSubdirPtr, outputFilename)) {
return false;
}
diff --git a/tools/image_expectations.cpp b/tools/image_expectations.cpp
index dfc2638..f0c9cfb 100644
--- a/tools/image_expectations.cpp
+++ b/tools/image_expectations.cpp
@@ -53,44 +53,69 @@
// ImageDigest class...
- ImageDigest::ImageDigest(const SkBitmap &bitmap) {
- if (!SkBitmapHasher::ComputeDigest(bitmap, &fHashValue)) {
- SkFAIL("unable to compute image digest");
- }
- }
+ ImageDigest::ImageDigest(const SkBitmap &bitmap) :
+ fBitmap(bitmap), fHashValue(0), fComputedHashValue(false) {}
- ImageDigest::ImageDigest(const SkString &hashType, uint64_t hashValue) {
+ ImageDigest::ImageDigest(const SkString &hashType, uint64_t hashValue) :
+ fBitmap(), fHashValue(hashValue), fComputedHashValue(true) {
if (!hashType.equals(kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5)) {
- SkFAIL((SkString("unsupported hashType ")+=hashType).c_str());
- } else {
- fHashValue = hashValue;
+ SkDebugf("unsupported hashType '%s'\n", hashType.c_str());
+ SkFAIL("unsupported hashType (see above)");
}
}
- SkString ImageDigest::getHashType() const {
+ bool ImageDigest::equals(ImageDigest &other) {
+ // TODO(epoger): The current implementation assumes that this
+ // and other always have hashType kJsonKey_Hashtype_Bitmap_64bitMD5
+ return (this->getHashValue() == other.getHashValue());
+ }
+
+ SkString ImageDigest::getHashType() {
// TODO(epoger): The current implementation assumes that the
// result digest is always of type kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5 .
return SkString(kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5);
}
- uint64_t ImageDigest::getHashValue() const {
- return fHashValue;
+ uint64_t ImageDigest::getHashValue() {
+ if (!this->fComputedHashValue) {
+ if (!SkBitmapHasher::ComputeDigest(this->fBitmap, &this->fHashValue)) {
+ SkFAIL("unable to compute image digest");
+ }
+ this->fComputedHashValue = true;
+ }
+ return this->fHashValue;
}
// BitmapAndDigest class...
- BitmapAndDigest::BitmapAndDigest(const SkBitmap &bitmap) : fBitmap(bitmap) {
- }
+ BitmapAndDigest::BitmapAndDigest(const SkBitmap &bitmap) :
+ fBitmap(bitmap), fImageDigest(bitmap) {}
- const ImageDigest *BitmapAndDigest::getImageDigestPtr() {
- if (NULL == fImageDigestRef.get()) {
- fImageDigestRef.reset(SkNEW_ARGS(ImageDigest, (fBitmap)));
- }
- return fImageDigestRef.get();
- }
+ const SkBitmap *BitmapAndDigest::getBitmapPtr() const {return &fBitmap;}
- const SkBitmap *BitmapAndDigest::getBitmapPtr() const {
- return &fBitmap;
+ ImageDigest *BitmapAndDigest::getImageDigestPtr() {return &fImageDigest;}
+
+ // Expectation class...
+
+ // For when we need a valid ImageDigest, but we don't care what it is.
+ static const ImageDigest kDummyImageDigest(
+ SkString(kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5), 0);
+
+ Expectation::Expectation(bool ignoreFailure) :
+ fIsEmpty(true), fIgnoreFailure(ignoreFailure), fImageDigest(kDummyImageDigest) {}
+
+ Expectation::Expectation(const SkString &hashType, uint64_t hashValue, bool ignoreFailure) :
+ fIsEmpty(false), fIgnoreFailure(ignoreFailure), fImageDigest(hashType, hashValue) {}
+
+ Expectation::Expectation(const SkBitmap& bitmap, bool ignoreFailure) :
+ fIsEmpty(false), fIgnoreFailure(ignoreFailure), fImageDigest(bitmap) {}
+
+ bool Expectation::ignoreFailure() const { return this->fIgnoreFailure; }
+
+ bool Expectation::empty() const { return this->fIsEmpty; }
+
+ bool Expectation::matches(ImageDigest &imageDigest) {
+ return !(this->fIsEmpty) && (this->fImageDigest.equals(imageDigest));
}
// ImageResultsAndExpectations class...
@@ -136,19 +161,11 @@
}
void ImageResultsAndExpectations::add(const char *sourceName, const char *fileName,
- const ImageDigest &digest, const int *tileNumber) {
+ ImageDigest &digest, const int *tileNumber) {
// Get expectation, if any.
- Json::Value expectedImage;
- if (!fExpectedResults.isNull()) {
- if (NULL == tileNumber) {
- expectedImage = fExpectedResults[sourceName][kJsonKey_Source_WholeImage];
- } else {
- expectedImage = fExpectedResults[sourceName][kJsonKey_Source_TiledImages]
- [*tileNumber];
- }
- }
+ Expectation expectation = this->getExpectation(sourceName, tileNumber);
- // Fill in info about the actual result itself.
+ // Fill in info about the actual result.
Json::Value actualChecksumAlgorithm = digest.getHashType().c_str();
Json::Value actualChecksumValue = Json::UInt64(digest.getHashValue());
Json::Value actualImage;
@@ -157,16 +174,15 @@
actualImage[kJsonKey_Image_Filepath] = fileName;
// Compare against expectedImage to fill in comparisonResult.
- Json::Value comparisonResult = kJsonValue_Image_ComparisonResult_NoComparison;
- if (!expectedImage.isNull()) {
- if ((actualChecksumAlgorithm == expectedImage[kJsonKey_Image_ChecksumAlgorithm]) &&
- (actualChecksumValue == expectedImage[kJsonKey_Image_ChecksumValue])) {
- comparisonResult = kJsonValue_Image_ComparisonResult_Succeeded;
- } else if (expectedImage[kJsonKey_Image_IgnoreFailure] == true) {
- comparisonResult = kJsonValue_Image_ComparisonResult_FailureIgnored;
- } else {
- comparisonResult = kJsonValue_Image_ComparisonResult_Failed;
- }
+ Json::Value comparisonResult;
+ if (expectation.empty()) {
+ comparisonResult = kJsonValue_Image_ComparisonResult_NoComparison;
+ } else if (expectation.matches(digest)) {
+ comparisonResult = kJsonValue_Image_ComparisonResult_Succeeded;
+ } else if (expectation.ignoreFailure()) {
+ comparisonResult = kJsonValue_Image_ComparisonResult_FailureIgnored;
+ } else {
+ comparisonResult = kJsonValue_Image_ComparisonResult_Failed;
}
actualImage[kJsonKey_Image_ComparisonResult] = comparisonResult;
@@ -182,11 +198,10 @@
fDescriptions[key] = value;
}
- bool ImageResultsAndExpectations::matchesExpectation(const char *sourceName,
- const ImageDigest &digest,
- const int *tileNumber) {
+ Expectation ImageResultsAndExpectations::getExpectation(const char *sourceName,
+ const int *tileNumber) {
if (fExpectedResults.isNull()) {
- return false;
+ return Expectation();
}
Json::Value expectedImage;
@@ -196,13 +211,13 @@
expectedImage = fExpectedResults[sourceName][kJsonKey_Source_TiledImages][*tileNumber];
}
if (expectedImage.isNull()) {
- return false;
+ return Expectation();
}
- Json::Value actualChecksumAlgorithm = digest.getHashType().c_str();
- Json::Value actualChecksumValue = Json::UInt64(digest.getHashValue());
- return ((actualChecksumAlgorithm == expectedImage[kJsonKey_Image_ChecksumAlgorithm]) &&
- (actualChecksumValue == expectedImage[kJsonKey_Image_ChecksumValue]));
+ bool ignoreFailure = (expectedImage[kJsonKey_Image_IgnoreFailure] == true);
+ return Expectation(SkString(expectedImage[kJsonKey_Image_ChecksumAlgorithm].asCString()),
+ expectedImage[kJsonKey_Image_ChecksumValue].asUInt64(),
+ ignoreFailure);
}
void ImageResultsAndExpectations::writeToFile(const char *filename) const {
diff --git a/tools/image_expectations.h b/tools/image_expectations.h
index ae1df19..2d58b92 100644
--- a/tools/image_expectations.h
+++ b/tools/image_expectations.h
@@ -11,7 +11,6 @@
#include "SkBitmap.h"
#include "SkJSONCPP.h"
#include "SkOSFile.h"
-#include "SkRefCnt.h"
namespace sk_tools {
@@ -20,14 +19,12 @@
*
* Currently, this is always a uint64_t hash digest of an SkBitmap.
*/
- class ImageDigest : public SkRefCnt {
+ class ImageDigest {
public:
/**
* Create an ImageDigest of a bitmap.
*
- * Note that this is an expensive operation, because it has to examine all pixels in
- * the bitmap. You may wish to consider using the BitmapAndDigest class, which will
- * compute the ImageDigest lazily.
+ * Computes the hash of the bitmap lazily, since that is an expensive operation.
*
* @param bitmap image to get the digest of
*/
@@ -43,36 +40,105 @@
explicit ImageDigest(const SkString &hashType, uint64_t hashValue);
/**
+ * Returns true iff this and other ImageDigest represent identical images.
+ */
+ bool equals(ImageDigest &other);
+
+ /**
* Returns the hash digest type as an SkString.
*
* For now, this always returns kJsonValue_Image_ChecksumAlgorithm_Bitmap64bitMD5 .
*/
- SkString getHashType() const;
+ SkString getHashType();
/**
* Returns the hash digest value as a uint64_t.
+ *
+ * Since the hash is computed lazily, this may take some time, and it may modify
+ * some fields on this object.
*/
- uint64_t getHashValue() const;
+ uint64_t getHashValue();
private:
+ const SkBitmap fBitmap;
uint64_t fHashValue;
+ bool fComputedHashValue;
};
/**
- * Container that holds a reference to an SkBitmap and computes its ImageDigest lazily.
- *
- * Computing the ImageDigest can be expensive, so this can help you postpone (or maybe even
- * avoid) that work.
+ * Container that holds a reference to an SkBitmap and its ImageDigest.
*/
class BitmapAndDigest {
public:
explicit BitmapAndDigest(const SkBitmap &bitmap);
- const ImageDigest *getImageDigestPtr();
const SkBitmap *getBitmapPtr() const;
+
+ /**
+ * Returns a pointer to the ImageDigest.
+ *
+ * Since the hash is computed lazily within the ImageDigest object, we cannot mandate
+ * that it be held const.
+ */
+ ImageDigest *getImageDigestPtr();
private:
const SkBitmap fBitmap;
- SkAutoTUnref<ImageDigest> fImageDigestRef;
+ ImageDigest fImageDigest;
+ };
+
+ /**
+ * Expected test result: expected image (if any), and whether we want to ignore failures on
+ * this test or not.
+ *
+ * This is just an ImageDigest (or lack thereof, if there is no expectation) and a boolean
+ * telling us whether to ignore failures.
+ */
+ class Expectation {
+ public:
+ /**
+ * No expectation at all.
+ */
+ explicit Expectation(bool ignoreFailure=kDefaultIgnoreFailure);
+
+ /**
+ * Expect an image, passed as hashType/hashValue.
+ */
+ explicit Expectation(const SkString &hashType, uint64_t hashValue,
+ bool ignoreFailure=kDefaultIgnoreFailure);
+
+ /**
+ * Expect an image, passed as a bitmap.
+ */
+ explicit Expectation(const SkBitmap& bitmap,
+ bool ignoreFailure=kDefaultIgnoreFailure);
+
+ /**
+ * Returns true iff we want to ignore failed expectations.
+ */
+ bool ignoreFailure() const;
+
+ /**
+ * Returns true iff there are no allowed results.
+ */
+ bool empty() const;
+
+ /**
+ * Returns true iff we are expecting a particular image, and imageDigest matches it.
+ *
+ * If empty() returns true, this will return false.
+ *
+ * If this expectation DOES contain an image, and imageDigest doesn't match it,
+ * this method will return false regardless of what ignoreFailure() would return.
+ * (The caller can check that separately.)
+ */
+ bool matches(ImageDigest &imageDigest);
+
+ private:
+ static const bool kDefaultIgnoreFailure = false;
+
+ const bool fIsEmpty;
+ const bool fIgnoreFailure;
+ ImageDigest fImageDigest; // cannot be const, because it computes its hash lazily
};
/**
@@ -107,7 +173,7 @@
* @param digest description of the image's contents
* @param tileNumber if not NULL, pointer to tile number
*/
- void add(const char *sourceName, const char *fileName, const ImageDigest &digest,
+ void add(const char *sourceName, const char *fileName, ImageDigest &digest,
const int *tileNumber=NULL);
/**
@@ -119,15 +185,15 @@
void addDescription(const char *key, const char *value);
/**
- * Returns true if this test result matches its expectations.
- * If there are no expectations for this test result, this will return false.
+ * Returns the Expectation for this test.
*
* @param sourceName name of the source file that generated this result
- * @param digest description of the image's contents
* @param tileNumber if not NULL, pointer to tile number
+ *
+ * TODO(stephana): To make this work for GMs, we will need to add parameters for
+ * config, and maybe renderMode/builder?
*/
- bool matchesExpectation(const char *sourceName, const ImageDigest &digest,
- const int *tileNumber=NULL);
+ Expectation getExpectation(const char *sourceName, const int *tileNumber=NULL);
/**
* Writes the summary (as constructed so far) to a file.
diff --git a/tools/render_pictures_main.cpp b/tools/render_pictures_main.cpp
index e62c3bc..595a78e 100644
--- a/tools/render_pictures_main.cpp
+++ b/tools/render_pictures_main.cpp
@@ -366,7 +366,7 @@
sk_tools::ImageDigest imageDigest(*bitmap);
jsonSummaryPtr->add(inputFilename.c_str(), outputFilename.c_str(), imageDigest);
if ((NULL != mismatchPath) && !mismatchPath->isEmpty() &&
- !jsonSummaryPtr->matchesExpectation(inputFilename.c_str(), imageDigest)) {
+ !jsonSummaryPtr->getExpectation(inputFilename.c_str()).matches(imageDigest)) {
success &= sk_tools::write_bitmap_to_disk(*bitmap, *mismatchPath, NULL,
outputFilename);
}
diff --git a/tools/tests/render_pictures_test.py b/tools/tests/render_pictures_test.py
index 1fdeb10..9ff3226 100755
--- a/tools/tests/render_pictures_test.py
+++ b/tools/tests/render_pictures_test.py
@@ -213,6 +213,43 @@
self._assert_directory_contents(
write_path_dir, ['red_skp.png', 'green_skp.png'])
+ def test_ignore_some_failures(self):
+ """test_tiled_whole_image, but ignoring some failed tests.
+ """
+ output_json_path = os.path.join(self._output_dir, 'actuals.json')
+ write_path_dir = self.create_empty_dir(
+ path=os.path.join(self._output_dir, 'writePath'))
+ self._generate_skps()
+ expectations_path = self._create_expectations(ignore_some_failures=True)
+ self._run_render_pictures([
+ '-r', self._input_skp_dir,
+ '--bbh', 'grid', '256', '256',
+ '--mode', 'tile', '256', '256',
+ '--readJsonSummaryPath', expectations_path,
+ '--writeJsonSummaryPath', output_json_path,
+ '--writePath', write_path_dir,
+ '--writeWholeImage'])
+ modified_red_tiles = copy.deepcopy(RED_TILES)
+ modified_red_tiles[5]['comparisonResult'] = 'failure-ignored'
+ expected_summary_dict = {
+ "header" : EXPECTED_HEADER_CONTENTS,
+ "descriptions" : None,
+ "actual-results" : {
+ "red.skp": {
+ "tiled-images": modified_red_tiles,
+ "whole-image": modified_dict(
+ RED_WHOLEIMAGE, {"comparisonResult" : "failure-ignored"}),
+ },
+ "green.skp": {
+ "tiled-images": GREEN_TILES,
+ "whole-image": GREEN_WHOLEIMAGE,
+ }
+ }
+ }
+ self._assert_json_contents(output_json_path, expected_summary_dict)
+ self._assert_directory_contents(
+ write_path_dir, ['red_skp.png', 'green_skp.png'])
+
def test_missing_tile_and_whole_image(self):
"""test_tiled_whole_image, but missing expectations for some images.
"""
@@ -591,12 +628,14 @@
[binary, '--config', '8888'] + args)
def _create_expectations(self, missing_some_images=False,
+ ignore_some_failures=False,
rel_path='expectations.json'):
"""Creates expectations JSON file within self._expectations_dir .
Args:
missing_some_images: (bool) whether to remove expectations for a subset
of the images
+ ignore_some_failures: (bool) whether to ignore some failing tests
rel_path: (string) relative path within self._expectations_dir to write
the expectations into
@@ -621,8 +660,13 @@
}
}
if missing_some_images:
- del expectations_dict['expected-results']['red.skp']['whole-image']
- del expectations_dict['expected-results']['red.skp']['tiled-images'][-1]
+ red_subdict = expectations_dict['expected-results']['red.skp']
+ del red_subdict['whole-image']
+ del red_subdict['tiled-images'][-1]
+ elif ignore_some_failures:
+ red_subdict = expectations_dict['expected-results']['red.skp']
+ red_subdict['whole-image']['ignoreFailure'] = True
+ red_subdict['tiled-images'][-1]['ignoreFailure'] = True
path = os.path.join(self._expectations_dir, rel_path)
with open(path, 'w') as fh:
json.dump(expectations_dict, fh)
