Bug 68039 – need command line util to print png checksums

• Component: Tools / Tests
• Status: ASSIGNED

[Dirk Pranke]

it would be nice if we had a command line utility to print/verify the PNG checksums.

[Dirk Pranke]

Created attachment 107250 [details]
Patch

[Tony Chang]

Comment on attachment 107250 [details]
Patch

Tools/scripts/read-checksum-from-png

[Dirk Pranke]

Created attachment 107256 [details]
revise to shell out to ImageDiff

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 107259 [details]
Kinda working patch

[Simon Fraser (smfr)]

Created attachment 107265 [details]
Patch

[Dirk Pranke]

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.

[Simon Fraser (smfr)]

Yeah, it's pretty slow to run on all PNG files (40 minutes and counting...)

[Dirk Pranke]

Created attachment 107276 [details]
clean up, add tests

[Dirk Pranke]

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?

[Simon Fraser (smfr)]

My patch was working for me on Mac. Of course I only changed Mac's ImageDiff, so this won't work on all platforms.

[Tony Chang]

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.

[Dirk Pranke]

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

[Eric Seidel (no email)]

I thought checksums were dead?  Did this get landed?

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 136991 [details]
Patch

[Dirk Pranke]

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?

[Ojan Vafai]

This approach seems fine to me.

[Dirk Pranke]

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

[Dirk Pranke]

Comment on attachment 136991 [details]
Patch

clearing r?

[Dirk Pranke]

Created attachment 140650 [details]
Patch

[Dirk Pranke]

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?

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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 :).

[Build Bot]

Comment on attachment 140650 [details]
Patch

Attachment 140650 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12649244

[Simon Fraser (smfr)]

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.

[Dirk Pranke]

Comment on attachment 140650 [details]
Patch

clearing r? on this ... i'll get back to it at some point with the cleanups

[Dirk Pranke]

resetting the owner in case someone else wants to take a look, as these bugs aren't on my immediate to-do list.