diff --git a/src/include/OpenImageIO/imagebuf.h b/src/include/OpenImageIO/imagebuf.h index c6a897d01b..8f7c6ee38b 100644 --- a/src/include/OpenImageIO/imagebuf.h +++ b/src/include/OpenImageIO/imagebuf.h @@ -189,7 +189,8 @@ class OIIO_API ImageBuf { /// to the opening and reading of the file. /// @param ioproxy /// Optional pointer to an IOProxy to use when reading from the - /// file. The caller retains ownership of the proxy. + /// file. The caller retains ownership of the proxy, and must + /// ensure that it remains valid for the lifetime of the ImageBuf. /// explicit ImageBuf(string_view name, int subimage = 0, int miplevel = 0, ImageCache* imagecache = nullptr, @@ -300,8 +301,8 @@ class OIIO_API ImageBuf { void reset(const ImageSpec& spec, InitializePixels zero = InitializePixels::Yes); - // Deprecated/useless synonym for `reset(spec, spec, zero)` and also - // give it an internal name. + // Deprecated/useless synonym for `reset(spec, zero)` and also give it an + // internal name. void reset(string_view name, const ImageSpec& spec, InitializePixels zero = InitializePixels::Yes); diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index b4da803712..b71b817832 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -701,9 +701,13 @@ ImageBuf::storage() const void ImageBufImpl::clear() { - if (storage() == ImageBuf::IMAGECACHE && m_imagecache && !m_name.empty()) { + if (m_imagecache && !m_name.empty() + && (storage() == ImageBuf::IMAGECACHE || m_rioproxy)) { + // If we were backed by an ImageCache, invalidate any IC entries we + // might have made. Also do so if we were using an IOProxy, because + // the proxy may not survive long after the ImageBuf is destroyed. m_imagecache->close(m_name); - invalidate(m_name, false); + m_imagecache->invalidate(m_name, false); } free_pixels(); m_name.clear(); @@ -926,7 +930,6 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel, // info for the file just in case it has changed on disk. if (!m_imagecache) m_imagecache = ImageCache::create(true /* shared cache */); - invalidate(m_name, false); m_pixels_valid = false; m_nsubimages = 0; @@ -939,6 +942,9 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel, m_imagecache->invalidate(m_name, true); m_imagecache->add_file(m_name, nullptr, m_configspec.get(), /*replace=*/true); + } else { + // 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); @@ -1153,6 +1159,25 @@ ImageBufImpl::read(int subimage, int miplevel, int chbegin, int chend, m_pixels_valid = false; error(OIIO::geterror()); } + // Since we have read in the entire image now, if we are using an + // IOProxy, we invalidate any cache entry to avoid lifetime issues + // related to the IOProxy. This helps to eliminate trouble emerging + // from the following idiom that looks totally reasonable to the user + // but is actually a recipe for disaster: + // IOProxy proxy(...); // temporary proxy + // ImageBuf A ("foo.exr", 0, 0, proxy); + // // ^^ now theres an IC entry that knows the proxy. + // A.read (0, 0, true); + // // ^^ looks like a forced immediate read, user thinks + // // they are done with the ImageBuf, but there's + // // STILL a cache entry that knows the proxy. + // proxy.close(); + // // ^^ now the proxy is gone, which seemed safe because + // // the user thinks the forced immediate read was the + // // last it'll be needed. But the cache entry still + // // has a pointer to it! Oh no! + if (m_rioproxy) + m_imagecache->invalidate(m_name); return m_pixels_valid; } @@ -1246,7 +1271,7 @@ ImageBuf::write(ImageOutput* out, ProgressCallback progress_callback, ok &= m_impl->validate_pixels(); if (out->supports("thumbnail") && has_thumbnail()) { auto thumb = get_thumbnail(); - Strutil::print("IB::write: has thumbnail ROI {}\n", thumb->roi()); + // Strutil::print("IB::write: has thumbnail ROI {}\n", thumb->roi()); out->set_thumbnail(*thumb); } const ImageSpec& bufspec(m_impl->m_spec); diff --git a/src/libtexture/imagecache_pvt.h b/src/libtexture/imagecache_pvt.h index 297d5f198a..2828ca9bc3 100644 --- a/src/libtexture/imagecache_pvt.h +++ b/src/libtexture/imagecache_pvt.h @@ -372,7 +372,7 @@ class OIIO_API ImageCacheFile final : public RefCnt { // Note that m_input, the shared pointer itself, is NOT safe to // access directly. ALWAYS retrieve its value with get_imageinput // (it's thread-safe to use that result) and set its value with - // get_imageinput -- those are guaranteed thread-safe. + // set_imageinput -- those are guaranteed thread-safe. std::vector m_subimages; ///< Info on each subimage TexFormat m_texformat; ///< Which texture format TextureOpt::Wrap m_swrap; ///< Default wrap modes