Bug 20011

Summary:

Printing test results differ between machines, we should use ImageDiff instead

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cjerdonek, darin, dglazkov, dpranke, eric, hamaji, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Bug Depends on:

37203

Bug Blocks:

Attachments:

Description	Flags
Patch v1	darin: review+

Eric Seidel (no email)

Reported 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.

Attachments
Patch v1 (70.45 KB, patch) 2010-05-10 01:46 PDT, Shinichiro Hamaji	darin: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Shinichiro Hamaji

Comment 1 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!

Eric Seidel (no email)

Comment 2 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. :)

Dimitri Glazkov (Google)

Comment 3 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?

Dirk Pranke

Comment 4 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.

Eric Seidel (no email)

Comment 5 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. :)

Dirk Pranke

Comment 6 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

Dirk Pranke

Comment 7 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.

Dirk Pranke

Comment 8 2010-03-25 18:28:54 PDT

of course, that may not produce the kind of diff we'd like to see.

Shinichiro Hamaji

Comment 9 2010-05-10 01:46:23 PDT

Created attachment 55528 [details] Patch v1

WebKit Review Bot

Comment 10 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

Shinichiro Hamaji

Comment 11 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?

Adam Barth

Comment 12 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.

Adam Barth

Comment 13 2010-06-21 18:57:34 PDT

This patch is really cool. Who should review it?

Shinichiro Hamaji

Comment 14 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?

Shinichiro Hamaji

Comment 15 2010-07-14 01:16:15 PDT

Hmm ping?

Darin Adler

Comment 16 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.

Shinichiro Hamaji

Comment 17 2010-07-15 23:26:07 PDT

Committed r63521: <http://trac.webkit.org/changeset/63521>

Shinichiro Hamaji

Comment 18 2010-07-15 23:30:17 PDT

Committed r63522: <http://trac.webkit.org/changeset/63522>

Shinichiro Hamaji

Comment 19 2010-07-15 23:33:20 PDT

Committed r63523: <http://trac.webkit.org/changeset/63523>

Shinichiro Hamaji

Comment 20 2010-07-15 23:38:49 PDT

Committed r63525: <http://trac.webkit.org/changeset/63525>