Log Message
Have ImageDiff print the diff image when any pixel is different https://bugs.webkit.org/show_bug.cgi?id=232294
Reviewed by Martin Robinson. ImageDiff currently only outputs the diff image when any pixel exceeds its built-in tolerance. To prepare for moving the "pass/fail" decision to script, have ImageDiff output the diff image when any pixel is different. Also have it write "#EOF" so that we're not reliant on the "diff:" line to terminate reading the output. Fix up webkitpy unit tests for #EOF parsing, presence of image when the test passes via tolerance, and to actually test which image data is present in the ImageDiffResult. * ImageDiff/ImageDiff.cpp: (processImages): (main): * ImageDiff/PlatformImage.cpp: (ImageDiff::PlatformImage::difference): Track legacyDistanceMax if any pixel diff is non-zero, since it's needed to scale the diff image. * Scripts/webkitpy/port/image_diff.py: (ImageDiffer._read): Look for "#EOF" to terminate the output. Save the diff image, even if the test passed. * Scripts/webkitpy/port/port_testcase.py: (PortTestCase.test_diff_image.make_proc): (PortTestCase.test_diff_image): (PortTestCase.test_diff_image_passed): (PortTestCase.test_diff_image_failed): (PortTestCase.test_diff_image_crashed):
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (284869 => 284870)
--- trunk/Tools/ChangeLog 2021-10-26 15:55:18 UTC (rev 284869)
+++ trunk/Tools/ChangeLog 2021-10-26 15:55:48 UTC (rev 284870)
@@ -1,5 +1,38 @@
2021-10-26 Simon Fraser <[email protected]>
+ Have ImageDiff print the diff image when any pixel is different
+ https://bugs.webkit.org/show_bug.cgi?id=232294
+
+ Reviewed by Martin Robinson.
+
+ ImageDiff currently only outputs the diff image when any pixel exceeds its built-in
+ tolerance.
+
+ To prepare for moving the "pass/fail" decision to script, have ImageDiff output the diff
+ image when any pixel is different. Also have it write "#EOF" so that we're not reliant on
+ the "diff:" line to terminate reading the output.
+
+ Fix up webkitpy unit tests for #EOF parsing, presence of image when the test passes via
+ tolerance, and to actually test which image data is present in the ImageDiffResult.
+
+ * ImageDiff/ImageDiff.cpp:
+ (processImages):
+ (main):
+ * ImageDiff/PlatformImage.cpp:
+ (ImageDiff::PlatformImage::difference): Track legacyDistanceMax if any pixel diff is non-zero,
+ since it's needed to scale the diff image.
+ * Scripts/webkitpy/port/image_diff.py:
+ (ImageDiffer._read): Look for "#EOF" to terminate the output. Save the diff image, even
+ if the test passed.
+ * Scripts/webkitpy/port/port_testcase.py:
+ (PortTestCase.test_diff_image.make_proc):
+ (PortTestCase.test_diff_image):
+ (PortTestCase.test_diff_image_passed):
+ (PortTestCase.test_diff_image_failed):
+ (PortTestCase.test_diff_image_crashed):
+
+2021-10-26 Simon Fraser <[email protected]>
+
Don't run ImageDiff a second time to generate diff images
https://bugs.webkit.org/show_bug.cgi?id=232288
Modified: trunk/Tools/ImageDiff/ImageDiff.cpp (284869 => 284870)
--- trunk/Tools/ImageDiff/ImageDiff.cpp 2021-10-26 15:55:18 UTC (rev 284869)
+++ trunk/Tools/ImageDiff/ImageDiff.cpp 2021-10-26 15:55:48 UTC (rev 284870)
@@ -73,9 +73,10 @@
legacyDifference = std::max<float>(legacyDifference, 0.01f); // round to 2 decimal places
}
+ if (diffImage)
+ diffImage->writeAsPNGToStdout();
+
if (legacyDifference > 0.0f) {
- if (diffImage)
- diffImage->writeAsPNGToStdout();
fprintf(stdout, "diff: %01.2f%% failed\n", legacyDifference);
} else
fprintf(stdout, "diff: %01.2f%% passed\n", legacyDifference);
@@ -83,6 +84,9 @@
if (printDifference)
fprintf(stdout, "maxDifference=%u; totalPixels=%lu\n", differenceData.maxDifference, differenceData.totalPixels);
+ fprintf(stdout, "#EOF\n");
+ fflush(stdout);
+
return EXIT_SUCCESS;
}
@@ -222,7 +226,6 @@
if (result != EXIT_SUCCESS)
return result;
}
- fflush(stdout);
}
return EXIT_SUCCESS;
Modified: trunk/Tools/ImageDiff/PlatformImage.cpp (284869 => 284870)
--- trunk/Tools/ImageDiff/PlatformImage.cpp 2021-10-26 15:55:18 UTC (rev 284869)
+++ trunk/Tools/ImageDiff/PlatformImage.cpp 2021-10-26 15:55:48 UTC (rev 284870)
@@ -73,6 +73,8 @@
unsigned blueDiff = std::abs(pixel[2] - basePixel[2]);
unsigned maxDiff = std::max({ redDiff, greenDiff, blueDiff });
difference.maxDifference = std::max(difference.maxDifference, maxDiff);
+
+ legacyDistanceMax = std::max(legacyDistanceMax, legacyDistance);
}
// Legacy difference code. Note there is some built-in tolerance here.
@@ -79,7 +81,6 @@
if (legacyDistance >= 1.0f / 255.0f) {
++pixelCountWithSignificantDifference;
legacyDistanceSum += legacyDistance;
- legacyDistanceMax = std::max(legacyDistanceMax, legacyDistance);
}
basePixel += 4;
@@ -93,19 +94,16 @@
else
difference.percentageDifference = 0.0f;
- if (!pixelCountWithSignificantDifference) {
- free(diffBuffer);
- return nullptr;
- }
-
- // Generate a normalized diff image if there is any difference.
- if (pixelCountWithSignificantDifference) {
+ if (difference.totalPixels) {
diffPixel = reinterpret_cast<unsigned char*>(diffBuffer);
for (size_t p = 0; p < height * width; ++p)
diffPixel[p] /= legacyDistanceMax;
+
+ return PlatformImage::createFromDiffData(diffBuffer, width, height);
}
- return PlatformImage::createFromDiffData(diffBuffer, width, height);
+ free(diffBuffer);
+ return nullptr;
}
} // namespace ImageDiff
Modified: trunk/Tools/Scripts/webkitpy/port/image_diff.py (284869 => 284870)
--- trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-10-26 15:55:18 UTC (rev 284869)
+++ trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-10-26 15:55:48 UTC (rev 284870)
@@ -101,8 +101,8 @@
def _read(self):
deadline = time.time() + 2.0
- output = None
- output_image = b''
+ output_image = None
+ diff_output = None
while not self._process.timed_out and not self._process.has_crashed():
output = self._process.read_stdout_line(deadline)
@@ -109,15 +109,16 @@
if self._process.timed_out or self._process.has_crashed() or not output:
break
- if output.startswith(b'diff'): # This is the last line ImageDiff prints.
+ if output.startswith(b'#EOF'):
break
+ if output.startswith(b'diff:'):
+ diff_output = output
+
if output.startswith(b'Content-Length'):
m = re.match(br'Content-Length: (\d+)', output)
content_length = int(string_utils.decode(m.group(1), target_type=str))
output_image = self._process.read_stdout(deadline, content_length)
- output = self._process.read_stdout_line(deadline)
- break
stderr = string_utils.decode(self._process.pop_all_buffered_stderr(), target_type=str)
err_str = ''
@@ -129,10 +130,10 @@
err_str += "ImageDiff crashed\n"
diff_percent = 0
- if output and output.startswith(b'diff'):
- m = re.match(b'diff: (.+)% (passed|failed)', output)
+ if diff_output:
+ m = re.match(b'diff: (.+)% (passed|failed)', diff_output)
if m.group(2) == b'passed':
- return ImageDiffResult(passed=True, diff_image=None, difference=0)
+ return ImageDiffResult(passed=True, diff_image=output_image, difference=0)
diff_percent = float(string_utils.decode(m.group(1), target_type=str))
return ImageDiffResult(passed=False, diff_image=output_image, difference=diff_percent, tolerance=self._tolerance, error_string=err_str or None)
Modified: trunk/Tools/Scripts/webkitpy/port/port_testcase.py (284869 => 284870)
--- trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-10-26 15:55:18 UTC (rev 284869)
+++ trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-10-26 15:55:48 UTC (rev 284870)
@@ -284,7 +284,7 @@
self.proc = None
def make_proc(port, nm, cmd, env, crash_message=None):
- self.proc = MockServerProcess(port, nm, cmd, env, lines=['diff: 100% failed\n', 'diff: 100% failed\n'])
+ self.proc = MockServerProcess(port, nm, cmd, env, lines=['Content-Length: 6\n', 'image1', 'diff: 90% failed\n', '#EOF\n', 'Content-Length: 6\n', 'image2', 'diff: 100% failed\n', '#EOF\n'])
return self.proc
# FIXME: Can't pretend to run setup for some ports, so just skip this test.
@@ -297,13 +297,13 @@
# First test the case of not using the JHBuild wrapper.
self.assertFalse(port._should_use_jhbuild())
- self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0"])
# Now test the case of using JHBuild wrapper.
@@ -310,13 +310,13 @@
port._filesystem.maybe_make_directory(port.path_from_webkit_base('WebKitBuild', 'Dependencies%s' % port.port_name.upper()))
self.assertTrue(port._should_use_jhbuild())
- self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0, tolerance=0.1))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'image1', difference=90.0))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0"])
port.clean_up_test_run()
@@ -325,15 +325,15 @@
def test_diff_image_passed(self):
port = self.make_port()
- port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 0% passed\n'])
+ port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 0% passed\n', '#EOF\n'])
image_differ = ImageDiffer(port)
self.assertEqual(image_differ.diff_image(b'foo', b'bar', tolerance=0.1), ImageDiffResult(passed=True, diff_image=None, difference=0))
def test_diff_image_failed(self):
port = self.make_port()
- port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 100% failed\n'])
+ port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['Content-Length: 4\n', 'test', 'diff: 100% failed\n', '#EOF\n'])
image_differ = ImageDiffer(port)
- self.assertEqual(image_differ.diff_image(b'foo', b'bar', tolerance=0.1), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
+ self.assertEqual(image_differ.diff_image(b'foo', b'bar', tolerance=0.1), ImageDiffResult(passed=False, diff_image=b'test', difference=100.0, tolerance=0.1))
def test_diff_image_crashed(self):
port = self.make_port()
@@ -349,7 +349,7 @@
port._server_process_constructor = make_proc
port.setup_test_run()
- self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=0, tolerance=0.1, error_string='ImageDiff crashed\n'))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=None, difference=0, tolerance=0.1, error_string='ImageDiff crashed\n'))
port.clean_up_test_run()
@slow
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
