ImageBuf+IOProxy vs ImageCache subtle fixes (#3666)

I uncovered a very subtle problem that can happen with the interaction
between ImageBuf, using an IOProxy, and the underlying ImageCache.
Remember that when using an IOProxy, it is critical that the lifetime
of the proxy outlasts any ImageInput, ImageBuf, or ImageCache that was
told about it.

Consider the following code:

    IOProxy proxy(...);   // temporary proxy in this local scope
    ImageBuf A ("foo.exr", 0, 0, proxy);

We've set up an ImageBuf that will read from an IOProxy.  Behind the
scenes, it made an ImageCache entry that remembers the IOProxy, as it
has to, since it needs to use that proxy for subsequent reads.
Now we read the image, and we use the `force=true` parameter to
force an immediate read of the file:

    A.read (0, 0, true);

It reads from the proxy. But the IB and the underlying entry in the IC
still know about the proxy. But in the user's mind, they have fully
read the image, will never read from it again, so of course (they
think) it is safe to close or destroy the proxy, knowing that they
made it last past its use:

    proxy.close();

But -- oops! -- the cache entry still has a pointer to the proxy, and
even though it won't need to read from it again, it still will access
it, close it, etc., but the user's proxy may by that point be out of
scope.

This is *technically* not a "bug" in the sense that it is the user who
violated the contract by closing or destroying the proxy before its
last use by anything they passed it to. But in context, it's very easy
to be confused by this -- the user thinks that the only observer of
the proxy is the ImageBuf, and they're done using the ImageBuf at that
point, but because they don't explicitly mention an ImageCache, they
don't realize that the ImageBuf passed the proxy reference along to
the underlying IC that could be backing any ImageBuf.

It's the user's fault in some sense, but let's just say that we have
given them an "attractive nuisance."

That's a long bit of background explanation, but I wanted to force
myself to explain it clearly and make sure it was spelled out in the
commit comments. Now here is the fix:

* When IB::read does an immediate forced read involving an IOProxy,
  after the read is complete, tell the cache to invalidate the entry
  for the file so that it no longer retains a pointer to the proxy.

* When an IB itself is destroyed or cleared, if it used an IOProxy,
  also invalidate the IC entry for that file.

* Unrelated cleanup: avoid a redundant invalidate call in init_spec().

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

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

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<SubimageInfo> m_subimages;  ///< Info on each subimage
     TexFormat m_texformat;                  ///< Which texture format
     TextureOpt::Wrap m_swrap;               ///< Default wrap modes