Bug 58486 – [WinCairo] Implement ImageDiff Logic

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 89481 [details]
Patch

[WebKit Review Bot]

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.

[Brent Fulgham]

(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.

[Martin Robinson]

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.

[Brent Fulgham]

Created attachment 89499 [details]
Patch

[WebKit Review Bot]

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.

[Martin Robinson]

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);

?

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 89623 [details]
Patch

[Brent Fulgham]

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.

[WebKit Review Bot]

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.

[Martin Robinson]

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;

[Brent Fulgham]

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

[Brent Fulgham]

Committed r83912: <http://trac.webkit.org/changeset/83912>