fix(idiff): Fail comparison when images have different dimensions
Previously, idiff would not explicitly fail when comparing images with different dimensions. Both numeric and perceptual comparisons use roi_union which silently handles dimension differences by comparing against black pixels for non-overlapping regions.
This fix adds an explicit dimension check before any comparison to ensure images with different sizes or channel counts fail with ErrDifferentSize (exit code 3) and print a clear error message.
Fixes #4948
Description
Tests
Checklist:
- [x] I have read the contribution guidelines.
- [x] I have updated the documentation, if applicable. (Check if there is no need to update the documentation, for example if this is a bug fix that doesn't change the API.)
- [x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
- [x] If I added or modified a C++ API call, I have also amended the corresponding Python bindings (and if altering ImageBufAlgo functions, also exposed the new functionality as oiiotool options).
- [x] My code follows the prevailing code style of this project. If I haven't already run clang-format before submitting, I definitely will look at the CI test that runs clang-format and fix anything that it highlights as being nonconforming.
It's very common (especially for openexr files, which support an explicit "display window" separately from a "pixel data window") for applications to "shrink wrap" the images by reducing the pixel data window to only encompass the non-zero part of the image. By definition, pixels outside the pixel window are simply black, and it would break a lot of workflows to suddenly start failing comparisons of shrink-wrapped image files.
We should not consider images to differ in contents merely on the basis of their windows being different. We should only consider if the pixel values are different (and honoring the precept that pixels outside the data window exist, but are 0 in all channels).
I think the real problem of Issue #4948 is perhaps different than the initial diagnoss. Note that it works fine when not using the -p flag. So it has probably is a case of ImageBufAlgo::compare_Yee in particular misbehaving under those circumstances.
Um... you can see that all of the CI is failing, and it's because our testsuite specifically tests this case of ensuring that two images are considered the same even if they have different crop windows, provided that they are always 0 in the areas where the two images do not overlap their windows.
So I feel compelled to ask: Before submitting the PR, did you not either run the tests locally on your machine, or at least check the results of the CI run that happened when you pushed your changes to your fork of OIIO?
I understand that sometimes we're in a rush and stuff slips through the cracks, but it's really always worth the time to pause after you push to your fork, check the CI results there, and then only submit the PR if it passes all tests at that stage.
Sure when i ran the CI worked, let me try locally and test again.
This doesn't make sense to me. There were only 2 reference outputs for this test before -- and one exists only to account for slightly different formatting for old fmt 6.0. Why should your fix require 7 additional reference outputs? That sounds fishy to me.