check-ref needs to comapre pixel data but not PNG metadata

[kmcallister]

Apparently our PNG encoding is not a deterministic function of just the pixel data:

$ shasum servo-reftest-000000-*
94642ca4161750023fa3c9639f9dba60e34e5a5d  servo-reftest-000000-left.png
4f2b7e8b975633e64016d1a6755a2f562f39f895  servo-reftest-000000-right.png
0137eb74467e781b512faf783a22568e5280eea5  servo-reftest-000000-left.ppm
0137eb74467e781b512faf783a22568e5280eea5  servo-reftest-000000-right.ppm

[metajack]

Yikes. I'll try to track this down.

[kmcallister]

We could just write out PPM instead of PNG. The format is trivial and it's still viewable in standard image programs (and easily convertible to PNG).

[metajack]

We could, but I assume it's a simple fix.

[eddyb]

Could check-ref load the two PNGs using rust-png and compare the the resulting Images?
If not, I would look at the contents of those file and see whether they contain some different information (like a timestamp).
If the IDAT chunks differ (e.g. two different implementations/encode settings), you really have to decode the PNGs or use something else.

[metajack]

We write these pngs ourselves, so we should be able to control the metadata that appears.

[SimonSapin]

We do have the pixel data before we write these PNG files, right? How about comparing that, and maybe only writing PNG files on failure, for troubleshooting?

[kmcallister]

I still think the right answer is to write PPM, which is just raw pixel data with a tiny header, but is also a standard format supported by lots of tools.

[SimonSapin]

PPM or PNG, I still think we should compare raw pixel data in memory before exporting them into any format :)

[metajack]

How would we compare it in memory? We run servo twice to generate two PNGs, then compare the SHAs. Are you proposing instead that we write a another program that loads two PNGs and then compares their pixels?

Or are you proposing we use a single invocation of Servo that will load two pages simultaneously and then compare textures or something?

The reason I push back on PPM is because it seems like this is a bug in our PNG stuff (or our pixel dumping) and we should fix that. Otherwise I don't much care what format we spit out.

[SimonSapin]

Sorry, I didn’t remember that we spawn a new Servo process for every test. (I was thinking of WeasyPrint that runs its whole test suite in one process.)

If we eventually want to do fuzzy matching like Gecko does we’ll need to compare pixel data anyway, but that can come later. But yes, if something seems buggy we should also fix that.

By the way, is there a reason to compare SHAs rather than comparing the files?

[metajack]

SHA seemed like a convenient way to compare the files.

[SimonSapin]

The reftest harness does not actually compute SHAs:
src/test/harness/reftest/reftest.rs#L137

cmp compares byte-by-byte, which is probably cheaper in CPU time than computing a cryptographically secure hash.

[metajack]

Hmm. Someone must have changed that. We shouldn't be shelling out to call cmp. We can probably just do that easily in Rust and save the fork.

[metajack]

@kmcallister You can use ImageMagick to compare the metadata and see if they are actually different:

identify -verbose foo.png

[kmcallister]

@metajack I changed it to use cmp after SHA1 was removed from the Rust std lib.

I don't think there's a performance concern here. servo -o foo.png acid1.html takes about 300ms and we do it twice. cmp takes 2ms. As for SHA1, it can handle hundreds of MB/s so that would be completely dominated by the time it takes to read the files. But I agree there's no need for hashing either.

This just isn't something we need to optimize so let's stick with the current solution which is also very little code.

[dhedlund]

This isn't an issue with metadata, but rather the rust-png library writing more data to the file than it should. Occasionally it writes out 1+MiB files instead of 16KiB ones, sometimes almost 5MiB. Converting or truncating those files to the same size as the smaller one results in an image that compares correctly.

[larsbergstrom]

Fixed by #1544