Bug 37790 – [DRT/Chromium] Import Chromium image_diff as ImageDiff

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kent Tamura]

[DRT/Chromium] Import Chromium image_diff as ImageDiff

[Kent Tamura]

Created attachment 53657 [details]
hoge

[WebKit Review Bot]

Attachment 53657 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/chromium/ImageDiff.cpp:39:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

(In reply to comment #2)
> Attachment 53657 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKitTools/DumpRenderTree/chromium/ImageDiff.cpp:39:  Found other header
> before a header this file implements. Should be: config.h, primary header,
> blank line, and then alphabetically sorted.  [build/include_order] [4]

This style error is ignorable because ImageDiff.cpp doesn't have the primary header.

[Tony Chang]

In https://bugs.webkit.org/show_bug.cgi?id=37645#c4 , Dirk suggested having a single ImageDiff.  Looking at this more closely, I'm not sure what that means :)

There's currently an ImageDiff CG and an ImageDiff QT which don't share any code.  I guess we have to add an ImageDiff for chromium, but I doubt there's any code to share.

A few questions:

- What is the difference between compareImages and untestedCompareImages?  Should we try to switch to untestedCompareImages so we can be more like the existing ImageDiffs?

- Also, should we add --tolerance as a command line flag like the others?

- I guess we have to include gfx for the pngencoder, I guess that's ok.  Can we avoid base/basictypes.h?

[Dirk Pranke]

(In reply to comment #4)
> In https://bugs.webkit.org/show_bug.cgi?id=37645#c4 , Dirk suggested having a
> single ImageDiff.  Looking at this more closely, I'm not sure what that means
> :)
> 
> There's currently an ImageDiff CG and an ImageDiff QT which don't share any
> code.  I guess we have to add an ImageDiff for chromium, but I doubt there's
> any code to share.
> 

The point was performing an image diff is really a function of the platform run-webkit-tests is executing on, rather than the port being tested. There's no reason to have two different binaries and code bases on a Mac (or Linux or Windows for that matter).

I think we should just get rid of --tolerance; doing pixel tests that aren't exact seems like a bad idea to me.

-- Dirk

[Tony Chang]

(In reply to comment #5)
> (In reply to comment #4)
> > There's currently an ImageDiff CG and an ImageDiff QT which don't share any
> > code.  I guess we have to add an ImageDiff for chromium, but I doubt there's
> > any code to share.
> 
> The point was performing an image diff is really a function of the platform
> run-webkit-tests is executing on, rather than the port being tested. There's no
> reason to have two different binaries and code bases on a Mac (or Linux or
> Windows for that matter).

We could use the existing ImageDiff CG on mac, since all the dependencies are the same, but on Windows and Linux we would still need our own ImageDiff.  I think adding another one is fine, although it's a little unfortunate that it'll be Chromium specific because it uses files from gfx and depends on libpng.

> I think we should just get rid of --tolerance; doing pixel tests that aren't
> exact seems like a bad idea to me.

That seems like a separate discussion.  Please file a new bug for it.  For now, we should probably try to match the existing behavior of ImageDiff.

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > There's currently an ImageDiff CG and an ImageDiff QT which don't share any
> > > code.  I guess we have to add an ImageDiff for chromium, but I doubt there's
> > > any code to share.
> > 
> > The point was performing an image diff is really a function of the platform
> > run-webkit-tests is executing on, rather than the port being tested. There's no
> > reason to have two different binaries and code bases on a Mac (or Linux or
> > Windows for that matter).
> 
> We could use the existing ImageDiff CG on mac, since all the dependencies are
> the same, but on Windows and Linux we would still need our own ImageDiff.

Of course, there are multiple WebKit ports on both Windows and Linux as well, so a similar point stands.

> think adding another one is fine, although it's a little unfortunate that it'll
> be Chromium specific because it uses files from gfx and depends on libpng.
> 
> > I think we should just get rid of --tolerance; doing pixel tests that aren't
> > exact seems like a bad idea to me.
> 
> That seems like a separate discussion.  Please file a new bug for it.  For now,
> we should probably try to match the existing behavior of ImageDiff.

I wouldn't want you to spend time implementing a feature that no one uses, though.

[Tony Chang]

(In reply to comment #7)
> (In reply to comment #6)
> > We could use the existing ImageDiff CG on mac, since all the dependencies are
> > the same, but on Windows and Linux we would still need our own ImageDiff.
> 
> Of course, there are multiple WebKit ports on both Windows and Linux as well,
> so a similar point stands.

I don't understand what you're suggesting we do.  There are 2 ImageDiffs checked in right now: CG and QT.  If we want to use the CG one for Windows chromium, we would have to grab all the WebKitSupport files used by WebKit/win (I don't think this is done by update-webkit --chromium).  If we want to use the QT one for Linux chromium, we'd have to install a bunch of QT dev libraries on linux (probably done manually via apt-get on the slaves and all developers).  Are you suggesting we go this route?

I'm suggesting we just check in our own ImageDiff (the one in this patch) to avoid these build dependencies.

[Kent Tamura]

(In reply to comment #4)
> - What is the difference between compareImages and untestedCompareImages? 
> Should we try to switch to untestedCompareImages so we can be more like the
> existing ImageDiffs?

Actually, I don't know.  ImageDiff.cpp is almost equivalent to Chromium image_diff.cc.
The code of untestedCompareImages() has been there since the initial commit.
It's not used for now.  We may remove it.

> - Also, should we add --tolerance as a command line flag like the others?

I don't think it should be handled in this bug.

> - I guess we have to include gfx for the pngencoder, I guess that's ok.  Can we
> avoid base/basictypes.h?

It seems we don't need basictypes.h on Mac.  I'll update the patch.

[Kent Tamura]

Created attachment 53765 [details]
Proposed patch (rev.2)

[WebKit Review Bot]

Attachment 53765 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/DumpRenderTree/chromium/ImageDiff.cpp:39:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > We could use the existing ImageDiff CG on mac, since all the dependencies are
> > > the same, but on Windows and Linux we would still need our own ImageDiff.
> > 
> > Of course, there are multiple WebKit ports on both Windows and Linux as well,
> > so a similar point stands.
> 
> I don't understand what you're suggesting we do.  There are 2 ImageDiffs
> checked in right now: CG and QT.  If we want to use the CG one for Windows
> chromium, we would have to grab all the WebKitSupport files used by WebKit/win
> (I don't think this is done by update-webkit --chromium).  If we want to use
> the QT one for Linux chromium, we'd have to install a bunch of QT dev libraries
> on linux (probably done manually via apt-get on the slaves and all developers).
>  Are you suggesting we go this route?

Not really. Mostly I was looking for a way for different ports to not have to port ImageDiff into their own framework when there's really no reason to do so. At one point I suggested that we could probably code this in Python using the Python Imaging Library and get a single portable implementation that way, but I'm not sure if that would really be any better.

If the ports aren't using common libraries like libpng then maybe there's nothing really to be gained. It just seems silly to have to duplicate them every time.

-- Dirk

[Dimitri Glazkov (Google)]

Comment on attachment 53765 [details]
Proposed patch (rev.2)

ok. Let's file a but about unifying ImageDiff across ports.

[Kent Tamura]

Comment on attachment 53765 [details]
Proposed patch (rev.2)

Clearing flags on attachment: 53765

Committed r57947: <http://trac.webkit.org/changeset/57947>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.