Bug 41779

Summary: [GTK] Implement ImageDiff and add it to the build system
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 31518    
Attachments:
Description Flags
ImageDiff implementation gustavo: review+

Martin Robinson
Reported 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.
Attachments
ImageDiff implementation (11.76 KB, patch)
2010-07-07 12:31 PDT, Martin Robinson
gustavo: review+
Martin Robinson
Comment 1 2010-07-07 12:31:30 PDT
Created attachment 60764 [details] ImageDiff implementation
WebKit Review Bot
Comment 2 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.
Martin Robinson
Comment 3 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.
Gustavo Noronha (kov)
Comment 4 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 =)
Martin Robinson
Comment 5 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.
Martin Robinson
Comment 6 2010-07-08 12:32:04 PDT
Committed as http://trac.webkit.org/changeset/62817 . Closing bug.
Note You need to log in before you can comment on or make changes to this bug.