Bug 94782 – [Qt] Make ImageDiff similar to Chromium's ImageDiff

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 160095 [details]
WIP patch

[Allan Sandfeld Jensen]

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]

Created attachment 176739 [details]
Patch

A variant of the original idea, combining the measures.

[Jocelyn Turcotte]

Comment on attachment 176739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176739&action=review

> Tools/ChangeLog:1
> [phone]  Allan Sandfeld Jensen  <[e-mail]>

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]

(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]

Created attachment 176750 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 182102 [details]
Patch

rebased after imagediff moved

[Jocelyn Turcotte]

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]

(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]

Created attachment 193831 [details]
Patch

[Jocelyn Turcotte]

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 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]

Committed r146206: <http://trac.webkit.org/changeset/146206>