The attached patch implements a WinCairo-compatible version of ImageDiff, as well as adjusting old-run-webkit-tests to work with the WinCairo project names.
Created attachment 89481 [details] Patch
Attachment 89481 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/c..." exit_code: 1 Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:34: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:43: Alphabetical sorting problem. [build/include_order] [4] Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:46: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 89481 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/c..." exit_code: 1 > > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:34: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:43: Alphabetical sorting problem. [build/include_order] [4] > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:46: Alphabetical sorting problem. [build/include_order] [4] > Total errors found: 3 in 7 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. 1. This file can't include "config.h", since its part of "Tools", not WebCore. 2. The weird header order is necessary because of Windows issues with winsock2.h being included after windows.h.
Comment on attachment 89481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89481&action=review In general this change looks pretty good to me, though it originates from before a lot of the current WebKit style guidelines. Across all code, I'd like to see: 1. All C++ style casts. 2. Remove 'f' after floating point numbers when it's not necessary (when the .0 is enough). It's a pity we don't share more of this with the GTK port, but perhaps another patch can remove the GTK version in favor of this one. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:47 > +#include <wtf/MathExtras.h> > + > +#include <fcntl.h> > +#include <io.h> > +#include <winsock2.h> > +#include <windows.h> > +#endif If possible please fix these style errors and if not leave a comment documenting why certain headers must be in an unusual order. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:60 > + CFMutableDataRef cfDataObj = (CFMutableDataRef)closure; Please use a C++ style cast here and complete words for variable names. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:79 > + return cairo_image_surface_create_from_png_stream ((cairo_read_func_t)readFromData, data.get()); Please use C++ style cast here as well. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:113 > + // Draw base image in bitmap context > + void* baseBuffer = calloc(height, rowBytes); > + cairo_surface_t* baseImageStore = cairo_image_surface_create_for_data((unsigned char*)baseBuffer, CAIRO_FORMAT_ARGB32, width, height, width * 4); > + cairo_t* baseContext = cairo_create(baseImageStore); > + cairo_surface_destroy(baseImageStore); > + > + cairo_set_source_surface(baseContext, baseImage, 0, 0); > + cairo_paint(baseContext); > + cairo_destroy(baseContext); > + > + // Draw test image in bitmap context > + void* buffer = calloc(height, rowBytes); > + cairo_surface_t* testImageStore = cairo_image_surface_create_for_data((unsigned char*)buffer, CAIRO_FORMAT_ARGB32, width, height, width * 4); > + cairo_t* testContext = cairo_create(testImageStore); > + cairo_surface_destroy(testImageStore); > + > + cairo_set_source_surface(testContext, testImage, 0, 0); > + cairo_paint(testContext); > + cairo_destroy(testContext); Why redraw the images again here? Can't you just do cairo_imag_surface_get_data on baseImage and testImage? > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:178 > + cairo_format_t info = cairo_image_surface_get_format(image); > + > + return (info == CAIRO_FORMAT_ARGB32); this can just be: return (cairo_image_surface_get_format(image) == CAIRO_FORMAT_ARGB32); > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:183 > + CFMutableDataRef cfDataObj = (CFMutableDataRef)closure; Ditto. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:214 > + // remove the CR The comment here should be a complete sentence. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:235 > + if ((cairo_image_surface_get_width(actualImage) == cairo_image_surface_get_width(baselineImage)) && (cairo_image_surface_get_height(actualImage) == cairo_image_surface_get_height(baselineImage)) && (imageHasAlpha(actualImage) == imageHasAlpha(baselineImage))) { Please split this line since it's longer than the customary 120 characters. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:241 > + difference = 0.0f; > + else { > + difference = roundf(difference * 100.0f) / 100.0f; > + difference = max(difference, 0.01f); // round to 2 decimal places Style guidelines suggest that you should avoid using the 'f' after floating point numbers. > Tools/DumpRenderTree/cairo/ImageDiffCairo.cpp:246 > + if (difference > 0.0f) { Ditto.
Created attachment 89499 [details] Patch
Attachment 89499 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/w..." exit_code: 1 Tools/DumpRenderTree/win/ImageDiffCairo.cpp:32: 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 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 89499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89499&action=review Looks good, but the difference calculation still looks suspicious to me. :) > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:43 > +#include <windows.h> > + > +#include <wtf/MathExtras.h> The newline isn't necessary here. > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:55 > +static inline float strtof(const char *nptr, char **endptr) Please use full variable names for these arguments and move the asterisks next to the type name. > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:128 > + // Draw base image in bitmap context > + void* baseBuffer = calloc(height, rowBytes); > + unsigned char* basePixel = reinterpret_cast<unsigned char*>(baseBuffer); > + cairo_surface_t* baseImageStore = cairo_image_surface_create_for_data(basePixel, CAIRO_FORMAT_ARGB32, width, height, width * 4); > + cairo_t* baseContext = cairo_create(baseImageStore); > + cairo_surface_destroy(baseImageStore); > + > + cairo_set_source_surface(baseContext, baseImage, 0, 0); > + cairo_paint(baseContext); > + cairo_destroy(baseContext); > + > + // Draw test image in bitmap context > + void* buffer = calloc(height, rowBytes); > + unsigned char* pixel = reinterpret_cast<unsigned char*>(buffer); > + cairo_surface_t* testImageStore = cairo_image_surface_create_for_data(pixel, CAIRO_FORMAT_ARGB32, width, height, width * 4); > + cairo_t* testContext = cairo_create(testImageStore); > + cairo_surface_destroy(testImageStore); I'm still not sure why this is necessary. Why not just do: unsigned char* basePixel = cairo_image_surface_get_data(baseImage); unsigned char* pixel = cairo_image_surface_get_data(testImage); ?
Comment on attachment 89499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89499&action=review >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:43 >> +#include <wtf/MathExtras.h> > > The newline isn't necessary here. Will fix. >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:55 >> +static inline float strtof(const char *nptr, char **endptr) > > Please use full variable names for these arguments and move the asterisks next to the type name. Will do. >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:128 >> + cairo_surface_destroy(testImageStore); > > I'm still not sure why this is necessary. Why not just do: > > unsigned char* basePixel = cairo_image_surface_get_data(baseImage); > unsigned char* pixel = cairo_image_surface_get_data(testImage); > > ? I'm not sure either. The image surfaces entering this method are PNG surfaces. I'm not sure if there is anything different about the way Cairo deals with the images when it turns them into "device independent" image surfaces. Probably not. I'll try to build a test to see.
Created attachment 89623 [details] Patch
Comment on attachment 89499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89499&action=review >>> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:128 >>> + cairo_surface_destroy(testImageStore); >> >> I'm still not sure why this is necessary. Why not just do: >> >> unsigned char* basePixel = cairo_image_surface_get_data(baseImage); >> unsigned char* pixel = cairo_image_surface_get_data(testImage); >> >> ? > > I'm not sure either. The image surfaces entering this method are PNG surfaces. I'm not sure if there is anything different about the way Cairo deals with the images when it turns them into "device independent" image surfaces. Probably not. I'll try to build a test to see. Okay -- I memcmp'd the two buffers, and they are identical. I'm not sure why the CoreGraphics implementation needed to draw to the context before diffing, but maybe there are color space or other considerations I'm not thinking of. At any rate, I've changed to the "cairo_image_surface_get_data" implementation.
Attachment 89623 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/w..." exit_code: 1 Tools/DumpRenderTree/win/ImageDiffCairo.cpp:32: 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 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 89623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89623&action=review Looks good. Please consider my suggestions before landing. > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:58 > +static inline float strtof(const char *nptr, char **endptr) > +{ > + return strtod(nptr, endptr); > +} > +#endif Looks like the fixes here were lost? Please fix before landing. :) > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:91 > +static cairo_user_data_key_t imageDataKey; Should give this the same naming convention as the statics above s_imageDataKey. > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:113 > + unsigned char* basePixel = cairo_image_surface_get_data(baseImage); > + unsigned char* pixel = cairo_image_surface_get_data(testImage); I think something like testPixel would be clearer here. Or even actualPixels and expectedPixels. :) > Tools/DumpRenderTree/win/ImageDiffCairo.cpp:161 > + if (difference > 0) { > + normalizeBuffer(maxDistance, reinterpret_cast<unsigned char*>(diffBuffer), height * width); > + > + diffImage = cairo_image_surface_create_for_data(diffPixel, CAIRO_FORMAT_ARGB32, width, height, width * s_bytesPerPixel); > + cairo_surface_set_user_data(diffImage, &imageDataKey, diffBuffer, releaseMallocBuffer); > + } else > + free(diffBuffer); > + > + return diffImage; I think an early return would be clearer here: if (difference <= 0) { free(diffBuffer); return 0; } normalizeBuffer(maxDistance, reinterpret_cast<unsigned char*>(diffBuffer), height * width); cairo_surface_t* diffImage = cairo_image_surface_create_for_data(diffPixel, CAIRO_FORMAT_ARGB32, width, height, width * s_bytesPerPixel); cairo_surface_set_user_data(diffImage, &imageDataKey, diffBuffer, releaseMallocBuffer); return diffImage;
Comment on attachment 89623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89623&action=review >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:58 >> +#endif > > Looks like the fixes here were lost? Please fix before landing. :) Whoops! Fixed. >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:91 >> +static cairo_user_data_key_t imageDataKey; > > Should give this the same naming convention as the statics above s_imageDataKey. Done. >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:113 >> + unsigned char* pixel = cairo_image_surface_get_data(testImage); > > I think something like testPixel would be clearer here. Or even actualPixels and expectedPixels. :) Agreed. I switched to baselinePixels and actualPixels, to match the image names in the main loop. >> Tools/DumpRenderTree/win/ImageDiffCairo.cpp:161 >> + return diffImage; > > I think an early return would be clearer here: > > if (difference <= 0) { > free(diffBuffer); > return 0; > } > > normalizeBuffer(maxDistance, reinterpret_cast<unsigned char*>(diffBuffer), height * width); > cairo_surface_t* diffImage = cairo_image_surface_create_for_data(diffPixel, CAIRO_FORMAT_ARGB32, width, height, width * s_bytesPerPixel); > cairo_surface_set_user_data(diffImage, &imageDataKey, diffBuffer, releaseMallocBuffer); > return diffImage; Done! > Tools/Scripts/old-run-webkit-tests:-129 > -my @additionalPlatformDirectories = (); Note: These changes were not mine. I must have a conflict in the diff. I'll make sure none of these changes are part of the commit. > Tools/Scripts/old-run-webkit-tests:-281 > - Look in the specified directory before looking in any of the default platform-specific directories Ditto. > Tools/Scripts/old-run-webkit-tests:-330 > - 'additional-platform-directory=s' => \@additionalPlatformDirectories, Ditto > Tools/Scripts/old-run-webkit-tests:-2179 > - Ditto
Committed r83912: <http://trac.webkit.org/changeset/83912>