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

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