Make reference tests simpler to use#2802
Make reference tests simpler to use#2802RunDevelopment wants to merge 11 commits intoimage-rs:mainfrom
Conversation
|
Could we break this up into smaller pieces? There are parts that I like, but tons of different things are changing at once. For instance, this completely rewrites the reference images test and regresses the output to just be: |
The only parts to break off that I can think of are the bad images and non-image files. Did you mean that?
Well, yes. The reference image test works very differently now, and the previous implementation didn't have much that was reusable (because it was one huge function).
Please explain how this is a regression. While true that all images passing will be a single line, that's hardly a problem, no? What's most important is that errors are explained well to enable the developer to fix them. If any subset of images fail the test, a simple overview will be generated. For example, say the reference for |
|
Each image having its own test failure status was the main motivation for changing it from the version-0.25 variant in the first place. It's valuable to know which test is failing if any is failing, makes it easy to identify any particular feature they might have especially for regression tests and suites that are constructed to cover them. |
Don't you get that same information in the output that is generated by this PR? I don't understand what advantage making each file one test has. |
|
If the test passes then you don't get any output detailed output. That includes if the test passes because your image was skipped or because you placed it in the wrong directory and it didn't get detected in the first place. And there's also the aspect that you're reverting a feature that we just added a few weeks ago. |
|
Sure. Then let's go back to Could you please also answer how you want the PR to be split up? |
|
Regarding QOI: The BE bug that causes CI to fail has been fixed around a year ago, but they haven't made a new release in over 3 years. So that's fun. Suggestions on how to deal with this? Removing the one (and only) QOI test image seems like the easiest solution. |
e3d17e5 to
70783c8
Compare
|
CI finally passes 🎉 I fixed it by ignoring QOI specifically on BE arches in reference tests. Hacky, but it works. With that, this PR is technically ready to merge (after review of course), but I'd like to talk about 2 topics before:
|
|
I think including the hash of the uncompressed data in the filename is useful at minimum when testing PNG and TIFF (float) formats, because they are the reference formats for integer and float pixel types. It ensures that decoder bugs (like incorrectly byte-swapping 16 bit data on big endian systems) that affect both the image and its reference will be caught. I can't say if it's the best way to do it, but it works. (Arguably reference images aren't needed at all to detect issues, and the hash suffices; but they do help with debugging. Edit: I should also note the current hash only applies to the raw decoded bytes and can miss dimension or format interpretation issues.) Looking over the test images, I see:
|
IMO, it's fine for the reference test to assume that reference images are decoded correctly. Plus, say there was a byte-swapping (or similar) bug in PNG/TIFF. For that bug to not be caught by the reference test, it would also need to be present in all other image formats that use PNG/TIFF for reference images.
Frankly, As for better compression: I'm open to it if it's a simple change. But I'm not going to integrate an external command line tool. The whole point of this PR is that it should be easy to add test images and update references.
This will change when #2785 enables compression for TIFF by default. Case in point, I actually copied the compression settings from #2785 to generate the TIFF reference images for this PR. See that |
|
We shouldn't be generating the reference images using the same decoders we're testing. They should be produced using some other independent decoder, and then compressed via The only exceptions are JPEG and AVIF(?) where the bit-exact output isn't fully defined. In those cases we should still compare against an independent reference implementation, but the checked in contents should be produced by our decoders (after running them through |
Fixes #2796
I changed the reference image test runner to a bless-based workflow. I also got rid of CRC and the custom test harness.
Changes:
tests/imagesare now decoded and compared to their references intests/references.BLESS=1env var to automatically create and update references.This directly led me to discovering 2 issues:
hpredict_cmyk.tiff(temporarily lives intests/bad/tiff/TODO hpredict_cmyk.tiff). Other programs can open this.tests\reference\webp\extended_images\anim.webp.04.png. This is an animation of a ball bouncing around, but we decode each frame with all previous frames stacked below. My guess is that some buffer isn't cleared when it should be, and we just draw opaque pixels on top of the previous frame.