Make reference tests simpler to use#2802
Make reference tests simpler to use#2802RunDevelopment wants to merge 15 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. |
RunDevelopment
force-pushed
the
new-ref-tests
branch
from
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 |
Is this a "they should ideally be produced by an independent decoder" or is this a "they must be produced by an independent decoder"? If you meant the latter (=a hard requirement), then I seriously question why you even reviewed this PR and asked me to make changes to begin with. A bless-based workflow necessarily means that our decoders' output is used as reference. If you meant the former, then why even bring it up? Judging by @mstoeckl's comment in #2796 and my own experience, people already copied what was in
I doubt many contributors ever did that. I didn't. Up until the comment by @mstoeckl, I haven't even heard of this tool. If this is supposed to be a requirement, then document it. I also want to say something more general: If you want tests to be written, then you have to make it easy to add them. If you have a million requirements for adding a single image to test, then your software simply isn't going to be tested well. |
|
Sorry for the confusion. The reference tests (like many areas in image-rs) exists in the current form mostly out of inertia. Someone designed a part of the code 5 or 10 years ago and it hasn't gotten very much attention since. Honestly, at this point I don't really have a fully thought out opinion on what reference tests should look like. So far, using On the topic of "do we want people to write more tests", IMO there's actually some nuance:
|
|
I see. While I don't fully agree with the hypothetical in your last point, everything else makes sense to me. To address the git size issues, we could not commit every reference image. In (Since reference hashes are generated directly from the decoded image data, this also address concerns about buggy encoders used to generate reference images.) Regarding how hashes are stored: I would keep them in one file. Maybe Then the workflow for adding new test images would be as follows:
This would allow us to impose a rule like "All reference images >32 KB must be git-ignored unless you have a good reason." (Not enforced in code, just documented as part of the contributing docs.) While unnecessary, I would advocate for committing at least some reference images. Makes it easier to review PRs without having to run their code locally. I also want to avoid a situation where a new contributor checks out the code base, runs tests, tests fail due to some issue, and all they see is an error saying "mismatching hashes" without any further info. (The
Fully agree with that. If we have more test images for a particular format than the upstream crate implementing that format, then something is seriously wrong upstream. However, even our binding layers are typically at least a hundred lines of code each, often with lots of branching code paths to handle different options within a format. E.g. TIFF has 1 or more branches for every supported color format. So even our binding layers require quite a few test images to test adequately. |
|
Regarding I also ran it on all reference images (with default compression level o2) to see how it performs, and the results are pretty good. Around 30% saved on average good. I did not think that it would be able to save this much. Size change by file(Small files <1 KB are ignored, because they don't matter for the git size problem.)
|
|
I added The problem might be a dependency of cargo tree |
The size savings would be for |
|
Yeah, of course. I'm not suggesting updating existing reference images. (I only used them as a dataset to evaluate how good |
|
Right. Good old crc doesn't do the same thing on BE, because bytes have a different layout there. I'd like some feedback on the approach before I fix that though. |
| let test_crc_actual = { | ||
| let mut hasher = Crc32::new(); | ||
| match test_img { | ||
| DynamicImage::ImageLuma8(_) | ||
| | DynamicImage::ImageLumaA8(_) | ||
| | DynamicImage::ImageRgb8(_) | ||
| | DynamicImage::ImageRgba8(_) => hasher.update(test_img.as_bytes()), | ||
| DynamicImage::ImageLuma16(_) | ||
| | DynamicImage::ImageLumaA16(_) | ||
| | DynamicImage::ImageRgb16(_) | ||
| | DynamicImage::ImageRgba16(_) => { | ||
| for v in test_img.as_bytes().chunks(2) { | ||
| hasher.update(&u16::from_ne_bytes(v.try_into().unwrap()).to_le_bytes()); | ||
| } | ||
| } | ||
| DynamicImage::ImageRgb32F(_) | DynamicImage::ImageRgba32F(_) => { | ||
| for v in test_img.as_bytes().chunks(4) { | ||
| hasher.update(&f32::from_ne_bytes(v.try_into().unwrap()).to_le_bytes()); | ||
| } | ||
| } | ||
| _ => panic!("Unsupported image format"), | ||
| } | ||
| hasher.finalize() | ||
| }; |
There was a problem hiding this comment.
The way to use hashing is to normalize the pixel matrix byte data to big-endian like CRC did here previously in pre-processing. clone should be cheap enough for tests. (Aand should probably happen in a dedicated method outside edit_image_hash because any change we make to the layout of images may need us to further normalize).
There was a problem hiding this comment.
Ah, sorry. When I said "feedback on the approach" I meant the approach to testing in general. Fixing CRC is trivial. Thanks though.
|
I don't see why these changes require a complete rewrite of the reference images test. The volume of changes makes it much harder to review. |
|
Firstly, some criticism. This isn't actionable feedback. What do you want me to do in response to this? Explain, make changes, cut back features? We have a communication delay of about 1/2 day due to time zones. Anytime I have to ask questions like this, I have to wait another day or do something hoping I guessed right. Please be clearer about what you want me to do. Secondly, I don't see why you think that the changes I made could be implemented in the previous reference test without replacing almost everything. This PR flips the direction images are processed to ensure all images in And lastly, the amount of code changes in this PR is comparable to #2742. The only reason so many files were renamed is that I removed the CRC from their name, since it's unnecessary now. I understand that large diffs are annoying to review, but it's not like I'm making it large for no reason. If you have suggestions for how the diff could be shrunk, please make them. I'm making this a draft until I figure out what approach to snapshot testing to take. See also here. Feedback welcome. |
|
So my specific feedback is to break this down into smaller PRs so that each one is a small diff against the current state. Large diffs aren't just more annoying to review, they take exponentially longer to review and have a much higher chance of stalling out without merging anything at all. They also ironically make communication delay worse: it is relatively fast to glance at a small change and give a line or two of feedback or immediately click approve, while it is much rarer to have a big chunk of time (and motivation!) to analyze a large PR or write a long comment. (To give you a sense, I saw your last comment only a few hours after you posted it but it has taken me until now to sit down and write a full reply...) Taken together, all the changes you listed do necessarily result in a large delta. But each change individually seems much smaller and since the high level code structure isn't changing I'd expect that at least some pieces wouldn't need to change. I'd expect a PR to remove the CRCs from filenames to be a handful of source line changes (mostly deletions) and then a ton of renamed images, with no added/removed files. I'd expect a PR to flip the processing order to only take a line or two for switching which directory was recursively walked, a few more for fixing up the filename logic, and then maybe a bunch of added reference images. The new snapshot idea probably takes more code, but it should nearly all additions rather than edits. |
|
Sure, that's a plan. Flipping processing order is unlikely to be just a few lines changed, but we'll see. |
4 participants
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.