Bug 151261 – [GTK] ImageDiff should normalize the diff image

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

(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

[Sergio Villar Senin]

We should definitely do that! There are a lot of 1-pixel diff test cases that fail from time to time.

[Carlos Garcia Campos]

Created attachment 265855 [details]
Patch

[Sergio Villar Senin]

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?

> Tools/ImageDiff/gtk/ImageDiff.cpp:85
> +                bufferPixel /= maxDistance;

What guarantees that maxDistance is not 0?

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Committed r192728: <http://trac.webkit.org/changeset/192728>