fix: ImageOutput::check_open logic was flawed #4779
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
This is needed for a high priority bug fix on my end. I would really love if somebody had a chance to review ASAP. |
Contributor
|
$#17 H4PP3N5 |
We documented this function as checking the valid range of pixel coordinates, but we CALLED it everywhere as if we were only checking the resolution. But we IMPLEMENTED as if we were checking the range, generating errors for outputting exr files with negative pixel window origins. But why was nobody seeing those errors when writing exr files with negative origins? Because the openexr writer BOTCHED it by not noticing the return value and considering it a failure. Oh boy. All my fault. So the fix: * Change the comment documenting check_open() to match the way we had been using it at the call sites. * Change the implementation of check_open() to match this by no longer checking whether all four corners were in the range -- only check if each dimension's size is within the limit for that dimension. * Fix the OpenEXR writer's failure to notice the check_open failure. * Does this eliminate necessary checks? No! We already separately checked that nonzero origins were only allowed for formats that support it, and that negative origins were only allowed for formats that support that. So the flawed check was redundant anyway. Ha! * Add a test verifying that we can write exr files with negative origins. (Technically, that would have passed before, but only because we were ignoring the incorrect error.) Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
Collaborator
Author
|
I let this linger for a few days because of a failing test that I didn't have time to chase down, but that turned out to be an unrelated issue I just fixed in #4782, and now that this PR has rebased on top of it, it passes. So I'd love to get a review on this important bug fix and be able to merge early next week at the latest. |
Collaborator
Author
|
Two weeks old, no objections, fixes important bug -> merging |
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Jun 5, 2025
…ion#4779) We documented this function as checking the valid range of pixel coordinates, but we CALLED it everywhere as if we were only checking the resolution. But we IMPLEMENTED as if we were checking the range, generating errors for outputting exr files with negative pixel window origins. But why was nobody seeing those errors when writing exr files with negative origins? Because the openexr writer BOTCHED it by not noticing the return value and considering it a failure. Oh boy. All my fault. So the fix: * Change the comment documenting check_open() to match the way we had been using it at the call sites. * Change the implementation of check_open() to match this by no longer checking whether all four corners were in the range -- only check if each dimension's size is within the limit for that dimension. * Fix the OpenEXR writer's failure to notice the check_open failure. * Does this eliminate necessary checks? No! We already separately checked that nonzero origins were only allowed for formats that support it, and that negative origins were only allowed for formats that support that. So the flawed check was redundant anyway. Ha! * Add a test verifying that we can write exr files with negative origins. (Technically, that would have passed before, but only because we were ignoring the incorrect error.) --------- Signed-off-by: Larry Gritz <[email protected]>
zachlewis
pushed a commit
to zachlewis/OpenImageIO
that referenced
this pull request
Aug 1, 2025
…ion#4779) We documented this function as checking the valid range of pixel coordinates, but we CALLED it everywhere as if we were only checking the resolution. But we IMPLEMENTED as if we were checking the range, generating errors for outputting exr files with negative pixel window origins. But why was nobody seeing those errors when writing exr files with negative origins? Because the openexr writer BOTCHED it by not noticing the return value and considering it a failure. Oh boy. All my fault. So the fix: * Change the comment documenting check_open() to match the way we had been using it at the call sites. * Change the implementation of check_open() to match this by no longer checking whether all four corners were in the range -- only check if each dimension's size is within the limit for that dimension. * Fix the OpenEXR writer's failure to notice the check_open failure. * Does this eliminate necessary checks? No! We already separately checked that nonzero origins were only allowed for formats that support it, and that negative origins were only allowed for formats that support that. So the flawed check was redundant anyway. Ha! * Add a test verifying that we can write exr files with negative origins. (Technically, that would have passed before, but only because we were ignoring the incorrect error.) --------- Signed-off-by: Larry Gritz <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We documented this function as checking the valid range of pixel coordinates, but we CALLED it everywhere as if we were only checking the resolution.
But we IMPLEMENTED as if we were checking the range, generating errors for outputting exr files with negative pixel window origins. But why was nobody seeing those errors when writing exr files with negative origins? Because the openexr writer BOTCHED it by not noticing the return value and considering it a failure.
Oh boy. All my fault.
So the fix:
Change the comment documenting check_open() to match the way we had been using it at the call sites.
Change the implementation of check_open() to match this by no longer checking whether all four corners were in the range -- only check if each dimension's size is within the limit for that dimension.
Fix the OpenEXR writer's failure to notice the check_open failure.
Does this eliminate necessary checks? No! We already separately checked that nonzero origins were only allowed for formats that support it, and that negative origins were only allowed for formats that support that. So the flawed check was redundant anyway. Ha!
Add a test verifying that we can write exr files with negative origins. (Technically, that would have passed before, but only because we were ignoring the incorrect error.)