Bug 20011 - Printing test results differ between machines, we should use ImageDiff instead
Summary: Printing test results differ between machines, we should use ImageDiff instead
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 37203
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-11 14:42 PDT by Eric Seidel (no email)
Modified: 2010-07-16 00:51 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (70.45 KB, patch)
2010-05-10 01:46 PDT, Shinichiro Hamaji
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-07-11 14:42:06 PDT
Printing test results differ between machines, we should use ImageDiff instead

The fancy new printing test support I added doesn't work. :(  Different machines spit out different pdf files, so we can't diff them nicely.

The PDF author and creation date differ
The PDF checksum also differs
and there is some small amount of binary difference.

We should send the resulting PDFs to ImageDiff instead.  For now, I've just marked the printing tests as Skipped on the Mac platform.
Comment 1 Shinichiro Hamaji 2010-03-18 02:38:34 PDT
Hi Eric, (of course, comments from other people are also welcomed! :)

Though I added two functions into layoutTestController so we can test some of paged media related stuff, we still think we eventually need a way to dump whole pages as a image. For example, even if we implement margin boxes, our current test harness provides no way to test it.

So, I'm thinking about the best way to provide a way to dump all pages. I'm not sure if dumping PDFs is the best as it would require some amount of efforts to make it portable among ports. I'm considering to create printToPNG, which would dump all pages into a PNG. With this approach, I think we can reuse fairly amount of existing code (I don't look at code closely yet, but PrintContext::spoolPage and PixelDumpSupport seem to do most tricks). One of good points of PDF would be that boundaries of pages are very clear. I think we can easily add some lines like <hr> into the PNG image as the signs of page boundaries, so this won't be a serious lowlight of PNG approach.

If this plan makes sense, I'd like to start implementing it. Any suggestions will be welcomed!
Comment 2 Eric Seidel (no email) 2010-03-24 22:41:21 PDT
printToPNG sounds like a good idea to me.  We could even do a printToMPNG so we could store all the frames in one file. :)
Comment 3 Dimitri Glazkov (Google) 2010-03-25 14:00:46 PDT
The problem with ImageDiff is that it's very port-specific. Is there a chance we could write a platform/port-independent ImageDiff that does all these things?
Comment 4 Dirk Pranke 2010-03-25 14:56:10 PDT
Eric, didn't you say at some point that you wanted to use some sort of ruby library to do cross-platform image diffing. There's probably an equivalent in Python that we could use, too.
Comment 5 Eric Seidel (no email) 2010-03-25 15:00:01 PDT
I may have said such.  I don't remember.  There is a "cross-platform" image library ImageMagick which has ruby bindings.  I expect it has python bindings.  I quote "cross-platform" because when we used ImageMagick at my startup we had lots of trouble between platforms, based on what options you had compiled it with, or its variously 10s of dependent libraries. :)
Comment 6 Dirk Pranke 2010-03-25 15:01:45 PDT
Ironically, simply decoding the bytes from a PNG should be incredibly portable. Maybe once we have gfx upstreamed we should just use that everywhere?

-- Dirk
Comment 7 Dirk Pranke 2010-03-25 18:28:28 PDT
Note that if we decide we can use the Python Imaging Library, producing a diff takes about 3 lines of code (although it may be slow):

http://www.tilander.org/aurora/2008/03/comparing-images.html

see http://athenageek.wordpress.com/2009/06/09/easy_install-pil-not-so-easy/ for the actual url to install PIL w/ easy_install.
Comment 8 Dirk Pranke 2010-03-25 18:28:54 PDT
of course, that may not produce the kind of diff we'd like to see.
Comment 9 Shinichiro Hamaji 2010-05-10 01:46:23 PDT
Created attachment 55528 [details]
Patch v1
Comment 10 WebKit Review Bot 2010-05-10 15:51:47 PDT
Attachment 55528 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2189118
Comment 11 Shinichiro Hamaji 2010-05-10 22:02:34 PDT
(In reply to comment #10)
> Attachment 55528 [details] did not build on win:
> Build output: http://webkit-commit-queue.appspot.com/results/2189118

Hmm, I couldn't understand why this is failing from this build log. Maybe a bot issue?
Comment 12 Adam Barth 2010-06-21 18:30:00 PDT
> Hmm, I couldn't understand why this is failing from this build log. Maybe a bot issue?

the win bot doesn't produce logs, sadly.
Comment 13 Adam Barth 2010-06-21 18:57:34 PDT
This patch is really cool.  Who should review it?
Comment 14 Shinichiro Hamaji 2010-06-21 21:35:41 PDT
(In reply to comment #13)
> This patch is really cool.  Who should review it?

Maybe Eric or Darin because they reviewed patch in Bug 37203?
Comment 15 Shinichiro Hamaji 2010-07-14 01:16:15 PDT
Hmm ping?
Comment 16 Darin Adler 2010-07-14 09:56:49 PDT
Comment on attachment 55528 [details]
Patch v1

Seems OK.

WebCore/page/PrintContext.cpp:244
 +              graphicsContext.drawLine(IntPoint(0, currentHeight),
 +                                       IntPoint(pageWidth, currentHeight));
Not our usual formatting. Put it all on one line, or indent by 4 spaces. Please don't indent to match the parenthesis on the line above.
Comment 17 Shinichiro Hamaji 2010-07-15 23:26:07 PDT
Committed r63521: <http://trac.webkit.org/changeset/63521>
Comment 18 Shinichiro Hamaji 2010-07-15 23:30:17 PDT
Committed r63522: <http://trac.webkit.org/changeset/63522>
Comment 19 Shinichiro Hamaji 2010-07-15 23:33:20 PDT
Committed r63523: <http://trac.webkit.org/changeset/63523>
Comment 20 Shinichiro Hamaji 2010-07-15 23:38:49 PDT
Committed r63525: <http://trac.webkit.org/changeset/63525>
Comment 25 Shinichiro Hamaji 2010-07-16 00:12:50 PDT
Committed r63526: <http://trac.webkit.org/changeset/63526>
Comment 26 WebKit Review Bot 2010-07-16 00:30:20 PDT
http://trac.webkit.org/changeset/63526 might have broken SnowLeopard Intel Release (Tests)
Comment 27 Shinichiro Hamaji 2010-07-16 00:36:21 PDT
Committed r63527: <http://trac.webkit.org/changeset/63527>
Comment 28 Shinichiro Hamaji 2010-07-16 00:51:07 PDT
Committed r63529: <http://trac.webkit.org/changeset/63529>