Diff
Modified: trunk/Tools/ChangeLog (285108 => 285109)
--- trunk/Tools/ChangeLog 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/ChangeLog 2021-11-01 16:23:21 UTC (rev 285109)
@@ -1,3 +1,33 @@
+2021-11-01 Simon Fraser <[email protected]>
+
+ ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images
+ https://bugs.webkit.org/show_bug.cgi?id=232522
+
+ Reviewed by Martin Robinson.
+
+ ImageDiff no longer deals with tolerance, so remove `--tolerance` handling code in ImageDiff
+ and the driving script.
+
+ Also fix an issue where the diff image could be all black pixels even when there was a diff;
+ we need to ensure that a pixel with any diff is non-zero before scaling, and we need to
+ ensure that scaling by legacyDistanceMax doesn't overflow.
+
+ * ImageDiff/ImageDiff.cpp:
+ (processImages):
+ (main):
+ * ImageDiff/PlatformImage.cpp:
+ (ImageDiff::PlatformImage::difference):
+ * ImageDiff/PlatformImage.h:
+ * ImageDiff/cg/PlatformImageCG.cpp:
+ (ImageDiff::PlatformImage::createFromFile):
+ * Scripts/webkitpy/port/image_diff.py:
+ (ImageDiffer.diff_image):
+ (ImageDiffer._start): No need to restart ImageDiff if the tolerance changed, since
+ ImageDiff doesn't consult it. Tolerance in the python here just feeds into
+ the ImageDiffResult.
+ * Scripts/webkitpy/port/port_testcase.py:
+ (PortTestCase.test_diff_image):
+
2021-11-01 Aakash Jain <[email protected]>
Fix a typo in EWS emails
Modified: trunk/Tools/ImageDiff/ImageDiff.cpp (285108 => 285109)
--- trunk/Tools/ImageDiff/ImageDiff.cpp 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/ImageDiff/ImageDiff.cpp 2021-11-01 16:23:21 UTC (rev 285109)
@@ -49,7 +49,7 @@
#define FORMAT_SIZE_T "zu"
#endif
-static int processImages(std::unique_ptr<PlatformImage>&& actualImage, std::unique_ptr<PlatformImage>&& baselineImage, float tolerance, bool printDifference)
+static int processImages(std::unique_ptr<PlatformImage>&& actualImage, std::unique_ptr<PlatformImage>&& baselineImage, bool exact, bool printDifference)
{
if (!actualImage->isCompatible(*baselineImage)) {
if (actualImage->width() != baselineImage->width() || actualImage->height() != baselineImage->height()) {
@@ -64,7 +64,7 @@
}
PlatformImage::Difference differenceData = { 100, 0, 0 };
- auto diffImage = actualImage->difference(*baselineImage, differenceData);
+ auto diffImage = actualImage->difference(*baselineImage, exact, differenceData);
if (diffImage)
diffImage->writeAsPNGToStdout();
@@ -86,25 +86,11 @@
_setmode(1, _O_BINARY);
#endif
- float tolerance = 0.0f;
bool verbose = false;
+ bool exact = false;
bool printDifference = false;
for (int i = 1; i < argc; ++i) {
- if (!strcmp(argv[i], "-t") || !strcmp(argv[i], "--tolerance")) {
- if (i >= argc - 1)
- return EXIT_FAILURE;
-
- char* readEnd = NULL;
- tolerance = strtof(argv[i + 1], &readEnd);
- if (readEnd == argv[i + 1]) {
- fprintf(stderr, "Failed to read numberic tolerance value from %s\n", argv[i + 1]);
- return EXIT_FAILURE;
- }
- ++i;
- continue;
- }
-
if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
verbose = true;
continue;
@@ -115,9 +101,14 @@
continue;
}
+ if (!strcmp(argv[i], "-e") || !strcmp(argv[i], "--exact")) {
+ exact = true;
+ continue;
+ }
+
if (!strcmp(argv[i], "-h") || !strcmp(argv[i], "--help")) {
fprintf(stdout,
- "usage: ImageDiff [-h] [-v] [-d] [-t TOLERANCE] ([actualImage baselineImage] | <stdin>)\n" \
+ "usage: ImageDiff [-h] [-v] [-d] [-e] ([actualImage baselineImage] | <stdin>)\n" \
"\n" \
"Reads two PNG-encoded images and compares them. If two file path arguments are supplied, \n" \
"reads from the specified files, otherwise from <stdin> where each file is preceded by \n" \
@@ -127,8 +118,7 @@
" -h, --help show this help message and exit\n" \
" -v, --verbose print diagnostic information to stderr\n" \
" -d, --difference print WPT-style maxDifference and totalPixels data\n" \
- " -t, --tolerance TOLERANCE\n" \
- " compare the images with the given tolerance\n"
+ " -e, --exact use exact matching (no built-in per-component tolerance)\n"
);
return EXIT_SUCCESS;
}
@@ -163,7 +153,7 @@
if (verbose)
fprintf(stderr, "Comparing files actual: %s and baseline: %s\n", file1Path, file2Path);
- return processImages(std::move(actualImage), std::move(baselineImage), tolerance, printDifference);
+ return processImages(std::move(actualImage), std::move(baselineImage), exact, printDifference);
}
}
@@ -209,9 +199,7 @@
}
if (actualImage && baselineImage) {
- if (verbose)
- fprintf(stderr, "ImageDiff: processing images with tolerance %01.2f%%\n", tolerance);
- auto result = processImages(std::exchange(actualImage, { }), std::exchange(baselineImage, { }), tolerance, printDifference);
+ auto result = processImages(std::exchange(actualImage, { }), std::exchange(baselineImage, { }), exact, printDifference);
if (result != EXIT_SUCCESS)
return result;
}
Modified: trunk/Tools/ImageDiff/PlatformImage.cpp (285108 => 285109)
--- trunk/Tools/ImageDiff/PlatformImage.cpp 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/ImageDiff/PlatformImage.cpp 2021-11-01 16:23:21 UTC (rev 285109)
@@ -40,13 +40,13 @@
&& hasAlpha() == other.hasAlpha();
}
-std::unique_ptr<PlatformImage> PlatformImage::difference(const PlatformImage& other, Difference& difference)
+std::unique_ptr<PlatformImage> PlatformImage::difference(const PlatformImage& other, bool exact, Difference& difference)
{
size_t width = this->width();
size_t height = this->height();
// Compare the content of the 2 bitmaps
- void* diffBuffer = malloc(width * height);
+ unsigned char* diffBuffer = static_cast<unsigned char*>(calloc(width * height, sizeof(unsigned char)));
size_t pixelCountWithSignificantDifference = 0;
float legacyDistanceSum = 0.0f;
float legacyDistanceMax = 0.0f;
@@ -53,7 +53,7 @@
unsigned char* basePixel = this->pixels();
unsigned char* pixel = other.pixels();
- unsigned char* diffPixel = reinterpret_cast<unsigned char*>(diffBuffer);
+ unsigned char* diffPixel = diffBuffer;
for (size_t y = 0; y < height; ++y) {
for (size_t x = 0; x < width; ++x) {
@@ -63,8 +63,6 @@
float alpha = (pixel[3] - basePixel[3]) / std::max<float>(255 - basePixel[3], basePixel[3]);
float legacyDistance = sqrtf(red * red + green * green + blue * blue + alpha * alpha) / 2.0f;
- *diffPixel++ = static_cast<unsigned char>(legacyDistance * 255.0f);
-
// WPT-style difference code.
if (legacyDistance) {
++difference.totalPixels;
@@ -75,10 +73,12 @@
difference.maxDifference = std::max(difference.maxDifference, maxDiff);
legacyDistanceMax = std::max(legacyDistanceMax, legacyDistance);
+ *diffPixel = static_cast<unsigned char>(std::ceil(legacyDistance * 255.0f));
}
- // Legacy difference code. Note there is some built-in tolerance here.
- if (legacyDistance >= 1.0f / 255.0f) {
+ // Legacy difference code.
+ bool pixelDiffers = exact ? legacyDistance : legacyDistance >= 1.0f / 255.0f;
+ if (pixelDiffers) {
++pixelCountWithSignificantDifference;
legacyDistanceSum += legacyDistance;
}
@@ -85,6 +85,7 @@
basePixel += 4;
pixel += 4;
+ ++diffPixel;
}
}
@@ -95,9 +96,14 @@
difference.percentageDifference = 0.0f;
if (difference.totalPixels) {
- diffPixel = reinterpret_cast<unsigned char*>(diffBuffer);
- for (size_t p = 0; p < height * width; ++p)
- diffPixel[p] /= legacyDistanceMax;
+ diffPixel = diffBuffer;
+ for (size_t p = 0; p < height * width; ++p) {
+ float pixel = static_cast<float>(diffPixel[p]);
+ if (!pixel)
+ continue;
+ pixel = std::min(pixel / legacyDistanceMax, 255.0f);
+ diffPixel[p] = static_cast<unsigned char>(pixel);
+ }
return PlatformImage::createFromDiffData(diffBuffer, width, height);
}
Modified: trunk/Tools/ImageDiff/PlatformImage.h (285108 => 285109)
--- trunk/Tools/ImageDiff/PlatformImage.h 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/ImageDiff/PlatformImage.h 2021-11-01 16:23:21 UTC (rev 285109)
@@ -62,7 +62,7 @@
unsigned maxDifference { 0 };
size_t totalPixels { 0 };
};
- std::unique_ptr<PlatformImage> difference(const PlatformImage&, Difference&);
+ std::unique_ptr<PlatformImage> difference(const PlatformImage&, bool exact, Difference&);
void writeAsPNGToStdout();
Modified: trunk/Tools/ImageDiff/cg/PlatformImageCG.cpp (285108 => 285109)
--- trunk/Tools/ImageDiff/cg/PlatformImageCG.cpp 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/ImageDiff/cg/PlatformImageCG.cpp 2021-11-01 16:23:21 UTC (rev 285109)
@@ -74,8 +74,13 @@
std::unique_ptr<PlatformImage> PlatformImage::createFromFile(const char* filePath)
{
auto dataProvider = CGDataProviderCreateWithFilename(filePath);
+ if (!dataProvider)
+ return nullptr;
+
auto image = CGImageCreateWithPNGDataProvider(dataProvider, 0, false, kCGRenderingIntentDefault);
CGDataProviderRelease(dataProvider);
+ if (!image)
+ return nullptr;
return std::make_unique<PlatformImage>(image);
}
Modified: trunk/Tools/Scripts/webkitpy/port/image_diff.py (285108 => 285109)
--- trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-11-01 16:23:21 UTC (rev 285109)
@@ -72,7 +72,7 @@
self._process = None
def diff_image(self, expected_contents, actual_contents, tolerance):
- if tolerance != self._tolerance or (self._process and self._process.has_available_stdout()):
+ if (self._process and self._process.has_available_stdout()):
self.stop()
try:
assert(expected_contents)
@@ -93,7 +93,7 @@
return (None, 0, "Failed to compute an image diff: %s" % str(exception))
def _start(self, tolerance):
- command = [self._port._path_to_image_diff(), '--difference', '--tolerance', str(tolerance)]
+ command = [self._port._path_to_image_diff(), '--difference']
if self._port._should_use_jhbuild():
command = self._port._jhbuild_wrapper + command
environment = self._port.setup_environ_for_server('ImageDiff')
Modified: trunk/Tools/Scripts/webkitpy/port/port_testcase.py (285108 => 285109)
--- trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-11-01 16:02:28 UTC (rev 285108)
+++ trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-11-01 16:23:21 UTC (rev 285109)
@@ -310,13 +310,13 @@
result_fuzzy_data = {'max_difference': 6, 'total_pixels': 50}
self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1, fuzzy_data=result_fuzzy_data))
- self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--difference", "--tolerance", "0.1"])
+ self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--difference"])
self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1, fuzzy_data=result_fuzzy_data))
- self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--difference", "--tolerance", "0.1"])
+ self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--difference"])
self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, fuzzy_data=result_fuzzy_data))
- self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--difference", "--tolerance", "0"])
+ self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--difference"])
# Now test the case of using JHBuild wrapper.
port._filesystem.maybe_make_directory(port.path_from_webkit_base('WebKitBuild', 'Dependencies%s' % port.port_name.upper()))
@@ -323,13 +323,13 @@
self.assertTrue(port._should_use_jhbuild())
self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1, fuzzy_data=result_fuzzy_data))
- self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--difference", "--tolerance", "0.1"])
+ self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--difference"])
self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1, fuzzy_data=result_fuzzy_data))
- self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--difference", "--tolerance", "0.1"])
+ self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--difference"])
self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, fuzzy_data=result_fuzzy_data))
- self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--difference", "--tolerance", "0"])
+ self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--difference"])
port.clean_up_test_run()
self.assertTrue(self.proc.stopped)
