ImageDiff should be rewritten so that it doesn't depend on GDK, only Cairo and GLib. This will allow it to run without an X11 server.
See also: https://bugs.webkit.org/show_bug.cgi?id=85292
Created attachment 166893 [details] Patch
Maybe we can completely share it between EFL and GTK then.
And perhaps even WinCairo.
Created attachment 167082 [details] Patch
Comment on attachment 167082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167082&action=review Okay. This seems like a good first step. The next one is to remove the GLib dependency and to start sharing this code with EFL and maybe even WinCairo. A few nits follow. > Tools/DumpRenderTree/gtk/ImageDiff.cpp:46 > +cairo_status_t cairoStdinReader(void *closure, unsigned char* data, unsigned length) Nit: The asterisk for closure are in the wrong place here. > Tools/DumpRenderTree/gtk/ImageDiff.cpp:48 > + int* pendingImageSize = reinterpret_cast<int*>(closure); I think this should be static_cast instead of reinterpret_cast. > Tools/DumpRenderTree/gtk/ImageDiff.cpp:49 > + if (*pendingImageSize > 0) { You should use an early return here: if (*pendingImageSize <= 0) return CAIRO_STATUS_SUCCESS; > Tools/DumpRenderTree/gtk/ImageDiff.cpp:50 > + size_t bytesToRead = min<int>(*pendingImageSize, length); Probably you should use min<size_t> since I think using min<int> will cast length to a signed integer. > Tools/DumpRenderTree/gtk/ImageDiff.cpp:166 > + GArray* array = reinterpret_cast<GArray*>(closure); You should be able to use static_cast here. > Tools/DumpRenderTree/gtk/ImageDiff.cpp:173 > + GArray* array = g_array_new(false, false, 1); Since g_array_new takes gboolean instead of bool, it's slightly more correct to write FALSE here.
Created attachment 184229 [details] Patch Removed glib dependency, now we depend only on Cairo
Comment on attachment 184229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184229&action=review This looks promising, it would be really cool to share an ImageDiff implementation across multiple ports. I have added a few nitpicks for things which were there before your change, so feel free to ignore them. One other thing I'd like to point out if we really move towards sharing ImageDiff is that EFL's calculateDifference() implementation more readable with the use of OwnPtrs and smaller helper functions. > Tools/ImageDiff/cairo/ImageDiff.cpp:137 > + *currentDiffPixel++ = (unsigned char)(distance * 255.0f); C-style cast. > Tools/ImageDiff/cairo/ImageDiff.cpp:181 > + fwrite(data, 1, dataLength, stdout); I find it easier to read if you swap the second and the third arguments: the second argument is `size' and the third one is `nmemb', not the other way around.
Created attachment 242719 [details] Patch
I updated the build system and addressed rakuco's comments.
Comment on attachment 242719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242719&action=review Looks good to me in general, I only have a few minor comments. > Tools/ImageDiff/cairo/ImageDiff.cpp:33 > +#include <cairo/cairo.h> #include <cairo.h> > Tools/ImageDiff/cairo/ImageDiff.cpp:50 > + int* pendingImageSize = static_cast<int*>(closure); The passed value is long, not int. > Tools/ImageDiff/cairo/ImageDiff.cpp:54 > + size_t bytesToRead = min<size_t>(*pendingImageSize, length); An here we cast to size_t, I think we should use the same type consistently. > Tools/ImageDiff/cairo/ImageDiff.cpp:65 > + return cairo_image_surface_create_from_png_stream(static_cast<cairo_read_func_t>(cairoStdinReader), &imageSize); I don't think you need the cast, since the function prototype matches cairo_read_funct_t. You could probably use a lambda here :-) > Tools/ImageDiff/cairo/ImageDiff.cpp:91 > + return 0; nullptr > Tools/ImageDiff/cairo/ImageDiff.cpp:112 > + unsigned numberOfChannels = formatChannels(format); You an move this after the if > Tools/ImageDiff/cairo/ImageDiff.cpp:145 > + maxDistance = max<float>(maxDistance, distance); Do we need the cast? both maxDistance and distance are floats > Tools/ImageDiff/cairo/ImageDiff.cpp:153 > + float difference = 0; I don't think this needs to be initialized. > Tools/ImageDiff/cairo/ImageDiff.cpp:178 > + if (cairo_surface_write_to_png_stream(image, static_cast<cairo_write_func_t>(writeToPixelData), &pixelData) != CAIRO_STATUS_SUCCESS) Same here about rhe function cast. We could probably use a lambda here. > Tools/ImageDiff/cairo/ImageDiff.cpp:181 > + const int dataLength = pixelData.size(); This should be size_t or unsigned. > Tools/ImageDiff/cairo/ImageDiff.cpp:189 > + cairo_surface_t* differenceImage = 0; nullptr. > Tools/ImageDiff/cairo/ImageDiff.cpp:221 > + cairo_surface_t* actualImage = 0; > + cairo_surface_t* baselineImage = 0; nullptr > Tools/ImageDiff/cairo/ImageDiff.cpp:231 > + long imageSize = strtol(strtok(0, " "), 0, 10); nullptr. We could cast this to size_t and use it consistently. > Tools/ImageDiff/cairo/ImageDiff.cpp:253 > + actualImage = 0; > + baselineImage = 0; nullptr
Created attachment 242737 [details] Patch
Comment on attachment 242737 [details] Patch I updated the patch with comments, but it turns out the original implementation does not really work - ImageDiff hangs, I may come back to this after the hackfest, unless jdapena beats me to it ;)
Is this something we are still interested in?
Created attachment 306514 [details] New patch
Attachment 306514 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cairo/PlatformImageCairo.cpp:70: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306519 [details] New patch
Attachment 306519 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cairo/PlatformImageCairo.cpp:70: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306520 [details] New patch
Attachment 306520 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cairo/PlatformImageCairo.cpp:70: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r215179: <http://trac.webkit.org/changeset/215179>