Bug 60569 - Add flags to Chromium ImageDiff to write image comparison metrics out to files.
Summary: Add flags to Chromium ImageDiff to write image comparison metrics out to files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-10 12:10 PDT by Tom Hudson
Modified: 2011-05-26 00:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.91 KB, patch)
2011-05-10 12:18 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch (11.67 KB, patch)
2011-05-10 12:59 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch (10.46 KB, patch)
2011-05-17 06:37 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch (11.46 KB, patch)
2011-05-25 12:04 PDT, Tom Hudson
no flags Details | Formatted Diff | Diff
Patch for landing (11.71 KB, patch)
2011-05-25 16:56 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Hudson 2011-05-10 12:10:34 PDT
Add flags to Chromium ImageDiff to write image comparison metrics out to files.
Comment 1 Tom Hudson 2011-05-10 12:18:53 PDT
Created attachment 92994 [details]
Patch
Comment 2 WebKit Review Bot 2011-05-10 12:21:41 PDT
Comment on attachment 92994 [details]
Patch

Attachment 92994 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8684083
Comment 3 David Levin 2011-05-10 12:49:07 PDT
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"
Comment 4 Tom Hudson 2011-05-10 12:59:45 PDT
Created attachment 92996 [details]
Patch
Comment 5 Tony Chang 2011-05-10 15:42:01 PDT
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?
Comment 6 Tom Hudson 2011-05-17 06:37:11 PDT
Created attachment 93760 [details]
Patch
Comment 7 Tom Hudson 2011-05-17 07:02:41 PDT
After discussions with Tony, ImageDiff writes metric to stdout. Output will be processed by chromium.py (see bug 60957).
Comment 8 Tony Chang 2011-05-17 10:08:43 PDT
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
Comment 9 Tom Hudson 2011-05-17 10:32:18 PDT
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.
Comment 10 Tom Hudson 2011-05-25 07:18:08 PDT
Comment on attachment 93760 [details]
Patch

This patch is meant to work together with 60957 and and 60964, but does not depend on them.
Comment 11 WebKit Review Bot 2011-05-25 07:18:42 PDT
Comment on attachment 93760 [details]
Patch

Rejecting attachment 93760 [details] from review queue.

tomhudson@google.com 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.
Comment 12 WebKit Review Bot 2011-05-25 07:19:22 PDT
Comment on attachment 93760 [details]
Patch

Rejecting attachment 93760 [details] from commit-queue.

tomhudson@google.com 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.
Comment 13 Tony Chang 2011-05-25 09:39:55 PDT
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.
Comment 14 Tom Hudson 2011-05-25 12:04:58 PDT
Created attachment 94830 [details]
Patch
Comment 15 WebKit Commit Bot 2011-05-25 16:48:18 PDT
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
Comment 16 Tony Chang 2011-05-25 16:54:18 PDT
This failed because the format of the reviewer line is incorrect.  I'll upload another copy with a fixed reviewer.
Comment 17 Tony Chang 2011-05-25 16:56:03 PDT
Created attachment 94881 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2011-05-26 00:43:47 PDT
The commit-queue encountered the following flaky tests while processing attachment 94881 [details]:

http/tests/websocket/tests/long-invalid-header.html bug 54131 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 19 WebKit Commit Bot 2011-05-26 00:44:57 PDT
Comment on attachment 94881 [details]
Patch for landing

Clearing flags on attachment: 94881

Committed r87367: <http://trac.webkit.org/changeset/87367>
Comment 20 WebKit Commit Bot 2011-05-26 00:45:04 PDT
All reviewed patches have been landed.  Closing bug.