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

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]>

Author: lgritz

src/include/OpenImageIO/imageio.h

@@ -1142,6 +1142,10 @@ class OIIO_API ImageInput {
     /// `seek_subimage()`. It is thus not thread-safe, since the spec may
     /// change if another thread calls `seek_subimage`, or any of the
     /// `read_*()` functions that take explicit subimage/miplevel.
+    ///
+    /// This method should be considered deprecated, and we advise
+    /// always using the thread-safe `spec(subimage, miplevel)` method
+    /// instead.
     virtual const ImageSpec &spec (void) const { return m_spec; }
 
     /// Return a full copy of the ImageSpec of the designated subimage and
@@ -1150,7 +1154,9 @@ class OIIO_API ImageInput {
     /// ImageSpec if there is lots of named metadata to allocate and copy.
     /// See also the less expensive `spec_dimensions()`. Errors (such as
     /// having requested a nonexistent subimage) are indicated by returning
-    /// an ImageSpec with `format==TypeUnknown`.
+    /// an ImageSpec with `format==TypeUnknown`, but does not call
+    /// errorfmt() to set an error message merely for being an out-of-range
+    /// subimage or miplevel.
     virtual ImageSpec spec (int subimage, int miplevel=0);
 
     /// Return a copy of the ImageSpec of the designated subimage and
@@ -1160,7 +1166,9 @@ class OIIO_API ImageInput {
     /// a relatively inexpensive operation if you don't need that
     /// information. It is guaranteed to be thread-safe. Errors (such as
     /// having requested a nonexistent subimage) are indicated by returning
-    /// an ImageSpec with `format==TypeUnknown`.
+    /// an ImageSpec with `format==TypeUnknown`, but does not call
+    /// errorfmt() to set an error message merely for being an out-of-range
+    /// subimage or miplevel.
     virtual ImageSpec spec_dimensions (int subimage, int miplevel=0);
 
     /// Retrieve a reduced-resolution ("thumbnail") version of the given
@@ -1204,12 +1212,16 @@ class OIIO_API ImageInput {
     /// Seek to the given subimage and MIP-map level within the open image
     /// file.  The first subimage of the file has index 0, the highest-
     /// resolution MIP level has index 0.  The new subimage's vital
-    /// statistics=may be retrieved by `this->spec()`.  The reader is
+    /// statistics may be retrieved by `this->spec()`.  The reader is
     /// expected to give the appearance of random access to subimages and
     /// MIP levels -- in other words, if it can't randomly seek to the given
     /// subimage/level, it should transparently close, reopen, and
     /// sequentially read through prior subimages and levels.
     ///
+    /// Inability to seek to an out-of-range subimage or miplevel is indicated
+    /// by returning false, but it does not call errorfmt() to set an error
+    /// message unless it's the result of a damaged file.
+    ///
     /// @returns
     ///         `true` upon success, or `false` upon failure. A failure may
     ///         indicate that no such subimage or MIP level exists in the

src/libOpenImageIO/imagebuf.cpp

@@ -185,6 +185,14 @@ class ImageBufImpl {
         error(Strutil::fmt::format(fmt, args...));
     }
 
+    // Another alias for error so we don't get mixed up with the global
+    // OIIO::errorfmt.
+    template<typename... Args>
+    void errorfmt(const char* fmt, const Args&... args) const
+    {
+        error(Strutil::fmt::format(fmt, args...));
+    }
+
     void error(string_view message) const;
 
     ImageBuf::IBStorage storage() const { return m_storage; }
@@ -1088,18 +1096,49 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel,
             // If no configspec, just do a regular soft invalidate
             invalidate(m_name, false);
         }
-        m_imagecache->get_image_info(m_name, subimage, miplevel, s_subimages,
-                                     TypeInt, &m_nsubimages);
-        m_imagecache->get_image_info(m_name, subimage, miplevel, s_miplevels,
-                                     TypeInt, &m_nmiplevels);
         const char* fmt = NULL;
-        m_imagecache->get_image_info(m_name, subimage, miplevel, s_fileformat,
-                                     TypeString, &fmt);
+        if (!(m_imagecache->get_image_info(m_name, subimage, miplevel,
+                                           s_subimages, TypeInt, &m_nsubimages)
+              && m_imagecache->get_image_info(m_name, subimage, miplevel,
+                                              s_miplevels, TypeInt,
+                                              &m_nmiplevels)
+              && m_imagecache->get_image_info(m_name, subimage, miplevel,
+                                              s_fileformat, TypeString, &fmt))) {
+            std::string err = m_imagecache->geterror();
+            error(err.size()
+                      ? err
+                      : Strutil::format("trouble reading from {}", m_name));
+            m_spec       = ImageSpec();
+            m_nativespec = m_spec;
+            return false;
+        }
         m_fileformat = ustring(fmt);
 
-        m_imagecache->get_imagespec(m_name, m_nativespec, subimage);
-        m_spec = m_nativespec;
-        m_imagecache->get_cache_dimensions(m_name, m_spec, subimage, miplevel);
+        if (subimage < 0 || subimage >= m_nsubimages) {
+            error("Invalid subimage {} requested (of {} subimages)", subimage,
+                  m_nsubimages);
+            m_spec       = ImageSpec();
+            m_nativespec = m_spec;
+            return false;
+        }
+        if (miplevel < 0 || miplevel >= m_nmiplevels) {
+            error(
+                "Invalid MIP level {} requested for subimage {} (of {} levels)",
+                miplevel, subimage, m_nmiplevels);
+            m_spec       = ImageSpec();
+            m_nativespec = m_spec;
+            return false;
+        }
+        bool ok = m_imagecache->get_imagespec(m_name, m_nativespec, subimage);
+        m_spec  = m_nativespec;
+        ok &= m_imagecache->get_cache_dimensions(m_name, m_spec, subimage,
+                                                 miplevel);
+        if (!ok || m_nativespec.format == TypeUnknown) {
+            std::string cacheerr = m_imagecache->geterror();
+            error("Unable to find subimage={}, miplevel={}{}{}",
+                  cacheerr.size() ? ": " : "", cacheerr);
+            return false;
+        }
 
         m_xstride = m_spec.pixel_bytes();
         m_ystride = m_spec.scanline_bytes();
@@ -1165,20 +1204,26 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel,
         m_current_miplevel = -1;
         auto input = ImageInput::open(filename, m_configspec.get(), m_rioproxy);
         if (!input) {
-            m_err = OIIO::geterror();
+            error("Could not open file: {}", OIIO::geterror());
             atomic_fetch_add(pvt::IB_total_open_time, float(timer()));
             return false;
         }
-        m_spec = input->spec(subimage, miplevel);
+        m_spec       = input->spec(subimage, miplevel);
+        m_nativespec = m_spec;
         if (input->has_error()) {
-            m_err = input->geterror();
+            errorfmt("Error reading: {}", input->geterror());
+            atomic_fetch_add(pvt::IB_total_open_time, float(timer()));
+            return false;
+        }
+        if (m_spec.format == TypeUnknown) {
+            errorfmt("Could not seek to subimage={} miplevel={}", subimage,
+                     miplevel);
             atomic_fetch_add(pvt::IB_total_open_time, float(timer()));
             return false;
         }
         m_badfile    = false;
         m_spec_valid = true;
         m_fileformat = ustring(input->format_name());
-        m_nativespec = m_spec;
         m_xstride    = m_spec.pixel_bytes();
         m_ystride    = m_spec.scanline_bytes();
         m_zstride    = clamped_mult64(m_ystride, (imagesize_t)m_spec.height);

src/openexr.imageio/exrinput.cpp

@@ -1011,8 +1011,11 @@ OpenEXRInput::seek_subimage(int subimage, int miplevel)
         const Imf::Header* header = NULL;
         if (m_input_multipart)
             header = &(m_input_multipart->header(subimage));
-        if (!part.parse_header(this, header))
+        if (!part.parse_header(this, header)) {
+            errorfmt("Could not seek to subimage={}: unable to parse header",
+                     subimage, miplevel);
             return false;
+        }
         part.initialized = true;
     }
 

src/openexr.imageio/exrinput_c.cpp

@@ -904,8 +904,10 @@ OpenEXRCoreInput::PartInfo::query_channels(OpenEXRCoreInput* in,
     spec.nchannels = 0;
     const exr_attr_chlist_t* chlist;
     exr_result_t rv = exr_get_channels(ctxt, subimage, &chlist);
-    if (rv != EXR_ERR_SUCCESS)
+    if (rv != EXR_ERR_SUCCESS) {
+        in->errorfmt("exr_get_channels failed");
         return false;
+    }
 
     std::vector<CChanNameHolder> cnh;
     int c = 0;
@@ -1053,8 +1055,11 @@ OpenEXRCoreInput::seek_subimage(int subimage, int miplevel)
 
     PartInfo& part(m_parts[subimage]);
     if (!part.initialized) {
-        if (!part.parse_header(this, m_exr_context, subimage, miplevel))
+        if (!part.parse_header(this, m_exr_context, subimage, miplevel)) {
+            errorfmt("Could not seek to subimage={}: unable to parse header",
+                     subimage, miplevel);
             return false;
+        }
         part.initialized = true;
     }
 
@@ -1087,6 +1092,8 @@ OpenEXRCoreInput::seek_subimage(int subimage, int miplevel)
 ImageSpec
 OpenEXRCoreInput::spec(int subimage, int miplevel)
 {
+    // By design, spec() inicates failure with empty spec, it does not call
+    // errorfmt().
     ImageSpec ret;
     if (subimage < 0 || subimage >= m_nsubimages)
         return ret;  // invalid
@@ -1112,6 +1119,8 @@ OpenEXRCoreInput::spec(int subimage, int miplevel)
 ImageSpec
 OpenEXRCoreInput::spec_dimensions(int subimage, int miplevel)
 {
+    // By design, spec_dimensions() inicates failure with empty spec, it does
+    // not call errorfmt().
     ImageSpec ret;
     if (subimage < 0 || subimage >= m_nsubimages)
         return ret;  // invalid
@@ -1434,7 +1443,7 @@ OpenEXRCoreInput::read_native_tiles(int subimage, int miplevel, int xbegin,
                                     int zend, void* data)
 {
     if (!m_exr_context) {
-        errorfmt("called OpenEXRInput::read_native_tile without an open file");
+        errorfmt("called OpenEXRInput::read_native_tiles without an open file");
         return false;
     }
 
@@ -1453,7 +1462,7 @@ OpenEXRCoreInput::read_native_tiles(int subimage, int miplevel, int xbegin,
                                     void* data)
 {
     if (!m_exr_context) {
-        errorfmt("called OpenEXRInput::read_native_tile without an open file");
+        errorfmt("called OpenEXRInput::read_native_tiles without an open file");
         return false;
     }
 

src/tiff.imageio/tiffinput.cpp

@@ -113,13 +113,13 @@ class TIFFInput final : public ImageInput {
     {
         // If m_emulate_mipmap is true, pretend subimages are mipmap levels
         lock_guard lock(*this);
-        return m_emulate_mipmap ? 0 : m_subimage;
+        return m_subimage;
     }
     int current_miplevel(void) const override
     {
         // If m_emulate_mipmap is true, pretend subimages are mipmap levels
         lock_guard lock(*this);
-        return m_emulate_mipmap ? m_subimage : 0;
+        return m_miplevel;
     }
     bool seek_subimage(int subimage, int miplevel) override;
     ImageSpec spec(int subimage, int miplevel) override;
@@ -173,7 +173,9 @@ class TIFFInput final : public ImageInput {
     std::string m_filename;                 ///< Stash the filename
     std::vector<unsigned char> m_scratch;   ///< Scratch space for us to use
     std::vector<unsigned char> m_scratch2;  ///< More scratch
-    int m_subimage;           ///< What subimage are we looking at?
+    int m_subimage;           ///< What subimage do we think we're on?
+    int m_miplevel;           ///< Which mip level do we think we're on?
+    int m_actual_subimage;    ///< Actual subimage we're on
     int m_next_scanline;      ///< Next scanline we'll read, relative to ymin
     bool m_no_random_access;  ///< Should we avoid random access?
     bool m_emulate_mipmap;    ///< Should we emulate mip with subimage?
@@ -202,6 +204,8 @@ class TIFFInput final : public ImageInput {
     {
         m_tif                     = NULL;
         m_subimage                = -1;
+        m_miplevel                = -1;
+        m_actual_subimage         = -1;
         m_emulate_mipmap          = false;
         m_keep_unassociated_alpha = false;
         m_raw_color               = false;
@@ -742,8 +746,10 @@ TIFFInput::valid_file(Filesystem::IOProxy* ioproxy) const
 bool
 TIFFInput::open(const std::string& name, ImageSpec& newspec)
 {
-    m_filename = name;
-    m_subimage = -1;
+    m_filename        = name;
+    m_subimage        = -1;
+    m_miplevel        = -1;
+    m_actual_subimage = -1;
 
     bool ok = seek_subimage(0, 0);
     newspec = spec();
@@ -775,8 +781,14 @@ TIFFInput::open(const std::string& name, ImageSpec& newspec,
 bool
 TIFFInput::seek_subimage(int subimage, int miplevel)
 {
-    if (subimage < 0)  // Illegal
+    if (subimage == m_subimage && miplevel == m_miplevel) {
+        // We're already pointing to the right subimage
+        return true;
+    }
+
+    if (subimage < 0 || miplevel < 0)  // Illegal
         return false;
+    int orig_subimage = subimage;  // the original request
     if (m_emulate_mipmap) {
         // Emulating MIPmap?  Pretend one subimage, many MIP levels.
         if (subimage != 0)
@@ -788,15 +800,12 @@ TIFFInput::seek_subimage(int subimage, int miplevel)
             return false;
     }
 
-    if (subimage == m_subimage) {
-        // We're already pointing to the right subimage
-        return true;
-    }
-
     // If we're emulating a MIPmap, only resolution is allowed to change
     // between MIP levels, so if we already have a valid level in m_spec,
     // we don't need to re-parse metadata, it's guaranteed to be the same.
-    bool read_meta = !(m_emulate_mipmap && m_tif && m_subimage >= 0);
+    bool read_meta = true;
+    if (m_emulate_mipmap && m_tif && m_miplevel >= 0)
+        read_meta = false;
 
     if (!m_tif) {
 #if OIIO_TIFFLIB_VERSION >= 40500
@@ -850,12 +859,12 @@ TIFFInput::seek_subimage(int subimage, int miplevel)
             return false;
         }
         m_is_byte_swapped = TIFFIsByteSwapped(m_tif);
-        m_subimage        = 0;
+        m_actual_subimage = 0;
     }
 
     m_next_scanline = 0;  // next scanline we'll read
-    if (subimage == m_subimage || TIFFSetDirectory(m_tif, subimage)) {
-        m_subimage = subimage;
+    if (subimage == m_actual_subimage || TIFFSetDirectory(m_tif, subimage)) {
+        m_actual_subimage = subimage;
         if (!readspec(read_meta))
             return false;
 
@@ -878,11 +887,17 @@ TIFFInput::seek_subimage(int subimage, int miplevel)
         if (!check_open(m_spec,
                         { 0, 1 << 20, 0, 1 << 20, 0, 1 << 16, 0, 1 << 16 }))
             return false;
+        m_subimage = orig_subimage;
+        m_miplevel = miplevel;
         return true;
     } else {
         std::string e = oiio_tiff_last_error();
-        errorfmt("Err: {}", e.length() ? e : m_filename);
-        m_subimage = -1;
+        errorfmt("could not seek to {} {}",
+                 m_emulate_mipmap ? "miplevel" : "subimage", subimage,
+                 e.length() ? ": " : "", e.length() ? e : std::string(""));
+        m_subimage        = -1;
+        m_miplevel        = -1;
+        m_actual_subimage = -1;
         return false;
     }
 }
@@ -905,6 +920,10 @@ TIFFInput::spec(int subimage, int miplevel)
         // Index into the spec list by miplevel instead, because that's
         // what it really contains.
         s = miplevel;
+    } else {
+        // Not emulating MIP levels -> there are none
+        if (miplevel)
+            return ret;
     }
 
     lock_guard lock(*this);
@@ -937,6 +956,10 @@ TIFFInput::spec_dimensions(int subimage, int miplevel)
         // Index into the spec list by miplevel instead, because that's
         // what it really contains.
         s = miplevel;
+    } else {
+        // Not emulating MIP levels -> there are none
+        if (miplevel)
+            return ret;
     }
 
     lock_guard lock(*this);
@@ -1298,7 +1321,7 @@ TIFFInput::readspec(bool read_meta)
         }
         // TIFFReadEXIFDirectory seems to do something to the internal state
         // that requires a TIFFSetDirectory to set things straight again.
-        TIFFSetDirectory(m_tif, m_subimage);
+        TIFFSetDirectory(m_tif, m_actual_subimage);
     }
 
     // Search for IPTC metadata in IIM form -- but older versions of

testsuite/jpeg2000/ref/out.txt

@@ -4,3 +4,5 @@ Comparing "../oiio-images/jpeg2000/broken/issue_3427.jp2" and "issue_3427.jp2"
 idiff ERROR: Could not read ../oiio-images/jpeg2000/broken/issue_3427.jp2:
 	Invalid image file "../oiio-images/jpeg2000/broken/issue_3427.jp2": Tile part length size inconsistent with stream length
 Failed to decode the codestream in the JP2 file
+Invalid image file "../oiio-images/jpeg2000/broken/issue_3427.jp2": Tile part length size inconsistent with stream length
+Failed to decode the codestream in the JP2 file

testsuite/png-damaged/ref/out.txt

@@ -9,3 +9,5 @@ libpng error: tEXt: Read error: hit end of file in png reader
 idiff ERROR: Could not read invalid_gray_alpha_sbit.png:
 	Invalid image file "invalid_gray_alpha_sbit.png": Read error: hit end of file in png reader
 PNG read error: tEXt: Read error: hit end of file in png reader
+Invalid image file "invalid_gray_alpha_sbit.png": Read error: hit end of file in png reader
+PNG read error: tEXt: Read error: hit end of file in png reader