Changeset 284878 in webkit

Timestamp:

Oct 26, 2021, 10:39:19 AM (5 years ago)

Author:

Simon Fraser

Message:

The script should decide when an image diff cases, not ImageDiff
​https://bugs.webkit.org/show_bug.cgi?id=232225

Reviewed by Martin Robinson.

Rather than have ImageDiff decide if the comparison passes or fails (with some built-in
tolerance), have it just print the percentage difference, and have the script compare it
against the tolerance.

Code to prettify diff_percent is moved into the script (but should eventually
move closer to display time).

• ImageDiff/ImageDiff.cpp:

(processImages):

• Scripts/webkitpy/port/image_diff.py:

(ImageDiffer._read):

• Scripts/webkitpy/port/port_testcase.py:

(PortTestCase.test_diff_image.make_proc):
(PortTestCase.test_diff_image_passed):
(PortTestCase):
(PortTestCase.test_diff_image_passed_with_tolerance):
(PortTestCase.test_diff_image_failed_with_rounded_diff):
(PortTestCase.test_diff_image_failed):

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• ImageDiff/ImageDiff.cpp (modified) (1 diff)
• Scripts/webkitpy/port/image_diff.py (modified) (1 diff)
• Scripts/webkitpy/port/port_testcase.py (modified) (2 diffs)

trunk/Tools/ChangeLog

@@ +1 @@
+2021-10-26  Simon Fraser  <simon.fraser@apple.com>
+
+        The script should decide when an image diff cases, not ImageDiff
+        https://bugs.webkit.org/show_bug.cgi?id=232225
+
+        Reviewed by Martin Robinson.
+
+        Rather than have ImageDiff decide if the comparison passes or fails (with some built-in
+        tolerance), have it just print the percentage difference, and have the script compare it
+        against the tolerance.
+
+        Code to prettify diff_percent is moved into the script (but should eventually
+        move closer to display time).
+
+        * ImageDiff/ImageDiff.cpp:
+        (processImages):
+        * Scripts/webkitpy/port/image_diff.py:
+        (ImageDiffer._read):
+        * Scripts/webkitpy/port/port_testcase.py:
+        (PortTestCase.test_diff_image.make_proc):
+        (PortTestCase.test_diff_image_passed):
+        (PortTestCase):
+        (PortTestCase.test_diff_image_passed_with_tolerance):
+        (PortTestCase.test_diff_image_failed_with_rounded_diff):
+        (PortTestCase.test_diff_image_failed):
+
 2021-10-26  Kate Cheney  <katherine_cheney@apple.com>
 

trunk/Tools/ImageDiff/ImageDiff.cpp

@@ -66 +66 @@
     PlatformImage::Difference differenceData = { 100, 0, 0 };
     auto diffImage = actualImage->difference(*baselineImage, differenceData);
-    float legacyDifference = differenceData.percentageDifference;
-    if (legacyDifference <= tolerance)
-        legacyDifference = 0.0f;
-    else {
-        legacyDifference = roundf(legacyDifference * 100.0f) / 100.0f;
-        legacyDifference = std::max<float>(legacyDifference, 0.01f); // round to 2 decimal places
-    }
-
     if (diffImage)
         diffImage->writeAsPNGToStdout();
 
-    if (legacyDifference > 0.0f) {
-        fprintf(stdout, "diff: %01.2f%% failed\n", legacyDifference);
-    } else
-        fprintf(stdout, "diff: %01.2f%% passed\n", legacyDifference);
+    fprintf(stdout, "diff: %01.8f%%\n", differenceData.percentageDifference);
 
     if (printDifference)

trunk/Tools/Scripts/webkitpy/port/image_diff.py

@@ -130 +130 @@
             err_str += "ImageDiff crashed\n"
 
-        diff_percent = 0
-        if diff_output:
-            m = re.match(b'diff: (.+)% (passed|failed)', diff_output)
-            if m.group(2) == b'passed':
-                return ImageDiffResult(passed=True, diff_image=output_image, difference=0)
-            diff_percent = float(string_utils.decode(m.group(1), target_type=str))
+        if not diff_output:
+            return ImageDiffResult(passed=False, diff_image=None, difference=0, tolerance=self._tolerance, error_string=err_str or "Failed to read ImageDiff output")
 
-        return ImageDiffResult(passed=False, diff_image=output_image, difference=diff_percent, tolerance=self._tolerance, error_string=err_str or None)
+        m = re.match(b'diff: (.+)%', diff_output)
+        if not m:
+            return ImageDiffResult(passed=False, diff_image=None, difference=0, tolerance=self._tolerance, error_string=err_str or "Failed to match ImageDiff output %s" % diff_output)
+
+        diff_percent = float(string_utils.decode(m.group(1), target_type=str))
+
+        passed = diff_percent <= self._tolerance
+        if not passed:
+            # FIXME: This prettification should happen at display time.
+            diff_percent = round(diff_percent * 100) / 100
+            diff_percent = max(diff_percent, 0.01)
+
+        return ImageDiffResult(passed=passed, diff_image=output_image, difference=diff_percent, tolerance=self._tolerance, error_string=err_str or None)
 
     def stop(self):

trunk/Tools/Scripts/webkitpy/port/port_testcase.py

@@ -285 +285 @@
 
         def make_proc(port, nm, cmd, env, crash_message=None):
-            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'])
+            self.proc = MockServerProcess(port, nm, cmd, env, lines=['Content-Length: 6\n', 'image1', 'diff: 90%\n', '#EOF\n', 'Content-Length: 6\n', 'image2', 'diff: 100%\n', '#EOF\n'])
             return self.proc
 
@@ -326 +326 @@
     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', '#EOF\n'])
+        port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 0%\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))
+        self.assertEqual(image_differ.diff_image(b'foo', b'foo', tolerance=0), ImageDiffResult(passed=True, diff_image=None, difference=0, tolerance=0))
+
+    def test_diff_image_passed_with_tolerance(self):
+        port = self.make_port()
+        port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['Content-Length: 4\n', 'test', 'diff: 0.05%\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=b'test', difference=0.05, tolerance=0.1))
+
+    def test_diff_image_failed_with_rounded_diff(self):
+        port = self.make_port()
+        port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['Content-Length: 4\n', 'test', 'diff: 0.101234%\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'test', difference=0.1, tolerance=0.1))
 
     def test_diff_image_failed(self):
         port = self.make_port()
-        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'])
+        port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['Content-Length: 4\n', 'test', 'diff: 10%\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'test', 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=10, tolerance=0.1))
 
     def test_diff_image_crashed(self):