WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
37790
[DRT/Chromium] Import Chromium image_diff as ImageDiff
https://bugs.webkit.org/show_bug.cgi?id=37790
Summary
[DRT/Chromium] Import Chromium image_diff as ImageDiff
Kent Tamura
Reported
2010-04-19 00:50:54 PDT
[DRT/Chromium] Import Chromium image_diff as ImageDiff
Attachments
hoge
(16.98 KB, patch)
2010-04-19 00:54 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Proposed patch (rev.2)
(17.00 KB, patch)
2010-04-19 19:30 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Kent Tamura
Comment 1
2010-04-19 00:54:38 PDT
Created
attachment 53657
[details]
hoge
WebKit Review Bot
Comment 2
2010-04-19 00:57:33 PDT
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
Comment 3
2010-04-19 01:15:09 PDT
(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
Comment 4
2010-04-19 02:23:31 PDT
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
Comment 5
2010-04-19 15:21:33 PDT
(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
Comment 6
2010-04-19 16:48:41 PDT
(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
Comment 7
2010-04-19 18:45:22 PDT
(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
Comment 8
2010-04-19 19:07:42 PDT
(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
Comment 9
2010-04-19 19:14:22 PDT
(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
Comment 10
2010-04-19 19:30:48 PDT
Created
attachment 53765
[details]
Proposed patch (rev.2)
WebKit Review Bot
Comment 11
2010-04-19 19:34:59 PDT
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
Comment 12
2010-04-19 21:21:52 PDT
(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 13
2010-04-20 09:13:33 PDT
Comment on
attachment 53765
[details]
Proposed patch (rev.2) ok. Let's file a but about unifying ImageDiff across ports.
Kent Tamura
Comment 14
2010-04-20 20:32:00 PDT
Comment on
attachment 53765
[details]
Proposed patch (rev.2) Clearing flags on attachment: 53765 Committed
r57947
: <
http://trac.webkit.org/changeset/57947
>
Kent Tamura
Comment 15
2010-04-20 20:32:10 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug