Now Qt's ImageDiff generate a grayscale image based on distance of actual and expected pngs: https://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/qt/ImageDiff.cpp?rev=124324#L98 Zero distance is black, white-black distance is white on the diff image now. Unfortunately there are: - many failing tests on different distros (for example Ubuntu and Debian because of different libfreetype system package): minor font differences because of anti-aliasing - many failing tests on 32/64 bit systems: All pixels of a green rectangle are same green(128) on 64 bit, but the down margin line of this rectangle are lighter green(127) It causes minor pixel differences: (1,1,1) coloured line on the diff image, which is absolutely undetectable now. And in most cases NRWT reports this failures as "pixel hash failed (but diff passed)", because Qt's ImageDiff now ignores differences smaller than 0.01%. difference. 0.01% is 48 full pixel difference (black/white) now. (800x600 png) But a 800 pixel width (127/128, 0/0, 0/0) difference means only 1/256 * 800 = 3.125 px distance. That's why we get ~1800 "pixel hash failed (but diff passed)" failures on 32/64 bit builds. To detect and debug these failures, it would be great if - Qt's ImageDiff marked all different pixels as red (255,0,0) - same as Chromium's ImageDiff - Qt's ImageDiff marked same pixels darker as the original (r/2, g/2, b/2) - same as Chromium's ImageDiff - tolerance was based on the number of different pixels instead of the euclidean distance Proposed patch and examples are coming soon.
Created attachment 160095 [details] WIP patch
This sounds like the compare util from ImageMagics, see http://www.imagemagick.org/Usage/compare/ Can we possibly use a standard tool like that?
Created attachment 176739 [details] Patch A variant of the original idea, combining the measures.
Comment on attachment 176739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176739&action=review > Tools/ChangeLog:1 > +2012-11-29 Allan Sandfeld Jensen <allan.jensen@digia.com> I think you can give a bit of credit to Csaba for this one. I can rubber-stamp the patch if he wants to review it.
(In reply to comment #4) > I think you can give a bit of credit to Csaba for this one. > I can rubber-stamp the patch if he wants to review it. Unless the two patches are totally different, but from my quick look it seems like an iteration over the previous patch.
Created attachment 176750 [details] Patch
Created attachment 182102 [details] Patch rebased after imagediff moved
Comment on attachment 182102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182102&action=review > Tools/ImageDiff/qt/ImageDiff.cpp:41 > + if (args[i] == "-f" || args[i] == "--fuzz") fuzz itself doesn't tell much about what it does. A comment here would be nice to explain its effects, until we get a "--help" (if ever). > Tools/ImageDiff/qt/ImageDiff.cpp:115 > count++; It seems like count had to be increased each time sum is. At this point though I think that we could remove count completely, seeing the change in bug #66299. > Tools/ImageDiff/qt/ImageDiff.cpp:119 > sum += distance; The fuzz then only applies to the visual result, it doesn't affect the difference ratio. Is this intended? If yes a note somewhere clarifying this could help.
(In reply to comment #8) > (From update of attachment 182102 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182102&action=review > > > Tools/ImageDiff/qt/ImageDiff.cpp:41 > > + if (args[i] == "-f" || args[i] == "--fuzz") > > fuzz itself doesn't tell much about what it does. > A comment here would be nice to explain its effects, until we get a "--help" (if ever). > I will remove it for now. It is dead code until we actually use it. > > Tools/ImageDiff/qt/ImageDiff.cpp:115 > > count++; > > It seems like count had to be increased each time sum is. At this point though I think that we could remove count completely, seeing the change in bug #66299. > It is an error count. > > Tools/ImageDiff/qt/ImageDiff.cpp:119 > > sum += distance; > > The fuzz then only applies to the visual result, it doesn't affect the difference ratio. > Is this intended? If yes a note somewhere clarifying this could help. Yeah. This is part of the hybrid approach. Fuzz indicated when 0.5 was added on top of a pixel difference.
Created attachment 193831 [details] Patch
Comment on attachment 193831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193831&action=review > Tools/ImageDiff/qt/ImageDiff.cpp:35 > + qreal fuzz = 0; // Tolerated difference in color to indicate a pixel error. Rests of a previous patch? > Tools/ImageDiff/qt/ImageDiff.cpp:41 > - for (int i = 0; i < argc; ++i) > + for (int i = 0; i < argc; ++i) { > if (args[i] == "-t" || args[i] == "--tolerance") > tolerance = args[i + 1].toDouble(); > + } ditto > Tools/ImageDiff/qt/ImageDiff.cpp:109 > + if (distance > fuzz) { ditto
Comment on attachment 193831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193831&action=review >> Tools/ImageDiff/qt/ImageDiff.cpp:109 >> + if (distance > fuzz) { > > ditto r=me if you replace this with distance > 0 and remove the fuzz variable.
Committed r146206: <http://trac.webkit.org/changeset/146206>