diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h index de9672b86d..9150ee8eb6 100644 --- a/src/include/OpenImageIO/imageio.h +++ b/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 diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index 0252a059ae..f600fcac90 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/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 + 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); diff --git a/src/openexr.imageio/exrinput.cpp b/src/openexr.imageio/exrinput.cpp index f4126a39ea..469f7d5894 100644 --- a/src/openexr.imageio/exrinput.cpp +++ b/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; } diff --git a/src/openexr.imageio/exrinput_c.cpp b/src/openexr.imageio/exrinput_c.cpp index c2613c1f04..18352aaa31 100644 --- a/src/openexr.imageio/exrinput_c.cpp +++ b/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 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; } diff --git a/src/tiff.imageio/tiffinput.cpp b/src/tiff.imageio/tiffinput.cpp index 2e774f2c8b..6f12aba102 100644 --- a/src/tiff.imageio/tiffinput.cpp +++ b/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 m_scratch; ///< Scratch space for us to use std::vector 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 diff --git a/testsuite/jpeg2000/ref/out.txt b/testsuite/jpeg2000/ref/out.txt index dd8afc9bb7..49df15bdff 100644 --- a/testsuite/jpeg2000/ref/out.txt +++ b/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 diff --git a/testsuite/png-damaged/ref/out.txt b/testsuite/png-damaged/ref/out.txt index 7f276f6bc9..0c2636e3d3 100644 --- a/testsuite/png-damaged/ref/out.txt +++ b/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 diff --git a/testsuite/python-imagebuf/ref/out-alt-python3.txt b/testsuite/python-imagebuf/ref/out-alt-python3.txt index a1fb5ec987..cb2f545958 100644 --- a/testsuite/python-imagebuf/ref/out-alt-python3.txt +++ b/testsuite/python-imagebuf/ref/out-alt-python3.txt @@ -234,6 +234,17 @@ Testing metadata copying camera:x = "Bx" camera:y = "By" +Testing error handling for out-of-range subimage, miplevel + bayer.png subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + bayer.png subimage 0 mip 0: True + bayer.png subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + grid-small.exr subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + grid-small.exr subimage 0 mip 0: True + grid-small.exr subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + tahoe-tiny.tif subimage 1 mip 0: False Error reading: could not seek to subimage 1 + tahoe-tiny.tif subimage 0 mip 0: True + tahoe-tiny.tif subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + Done. Comparing "out.tif" and "ref/out.tif" PASS diff --git a/testsuite/python-imagebuf/ref/out-alt.txt b/testsuite/python-imagebuf/ref/out-alt.txt index c2bc853791..de8a31b16b 100644 --- a/testsuite/python-imagebuf/ref/out-alt.txt +++ b/testsuite/python-imagebuf/ref/out-alt.txt @@ -234,6 +234,17 @@ Testing metadata copying camera:x = "Bx" camera:y = "By" +Testing error handling for out-of-range subimage, miplevel + bayer.png subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + bayer.png subimage 0 mip 0: True + bayer.png subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + grid-small.exr subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + grid-small.exr subimage 0 mip 0: True + grid-small.exr subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + tahoe-tiny.tif subimage 1 mip 0: False Error reading: could not seek to subimage 1 + tahoe-tiny.tif subimage 0 mip 0: True + tahoe-tiny.tif subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + Done. Comparing "out.tif" and "ref/out.tif" PASS diff --git a/testsuite/python-imagebuf/ref/out-python3.txt b/testsuite/python-imagebuf/ref/out-python3.txt index a1fb5ec987..cb2f545958 100644 --- a/testsuite/python-imagebuf/ref/out-python3.txt +++ b/testsuite/python-imagebuf/ref/out-python3.txt @@ -234,6 +234,17 @@ Testing metadata copying camera:x = "Bx" camera:y = "By" +Testing error handling for out-of-range subimage, miplevel + bayer.png subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + bayer.png subimage 0 mip 0: True + bayer.png subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + grid-small.exr subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + grid-small.exr subimage 0 mip 0: True + grid-small.exr subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + tahoe-tiny.tif subimage 1 mip 0: False Error reading: could not seek to subimage 1 + tahoe-tiny.tif subimage 0 mip 0: True + tahoe-tiny.tif subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + Done. Comparing "out.tif" and "ref/out.tif" PASS diff --git a/testsuite/python-imagebuf/ref/out.txt b/testsuite/python-imagebuf/ref/out.txt index c2bc853791..de8a31b16b 100644 --- a/testsuite/python-imagebuf/ref/out.txt +++ b/testsuite/python-imagebuf/ref/out.txt @@ -234,6 +234,17 @@ Testing metadata copying camera:x = "Bx" camera:y = "By" +Testing error handling for out-of-range subimage, miplevel + bayer.png subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + bayer.png subimage 0 mip 0: True + bayer.png subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + grid-small.exr subimage 1 mip 0: False Could not seek to subimage=1 miplevel=0 + grid-small.exr subimage 0 mip 0: True + grid-small.exr subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + tahoe-tiny.tif subimage 1 mip 0: False Error reading: could not seek to subimage 1 + tahoe-tiny.tif subimage 0 mip 0: True + tahoe-tiny.tif subimage 0 mip 1: False Could not seek to subimage=0 miplevel=1 + Done. Comparing "out.tif" and "ref/out.tif" PASS diff --git a/testsuite/python-imagebuf/src/test_imagebuf.py b/testsuite/python-imagebuf/src/test_imagebuf.py index fc0021b758..789efb3724 100755 --- a/testsuite/python-imagebuf/src/test_imagebuf.py +++ b/testsuite/python-imagebuf/src/test_imagebuf.py @@ -205,6 +205,23 @@ def test_copy_metadata() : print_imagespec (C.spec(), msg=" result of A.merge_metadata(B, pattern='^camera:'):") +# Test proper error handling for asking for out-of-range subimages or MIP levels. +# Test with one format that supports neither subimages nor MIP levels, +# and with one format that supports them, but the file we try has none. +def test_outofrange_subimage_miplevel() : + print("\nTesting error handling for out-of-range subimage, miplevel") + for f in [ "bayer.png", "grid-small.exr", "tahoe-tiny.tif" ] : + buf = oiio.ImageBuf() + ok = buf.init_spec("../common/" + f, 1, 0) + print(" ", f, "subimage 1 mip 0:", ok, buf.geterror()) + buf = oiio.ImageBuf() + ok = buf.init_spec("../common/" + f, 0, 0) + print(" ", f, "subimage 0 mip 0:", ok, buf.geterror()) + buf = oiio.ImageBuf() + ok = buf.init_spec("../common/" + f, 0, 1) + print(" ", f, "subimage 0 mip 1:", ok, buf.geterror()) + + ###################################################################### # main test starts here @@ -316,6 +333,7 @@ def test_copy_metadata() : test_uninitialized () test_copy_metadata () test_repr_png () + test_outofrange_subimage_miplevel () print ("\nDone.") except Exception as detail: