Bug 56286 – layout test checksums should be embedded in the png as a comment

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Tony Chang]

In bug 55236, Mihai listed a bunch of layout tests where we have .checksums without .pngs and vice versa.  It occurred to me that we can just embed the .checksum value in the pngs as a comment and remove the .checksum files from the tree.

For example, when DRT generates the actual pngs, it could place the checksum in the png comment.  NRWT could be updated to look for a .checksum file and if one isn't found, open the png and read the checksum from it (an iTXt section can be near the beginning of the file: http://en.wikipedia.org/wiki/Portable_Network_Graphics#Ancillary_chunks ).

Pros:
- less files in the checkout (should make svn/git operations faster)
- harder for .checksums and .pngs to get out of sync

Cons:
- harder to view/manually edit the checksum, e.g. it won't be in the diff.  I'm not sure anyone ever actually does this.
- might make NRWT a tad bit slower since we have to check to see if .checksum exists first and we have to scan the png for the checksum, although I bet this will be negligible.

I think I could rolls this out gradually, starting with the chromium port to see if it works.

Thoughts?

[Mihai Parparita]

(In reply to comment #0)
> - harder to view/manually edit the checksum, e.g. it won't be in the diff.  I'm not sure anyone ever actually does this.

I've used this to quickly determine whether chromium-mac baselines should be moved to mac-leopard (i.e. if a test fails on Mac Leopard, I've run it with --reset-results and checked whether the new checksum was the same as the one that was in the Chromium directory). However, I think I could use ImageDiff for this too.

[Tony Chang]

(In reply to comment #1)
> (In reply to comment #0)
> > - harder to view/manually edit the checksum, e.g. it won't be in the diff.  I'm not sure anyone ever actually does this.
> 
> I've used this to quickly determine whether chromium-mac baselines should be moved to mac-leopard (i.e. if a test fails on Mac Leopard, I've run it with --reset-results and checked whether the new checksum was the same as the one that was in the Chromium directory). However, I think I could use ImageDiff for this too.

I could also write some tools to help with this.  E.g., it seems like PrettyPatch.rb could know how to pull the checksum out of the png or another script.

[Dirk Pranke]

It might be helpful to sketch out the longer-term plan for how we get rid of the .checksum files, e.g., what lands when, how do we update the rebaselining tools, will other ports be able to support this, how many checksum files are generic vs. port-specific, etc.

It seems like a good idea in the end-state but if only some PNGs will ever have checksums in them, it seems like this might make things more confusing rather than less, by virtue of the two code paths.

[Mihai Parparita]

I agree with Dirk. I've occasionally "promoted" chromium-mac baselines to mac (or mac-leopard) as part of getting the Chromium Snow Leopard bot green, it'll be more difficult to do this if the Mac port expects .checksum files (though this could be mitigated with tools to  re-generate the standalone .checksum file from a .png).

[Tony Chang]

Sure, I plan on sending an email to the gardeners mailing list once the rebaseline tool supports this (the patch has been reviewed but not yet landed).

Until then, this change and the change in bug 57280 doesn't actually impact current behavior so they should be harmless to land.

[Tony Chang]

(In reply to comment #5)
> Until then, this change and the change in bug 57280 doesn't actually impact current behavior so they should be harmless to land.

Err, the change in bug 57255 and bug 57280.

[Ojan Vafai]

I like the idea. Certainly, if we can get all ports to do it this way it'd be better. My concern is what we do in the interim where Chromium does this. I guess we check for the checksum first, so it just works?

Also, it would be nice for the layout test dashboard to be able to show the checksums for easily comparing the images from different ports. Although, I guess it would be better if the dashboard just let you diff any two images.

As for your pros/cons, I don't see any benefit in people manually editing checksums.

[Tony Chang]

(In reply to comment #7)
> My concern is what we do in the interim where Chromium does this. I guess we check for the checksum first, so it just works?

That's correct.  We first check to see if there's a checksum and only if one doesn't exist do we look at the png comment.

> Also, it would be nice for the layout test dashboard to be able to show the checksums for easily comparing the images from different ports. Although, I guess it would be better if the dashboard just let you diff any two images.

This should be possible to do in JS via XHR.  I think we need to add "Access-Control-Allow-Origin: *" to build.chromium.org/f/chromium/layout_test_results/ so we can fetch the resource, but otherwise this seems possible.  I'll talk to the bot admins to see what needs to be done to allow this.

[Tony Chang]

I declare victory on this bug.  There's still some obsolete code in NRWT that needs to be deleted and some .checksum files will probably creep in over the next few days, but that's cleanup I'll address in the next few work days.