Bug 41779 – [GTK] Implement ImageDiff and add it to the build system

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The ImageDiff program is an essential component for pixel tests. It takes two PNG images via stdin and compares their pixels reporting the percentage difference and a PNG image showing the difference.

[Martin Robinson]

Created attachment 60764 [details]
ImageDiff implementation

[WebKit Review Bot]

Attachment 60764 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/ImageDiff.cpp:30:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

This is a stand-alone utility, therefore I think it shouldn't need config.h. Once it's in the tree I'll post a patch for this issue.

[Gustavo Noronha (kov)]

Comment on attachment 60764 [details]
ImageDiff implementation

 102     void* diffBuffer = malloc(width * height);
[...]
 110     unsigned char* diff = static_cast<unsigned char*>(diffBuffer);
[...]
143         *differenceImage = differenceImageFromDifferenceBuffer(static_cast<unsigned char*>(diffBuffer), width, height);

Given that you never use the diffBuffer as a void*, I would prefer having the cast in the malloc call, it makes it a bit easier on the reader of the code.

I find the mixed usage of g_print, fputs and fprintf unnecessary, though I do not oppose to it. I feel like you cal also make the ifs that are under the else ifs here be just one more condition of the else if:

 206             if (imageSize > 0 && !actualImage) {
 207                 if (!(actualImage = readPixbufFromStdin(imageSize))) {
 208                     fputs("Error, could not read actual image.\n", stdout);
 209                     return 1;
 210                 }
 211             } else if (imageSize > 0 && !baselineImage) {
 212                 if (!(baselineImage = readPixbufFromStdin(imageSize))) {
 213                     fputs("Error, could not read baseline image.\n", stdout);
 214                     return 1;
 215                 }

It will make it a bit easier to read, IMO. Other than the above comments, I think the code looks right, let's start getting some experience with how well pixel tests will work for us, I guess =)

[Martin Robinson]

(In reply to comment #4)

Thanks for the review!

> Given that you never use the diffBuffer as a void*, I would prefer having
> the cast in the malloc call, it makes it a bit easier on the reader of the
> code.

Okay, will fix.

> I find the mixed usage of g_print, fputs and fprintf unnecessary,
> though I do not oppose to it. I feel like you cal also make the ifs
> that are under the else ifs here be just one more condition of the else if:

Okay, I'll switch it to all (f)printf.

> 
>  206             if (imageSize > 0 && !actualImage) {
>  207                 if (!(actualImage = readPixbufFromStdin(imageSize))) {
>  208                     fputs("Error, could not read actual image.\n", stdout);
>  209                     return 1;
>  210                 }
>  211             } else if (imageSize > 0 && !baselineImage) {
>  212                 if (!(baselineImage = readPixbufFromStdin(imageSize))) {
>  213                     fputs("Error, could not read baseline image.\n", stdout);
>  214                     return 1;
>  215                 }

I think this has to stay. I want to actually bail out if the image fails
to load, but I don't want to bail out if the first condition fails.

[Martin Robinson]

Committed as http://trac.webkit.org/changeset/62817 . Closing bug.