Bug 85299 – [GTK] Remove the GDK dependency from ImageDiff

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

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.

[Martin Robinson]

See also: https://bugs.webkit.org/show_bug.cgi?id=85292

[José Dapena Paz]

Created attachment 166893 [details]
Patch

[Dominik Röttsches (drott)]

Maybe we can completely share it between EFL and GTK then.

[Martin Robinson]

And perhaps even WinCairo.

[José Dapena Paz]

Created attachment 167082 [details]
Patch

[Martin Robinson]

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.

[José Dapena Paz]

Created attachment 184229 [details]
Patch

Removed glib dependency, now we depend only on Cairo

[Raphael Kubo da Costa (:rakuco)]

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.

[Gustavo Noronha (kov)]

Created attachment 242719 [details]
Patch

[Gustavo Noronha (kov)]

I updated the build system and addressed rakuco's comments.

[Carlos Garcia Campos]

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

[Gustavo Noronha (kov)]

Created attachment 242737 [details]
Patch

[Gustavo Noronha (kov)]

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

[Gustavo Noronha (kov)]

Is this something we are still interested in?

[Carlos Garcia Campos]

Created attachment 306514 [details]
New patch

[Build Bot]

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.

[Carlos Garcia Campos]

Created attachment 306519 [details]
New patch

[Build Bot]

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.

[Carlos Garcia Campos]

Created attachment 306520 [details]
New patch

[Build Bot]

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.

[Carlos Garcia Campos]

Committed r215179: <http://trac.webkit.org/changeset/215179>