Bug 68039

Summary: need command line util to print png checksums
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: ASSIGNED ---    
Severity: Normal CC: abarth, eric, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 67880    
Attachments:
Description Flags
Patch
none
revise to shell out to ImageDiff
none
Kinda working patch
none
Patch
none
clean up, add tests
none
Patch
none
Patch none

Description Dirk Pranke 2011-09-13 16:17:19 PDT
it would be nice if we had a command line utility to print/verify the PNG checksums.
Comment 1 Dirk Pranke 2011-09-13 16:18:00 PDT
Created attachment 107250 [details]
Patch
Comment 2 Tony Chang 2011-09-13 16:19:15 PDT
Comment on attachment 107250 [details]
Patch

Tools/scripts/read-checksum-from-png
Comment 3 Dirk Pranke 2011-09-13 16:36:52 PDT
Created attachment 107256 [details]
revise to shell out to ImageDiff
Comment 4 Simon Fraser (smfr) 2011-09-13 16:38:20 PDT
Comment on attachment 107256 [details]
revise to shell out to ImageDiff

View in context: https://bugs.webkit.org/attachment.cgi?id=107256&action=review

> Tools/Scripts/read-checksum-from-png:45
> +        proc = port_obj.popen([image_diff_path, '--validate-checksum', '--tolerance', str(tolerance)],

I don't think you want to send --tolerance when validating.
Comment 5 Simon Fraser (smfr) 2011-09-13 16:53:02 PDT
Created attachment 107259 [details]
Kinda working patch
Comment 6 Simon Fraser (smfr) 2011-09-13 17:16:28 PDT
Created attachment 107265 [details]
Patch
Comment 7 Dirk Pranke 2011-09-13 17:38:53 PDT
Comment on attachment 107265 [details]
Patch

R-'ing just so I can clean it up a bit as per our discussion ... there should be some tests and it might be nice if we didn't have to launch imagediff for each PNG.
Comment 8 Simon Fraser (smfr) 2011-09-13 17:52:35 PDT
Yeah, it's pretty slow to run on all PNG files (40 minutes and counting...)
Comment 9 Dirk Pranke 2011-09-13 19:56:43 PDT
Created attachment 107276 [details]
clean up, add tests
Comment 10 Dirk Pranke 2011-09-13 19:57:59 PDT
Okay, I've uploaded a version with the python coded to my satisfaction. However, when I try to run things with the patched ImageDiff, it just hangs for me (ImageDiff hangs even if I feed in sample data on stdin by hand).

Simon, does this patch work for you?
Comment 11 Simon Fraser (smfr) 2011-09-13 21:16:57 PDT
My patch was working for me on Mac. Of course I only changed Mac's ImageDiff, so this won't work on all platforms.
Comment 12 Tony Chang 2011-09-14 09:59:26 PDT
Comment on attachment 107276 [details]
clean up, add tests

View in context: https://bugs.webkit.org/attachment.cgi?id=107276&action=review

> Tools/Scripts/webkitpy/layout_tests/read_checksum_from_png.py:76
> +def main(argv, stdout, port):
> +    parser = optparse.OptionParser()

Nit: It's a bit weird to have a main() function in a random .py file.  It might make more sense to push the command line parsing into read-checksum-from-png and put 2 functions here: one for validating and one for just reading the checksum.  It would make the unittests more clear too.  *shrug*

> Tools/Scripts/webkitpy/layout_tests/read_checksum_from_png.py:78
> +    parser.add_option('-v', '--validate', action='store_true',
> +                        help='recompute/validate checksums using ImageDiff')

Should we only add --validate for the mac port since it's the only one that supports this mode?

> Tools/Scripts/webkitpy/layout_tests/read_checksum_from_png.py:91
> +            stdout.write("%s: %s\n" % (filename, expected_checksum))

FWIW, the old output format put the checksum first because they line up nicely (unlike filenames).  It makes it easier to scan the output.
Comment 13 Dirk Pranke 2011-09-14 10:10:50 PDT
(In reply to comment #12)
> (From update of attachment 107276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107276&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/read_checksum_from_png.py:76
> > +def main(argv, stdout, port):
> > +    parser = optparse.OptionParser()
> 
> Nit: It's a bit weird to have a main() function in a random .py file.  It might make more sense to push the command line parsing into read-checksum-from-png and put 2 functions here: one for validating and one for just reading the checksum.  It would make the unittests more clear too.  *shrug*
> 

Yeah, I waffled on this a bit myself. NRWT pushes the main() into the .py module as well. Mostly I wanted to be able to get test coverage of the main routine, and you can't import the top-level script to test it. I could've written integration tests for it, I suppose, but we don't run those by default.

> > Tools/Scripts/webkitpy/layout_tests/read_checksum_from_png.py:78
> > +    parser.add_option('-v', '--validate', action='store_true',
> > +                        help='recompute/validate checksums using ImageDiff')
> 
> Should we only add --validate for the mac port since it's the only one that supports this mode?
>

Good point. I should probably update the help comment as well to indicate that this only works with an Apple Mac build (since even Chromium on Mac won't work).
 
> > Tools/Scripts/webkitpy/layout_tests/read_checksum_from_png.py:91
> > +            stdout.write("%s: %s\n" % (filename, expected_checksum))
> 
> FWIW, the old output format put the checksum first because they line up nicely (unlike filenames).  It makes it easier to scan the output.

Good idea. Will fix.
Comment 14 Eric Seidel (no email) 2011-10-12 14:27:54 PDT
I thought checksums were dead?  Did this get landed?
Comment 15 Dirk Pranke 2011-10-12 14:47:33 PDT
(In reply to comment #14)
> I thought checksums were dead?  Did this get landed?

The checksum still exist, they're just embedded directly into the PNGs. This has not been landed yet; it's still on my to-do plate.
Comment 16 Dirk Pranke 2012-04-12 16:21:36 PDT
Created attachment 136991 [details]
Patch
Comment 17 Dirk Pranke 2012-04-12 16:25:24 PDT
Okay, coming back to this at long last, I've re-cast this as a webkit-patch print-* command, and made this a pure-python solution (using a third party library called pypng, which is MIT-licensed). This is slow, but has the advantage that it is (a) dirt simple and (b) does not require N different ImageDiff routines to be updated and plumbed through the Port classes.

Theoretically we could even use this lib and get rid of ImageDiff altogether, although it's probably too slow for that. I have not benchmarked this code.

We could potentially also use the Python Imaging Library, which does have native implementations of these routines, but I figure if we go that route we should just modify the ImageDiffs.

If this route is preferable, I can write a couple of unit tests and land as-is; otherwise I can re-cast the original implementation into the webkit-patch command framework and land that instead.

WDYT?
Comment 18 Ojan Vafai 2012-04-12 16:31:38 PDT
This approach seems fine to me.
Comment 19 Dirk Pranke 2012-04-12 17:25:02 PDT
Sigh. Unfortunately, it appears that we get a bunch of MISMATCH reports on PNGs that I'm assuming don't actually mismatch. It is unclear whether it will be more effort to debug what's going on than to use the ImageDiff based approaches ...
Comment 20 Dirk Pranke 2012-05-04 14:37:54 PDT
Comment on attachment 136991 [details]
Patch

clearing r?
Comment 21 Dirk Pranke 2012-05-07 19:39:25 PDT
Created attachment 140650 [details]
Patch
Comment 22 Dirk Pranke 2012-05-07 19:40:49 PDT
Okay, I've abandoned the pure-python approach for now, and replaced the read-checksum-from-png script with a webkit-patch print-checksums command (and added unit tests).

Can someone please take a look?
Comment 23 Eric Seidel (no email) 2012-05-07 19:54:37 PDT
Comment on attachment 140650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140650&action=review

> Tools/DumpRenderTree/cg/ImageDiffCG.cpp:174
> +// FIXME: duplicated from PixelDumpSupportCG, but that has dependencies that ImageDiff doesn't want.

Could you be more explicit?  This seems like a huge amount of code to copy/paste.
Comment 24 Dirk Pranke 2012-05-07 19:58:32 PDT
(In reply to comment #23)
> (From update of attachment 140650 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140650&action=review
> 
> > Tools/DumpRenderTree/cg/ImageDiffCG.cpp:174
> > +// FIXME: duplicated from PixelDumpSupportCG, but that has dependencies that ImageDiff doesn't want.
> 
> Could you be more explicit?  This seems like a huge amount of code to copy/paste.

That's up to smfr, I didn't do that part :).
Comment 25 Build Bot 2012-05-07 20:00:21 PDT
Comment on attachment 140650 [details]
Patch

Attachment 140650 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12649244
Comment 26 Simon Fraser (smfr) 2012-05-11 16:32:37 PDT
Comment on attachment 140650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140650&action=review

>>> Tools/DumpRenderTree/cg/ImageDiffCG.cpp:174
>>> +// FIXME: duplicated from PixelDumpSupportCG, but that has dependencies that ImageDiff doesn't want.
>> 
>> Could you be more explicit?  This seems like a huge amount of code to copy/paste.
> 
> That's up to smfr, I didn't do that part :).

I can't recall what dependencies I was talking about (if indeed I wrote that comment).

But it seems that we could call computeMD5HashStringForBitmapContext() here and share code.
Comment 27 Dirk Pranke 2012-06-28 19:51:31 PDT
Comment on attachment 140650 [details]
Patch

clearing r? on this ... i'll get back to it at some point with the cleanups
Comment 28 Dirk Pranke 2012-07-13 19:30:19 PDT
resetting the owner in case someone else wants to take a look, as these bugs aren't on my immediate to-do list.