Bug 60569 – Add flags to Chromium ImageDiff to write image comparison metrics out to files.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tom Hudson]

Add flags to Chromium ImageDiff to write image comparison metrics out to files.

[Tom Hudson]

Created attachment 92994 [details]
Patch

[WebKit Review Bot]

Comment on attachment 92994 [details]
Patch

Attachment 92994 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8684083

[David Levin]

Comment on attachment 92994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92994&action=review

Doesn't build.

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:269
>  {

avoid abbreviations: "cp"

[Tom Hudson]

Created attachment 92996 [details]
Patch

[Tony Chang]

Comment on attachment 92996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92996&action=review

In general, this looks fine, mostly just style nits.

Out of curiosity, what is this for?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:199
> +#define MAX2(a, b) ((a) < (b) ? (b) : (a))
> +#define MIN2(a, b) ((a) < (b) ? (a) : (b))

Do we really need to use macros here?  What's wrong with std::min and std::max?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:203
> +#define MAX3(a, b, c) ((a) < (b) ? MAX2((b), (c)) : MAX2((a), (c)))
> +#define RED(a) ((a << 24) >> 24)
> +#define GREEN(a) ((a << 16) >> 24)
> +#define BLUE(a) ((a << 8) >> 24)

I would also just make these functions.  If you want you can even mark them as inline.

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:214
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++) {

Nit: WebKit style prefers ++y and ++x.

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:433
> +    fprintf(stderr, "ImageDiff::diffImages\n");

Did you mean to keep this line here?

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:442
> +        int fileTypeDotOffset = strrchr(outFile, '.') - outFile;
> +        strncpy(metadataOutFilename, outFile, fileTypeDotOffset);
> +        strcpy(metadataOutFilename + fileTypeDotOffset, "-meta.json");

Can we just use std::string here?  Something like:
std::string outFileString(outFile);
outFileString = outFileString.substring(0, outFileString.find('.') - 1) + "-meta.json";

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:443
> +        fprintf(stderr, "ImageDiff: metadata in %s\n", metadataOutFilename);

Do we need this logging statement?

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:156
> -                   actual_filename, diff_filename]
> +                   actual_filename, diff_filename,
> +                   '--writePercent', '--weighted']

Can we add a new test case rather than replacing an existing test case?

[Tom Hudson]

Created attachment 93760 [details]
Patch

[Tom Hudson]

After discussions with Tony, ImageDiff writes metric to stdout. Output will be processed by chromium.py (see bug 60957).

[Tony Chang]

Comment on attachment 93760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93760&action=review

This change LGTM.  Did you want this patch reviewed?  If so, please set r? and set cq? if you want someone to commit it for you.

> Tools/DumpRenderTree/chromium/ImageDiff.cpp:228
> +    for (int y = 0; y < h; y++) {
> +        for (int x = 0; x < w; x++) {

Nit: WebKit style prefers ++y and ++x

[Tom Hudson]

Tony, Mike had another feature request that was going to impact this, 60957, and 60964; I uploaded these so that Dirk could look at them for context on our discussion of 60957, but intended to do another round of revision today/tomorrow before I was ready for review, which is why I didn't specify r? on this.

Ugh, my careless regression on that Nit. Fixed locally.

[Tom Hudson]

Comment on attachment 93760 [details]
Patch

This patch is meant to work together with 60957 and and 60964, but does not depend on them.

[WebKit Review Bot]

Comment on attachment 93760 [details]
Patch

Rejecting attachment 93760 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 93760 [details]
Patch

Rejecting attachment 93760 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Tony Chang]

Comment on attachment 93760 [details]
Patch

cq- for the x++/++x nit.  If you upload a fixed version, I can cq+ it for you to get it landed.

[Tom Hudson]

Created attachment 94830 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 94830 [details]
Patch

Rejecting attachment 94830 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1

Last 500 characters of output:
WebCore.exp.in
	M	Source/WebCore/ChangeLog
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp
	M	Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h
	M	Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp
	M	Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h
r87335 = 3586f9721775466be55e056bd291dec1aea7b1be (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8734587

[Tony Chang]

This failed because the format of the reviewer line is incorrect.  I'll upload another copy with a fixed reviewer.

[Tony Chang]

Created attachment 94881 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 94881 [details]:

http/tests/websocket/tests/long-invalid-header.html bug 54131 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 94881 [details]
Patch for landing

Clearing flags on attachment: 94881

Committed r87367: <http://trac.webkit.org/changeset/87367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.