fix(idiff): Fail comparison when images have different dimensions #4976
Conversation
pmady
force-pushed
the
fix/idiff-perceptual-dimension-check
branch
from
a74330a to
0fcf1aa
Compare
pmady
force-pushed
the
fix/idiff-perceptual-dimension-check
branch
from
0fcf1aa to
ec8b63c
Compare
|
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 |
|
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. |
pmady
force-pushed
the
fix/idiff-perceptual-dimension-check
branch
from
ec8b63c to
b0f655e
Compare
|
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. |
The GaussianPyramid::value() method was incorrectly accessing channel 1 instead of channel 0. This fix corrects the channel access to properly retrieve the luminance value from the single-channel pyramid levels. Update reference outputs to reflect the corrected perceptual diff results (Max error changes from 1 to 10 for the test images). Signed-off-by: pmady <pavan4devops@gmail.com>
pmady
force-pushed
the
fix/idiff-perceptual-dimension-check
branch
from
7249e96 to
384d5da
Compare
| return 0.0f; | ||
| else | ||
| return level[lev].getchannel(x, y, 0, 1); | ||
| return level[lev].getchannel(x, y, 0, 0); |
There was a problem hiding this comment.
It was this all along? Weird. Why did this ever work?
There was a problem hiding this comment.
"It 'worked' in the sense that it didn't crash - getchannel() likely returns 0 for out-of-bounds channels. Since both pyramids returned the same incorrect value (0), the computed differences were always 0, making images appear identical. This explains why perceptual comparison was passing for clearly different images in #4948."
lgritz
merged commit 4e9e70f
into
AcademySoftwareFoundation:main
…areFoundation#4976) The GaussianPyramid::value() method used by IBA::compare_Yee() was incorrectly accessing channel 1 instead of channel 0. This fix corrects the channel access to properly retrieve the luminance value from the single-channel pyramid levels. One oddity this led to is that `idiff -p` would appear to PASS for some clearly different images if they had different data windows. Update reference outputs to reflect the corrected perceptual diff results (Max error changes from 1 to 10 for the test images). Fixes AcademySoftwareFoundation#4948 Signed-off-by: pmady <pavan4devops@gmail.com>
…areFoundation#4976) The GaussianPyramid::value() method used by IBA::compare_Yee() was incorrectly accessing channel 1 instead of channel 0. This fix corrects the channel access to properly retrieve the luminance value from the single-channel pyramid levels. One oddity this led to is that `idiff -p` would appear to PASS for some clearly different images if they had different data windows. Update reference outputs to reflect the corrected perceptual diff results (Max error changes from 1 to 10 for the test images). Fixes AcademySoftwareFoundation#4948 Signed-off-by: pmady <pavan4devops@gmail.com>
2 participants
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:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
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.