-
Notifications
You must be signed in to change notification settings - Fork 566
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
feat(api): ImageInput::open_check #3967
feat(api): ImageInput::open_check #3967
Conversation
Not unlike the ImageOutput::open_check we added a few months back, this creates an ImageInput utility function that centralizes a number of validation checks. Currently, this is primarily a check on allowed resolution and total size, aimed at making a guess at files that are corrupted or malicious. Also adds a new `supports("noimage")` query. Signed-off-by: Larry Gritz <lg@larrygritz.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except for the possible logic error I commented on.
src/libOpenImageIO/imageinput.cpp
Outdated
format_name(), range.depth(), spec.width, spec.height, spec.depth); | ||
return false; | ||
} | ||
if (spec.nchannels < range.chbegin || spec.nchannels >= range.chend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in the docs:
/// And the channel extent:
/// [chbegin,chend)]
So assuming 0-based indexing, that means if nchannels==1
is specified, chbegin
of 1 is an error? And chend==1
is valid? If so maybe the code should be
if (spec.nchannels < range.chbegin || spec.nchannels >= range.chend) { | |
if (spec.nchannels <= range.chbegin || spec.nchannels > range.chend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, perhaps I've done this inconsistently with how it works for ImageOutput::check_image. Let me edit a bit and I'll update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The surprising thing for me is that all the CI checks passed. I would have assumed there would be tons of fails if what I said was true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was both interpreting it wrong, and specifying it wrong (in the two places I used it so far), so it all worked out. But now I'm changing it to match the way it is specified for ImageOutput::check_open that we've been using for a while.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Not unlike the ImageOutput::open_check we added a few months back, this creates an ImageInput utility function that centralizes a number of validation checks. Currently, this is primarily a check on allowed resolution and total size, aimed at making a guess at files that are corrupted or malicious.
Also adds a new
supports("noimage")
query.