api: use shared_ptr for ImageCache and TextureSystem (#4377)

Change IC::create() and TS::create() to return shared_ptr instead of a
raw pointer. This cleans up a lot of lingering lifetime management issue
of these classes. The switch to 3.0 is an opportunity to make breaking
changes to these APIs.

For the sake of downstream users, we define preprocessor symbols
OIIO_IMAGECACHE_CREATE_SHARED and OIIO_TEXTURESYSTEM_CREATE_SHARED to
signal that the new APIs are present. Some very minor changes may be
needed at the call site.

---------

Signed-off-by: Larry Gritz <[email protected]>

src/iconvert/iconvert.cpp

@@ -356,8 +356,8 @@ convert_file(const std::string& in_filename, const std::string& out_filename)
     // subimage appending, we gather them all first.
     std::vector<ImageSpec> subimagespecs;
     if (out->supports("multiimage") && !out->supports("appendsubimage")) {
-        ImageCache* imagecache = ImageCache::create();
-        int nsubimages         = 0;
+        auto imagecache = ImageCache::create();
+        int nsubimages  = 0;
         ustring ufilename(in_filename);
         imagecache->get_image_info(ufilename, 0, 0, ustring("subimages"),
                                    TypeInt, &nsubimages);
@@ -370,7 +370,6 @@ convert_file(const std::string& in_filename, const std::string& out_filename)
                 adjust_spec(in.get(), out.get(), inspec, subimagespecs[i]);
             }
         }
-        ImageCache::destroy(imagecache);
     }
 
     bool ok                      = true;

src/idiff/idiff.cpp

@@ -119,8 +119,9 @@ getargs(int argc, char* argv[])
 
 
 static bool
-read_input(const std::string& filename, ImageBuf& img, ImageCache* cache,
-           int subimage = 0, int miplevel = 0)
+read_input(const std::string& filename, ImageBuf& img,
+           std::shared_ptr<ImageCache> cache, int subimage = 0,
+           int miplevel = 0)
 {
     if (img.subimage() >= 0 && img.subimage() == subimage
         && img.miplevel() == miplevel)
@@ -232,7 +233,7 @@ main(int argc, char* argv[])
 
     // Create a private ImageCache so we can customize its cache size
     // and instruct it store everything internally as floats.
-    ImageCache* imagecache = ImageCache::create(true);
+    std::shared_ptr<ImageCache> imagecache = ImageCache::create(true);
     imagecache->attribute("forcefloat", 1);
     if (sizeof(void*) == 4)  // 32 bit or 64?
         imagecache->attribute("max_memory_MB", 512.0);
@@ -418,7 +419,6 @@ main(int argc, char* argv[])
     }
 
     imagecache->invalidate_all(true);
-    ImageCache::destroy(imagecache);
     shutdown();
     return ret;
 }

src/include/OpenImageIO/imagebuf.h

@@ -77,7 +77,7 @@ enum class InitializePixels { No = 0, Yes = 1 };
 /// translate into a different data format than appears in the file).
 ///
 /// ImageBuf data coming from disk files may optionally be backed by
-/// ImageCache, by explicitly passing an `ImageCache*` to the ImageBuf
+/// ImageCache, by explicitly passing an ImageCache to the ImageBuf
 /// constructor or `reset()` method (pass `ImageCache::create()` to get a
 /// pointer to the default global ImageCache), or by having previously set the
 /// global OIIO attribute `"imagebuf:use_imagecache"` to a nonzero value. When
@@ -186,9 +186,9 @@ class OIIO_API ImageBuf {
     ///         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,
-                      const ImageSpec* config      = nullptr,
-                      Filesystem::IOProxy* ioproxy = nullptr);
+                      std::shared_ptr<ImageCache> imagecache = {},
+                      const ImageSpec* config                = nullptr,
+                      Filesystem::IOProxy* ioproxy           = nullptr);
 
     /// Construct a writable ImageBuf with the given specification
     /// (including resolution, data type, metadata, etc.). The ImageBuf will
@@ -270,9 +270,9 @@ class OIIO_API ImageBuf {
     /// as if newly constructed with the same arguments, as a read-only
     /// representation of an existing image file.
     void reset(string_view name, int subimage = 0, int miplevel = 0,
-               ImageCache* imagecache       = nullptr,
-               const ImageSpec* config      = nullptr,
-               Filesystem::IOProxy* ioproxy = nullptr);
+               std::shared_ptr<ImageCache> imagecache = {},
+               const ImageSpec* config                = nullptr,
+               Filesystem::IOProxy* ioproxy           = nullptr);
 
     /// Destroy any previous contents of the ImageBuf and re-initialize it
     /// as if newly constructed with the same arguments, as a read/write
@@ -1005,9 +1005,9 @@ class OIIO_API ImageBuf {
     /// image being in RAM somewhere?
     bool cachedpixels() const;
 
-    /// A pointer to the underlying ImageCache, or nullptr if this ImageBuf
-    /// is not backed by an ImageCache.
-    ImageCache* imagecache() const;
+    /// A shared pointer to the underlying ImageCache, or empty if this
+    /// ImageBuf is not backed by an ImageCache.
+    std::shared_ptr<ImageCache> imagecache() const;
 
     /// Return the address where pixel `(x,y,z)`, channel `ch`, is stored in
     /// the image buffer.  Use with extreme caution!  Will return `nullptr`
@@ -1151,7 +1151,7 @@ class OIIO_API ImageBuf {
     // Deprecated things -- might be removed at any time
 
     OIIO_DEPRECATED("Use `ImageBuf(name, 0, 0, imagecache, nullptr)` (2.2)")
-    ImageBuf(string_view name, ImageCache* imagecache)
+    ImageBuf(string_view name, std::shared_ptr<ImageCache> imagecache)
         : ImageBuf(name, 0, 0, imagecache)
     {
     }
@@ -1164,7 +1164,7 @@ class OIIO_API ImageBuf {
     }
 
     OIIO_DEPRECATED("Use `reset(name, 0, 0, imagecache)` (2.2)")
-    void reset(string_view name, ImageCache* imagecache)
+    void reset(string_view name, std::shared_ptr<ImageCache> imagecache)
     {
         reset(name, 0, 0, imagecache);
     }

src/include/OpenImageIO/imagecache.h

@@ -24,6 +24,9 @@
 // Does invalidate() support the optional `force` flag?
 #define OIIO_IMAGECACHE_INVALIDATE_FORCE 1
 
+// Does ImageCache::create() return a shared pointer?
+#define OIIO_IMAGECACHE_CREATE_SHARED 1
+
 
 
 OIIO_NAMESPACE_BEGIN
@@ -56,8 +59,7 @@ class OIIO_API ImageCache {
     /// or destroy the concrete implementation, so two static methods of
     /// ImageCache are provided:
 
-    /// Create a ImageCache and return a raw pointer to it.  This should
-    /// only be freed by passing it to `ImageCache::destroy()`!
+    /// Create a ImageCache and return a shared pointer to it.
     ///
     /// @param  shared
     ///     If `true`, the pointer returned will be a shared ImageCache (so
@@ -66,30 +68,28 @@ class OIIO_API ImageCache {
     ///     completely unique ImageCache will be created and returned.
     ///
     /// @returns
-    ///     A raw pointer to an ImageCache, which can only be freed with
+    ///     A shared pointer to an ImageCache, which can only be freed with
     ///     `ImageCache::destroy()`.
     ///
     /// @see    ImageCache::destroy
-    static ImageCache* create(bool shared = true);
+    static std::shared_ptr<ImageCache> create(bool shared = true);
 
-    /// Destroy an allocated ImageCache, including freeing all system
-    /// resources that it holds.
-    ///
-    /// It is safe to destroy even a shared ImageCache, as the implementation
-    /// of `destroy()` will recognize a shared one and only truly release
-    /// its resources if it has been requested to be destroyed as many times as
-    /// shared ImageCache's were created.
+    /// Release the shared_ptr to an ImageCache, including freeing all
+    /// system resources that it holds if no one else is still using it. This
+    /// is not strictly necessary to call, simply destroying the shared_ptr
+    /// will do the same thing, but this call is for backward compatibility
+    /// and is helpful if you want to use the teardown option.
     ///
     /// @param  cache
-    ///     Raw pointer to the ImageCache to destroy.
+    ///     Shared pointer to the ImageCache to destroy.
     ///
     /// @param  teardown
     ///     For a shared ImageCache, if the `teardown` parameter is
     ///     `true`, it will try to truly destroy the shared cache if
     ///     nobody else is still holding a reference (otherwise, it will
     ///     leave it intact). This parameter has no effect if `cache` was
     ///     not the single globally shared ImageCache.
-    static void destroy(ImageCache* cache, bool teardown = false);
+    static void destroy(std::shared_ptr<ImageCache>& cache, bool teardown = false);
 
     /// @}
 

src/include/OpenImageIO/texture.h

@@ -27,6 +27,9 @@
 #define OIIO_TEXTURESYSTEM_SUPPORTS_STOCHASTIC 1
 #define OIIO_TEXTURESYSTEM_SUPPORTS_DECODE_BY_USTRINGHASH 1
 
+// Does TextureSystem::create() return a shared pointer?
+#define OIIO_TEXTURESYSTEM_CREATE_SHARED 1
+
 #ifndef INCLUDED_IMATHVEC_H
 // Placeholder declaration for Imath::V3f if no Imath headers have been
 // included.
@@ -365,8 +368,7 @@ class OIIO_API TextureSystem {
     /// or destroy the concrete implementation, so two static methods of
     /// TextureSystem are provided:
 
-    /// Create a TextureSystem and return a pointer to it.  This should only
-    /// be freed by passing it to TextureSystem::destroy()!
+    /// Create a TextureSystem and return a shared pointer to it.
     ///
     /// @param  shared
     ///     If `shared` is `true`, the pointer returned will be a shared
@@ -376,41 +378,39 @@ class OIIO_API TextureSystem {
     ///     completely unique TextureCache will be created and returned.
     ///
     /// @param  imagecache
-    ///     If `shared` is `false` and `imagecache` is not `nullptr`, the
+    ///     If `shared` is `false` and `imagecache` is not empty, the
     ///     TextureSystem will use this as its underlying ImageCache. In
     ///     that case, it is the caller who is responsible for eventually
     ///     freeing the ImageCache after the TextureSystem is destroyed.  If
-    ///     `shared` is `false` and `imagecache` is `nullptr`, then a custom
+    ///     `shared` is `false` and `imagecache` is empty, then a custom
     ///     ImageCache will be created, owned by the TextureSystem, and
     ///     automatically freed when the TS destroys.
     ///
     /// @returns
-    ///     A raw pointer to a TextureSystem, which can only be freed with
-    ///     `TextureSystem::destroy()`.
+    ///     A shared pointer to a TextureSystem which will be destroyed only
+    ///     when the last shared_ptr to it is destroyed.
     ///
     /// @see    TextureSystem::destroy
-    static TextureSystem *create (bool shared=true,
-                                  ImageCache *imagecache=nullptr);
+    static std::shared_ptr<TextureSystem> create(bool shared=true,
+                                 std::shared_ptr<ImageCache> imagecache = {});
 
-    /// Destroy an allocated TextureSystem, including freeing all system
-    /// resources that it holds.
-    ///
-    /// It is safe to destroy even a shared TextureSystem, as the
-    /// implementation of `destroy()` will recognize a shared one and only
-    /// truly release its resources if it has been requested to be destroyed
-    /// as many times as shared TextureSystem's were created.
+    /// Release the shared_ptr to a TextureSystem, including freeing all
+    /// system resources that it holds if no one else is still using it. This
+    /// is not strictly necessary to call, simply destroying the shared_ptr
+    /// will do the same thing, but this call is for backward compatibility
+    /// and is helpful if you want to use the teardown_imagecache option.
     ///
     /// @param  ts
-    ///     Raw pointer to the TextureSystem to destroy.
+    ///     Shared pointer to the TextureSystem to destroy.
     ///
     /// @param  teardown_imagecache
     ///     For a shared TextureSystem, if the `teardown_imagecache`
     ///     parameter is `true`, it will try to truly destroy the shared
     ///     cache if nobody else is still holding a reference (otherwise, it
     ///     will leave it intact). This parameter has no effect if `ts` was
     ///     not the single globally shared TextureSystem.
-    static void destroy (TextureSystem *ts,
-                         bool teardown_imagecache = false);
+    static void destroy(std::shared_ptr<TextureSystem>& ts,
+                        bool teardown_imagecache = false);
 
     /// @}
 
@@ -1652,7 +1652,7 @@ class OIIO_API TextureSystem {
 
     /// Return an opaque, non-owning pointer to the underlying ImageCache
     /// (if there is one).
-    virtual ImageCache *imagecache () const = 0;
+    virtual std::shared_ptr<ImageCache> imagecache() const = 0;
 
     virtual ~TextureSystem () { }
 

src/iv/imageviewer.cpp

@@ -791,7 +791,7 @@ ImageViewer::readSettings(bool ui_is_set_up)
 
     OIIO::attribute("imagebuf:use_imagecache", 1);
 
-    ImageCache* imagecache = ImageCache::create(true);
+    auto imagecache = ImageCache::create(true);
     imagecache->attribute("automip", autoMipmap->isChecked());
     imagecache->attribute("max_memory_MB", (float)maxMemoryIC->value());
 }

src/iv/ivmain.cpp

@@ -129,7 +129,7 @@ main(int argc, char* argv[])
     mainWin->show();
 
     // Set up the imagecache with parameters that make sense for iv
-    ImageCache* imagecache = ImageCache::create(true);
+    auto imagecache = ImageCache::create(true);
     imagecache->attribute("autotile", 256);
     imagecache->attribute("deduplicate", (int)0);
     if (ap["no-autopremult"].get<int>())