Bug 41779 - [GTK] Implement ImageDiff and add it to the build system
Summary: [GTK] Implement ImageDiff and add it to the build system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 31518
  Show dependency treegraph
 
Reported: 2010-07-07 11:13 PDT by Martin Robinson
Modified: 2010-07-08 12:32 PDT (History)
1 user (show)

See Also:


Attachments
ImageDiff implementation (11.76 KB, patch)
2010-07-07 12:31 PDT, Martin Robinson
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-07-07 11:13:12 PDT
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.
Comment 1 Martin Robinson 2010-07-07 12:31:30 PDT
Created attachment 60764 [details]
ImageDiff implementation
Comment 2 WebKit Review Bot 2010-07-07 12:34:35 PDT
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.
Comment 3 Martin Robinson 2010-07-08 08:35:24 PDT
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.
Comment 4 Gustavo Noronha (kov) 2010-07-08 10:49:07 PDT
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 =)
Comment 5 Martin Robinson 2010-07-08 11:24:09 PDT
(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.
Comment 6 Martin Robinson 2010-07-08 12:32:04 PDT
Committed as http://trac.webkit.org/changeset/62817 . Closing bug.