151261 2015-11-13 07:07:21 -0800 [GTK] ImageDiff should normalize the diff image 2015-11-22 00:54:11 -0800 1 1 1 Unclassified WebKit Tools / Tests Other Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=85299 P2 Normal --- 1 clopez webkit-unassigned bugs-noreply cgarcia lforschler svillar zan oldest_to_newest 1141831 0 clopez 2015-11-13 07:07:21 -0800 I was checking the diff image for the following ref test that is failing by 1-pixel test: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20%28Tests%29/r192415%20%2812243%29/imported/blink/fast/canvas/canvas-clip-stack-persistence-diffs.html As you can check, on the diff image is hard to see what is different (everything looks black). So I tried to manually to create a diff image using imagemagick as follows: $ wget https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20%28Tests%29/r192415%20%2812243%29/imported/blink/fast/canvas/canvas-clip-stack-persistence-actual.png $ wget https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20%28Tests%29/r192415%20%2812243%29/imported/blink/fast/canvas/canvas-clip-stack-persistence-expected.png $ composite canvas-clip-stack-persistence-expected.png canvas-clip-stack-persistence-actual.png -compose difference diff.png The diff.png image generated was like the current we produce. Hard to tell if there is any difference or where. The I normalized the diff image as follows: $ convert diff.png -auto-level diff_norm.png Now the diff_norm.png shows clearly where the problem is. So I think we can teach ImageDiff to auto normalize the diff image. I see that each ports seems to have its own version of ImageDiff. I only tested the GTK one, and opening the bug for this one. A quick grep suggests that other ports are already normalizing the diff image: $ find Tools/ -type f -iname "imagediff*.*cpp"|xargs grep -i normalize 1141874 1 clopez 2015-11-13 10:42:37 -0800 (In reply to comment #0) > The I normalized the diff image as follows: > > $ convert diff.png -auto-level diff_norm.png > > Now the diff_norm.png shows clearly where the problem is. > This is the result: http://people.igalia.com/clopez/wkbug/151261/canvas-clip-stack-persistence-diff_normalized.png 1142495 2 svillar 2015-11-17 01:12:54 -0800 We should definitely do that! There are a lot of 1-pixel diff test cases that fail from time to time. 1143239 3 265855 cgarcia 2015-11-19 01:25:14 -0800 Created attachment 265855 Patch 1143617 4 265855 svillar 2015-11-20 03:20:01 -0800 Comment on attachment 265855 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265855&action=review Can't we immediately unskip a bunch of tests? > Tools/ImageDiff/gtk/ImageDiff.cpp:85 > + bufferPixel /= maxDistance; What guarantees that maxDistance is not 0? 1143618 5 cgarcia 2015-11-20 03:28:04 -0800 (In reply to comment #4) > Comment on attachment 265855 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265855&action=review > > Can't we immediately unskip a bunch of tests? No, this doesn't fix any test, this only makes it easier to see the differences when they are minimum, see the examples Carlos posted here, without the patch you see a diff that looks completely black, while after the patch you easily see the pixel that is different. > > Tools/ImageDiff/gtk/ImageDiff.cpp:85 > > + bufferPixel /= maxDistance; > > What guarantees that maxDistance is not 0? I think differenceImageFromDifferenceBuffer wouldn't be called in that case, I could ensure it just in case 1144011 6 cgarcia 2015-11-22 00:54:11 -0800 Committed r192728: <http://trac.webkit.org/changeset/192728> 265855 2015-11-19 01:25:14 -0800 2015-11-20 03:20:01 -0800 Patch wk-image-diff-normalize.diff text/plain 2691 cgarcia ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCBiZDA5 YmE2Li4zNTEyYTdiIDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTUtMTEtMTkgIENhcmxvcyBHYXJjaWEgQ2FtcG9z ICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgorCisgICAgICAgIFtHVEtdIEltYWdlRGlmZiBzaG91bGQg bm9ybWFsaXplIHRoZSBkaWZmIGltYWdlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNTEyNjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIEltYWdlRGlmZi9ndGsvSW1hZ2VEaWZmLmNwcDoKKyAgICAgICAg KHJlYWRQaXhidWZGcm9tU3RkaW4pOiBGaXggbWVtb3J5IGxlYWsuCisgICAgICAgIChkaWZmZXJl bmNlSW1hZ2VGcm9tRGlmZmVyZW5jZUJ1ZmZlcik6IE5vcm1hbGl6ZSBkaWZmIGJ1ZmZlci4KKyAg ICAgICAgKGNhbGN1bGF0ZURpZmZlcmVuY2UpOiBQYXNzIG1heCBkaXN0YW5jZSB0byBkaWZmZXJl bmNlSW1hZ2VGcm9tRGlmZmVyZW5jZUJ1ZmZlci4KKwogMjAxNS0xMS0xOSAgWW91ZW5uIEZhYmxl dCAgPHlvdWVubi5mYWJsZXRAY3JmLmNhbm9uLmZyPgogCiAgICAgICAgIFdQVCBzZXJ2ZXIgc2hv dWxkIHVzZSBpdHMgb3duIHRlc3RoYXJuZXNzLmpzIGZpbGUgYW5kIGdlbmVyYXRlIGEgd2Fybmlu ZyB3aGVuIGl0IGRvZXMgbm90IG1hdGNoIFdlYktpdCB2ZXJzaW9uCmRpZmYgLS1naXQgYS9Ub29s cy9JbWFnZURpZmYvZ3RrL0ltYWdlRGlmZi5jcHAgYi9Ub29scy9JbWFnZURpZmYvZ3RrL0ltYWdl RGlmZi5jcHAKaW5kZXggYzE1ZmNmZS4uNDZhM2ViMSAxMDA2NDQKLS0tIGEvVG9vbHMvSW1hZ2VE aWZmL2d0ay9JbWFnZURpZmYuY3BwCisrKyBiL1Rvb2xzL0ltYWdlRGlmZi9ndGsvSW1hZ2VEaWZm LmNwcApAQCAtNjQsMTIgKzY0LDEyIEBAIEdka1BpeGJ1ZiogcmVhZFBpeGJ1ZkZyb21TdGRpbihs b25nIGltYWdlU2l6ZSkKICAgICB9CiAKICAgICBnZGtfcGl4YnVmX2xvYWRlcl9jbG9zZShsb2Fk ZXIsIDApOwotICAgIEdka1BpeGJ1ZiogZGVjb2RlZEltYWdlID0gZ2RrX3BpeGJ1Zl9sb2FkZXJf Z2V0X3BpeGJ1Zihsb2FkZXIpOwotICAgIGdfb2JqZWN0X3JlZihkZWNvZGVkSW1hZ2UpOworICAg IEdka1BpeGJ1ZiogZGVjb2RlZEltYWdlID0gR0RLX1BJWEJVRihnX29iamVjdF9yZWYoZ2RrX3Bp eGJ1Zl9sb2FkZXJfZ2V0X3BpeGJ1Zihsb2FkZXIpKSk7CisgICAgZ19vYmplY3RfdW5yZWYobG9h ZGVyKTsKICAgICByZXR1cm4gZGVjb2RlZEltYWdlOwogfQogCi1HZGtQaXhidWYqIGRpZmZlcmVu Y2VJbWFnZUZyb21EaWZmZXJlbmNlQnVmZmVyKHVuc2lnbmVkIGNoYXIqIGJ1ZmZlciwgaW50IHdp ZHRoLCBpbnQgaGVpZ2h0KQorR2RrUGl4YnVmKiBkaWZmZXJlbmNlSW1hZ2VGcm9tRGlmZmVyZW5j ZUJ1ZmZlcih1bnNpZ25lZCBjaGFyKiBidWZmZXIsIGludCB3aWR0aCwgaW50IGhlaWdodCwgZmxv YXQgbWF4RGlzdGFuY2UpCiB7CiAgICAgR2RrUGl4YnVmKiBpbWFnZSA9IGdka19waXhidWZfbmV3 KEdES19DT0xPUlNQQUNFX1JHQiwgRkFMU0UsIDgsIHdpZHRoLCBoZWlnaHQpOwogICAgIGlmICgh aW1hZ2UpCkBAIC04MCw3ICs4MCwxMCBAQCBHZGtQaXhidWYqIGRpZmZlcmVuY2VJbWFnZUZyb21E aWZmZXJlbmNlQnVmZmVyKHVuc2lnbmVkIGNoYXIqIGJ1ZmZlciwgaW50IHdpZHRoLAogICAgIGZv ciAoaW50IHggPSAwOyB4IDwgd2lkdGg7IHgrKykgewogICAgICAgICBmb3IgKGludCB5ID0gMDsg eSA8IGhlaWdodDsgeSsrKSB7CiAgICAgICAgICAgICB1bnNpZ25lZCBjaGFyKiBkaWZmUGl4ZWwg PSBkaWZmUGl4ZWxzICsgKHkgKiByb3dTdHJpZGUpICsgKHggKiAzKTsKLSAgICAgICAgICAgIGRp ZmZQaXhlbFswXSA9IGRpZmZQaXhlbFsxXSA9IGRpZmZQaXhlbFsyXSA9ICpidWZmZXIrKzsKKyAg ICAgICAgICAgIHVuc2lnbmVkIGNoYXIgYnVmZmVyUGl4ZWwgPSAqYnVmZmVyKys7CisgICAgICAg ICAgICBpZiAobWF4RGlzdGFuY2UgPCAxKQorICAgICAgICAgICAgICAgIGJ1ZmZlclBpeGVsIC89 IG1heERpc3RhbmNlOworICAgICAgICAgICAgZGlmZlBpeGVsWzBdID0gZGlmZlBpeGVsWzFdID0g ZGlmZlBpeGVsWzJdID0gYnVmZmVyUGl4ZWw7CiAgICAgICAgIH0KICAgICB9CiAKQEAgLTE0MSw3 ICsxNDQsNyBAQCBmbG9hdCBjYWxjdWxhdGVEaWZmZXJlbmNlKEdka1BpeGJ1ZiogYmFzZWxpbmVJ bWFnZSwgR2RrUGl4YnVmKiBhY3R1YWxJbWFnZSwgR2RrUAogICAgIGVsc2UgewogICAgICAgICBk aWZmZXJlbmNlID0gcm91bmRmKGRpZmZlcmVuY2UgKiAxMDAuMGYpIC8gMTAwLjBmOwogICAgICAg ICBkaWZmZXJlbmNlID0gbWF4KGRpZmZlcmVuY2UsIDAuMDFmKTsgLy8gcm91bmQgdG8gMiBkZWNp bWFsIHBsYWNlcwotICAgICAgICAqZGlmZmVyZW5jZUltYWdlID0gZGlmZmVyZW5jZUltYWdlRnJv bURpZmZlcmVuY2VCdWZmZXIoZGlmZkJ1ZmZlciwgd2lkdGgsIGhlaWdodCk7CisgICAgICAgICpk aWZmZXJlbmNlSW1hZ2UgPSBkaWZmZXJlbmNlSW1hZ2VGcm9tRGlmZmVyZW5jZUJ1ZmZlcihkaWZm QnVmZmVyLCB3aWR0aCwgaGVpZ2h0LCBtYXhEaXN0YW5jZSk7CiAgICAgfQogCiAgICAgZnJlZShk aWZmQnVmZmVyKTsK