Skip to content

fix(ImageBuf): Better errors for nonexistant subimages/mips #4801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Jun 14, 2025

An important clarification: ImageInput::seek_subimage() returns false
but does not set an error message if the only thing wrong is
requesting a non-existent subimage or miplevel, and similarly,
ImageInput::spec() and spec_dimensions() set an empty spec (easy to
tell by spec.format == TypeUnknown) in that case. All three of these
only set an error message if the reason it couldn't perform the task
because there was actually something wrong with the file or the
I/O. (Put another way, it's common to use these to simply probe which
subimages or mip levels exist, and returning false for "no it doesn't"
isn't really an error, it's expected behavior.)

OK, then.

We weren't always following these rules, sometimes setting
error messages when we shouldn't.

Other times (in different routines) when we didn't set error messages
but should have. And also, there are some downstream places such as
ImageBuf::init_spec() that assumed that seek_subimage / spec /
spec_dimensions set an error message (but they didn't, by design) and
should have raised their own errors when discovering that a requested
subimage or miplevel didn't exist.

Also, there were a couple spots in ImageBuf internals where we tried
to do the right thing, but instead of calling ImageBufImpl::error(),
we called errorfmt(), which didn't exist in ImageBufImpl, and so was
calling the global OIIO::errorfmt(), which therefore was setting the
error on the wrong object entirely. (Fix that by adding an errorfmt
method in ImageBufImpl so that mistake is not possible. Should have
been like that all along.)

All this leads to more consistent error reporting (and not reporting,
in the cases we weren't supposed to), especially for people calling
ImageBuf::read() and ImageBuf::init_spec().

@lgritz
Copy link
Collaborator Author

lgritz commented Jun 15, 2025

Hold off on reviewing this; I think it's not fully baked yet. I will update after some more thought about what the consistent behavior should be.

@lgritz lgritz changed the title Better error checking when requesting nonexistant subimages/mips fix(ImageBuf): Better errors for nonexistant subimages/mips Jun 16, 2025
@lgritz
Copy link
Collaborator Author

lgritz commented Jun 16, 2025

Updated the PR, including revising the title and description. I think I have it straightened out now.

An important clarification: ImageInput::seek_subimage() returns false
but does not set an error message if the only thing wrong is
requesting a non-existent subimage or miplevel, and similarly,
ImageInput::spec() and spec_dimensions() set an empty spec (easy to
tell by spec.format == TypeUnknown) in that case. All three of these
only set an error message if the reason it couldn't perform the task
because there was actually something wrong with the file or the
I/O. (Put another way, it's common to use these to simply probe which
subimages or mip levels exist, and returning false for "no it doesn't"
isn't really an error, it's expected behavior.)

OK, then.

We weren't always following these rules, sometimes setting
error messages when we shouldn't.

Other times (in different routines) when we didn't set error messages
but should have. And also, there are some downstream places such as
ImageBuf::init_spec() that assumed that seek_subimage / spec /
spec_dimensions set an error message (but they didn't, by design) and
should have raised their own errors when discovering that a requested
subimage or miplevel didn't exist.

Also, there were a couple spots in ImageBuf internals where we tried
to do the right thing, but instead of calling ImageBufImpl::error(),
we called errorfmt(), which didn't exist in ImageBufImpl, and so was
calling the global OIIO::errorfmt(), which therefore was setting the
error on the wrong object entirely. (Fix that by adding an errorfmt
method in ImageBufImpl so that mistake is not possible. Should have
been like that all along.)

All this leads to more consistent error reporting (and not reporting,
in the cases we weren't supposed to), especially for people calling
ImageBuf::read() and ImageBuf::init_spec().

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Jun 20, 2025

Opinions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant