Centralize ImageOutput validity checks #3686
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
Many ImageOutput::open implementations start out with various validity checks -- such as whether the ImageSpec requests a resolution, number of channels, or other features simply not allowed by that file format. This is not only repetitive, but which checks are done by each file type is somewhat haphazard, error-prone, hard to get full test coverage (must we separately test each possible failure case for every file format?), and the error messages are not always consistent across formats. Frankly, most of the format writers assume that the caller has a full understanding of supports() and will never send requests for features that are unsupported. This PR proposes adding a utility method ImageOutput::check_open(), which performs all validity checks on the ImageSpec that can be deduced from supports(). The various ImageOutput::open() implementations may call this function and avoid having to replicate (or forget) all the checks done in other output formats. This is a protected method that is used by ImageOutput implementations internally, but has no use for client-side code. I've implemented check_open, and also used it here for all the output formats. If this seems ok to everyone, I also plan to do an analogous helper for ImageInput common validity checks (which will be an even bigger reduction of redundant code and greatly increase the safety against corrupted or nonsensical input files). Along the way I also instituted a new supports() query: "noimage". That is to indicate a file for which it is permitted to have a pixel data window consisting of 0 pixels (i.e. an "image" that is only a container for metadata). Currently, FITS is the only format we support that allows this, though it's something we occasionally talk about for OpenEXR. Allowing an explicit way for formats to say they allow no pixels is what allows check_open to issue error messages for all the other formats if it is passed zero-sized width, height, depth, or nchannels.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Many ImageOutput::open implementations start out with various validity checks -- such as whether the ImageSpec requests a resolution, number of channels, or other features simply not allowed by that file format. This is not only repetitive, but which checks are done by each file type is somewhat haphazard, error-prone, hard to get full test coverage (must we separately test each possible failure case for every file format?), and the error messages are not always consistent across formats. Frankly, most of the format writers assume that the caller has a full understanding of supports() and will never send requests for features that are unsupported, which is obviously not a careful approach.
This PR proposes adding a utility method ImageOutput::check_open(), which performs all validity checks on the ImageSpec that can be deduced from supports(). The various ImageOutput::open() implementations may call this function and avoid having to replicate (or forget) all the checks done in other output formats. This is a protected method that is used by ImageOutput implementations internally, but has no use for client-side code.
I've implemented check_open, and also used it here for all the output formats. If this seems ok to everyone, I also plan to do an analogous helper for ImageInput common validity checks (which will be an even bigger reduction of redundant code and greatly increase the safety against corrupted or nonsensical input files).
Along the way I also instituted a new supports() query: "noimage". That is to indicate a file for which it is permitted to have a pixel data window consisting of 0 pixels (i.e. an "image" that is only a container for metadata). Currently, FITS is the only format we support that allows this, though it's something we occasionally talk about for OpenEXR. Allowing an explicit way for formats to say they allow no pixels is what allows check_open to issue error messages for all the other formats if it is passed zero-sized width, height, depth, or nchannels.