168221 2017-02-12 22:08:46 -0800 Nwtr unexpectedly passes mismatch ref test if the hashes doesn't match but no diff 2017-02-14 16:16:49 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified CLOSED INVALID P2 Normal --- 168298 168033 1 fujii fujii ap commit-queue glenn lforschler rniwa simon.fraser oldest_to_newest 1276092 0 fujii 2017-02-12 22:08:46 -0800 Nwtr unexpectedly passes mismatch ref test if the hashes doesn't match but no diff There is a logic error in comparing mismatch ref test images. In mismatch ref test images, it should be failed if two images has no diff. But, if the hashes are different, nwtr unexpectedly pass the mismatch ref test. It is difficult to test this bug. First, you need to find a *match* ref test which does not match the hashes. For example, in GTK port, I found a following test is the case: > fast/css/sticky/sticky-left-percentage.html -> ref test hashes didn't match but diff passed Next, convert this match ref test to a mismatch ref test: > mv LayoutTests/fast/css/sticky/sticky-left-percentage-expected{,-mismatch}.html And, run the test. In this time, it should fail: > $ ./Tools/Scripts/run-webkit-tests --gtk --release --no-new-test-results fast/css/sticky/sticky-left-percentage.html -v But, passed: > [1/1] fast/css/sticky/sticky-left-percentage.html passed 1276095 1 301332 fujii 2017-02-12 22:21:28 -0800 Created attachment 301332 Patch 1276220 2 301332 ap 2017-02-13 10:06:48 -0800 Comment on attachment 301332 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301332&action=review > Tools/ChangeLog:3 > + Nwtr unexpectedly passes mismatch ref test if the hashes doesn't match but no diff Why does the logic for expected mismatch run ImageDiff in this case? I'd expect the test to pass as soon as there is a hash mismatch. 1276268 3 301332 rniwa 2017-02-13 11:35:03 -0800 Comment on attachment 301332 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301332&action=review >> Tools/ChangeLog:3 >> + Nwtr unexpectedly passes mismatch ref test if the hashes doesn't match but no diff > > Why does the logic for expected mismatch run ImageDiff in this case? > > I'd expect the test to pass as soon as there is a hash mismatch. The problem is that sometimes two images look identical even though hashes are different. e.g. due to anti-aliasing differences on Mac when a layer is composed. 1276281 4 301332 commit-queue 2017-02-13 12:00:15 -0800 Comment on attachment 301332 Patch Clearing flags on attachment: 301332 Committed r212237: <http://trac.webkit.org/changeset/212237> 1276282 5 commit-queue 2017-02-13 12:00:22 -0800 All reviewed patches have been landed. Closing bug. 1276283 6 ap 2017-02-13 12:02:34 -0800 > The problem is that sometimes two images look identical even though hashes are different. > e.g. due to anti-aliasing differences on Mac when a layer is composed. I understand this, but that doesn't answer my question. I still think that this patch is not the right approach to the issue. 1276285 7 rniwa 2017-02-13 12:08:38 -0800 (In reply to comment #6) > > The problem is that sometimes two images look identical even though hashes are different. > > e.g. due to anti-aliasing differences on Mac when a layer is composed. > > I understand this, but that doesn't answer my question. I still think that > this patch is not the right approach to the issue. What do you think will be the right fix? 1276297 8 ap 2017-02-13 12:26:13 -0800 I think that we should report pass as soon as there is a hash difference. We already know that there is some mismatch at this point, and it's not the tool's responsibility to make guesses about how much of a difference is really a difference. 1276342 9 rniwa 2017-02-13 13:38:29 -0800 (In reply to comment #8) > I think that we should report pass as soon as there is a hash difference. We > already know that there is some mismatch at this point, and it's not the > tool's responsibility to make guesses about how much of a difference is > really a difference. The problem is that tests with mismatch expectation MUST fail when two images look the same, and hashes don't seem to be a good indicator of whether they're the same or not since many of the existing ref tests with match expectation only pass under image-diff, meaning that many tests have different hashes from their respective expected results. Conversely, it is then possible for many tests with mismatch expectation to have different hashes from expected results yet look identical (failing condition). 1276364 10 ap 2017-02-13 14:22:57 -0800 I don't think that mismatch is about looking the same. For example, if you have a mismatch test that confirms that two different forms of antialiasing are not identical, the two results will look almost the same. Do you have a specific counter example where running an image diff would be helpful? We don't have that many "failed hash but passed diff" tests to serve as a generic counter example. 1276453 11 fujii 2017-02-13 16:58:08 -0800 I don't understand your question correctly. I have a question. Do you mean you are against using ImageDiff only for mismatch ref tests, even though you think it's ok to use ImageDiff for match ref test? 1276552 12 fujii 2017-02-13 21:05:16 -0800 There were some ideas suggested here: https://lists.webkit.org/pipermail/webkit-dev/2015-January/027179.html * Add ImageDiff or ImageHashMismatch expectation * Expose a new internals or testRunner methods to mark a test such 1276581 13 fujii 2017-02-13 22:42:05 -0800 Bug 94704 – REGRESSION(r126189): Reftest mismatches are (again) run through ImageDiff with 0.1 tolerance tolerance=0.1 is given to pixel tests, and tolerance=0 for ref tests. Pixels should be equal in ref tests. 1276583 14 rniwa 2017-02-13 22:52:19 -0800 (In reply to comment #13) > Bug 94704 – REGRESSION(r126189): Reftest mismatches are (again) run > through ImageDiff with 0.1 tolerance > > tolerance=0.1 is given to pixel tests, and tolerance=0 for ref tests. > Pixels should be equal in ref tests. Wait, if that's the case how could this code path ever be useful? 1276601 15 fujii 2017-02-13 23:09:23 -0800 I had a big mistake. I was using fast/css/sticky/sticky-left-percentage.html for testing (comment 0). But this test has other problem (bug 168033 comment 7). 1276604 16 fujii 2017-02-13 23:11:56 -0800 (In reply to comment #14) > Wait, if that's the case how could this code path ever be useful? That's right. Calling diff_image with tolerance=0 is useless. Checking checksum seems enough. 1276610 17 ap 2017-02-13 23:17:28 -0800 Note that there is some tolerance forced by ImageDiffCG.cpp even when tolerance=0 is requested. That may be not easy to clean up. 1276612 18 fujii 2017-02-13 23:20:21 -0800 (In reply to comment #17) > Note that there is some tolerance forced by ImageDiffCG.cpp even when > tolerance=0 is requested. That may be not easy to clean up. I can not find such code in ImageDiffCG.cpp. Could you tell me which code? 1276626 19 ap 2017-02-13 23:56:01 -0800 Distances lower than 1/255 are ignored: if (distance >= 1.0f / 255.0f) { count += 1.0f; sum += distance; if (distance > maxDistance) maxDistance = distance; } 1276643 20 fujii 2017-02-14 01:11:25 -0800 (In reply to comment #19) > Distances lower than 1/255 are ignored: > > if (distance >= 1.0f / 255.0f) { > count += 1.0f; > sum += distance; > if (distance > maxDistance) > maxDistance = distance; > } Thank you so much. This color distance threshold is needed for match ref tests, but for mismatch ref tests at the moment. Should I stop calling image_diff and just do checksum checking for mismatch ref tests? 1276653 21 fujii 2017-02-14 02:10:29 -0800 (In reply to comment #20) > Should I stop calling image_diff and just do checksum checking > for mismatch ref tests? I checked some mismatch ref tests to see how do they expect. Most of them expect substantial mismatch. But, a following test expects tiny mismatch. I understand Alexey's comment 10. css3/filters/drop-shadow-blur-radius.html Original code before my change <http://trac.webkit.org/changeset/212237> was actually just doing checksum checking for mismatch ref tests. So, I prefer rolling back r212237 and closing this bug as invalid. I'll remove calling image_diff for mismatch ref tests in other bug. I don't have a permission to roll back. Could you roll back r212237? 1276659 22 commit-queue 2017-02-14 02:48:45 -0800 Re-opened since this is blocked by bug 168298 1276690 23 fujii 2017-02-14 05:24:17 -0800 Thank you, Ryosuke. Closed this bug as INVALID. 1276784 24 ap 2017-02-14 09:21:21 -0800 I do think that there is a bug here though, as you've demonstrated how the logic is clearly wrong. It just seems better to not run ImageDiff in this case. 1276828 25 simon.fraser 2017-02-14 11:01:57 -0800 There's still a valid need to have ref tests that are a close, but not exact match (for example, software-rendered clipping vs. GPU-rendered clipping). 1276987 26 fujii 2017-02-14 16:14:39 -0800 (In reply to comment #24) > I do think that there is a bug here though, as you've demonstrated how the > logic is clearly wrong. It just seems better to not run ImageDiff in this > case. I agree and I'll do it (comment 21). 1276988 27 fujii 2017-02-14 16:16:49 -0800 (In reply to comment #25) > There's still a valid need to have ref tests that are a close, but not exact > match (for example, software-rendered clipping vs. GPU-rendered clipping). Right. If someone will add a new mismatch ref test which needs the color distance threshold, it will be the counter example which Alexey requested (comment 10). 301332 2017-02-12 22:21:28 -0800 2017-02-13 12:00:15 -0800 Patch bug-168221-20170213151856.patch text/plain 2670 fujii U3VidmVyc2lvbiBSZXZpc2lvbjogMjEyMTExCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggNTg2NzQ4ZTQ1NjExYmRmNzE3YmE1Y2Y5Y2ZmMzRiNDMz NjJlZDM1Yi4uZDE2YzVjYjNiNDgwYjUyYTBmMTFjMDM3MzQzNTFhYzQxZTM0YjVjMSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIw IEBACisyMDE3LTAyLTEyICBGdWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29t PgorCisgICAgICAgIE53dHIgdW5leHBlY3RlZGx5IHBhc3NlcyBtaXNtYXRjaCByZWYgdGVzdCBp ZiB0aGUgaGFzaGVzIGRvZXNuJ3QgbWF0Y2ggYnV0IG5vIGRpZmYKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2ODIyMQorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZXJlIGlzIGEgbG9naWMgZXJyb3IgaW4g Y29tcGFyaW5nIG1pc21hdGNoIHJlZiB0ZXN0IGltYWdlcy4gSW4KKyAgICAgICAgbWlzbWF0Y2gg cmVmIHRlc3RzLCBpdCBzaG91bGQgYmUgZmFpbGVkIGlmIHR3byBpbWFnZXMgaGFzIG5vIGRpZmYu CisgICAgICAgIEJ1dCwgaWYgdGhlIGhhc2hlcyBhcmUgZGlmZmVyZW50LCBud3RyIHVuZXhwZWN0 ZWRseSBwYXNzIHRoZQorICAgICAgICBtaXNtYXRjaCByZWYgdGVzdC4KKworICAgICAgICAqIFNj cmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL2NvbnRyb2xsZXJzL3NpbmdsZV90ZXN0X3J1bm5l ci5weToKKyAgICAgICAgKFNpbmdsZVRlc3RSdW5uZXIuX2NvbXBhcmVfb3V0cHV0X3dpdGhfcmVm ZXJlbmNlKTogRmFpbCB0aGUKKyAgICAgICAgbWlzbWF0Y2ggcmVmIHRlc3QgaWYgaGFzaGVzIG9m IHR3byBpbWFnZXMgYXJlIGVxdWFsLiBJbnZva2UKKyAgICAgICAgSW1hZ2VEaWZmIGlmIHRoZSBo YXNoZXMgZG9uJ3QgbWF0Y2guCisKIDIwMTctMDItMTAgIEFuZHkgRXN0ZXMgIDxhZXN0ZXNAYXBw bGUuY29tPgogCiAgICAgICAgIE1vdmUgdHdvIE1pbmlCcm93c2VyQnVuZGxlIHNldHRpbmdzIG91 dCBvZiB0aGUgLnBieHByb2oKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5 b3V0X3Rlc3RzL2NvbnRyb2xsZXJzL3NpbmdsZV90ZXN0X3J1bm5lci5weSBiL1Rvb2xzL1Njcmlw dHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL2NvbnRyb2xsZXJzL3NpbmdsZV90ZXN0X3J1bm5lci5w eQppbmRleCA1MDI4YWU3ZTM2Y2RhNjcwYzk1MmZiZThiNzM0ZjIwYjcyZjFkZTM1Li4zZGVjYzE2 N2IzYjVhZTExYzg3YTExMWY4ZmVhMzU1NWQ3ZWRkZGE3IDEwMDY0NAotLS0gYS9Ub29scy9TY3Jp cHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9jb250cm9sbGVycy9zaW5nbGVfdGVzdF9ydW5uZXIu cHkKKysrIGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvY29udHJvbGxlcnMv c2luZ2xlX3Rlc3RfcnVubmVyLnB5CkBAIC0zMzEsMTIgKzMzMSwxMiBAQCBjbGFzcyBTaW5nbGVU ZXN0UnVubmVyKG9iamVjdCk6CiAgICAgICAgIGlmIG5vdCByZWZlcmVuY2VfZHJpdmVyX291dHB1 dC5pbWFnZV9oYXNoIGFuZCBub3QgYWN0dWFsX2RyaXZlcl9vdXRwdXQuaW1hZ2VfaGFzaDoKICAg ICAgICAgICAgIGZhaWx1cmVzLmFwcGVuZCh0ZXN0X2ZhaWx1cmVzLkZhaWx1cmVSZWZ0ZXN0Tm9J bWFnZXNHZW5lcmF0ZWQocmVmZXJlbmNlX2ZpbGVuYW1lKSkKICAgICAgICAgZWxpZiBtaXNtYXRj aDoKLSAgICAgICAgICAgIGlmIHJlZmVyZW5jZV9kcml2ZXJfb3V0cHV0LmltYWdlX2hhc2ggPT0g YWN0dWFsX2RyaXZlcl9vdXRwdXQuaW1hZ2VfaGFzaDoKKyAgICAgICAgICAgIGlmIHJlZmVyZW5j ZV9kcml2ZXJfb3V0cHV0LmltYWdlX2hhc2ggIT0gYWN0dWFsX2RyaXZlcl9vdXRwdXQuaW1hZ2Vf aGFzaDoKICAgICAgICAgICAgICAgICBkaWZmX3Jlc3VsdCA9IHNlbGYuX3BvcnQuZGlmZl9pbWFn ZShyZWZlcmVuY2VfZHJpdmVyX291dHB1dC5pbWFnZSwgYWN0dWFsX2RyaXZlcl9vdXRwdXQuaW1h Z2UsIHRvbGVyYW5jZT0wKQogICAgICAgICAgICAgICAgIGlmIG5vdCBkaWZmX3Jlc3VsdFswXToK ICAgICAgICAgICAgICAgICAgICAgZmFpbHVyZXMuYXBwZW5kKHRlc3RfZmFpbHVyZXMuRmFpbHVy ZVJlZnRlc3RNaXNtYXRjaERpZE5vdE9jY3VyKHJlZmVyZW5jZV9maWxlbmFtZSkpCi0gICAgICAg ICAgICAgICAgZWxzZToKLSAgICAgICAgICAgICAgICAgICAgX2xvZy53YXJuaW5nKCIgICVzIC0+ IHJlZiB0ZXN0IGhhc2hlcyBtYXRjaGVkIGJ1dCBkaWZmIGZhaWxlZCIgJSBzZWxmLl90ZXN0X25h bWUpCisgICAgICAgICAgICBlbHNlOgorICAgICAgICAgICAgICAgIGZhaWx1cmVzLmFwcGVuZCh0 ZXN0X2ZhaWx1cmVzLkZhaWx1cmVSZWZ0ZXN0TWlzbWF0Y2hEaWROb3RPY2N1cihyZWZlcmVuY2Vf ZmlsZW5hbWUpKQogCiAgICAgICAgIGVsaWYgcmVmZXJlbmNlX2RyaXZlcl9vdXRwdXQuaW1hZ2Vf aGFzaCAhPSBhY3R1YWxfZHJpdmVyX291dHB1dC5pbWFnZV9oYXNoOgogICAgICAgICAgICAgZGlm Zl9yZXN1bHQgPSBzZWxmLl9wb3J0LmRpZmZfaW1hZ2UocmVmZXJlbmNlX2RyaXZlcl9vdXRwdXQu aW1hZ2UsIGFjdHVhbF9kcml2ZXJfb3V0cHV0LmltYWdlLCB0b2xlcmFuY2U9MCkK