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>

Author: mvanhorn

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<std::byte>& data);
+    OIIO_NODISCARD_ERROR virtual bool read_image(int subimage, int miplevel,
+                                           int chbegin, int chend,
+                                           TypeDesc format,
+                                           const image_span<std::byte>& data);
 
     /// A version of `read_image()` taking an `image_span<T>`, where the type
     /// of the underlying data is `T`.  This is a convenience wrapper around
     /// the `read_image()` that takes an `image_span<std::byte>`.
     template<typename T>
-    bool read_image(int subimage, int miplevel, int chbegin, int chend,
-                    const image_span<T>& data)
+    OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin,
+                                   int chend, const image_span<T>& data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_image() does not accept image_span<const T>");
@@ -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<T>`.
     template<typename T>
-    bool read_image(int subimage, int miplevel, int chbegin, int chend,
-                    span<T> data)
+    OIIO_NODISCARD_ERROR bool read_image(int subimage, int miplevel, int chbegin,
+                                   int chend, span<T> data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_image() does not accept span<const T>");
@@ -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<std::byte>& 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<std::byte>& data);
 
     /// A version of `read_scanlines()` taking an `image_span<T>`, where the
     /// type of the underlying data is `T`.  This is a convenience wrapper
     /// around the `read_scanlines()` that takes an `image_span<std::byte>`.
     template<typename T>
-    bool read_scanlines(int subimage, int miplevel, int ybegin, int yend,
-                        int chbegin, int chend, const image_span<T>& data)
+    OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin,
+                                       int yend, int chbegin, int chend,
+                                       const image_span<T>& data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_scanlines() does not accept span<const T>");
@@ -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<T>`.
     template<typename T>
-    bool read_scanlines(int subimage, int miplevel, int ybegin, int yend,
-                        int chbegin, int chend, span<T> data)
+    OIIO_NODISCARD_ERROR bool read_scanlines(int subimage, int miplevel, int ybegin,
+                                       int yend, int chbegin, int chend,
+                                       span<T> data)
     {
         static_assert(!std::is_const_v<T>,
                       "read_scanlines() does not accept span<const T>");
@@ -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);
 
     /// @}
 

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

src/jpeg.imageio/jpegoutput.cpp

@@ -573,15 +573,17 @@ 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;
         ImageSpec orig_out_spec = spec();
         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

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);
     }
 }

src/python/py_imagebuf.cpp

@@ -270,7 +270,8 @@ ImageBuf_repr_png(const ImageBuf& self)
 
     std::unique_ptr<ImageOutput> 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();