WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
94782
[Qt] Make ImageDiff similar to Chromium's ImageDiff
https://bugs.webkit.org/show_bug.cgi?id=94782
Summary
[Qt] Make ImageDiff similar to Chromium's ImageDiff
Csaba Osztrogonác
Reported
2012-08-22 23:42:06 PDT
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.
Attachments
WIP patch
(2.78 KB, patch)
2012-08-22 23:48 PDT
,
Csaba Osztrogonác
no flags
Details
Formatted Diff
Diff
Patch
(4.15 KB, patch)
2012-11-29 08:23 PST
,
Allan Sandfeld Jensen
no flags
Details
Formatted Diff
Diff
Patch
(4.16 KB, patch)
2012-11-29 09:40 PST
,
Allan Sandfeld Jensen
no flags
Details
Formatted Diff
Diff
Patch
(4.37 KB, patch)
2013-01-10 02:51 PST
,
Allan Sandfeld Jensen
no flags
Details
Formatted Diff
Diff
Patch
(4.13 KB, patch)
2013-03-19 08:00 PDT
,
Allan Sandfeld Jensen
jturcotte
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1
2012-08-22 23:48:15 PDT
Created
attachment 160095
[details]
WIP patch
Allan Sandfeld Jensen
Comment 2
2012-11-27 09:27:32 PST
This sounds like the compare util from ImageMagics, see
http://www.imagemagick.org/Usage/compare/
Can we possibly use a standard tool like that?
Allan Sandfeld Jensen
Comment 3
2012-11-29 08:23:38 PST
Created
attachment 176739
[details]
Patch A variant of the original idea, combining the measures.
Jocelyn Turcotte
Comment 4
2012-11-29 08:59:42 PST
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.
Jocelyn Turcotte
Comment 5
2012-11-29 09:00:29 PST
(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.
Allan Sandfeld Jensen
Comment 6
2012-11-29 09:40:49 PST
Created
attachment 176750
[details]
Patch
Allan Sandfeld Jensen
Comment 7
2013-01-10 02:51:50 PST
Created
attachment 182102
[details]
Patch rebased after imagediff moved
Jocelyn Turcotte
Comment 8
2013-03-19 04:58:40 PDT
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.
Allan Sandfeld Jensen
Comment 9
2013-03-19 07:35:23 PDT
(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.
Allan Sandfeld Jensen
Comment 10
2013-03-19 08:00:16 PDT
Created
attachment 193831
[details]
Patch
Jocelyn Turcotte
Comment 11
2013-03-19 08:12:24 PDT
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
Jocelyn Turcotte
Comment 12
2013-03-19 08:32:23 PDT
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.
Allan Sandfeld Jensen
Comment 13
2013-03-19 08:42:08 PDT
Committed
r146206
: <
http://trac.webkit.org/changeset/146206
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug