From 5b166effbe75bdb93827ca909c384c8b9b8bf7f7 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sat, 4 Apr 2026 22:18:51 -0700 Subject: [PATCH 1/2] Add two-tier OIIO_NODISCARD for ImageInput methods Per @lgritz feedback, use a two-tier approach: - OIIO_NODISCARD (hard [[nodiscard]]) on create() and open(), where ignoring the return is always a bug. - OIIO_NODISCARD_ERROR (initially a no-op) on read_image() and read_scanlines(), where ignoring is sloppy but not catastrophic. This allows a gentle transition for downstream callers. Also fix unchecked return values at internal call sites: - imageioplugin.cpp: add null check after ImageInput::create() - jpegoutput.cpp: check open() return and propagate failure - py_imagebuf.cpp: check create()/open() return before writing Refs: #4781 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- src/include/OpenImageIO/imageio.h | 93 +++++++++++++++------------- src/include/OpenImageIO/platform.h | 8 +++ src/jpeg.imageio/jpegoutput.cpp | 6 +- src/libOpenImageIO/imageioplugin.cpp | 2 +- src/python/py_imagebuf.cpp | 3 +- 5 files changed, 66 insertions(+), 46 deletions(-) diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h index 6e61a3e213..7dc6049260 100644 --- a/src/include/OpenImageIO/imageio.h +++ b/src/include/OpenImageIO/imageio.h @@ -1051,16 +1051,18 @@ class OIIO_API ImageInput { /// A `unique_ptr` that will close and free the ImageInput when /// it exits scope or is reset. The pointer will be empty if the /// required writer was not able to be created. - static unique_ptr create (string_view filename, bool do_open=false, - const ImageSpec *config=nullptr, - Filesystem::IOProxy* ioproxy = nullptr, - string_view plugin_searchpath = ""); + OIIO_NODISCARD static unique_ptr create (string_view filename, + bool do_open=false, + const ImageSpec *config=nullptr, + Filesystem::IOProxy* ioproxy = nullptr, + string_view plugin_searchpath = ""); /// Create an ImageInput using a UTF-16 encoded wstring filename. - static unique_ptr create (const std::wstring& filename, bool do_open=false, - const ImageSpec *config=nullptr, - Filesystem::IOProxy* ioproxy = nullptr, - string_view plugin_searchpath = "") { + OIIO_NODISCARD static unique_ptr create (const std::wstring& filename, + bool do_open=false, + const ImageSpec *config=nullptr, + Filesystem::IOProxy* ioproxy = nullptr, + string_view plugin_searchpath = "") { return create(Strutil::utf16_to_utf8(filename), do_open, config, ioproxy, plugin_searchpath); } @@ -1180,10 +1182,11 @@ class OIIO_API ImageInput { /// /// @returns /// `true` if the file was found and opened successfully. - virtual bool open (const std::string& name, ImageSpec &newspec) = 0; + OIIO_NODISCARD virtual bool open (const std::string& name, + ImageSpec &newspec) = 0; /// Open the ImageInput using a UTF-16 encoded wstring filename. - bool open (const std::wstring& name, ImageSpec &newspec) { + OIIO_NODISCARD bool open (const std::wstring& name, ImageSpec &newspec) { return open(Strutil::utf16_to_utf8(name), newspec); } @@ -1207,13 +1210,14 @@ class OIIO_API ImageInput { /// /// @returns /// `true` if the file was found and opened successfully. - virtual bool open (const std::string& name, ImageSpec &newspec, - const ImageSpec& config OIIO_MAYBE_UNUSED) { + OIIO_NODISCARD virtual bool open (const std::string& name, + ImageSpec &newspec, + const ImageSpec& config OIIO_MAYBE_UNUSED) { return open(name,newspec); } /// Open the ImageInput using a UTF-16 encoded wstring filename. - bool open (const std::wstring& name, ImageSpec &newspec, - const ImageSpec& config OIIO_MAYBE_UNUSED) { + OIIO_NODISCARD bool open (const std::wstring& name, ImageSpec &newspec, + const ImageSpec& config OIIO_MAYBE_UNUSED) { return open(name,newspec); } @@ -1411,15 +1415,17 @@ class OIIO_API ImageInput { /// y, and z). /// @returns `true` upon success, or `false` upon failure. /// - virtual bool read_image(int subimage, int miplevel, int chbegin, int chend, - TypeDesc format, const image_span& data); + OIIO_NODISCARD_ERROR virtual bool read_image(int subimage, int miplevel, + int chbegin, int chend, + TypeDesc format, + const image_span& data); /// A version of `read_image()` taking an `image_span`, where the type /// of the underlying data is `T`. This is a convenience wrapper around /// the `read_image()` that takes an `image_span`. template - bool read_image(int subimage, int miplevel, int chbegin, int chend, - const image_span& data) + OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin, + int chend, const image_span& data) { static_assert(!std::is_const_v, "read_image() does not accept image_span"); @@ -1432,8 +1438,8 @@ class OIIO_API ImageInput { /// contiguous strides in all dimensions. This is a convenience wrapper /// around the `read_image()` that takes an `image_span`. template - bool read_image(int subimage, int miplevel, int chbegin, int chend, - span data) + OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin, + int chend, span data) { static_assert(!std::is_const_v, "read_image() does not accept span"); @@ -1480,17 +1486,19 @@ class OIIO_API ImageInput { /// Added in OIIO 3.1, this is the "safe" preferred alternative to /// the version of read_scanlines that takes raw pointers. /// - virtual bool read_scanlines(int subimage, int miplevel, int ybegin, - int yend, int chbegin, int chend, - TypeDesc format, - const image_span& data); + OIIO_NODISCARD_ERROR virtual bool read_scanlines(int subimage, int miplevel, + int ybegin, int yend, + int chbegin, int chend, + TypeDesc format, + const image_span& data); /// A version of `read_scanlines()` taking an `image_span`, where the /// type of the underlying data is `T`. This is a convenience wrapper /// around the `read_scanlines()` that takes an `image_span`. template - bool read_scanlines(int subimage, int miplevel, int ybegin, int yend, - int chbegin, int chend, const image_span& data) + OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin, + int yend, int chbegin, int chend, + const image_span& data) { static_assert(!std::is_const_v, "read_scanlines() does not accept span"); @@ -1504,8 +1512,9 @@ class OIIO_API ImageInput { /// contiguous strides in all dimensions. This is a convenience wrapper /// around the `read_scanlines()` that takes an `image_span`. template - bool read_scanlines(int subimage, int miplevel, int ybegin, int yend, - int chbegin, int chend, span data) + OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin, + int yend, int chbegin, int chend, + span data) { static_assert(!std::is_const_v, "read_scanlines() does not accept span"); @@ -1773,12 +1782,12 @@ class OIIO_API ImageInput { /// /// @note This call was changed for OpenImageIO 2.0 to include the /// explicit subimage and miplevel parameters. - virtual bool read_scanlines (int subimage, int miplevel, - int ybegin, int yend, int z, - int chbegin, int chend, - TypeDesc format, void *data, - stride_t xstride=AutoStride, - stride_t ystride=AutoStride); + OIIO_NODISCARD_ERROR virtual bool read_scanlines (int subimage, int miplevel, + int ybegin, int yend, int z, + int chbegin, int chend, + TypeDesc format, void *data, + stride_t xstride=AutoStride, + stride_t ystride=AutoStride); /// Read the tile whose upper-left origin is (x,y,z) into `data[]`, /// converting if necessary from the native data format of the file into @@ -1904,14 +1913,14 @@ class OIIO_API ImageInput { /// @param progress_callback/progress_callback_data /// Optional progress callback. /// @returns `true` upon success, or `false` upon failure. - virtual bool read_image (int subimage, int miplevel, - int chbegin, int chend, - TypeDesc format, void *data, - stride_t xstride=AutoStride, - stride_t ystride=AutoStride, - stride_t zstride=AutoStride, - ProgressCallback progress_callback=NULL, - void *progress_callback_data=NULL); + OIIO_NODISCARD_ERROR virtual bool read_image (int subimage, int miplevel, + int chbegin, int chend, + TypeDesc format, void *data, + stride_t xstride=AutoStride, + stride_t ystride=AutoStride, + stride_t zstride=AutoStride, + ProgressCallback progress_callback=NULL, + void *progress_callback_data=NULL); /// @} diff --git a/src/include/OpenImageIO/platform.h b/src/include/OpenImageIO/platform.h index 1c52a217ba..18cdbc3b58 100644 --- a/src/include/OpenImageIO/platform.h +++ b/src/include/OpenImageIO/platform.h @@ -461,6 +461,14 @@ # define OIIO_NODISCARD #endif +// OIIO_NODISCARD_ERROR is for functions returning error status (bool) where +// ignoring the return is a bad practice but not always catastrophic. This is +// initially a no-op to allow a gentle transition; it can later be defined as +// [[nodiscard]] once downstream callers have been updated. +#ifndef OIIO_NODISCARD_ERROR +# define OIIO_NODISCARD_ERROR /* nothing for now */ +#endif + // OIIO_NO_SANITIZE_ADDRESS can be used to mark a function that you don't // want address sanitizer to catch. Only use this if you know there are diff --git a/src/jpeg.imageio/jpegoutput.cpp b/src/jpeg.imageio/jpegoutput.cpp index 68156aad0d..69c17a25fe 100644 --- a/src/jpeg.imageio/jpegoutput.cpp +++ b/src/jpeg.imageio/jpegoutput.cpp @@ -573,7 +573,8 @@ JpgOutput::copy_image(ImageInput* in) ImageSpec in_spec; ImageSpec config_spec; config_spec.attribute("_jpeg:raw", 1); - in->open(in_name, in_spec, config_spec); + if (!in->open(in_name, in_spec, config_spec)) + return false; // Re-open the output std::string out_name = m_filename; @@ -581,7 +582,8 @@ JpgOutput::copy_image(ImageInput* in) close(); m_copy_coeffs = (jvirt_barray_ptr*)jpg_in->coeffs(); m_copy_decompressor = &jpg_in->m_cinfo; - open(out_name, orig_out_spec); + if (!open(out_name, orig_out_spec)) + return false; // Strangeness -- the write_coefficients somehow sets things up // so that certain writes only happen in close(), which MUST diff --git a/src/libOpenImageIO/imageioplugin.cpp b/src/libOpenImageIO/imageioplugin.cpp index 68462cebf1..0ea86da08a 100644 --- a/src/libOpenImageIO/imageioplugin.cpp +++ b/src/libOpenImageIO/imageioplugin.cpp @@ -504,7 +504,7 @@ pvt::catalog_all_plugins(std::string searchpath) // ImageInput::create will take a lock of imageio_mutex auto inp = ImageInput::create(f.first); lock.lock(); - if (inp->supports("procedural")) + if (inp && inp->supports("procedural")) procedural_plugins.insert(f.first); } } diff --git a/src/python/py_imagebuf.cpp b/src/python/py_imagebuf.cpp index ec9d23bf4f..346204e715 100644 --- a/src/python/py_imagebuf.cpp +++ b/src/python/py_imagebuf.cpp @@ -270,7 +270,8 @@ ImageBuf_repr_png(const ImageBuf& self) std::unique_ptr out = ImageOutput::create("temp.png", &file_vec); - out->open("temp.png", altered_spec); + if (!out || !out->open("temp.png", altered_spec)) + return py::none(); self.write(out.get()); out->close(); From 0c93039cd3f1501bf6ee12c871b48ff0b38ccaaf Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Mon, 6 Apr 2026 06:48:28 -0700 Subject: [PATCH 2/2] Address review feedback: version-gated NODISCARD_ERROR, fix behavioral changes - platform.h: Replace simple no-op macro with version-gated OIIO_NODISCARD_ERROR_ENABLE that defaults to disabled for OIIO < 3.3 and enabled for >= 3.3, overridable via -D flag - py_imagebuf.cpp: Restore returning empty py::bytes() instead of py::none() on open failure to preserve backward compatibility - jpegoutput.cpp: Restructure open/close flow to guarantee close() is always called even when open() fails, matching previous behavior - imageio.h: Apply clang-format to NODISCARD_ERROR-annotated functions Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- src/include/OpenImageIO/imageio.h | 39 +++++++++++++++--------------- src/include/OpenImageIO/platform.h | 24 ++++++++++++++---- src/jpeg.imageio/jpegoutput.cpp | 15 +++--------- src/python/py_imagebuf.cpp | 14 +++-------- 4 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h index 7dc6049260..4aaec86681 100644 --- a/src/include/OpenImageIO/imageio.h +++ b/src/include/OpenImageIO/imageio.h @@ -1415,17 +1415,17 @@ class OIIO_API ImageInput { /// y, and z). /// @returns `true` upon success, or `false` upon failure. /// - OIIO_NODISCARD_ERROR virtual bool read_image(int subimage, int miplevel, - int chbegin, int chend, - TypeDesc format, - const image_span& data); + OIIO_NODISCARD_ERROR virtual bool + read_image(int subimage, int miplevel, int chbegin, int chend, + TypeDesc format, const image_span& data); /// A version of `read_image()` taking an `image_span`, where the type /// of the underlying data is `T`. This is a convenience wrapper around /// the `read_image()` that takes an `image_span`. template - OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin, - int chend, const image_span& data) + OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, + int chbegin, int chend, + const image_span& data) { static_assert(!std::is_const_v, "read_image() does not accept image_span"); @@ -1438,8 +1438,8 @@ class OIIO_API ImageInput { /// contiguous strides in all dimensions. This is a convenience wrapper /// around the `read_image()` that takes an `image_span`. template - OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin, - int chend, span data) + OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, + int chbegin, int chend, span data) { static_assert(!std::is_const_v, "read_image() does not accept span"); @@ -1486,19 +1486,18 @@ class OIIO_API ImageInput { /// Added in OIIO 3.1, this is the "safe" preferred alternative to /// the version of read_scanlines that takes raw pointers. /// - OIIO_NODISCARD_ERROR virtual bool read_scanlines(int subimage, int miplevel, - int ybegin, int yend, - int chbegin, int chend, - TypeDesc format, - const image_span& data); + OIIO_NODISCARD_ERROR virtual bool + read_scanlines(int subimage, int miplevel, int ybegin, int yend, + int chbegin, int chend, TypeDesc format, + const image_span& data); /// A version of `read_scanlines()` taking an `image_span`, where the /// type of the underlying data is `T`. This is a convenience wrapper /// around the `read_scanlines()` that takes an `image_span`. template - OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin, - int yend, int chbegin, int chend, - const image_span& data) + OIIO_NODISCARD_ERROR bool + read_scanlines(int subimage, int miplevel, int ybegin, int yend, + int chbegin, int chend, const image_span& data) { static_assert(!std::is_const_v, "read_scanlines() does not accept span"); @@ -1512,9 +1511,9 @@ class OIIO_API ImageInput { /// contiguous strides in all dimensions. This is a convenience wrapper /// around the `read_scanlines()` that takes an `image_span`. template - OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin, - int yend, int chbegin, int chend, - span data) + OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, + int ybegin, int yend, int chbegin, + int chend, span data) { static_assert(!std::is_const_v, "read_scanlines() does not accept span"); @@ -4704,6 +4703,6 @@ OIIO_NAMESPACE_END #if FMT_VERSION >= 100000 FMT_BEGIN_NAMESPACE -template<> struct formatter : ostream_formatter {}; +template<> struct formatter : ostream_formatter { }; FMT_END_NAMESPACE #endif diff --git a/src/include/OpenImageIO/platform.h b/src/include/OpenImageIO/platform.h index 18cdbc3b58..6d025d81d9 100644 --- a/src/include/OpenImageIO/platform.h +++ b/src/include/OpenImageIO/platform.h @@ -462,11 +462,25 @@ #endif // OIIO_NODISCARD_ERROR is for functions returning error status (bool) where -// ignoring the return is a bad practice but not always catastrophic. This is -// initially a no-op to allow a gentle transition; it can later be defined as -// [[nodiscard]] once downstream callers have been updated. -#ifndef OIIO_NODISCARD_ERROR -# define OIIO_NODISCARD_ERROR /* nothing for now */ +// ignoring the return is a bad practice but not always catastrophic. +// +// OIIO_NODISCARD_ERROR_ENABLE, if nonzero, enables OIIO_NODISCARD_ERROR to +// take effect. The default is to disable it for OIIO < 3.3, and enable it for +// OIIO >= 3.3. But `-DOIIO_NODISCARD_ERROR_ENABLE=...` can be used to +// override the default, for example to flag discarded errors in older +// versions of OIIO, or to disable the warnings in future versions of OIIO. +#ifndef OIIO_NODISCARD_ERROR_ENABLE +# if OIIO_VERSION_LESS(3, 3, 0) +# define OIIO_NODISCARD_ERROR_ENABLE 0 /* disable for now */ +# else +# define OIIO_NODISCARD_ERROR_ENABLE 1 /* enable for OIIO >= 3.3 */ +# endif +#endif + +#if OIIO_NODISCARD_ERROR_ENABLE +# define OIIO_NODISCARD_ERROR OIIO_NODISCARD +#else +# define OIIO_NODISCARD_ERROR /* nothing */ #endif diff --git a/src/jpeg.imageio/jpegoutput.cpp b/src/jpeg.imageio/jpegoutput.cpp index 69c17a25fe..bf0e27d898 100644 --- a/src/jpeg.imageio/jpegoutput.cpp +++ b/src/jpeg.imageio/jpegoutput.cpp @@ -33,9 +33,7 @@ class JpgOutput final : public ImageOutput { ~JpgOutput() override { close(); } const char* format_name(void) const override { return "jpeg"; } int supports(string_view feature) const override - { - return (feature == "exif" || feature == "iptc" || feature == "ioproxy"); - } + { return (feature == "exif" || feature == "iptc" || feature == "ioproxy"); } bool open(const std::string& name, const ImageSpec& spec, OpenMode mode = Create) override; bool write_scanline(int y, int z, TypeDesc format, const void* data, @@ -104,9 +102,7 @@ OIIO_PLUGIN_EXPORTS_BEGIN OIIO_EXPORT ImageOutput* jpeg_output_imageio_create() -{ - return new JpgOutput; -} +{ return new JpgOutput; } OIIO_EXPORT const char* jpeg_output_extensions[] = { "jpg", "jpe", "jpeg", "jif", "jfif", "jfi", nullptr }; @@ -582,17 +578,14 @@ JpgOutput::copy_image(ImageInput* in) close(); m_copy_coeffs = (jvirt_barray_ptr*)jpg_in->coeffs(); m_copy_decompressor = &jpg_in->m_cinfo; - if (!open(out_name, orig_out_spec)) - return false; - + bool ok = open(out_name, orig_out_spec); // Strangeness -- the write_coefficients somehow sets things up // so that certain writes only happen in close(), which MUST // happen while the input file is still open. So we go ahead // and close() now, so that the caller of copy_image() doesn't // close the input file first and then wonder why they crashed. close(); - - return true; + return ok; } return ImageOutput::copy_image(in); diff --git a/src/python/py_imagebuf.cpp b/src/python/py_imagebuf.cpp index 346204e715..481c26e6dd 100644 --- a/src/python/py_imagebuf.cpp +++ b/src/python/py_imagebuf.cpp @@ -154,9 +154,7 @@ ImageBuf_setpixel(ImageBuf& buf, int x, int y, int z, py::object p) void ImageBuf_setpixel2(ImageBuf& buf, int x, int y, py::object p) -{ - ImageBuf_setpixel(buf, x, y, 0, p); -} +{ ImageBuf_setpixel(buf, x, y, 0, p); } void @@ -194,16 +192,12 @@ ImageBuf_get_pixels(const ImageBuf& buf, TypeDesc format, ROI roi = ROI::All()) void ImageBuf_set_deep_value(ImageBuf& buf, int x, int y, int z, int c, int s, float value) -{ - buf.set_deep_value(x, y, z, c, s, value); -} +{ buf.set_deep_value(x, y, z, c, s, value); } void ImageBuf_set_deep_value_uint(ImageBuf& buf, int x, int y, int z, int c, int s, uint32_t value) -{ - buf.set_deep_value(x, y, z, c, s, value); -} +{ buf.set_deep_value(x, y, z, c, s, value); } @@ -271,7 +265,7 @@ ImageBuf_repr_png(const ImageBuf& self) std::unique_ptr out = ImageOutput::create("temp.png", &file_vec); if (!out || !out->open("temp.png", altered_spec)) - return py::none(); + return py::bytes(); self.write(out.get()); out->close();