Bug 232522 – ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images

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

[Simon Fraser (smfr)]

ImageDiff no longer needs a --tolerance argument, and fix sometimes-black diff images

[Simon Fraser (smfr)]

Created attachment 442897 [details]
Patch

[Simon Fraser (smfr)]

*** Bug 232467 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Comment on attachment 442897 [details]
Patch

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

Looks good. Just a few suggestions before landing.

> Tools/ImageDiff/ImageDiff.cpp:111
> +                "usage: ImageDiff [-h] [-v] [-d] [-e] [-t TOLERANCE] ([actualImage baselineImage] | <stdin>)\n" \

I think you should remove `[-t TOLERANCE]` from this line now as well.

> Tools/ImageDiff/ImageDiff.cpp:123
>                  "  -t, --tolerance TOLERANCE\n" \
>                  "                        compare the images with the given tolerance\n"

These two lines should be removed as well.

> Tools/ImageDiff/PlatformImage.cpp:101
> +            float pixel = static_cast<float>(diffPixel[p]);

Is it possible that this cast is unnecessary?

> Tools/ImageDiff/PlatformImage.cpp:105
> +            diffPixel[p] = static_cast<unsigned char>(pixel);

Ditto.

[Martin Robinson]

Ah, there is also the style error which seems pretty simple to fix as well.

[Simon Fraser (smfr)]

(In reply to Martin Robinson from comment #3)
> Comment on attachment 442897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442897&action=review
> 
> Looks good. Just a few suggestions before landing.
> 
> > Tools/ImageDiff/ImageDiff.cpp:111
> > +                "usage: ImageDiff [-h] [-v] [-d] [-e] [-t TOLERANCE] ([actualImage baselineImage] | <stdin>)\n" \
> 
> I think you should remove `[-t TOLERANCE]` from this line now as well.
> 
> > Tools/ImageDiff/ImageDiff.cpp:123
> >                  "  -t, --tolerance TOLERANCE\n" \
> >                  "                        compare the images with the given tolerance\n"
> 
> These two lines should be removed as well.

Thanks, I know I did that in some other incarnation of this patch!

> 
> > Tools/ImageDiff/PlatformImage.cpp:101
> > +            float pixel = static_cast<float>(diffPixel[p]);
> 
> Is it possible that this cast is unnecessary?
> 
> > Tools/ImageDiff/PlatformImage.cpp:105
> > +            diffPixel[p] = static_cast<unsigned char>(pixel);

Both casts are not necessary but I like having them because it makes them clear to the reader.

[Simon Fraser (smfr)]

Style warning is unavoidable; we don't link ImageDiff with WTF so can only use std:: types.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/285109/webkit

[Radar WebKit Bug Importer]

<rdar://problem/84884082>