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.
Created attachment 60764 [details] ImageDiff implementation
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.
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 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 =)
(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.
Committed as http://trac.webkit.org/changeset/62817 . Closing bug.