Bug 85299 - [GTK] Remove the GDK dependency from ImageDiff
Summary: [GTK] Remove the GDK dependency from ImageDiff
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: José Dapena Paz
URL:
Keywords:
Depends on:
Blocks: 170608
  Show dependency treegraph
 
Reported: 2012-05-01 11:38 PDT by Martin Robinson
Modified: 2017-04-10 04:48 PDT (History)
11 users (show)

See Also:


Attachments
Patch (10.25 KB, patch)
2012-10-03 07:55 PDT, José Dapena Paz
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2012-10-04 05:42 PDT, José Dapena Paz
no flags Details | Formatted Diff | Diff
Patch (21.81 KB, patch)
2013-01-23 07:34 PST, José Dapena Paz
no flags Details | Formatted Diff | Diff
Patch (21.26 KB, patch)
2014-12-06 10:46 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (21.18 KB, patch)
2014-12-07 03:29 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
New patch (30.01 KB, patch)
2017-04-07 10:30 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
New patch (30.06 KB, patch)
2017-04-07 11:24 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
New patch (30.05 KB, patch)
2017-04-07 11:38 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2012-05-01 11:38:05 PDT
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.
Comment 1 Martin Robinson 2012-05-01 11:38:19 PDT
See also: https://bugs.webkit.org/show_bug.cgi?id=85292
Comment 2 José Dapena Paz 2012-10-03 07:55:04 PDT
Created attachment 166893 [details]
Patch
Comment 3 Dominik Röttsches (drott) 2012-10-03 09:59:52 PDT
Maybe we can completely share it between EFL and GTK then.
Comment 4 Martin Robinson 2012-10-03 10:08:34 PDT
And perhaps even WinCairo.
Comment 5 José Dapena Paz 2012-10-04 05:42:33 PDT
Created attachment 167082 [details]
Patch
Comment 6 Martin Robinson 2012-10-04 14:59:10 PDT
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.
Comment 7 José Dapena Paz 2013-01-23 07:34:18 PST
Created attachment 184229 [details]
Patch

Removed glib dependency, now we depend only on Cairo
Comment 8 Raphael Kubo da Costa (:rakuco) 2013-01-23 07:56:41 PST
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.
Comment 9 Gustavo Noronha (kov) 2014-12-06 10:46:42 PST
Created attachment 242719 [details]
Patch
Comment 10 Gustavo Noronha (kov) 2014-12-06 10:48:00 PST
I updated the build system and addressed rakuco's comments.
Comment 11 Carlos Garcia Campos 2014-12-07 01:28:47 PST
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
Comment 12 Gustavo Noronha (kov) 2014-12-07 03:29:17 PST
Created attachment 242737 [details]
Patch
Comment 13 Gustavo Noronha (kov) 2014-12-07 03:31:05 PST
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 ;)
Comment 14 Gustavo Noronha (kov) 2016-10-07 04:33:01 PDT
Is this something we are still interested in?
Comment 15 Carlos Garcia Campos 2017-04-07 10:30:39 PDT
Created attachment 306514 [details]
New patch
Comment 16 Build Bot 2017-04-07 10:32:30 PDT
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.
Comment 17 Carlos Garcia Campos 2017-04-07 11:24:23 PDT
Created attachment 306519 [details]
New patch
Comment 18 Build Bot 2017-04-07 11:26:14 PDT
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.
Comment 19 Carlos Garcia Campos 2017-04-07 11:38:39 PDT
Created attachment 306520 [details]
New patch
Comment 20 Build Bot 2017-04-07 11:41:16 PDT
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.
Comment 21 Carlos Garcia Campos 2017-04-10 04:48:32 PDT
Committed r215179: <http://trac.webkit.org/changeset/215179>