From e37deb97fd1dff4167ca0d169406031bedcd64d4 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Mon, 12 Aug 2024 13:42:15 -0700 Subject: [PATCH 1/3] api: use shared_ptr for ImageCache and TextureSystem 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 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 --- src/iconvert/iconvert.cpp | 5 +-- src/idiff/idiff.cpp | 9 +++-- src/include/OpenImageIO/imagebuf.h | 24 ++++++------ src/include/OpenImageIO/imagecache.h | 26 ++++++------- src/include/OpenImageIO/texture.h | 38 +++++++++--------- src/iv/imageviewer.cpp | 2 +- src/iv/ivmain.cpp | 2 +- src/libOpenImageIO/imagebuf.cpp | 37 +++++++++--------- src/libOpenImageIO/imagebuf_test.cpp | 3 +- src/libOpenImageIO/imagecache_test.cpp | 20 ++++------ src/libOpenImageIO/imagespeed_test.cpp | 3 +- src/libtexture/imagecache.cpp | 34 ++++++----------- src/libtexture/texture_pvt.h | 8 +++- src/libtexture/texturesys.cpp | 53 ++++++++++---------------- src/maketx/maketx.cpp | 4 +- src/oiiotool/imagerec.cpp | 2 +- src/oiiotool/oiiotool.h | 8 ++-- src/python/py_imagecache.cpp | 7 +--- src/python/py_texturesys.cpp | 9 ++--- src/testtex/testtex.cpp | 4 +- 20 files changed, 133 insertions(+), 165 deletions(-) diff --git a/src/iconvert/iconvert.cpp b/src/iconvert/iconvert.cpp index 66b513c381..239b74a11c 100644 --- a/src/iconvert/iconvert.cpp +++ b/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 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; diff --git a/src/idiff/idiff.cpp b/src/idiff/idiff.cpp index 814d854f12..a4b927e221 100644 --- a/src/idiff/idiff.cpp +++ b/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 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::create(true); imagecache->attribute("forcefloat", 1); if (sizeof(void*) == 4) // 32 bit or 64? imagecache->attribute("max_memory_MB", 512.0); @@ -418,7 +419,7 @@ main(int argc, char* argv[]) } imagecache->invalidate_all(true); - ImageCache::destroy(imagecache); + imagecache.reset(); shutdown(); return ret; } diff --git a/src/include/OpenImageIO/imagebuf.h b/src/include/OpenImageIO/imagebuf.h index de9b13a052..61946a17a4 100644 --- a/src/include/OpenImageIO/imagebuf.h +++ b/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 = {}, + 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 = {}, + 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() 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) : 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) { reset(name, 0, 0, imagecache); } diff --git a/src/include/OpenImageIO/imagecache.h b/src/include/OpenImageIO/imagecache.h index 2cb7bb89ad..63ff62c491 100644 --- a/src/include/OpenImageIO/imagecache.h +++ b/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,22 +68,20 @@ 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 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 @@ -89,7 +89,7 @@ class OIIO_API ImageCache { /// 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& cache, bool teardown = false); /// @} diff --git a/src/include/OpenImageIO/texture.h b/src/include/OpenImageIO/texture.h index 3dea2d822f..43eba9a7f8 100644 --- a/src/include/OpenImageIO/texture.h +++ b/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,32 +378,30 @@ 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 create(bool shared=true, + std::shared_ptr 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` @@ -409,8 +409,8 @@ class OIIO_API TextureSystem { /// 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& 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() const = 0; virtual ~TextureSystem () { } diff --git a/src/iv/imageviewer.cpp b/src/iv/imageviewer.cpp index a22c5eae81..e4e3822f37 100644 --- a/src/iv/imageviewer.cpp +++ b/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()); } diff --git a/src/iv/ivmain.cpp b/src/iv/ivmain.cpp index a9c1181f55..b63892a58f 100644 --- a/src/iv/ivmain.cpp +++ b/src/iv/ivmain.cpp @@ -124,7 +124,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()) diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index 6781d7ef2a..883f388925 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -89,7 +89,7 @@ set_roi_full(ImageSpec& spec, const ROI& newroi) class ImageBufImpl { public: ImageBufImpl(string_view filename, int subimage, int miplevel, - ImageCache* imagecache = nullptr, + std::shared_ptr imagecache = {}, const ImageSpec* spec = nullptr, void* buffer = nullptr, const ImageSpec* config = nullptr, Filesystem::IOProxy* ioproxy = nullptr, @@ -100,7 +100,7 @@ class ImageBufImpl { void clear(); void reset(string_view name, int subimage, int miplevel, - ImageCache* imagecache, const ImageSpec* config, + std::shared_ptr imagecache, const ImageSpec* config, Filesystem::IOProxy* ioproxy); // Reset the buf to blank, given the spec. If nativespec is also // supplied, use it for the "native" spec, otherwise make the nativespec @@ -252,7 +252,7 @@ class ImageBufImpl { // Invalidate the file in our imagecache and the shared one void invalidate(ustring filename, bool force) { - ImageCache* shared_imagecache = ImageCache::create(true); + auto shared_imagecache = ImageCache::create(true); OIIO_DASSERT(shared_imagecache); if (m_imagecache) m_imagecache->invalidate(filename, force); // *our* IC @@ -298,11 +298,11 @@ class ImageBufImpl { stride_t m_zstride; stride_t m_channel_stride; bool m_contiguous; - ImageCache* m_imagecache = nullptr; ///< ImageCache to use - TypeDesc m_cachedpixeltype; ///< Data type stored in the cache - DeepData m_deepdata; ///< Deep data - size_t m_allocated_size; ///< How much memory we've allocated - std::vector m_blackpixel; ///< Pixel-sized zero bytes + std::shared_ptr m_imagecache; ///< ImageCache to use + TypeDesc m_cachedpixeltype; ///< Data type stored in the cache + DeepData m_deepdata; ///< Deep data + size_t m_allocated_size; ///< How much memory we've allocated + std::vector m_blackpixel; ///< Pixel-sized zero bytes std::vector m_write_format; /// Pixel data format to use for write() int m_write_tile_width; int m_write_tile_height; @@ -348,8 +348,9 @@ ImageBuf::impl_deleter(ImageBufImpl* todel) ImageBufImpl::ImageBufImpl(string_view filename, int subimage, int miplevel, - ImageCache* imagecache, const ImageSpec* spec, - void* buffer, const ImageSpec* config, + std::shared_ptr imagecache, + const ImageSpec* spec, void* buffer, + const ImageSpec* config, Filesystem::IOProxy* ioproxy, stride_t xstride, stride_t ystride, stride_t zstride) : m_storage(ImageBuf::UNINITIALIZED) @@ -503,8 +504,8 @@ ImageBuf::ImageBuf() ImageBuf::ImageBuf(string_view filename, int subimage, int miplevel, - ImageCache* imagecache, const ImageSpec* config, - Filesystem::IOProxy* ioproxy) + std::shared_ptr imagecache, + const ImageSpec* config, Filesystem::IOProxy* ioproxy) : m_impl(new ImageBufImpl(filename, subimage, miplevel, imagecache, nullptr /*spec*/, nullptr /*buffer*/, config, ioproxy), @@ -710,7 +711,7 @@ ImageBufImpl::clear() m_zstride = 0; m_channel_stride = 0; m_contiguous = false; - m_imagecache = nullptr; + m_imagecache.reset(); m_deepdata.free(); m_blackpixel.clear(); m_write_format.clear(); @@ -735,8 +736,8 @@ ImageBuf::clear() void ImageBufImpl::reset(string_view filename, int subimage, int miplevel, - ImageCache* imagecache, const ImageSpec* config, - Filesystem::IOProxy* ioproxy) + std::shared_ptr imagecache, + const ImageSpec* config, Filesystem::IOProxy* ioproxy) { clear(); m_name = ustring(filename); @@ -768,7 +769,7 @@ ImageBufImpl::reset(string_view filename, int subimage, int miplevel, void ImageBuf::reset(string_view filename, int subimage, int miplevel, - ImageCache* imagecache, const ImageSpec* config, + std::shared_ptr imagecache, const ImageSpec* config, Filesystem::IOProxy* ioproxy) { m_impl->reset(filename, subimage, miplevel, imagecache, config, ioproxy); @@ -899,7 +900,7 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel, // If we weren't given an imagecache but "imagebuf:use_imagecache" // attribute was set, use a shared IC. - if (m_imagecache == nullptr && pvt::imagebuf_use_imagecache) + if (!m_imagecache && pvt::imagebuf_use_imagecache) m_imagecache = ImageCache::create(true); if (m_imagecache) { @@ -1849,7 +1850,7 @@ ImageBuf::cachedpixels() const -ImageCache* +std::shared_ptr ImageBuf::imagecache() const { return m_impl->m_imagecache; diff --git a/src/libOpenImageIO/imagebuf_test.cpp b/src/libOpenImageIO/imagebuf_test.cpp index c9d67a5524..912a7ef6d6 100644 --- a/src/libOpenImageIO/imagebuf_test.cpp +++ b/src/libOpenImageIO/imagebuf_test.cpp @@ -306,7 +306,7 @@ test_open_with_config() { // N.B. This function must run after ImageBuf_test_appbuffer, which // writes "A.tif". - ImageCache* ic = ImageCache::create(false); + auto ic = ImageCache::create(false); ImageSpec config; config.attribute("oiio:DebugOpenConfig!", 1); ImageBuf A("A_imagebuf_test.tif", 0, 0, ic, &config); @@ -315,7 +315,6 @@ test_open_with_config() // Clear A because it would be unwise to let the ImageBuf outlive the // custom ImageCache we passed it to use. A.clear(); - ic->destroy(ic); } diff --git a/src/libOpenImageIO/imagecache_test.cpp b/src/libOpenImageIO/imagecache_test.cpp index 54432be314..f766d88ca4 100644 --- a/src/libOpenImageIO/imagecache_test.cpp +++ b/src/libOpenImageIO/imagecache_test.cpp @@ -53,7 +53,7 @@ static void test_get_pixels_errors() { Strutil::print("\nTesting get_pixels error handling\n"); - ImageCache* ic = ImageCache::create(); + std::shared_ptr ic = ImageCache::create(); float fpixels[4 * 4 * 3]; const int fpixelsize = 3 * sizeof(float); @@ -121,7 +121,7 @@ test_get_pixels_cachechannels(int chbegin = 0, int chend = 4, std::cout << "\nTesting IC get_pixels of chans [" << chbegin << "," << chend << ") with cache range [" << cache_chbegin << "," << cache_chend << "):\n"; - ImageCache* imagecache = ImageCache::create(false /*not shared*/); + auto imagecache = ImageCache::create(false); // Create a 10 channel file ustring filename("tenchannels.tif"); @@ -151,8 +151,6 @@ test_get_pixels_cachechannels(int chbegin = 0, int chend = 4, } for (int c = 2 * nc; c < 2 * nchans; ++c) OIIO_CHECK_EQUAL(p[c], -1.0f); - - ImageCache::destroy(imagecache); } @@ -172,7 +170,7 @@ NullInputCreator() void test_app_buffer() { - ImageCache* imagecache = ImageCache::create(false /*not shared*/); + auto imagecache = ImageCache::create(false /*not shared*/); // Add a file entry with a "null" ImageInput proxy configured to look // like a 2x2 RGB float image. @@ -226,8 +224,6 @@ test_app_buffer() OIIO_CHECK_EQUAL(testpixel[0], pixels[1][1][0]); OIIO_CHECK_EQUAL(testpixel[1], pixels[1][1][1]); OIIO_CHECK_EQUAL(testpixel[2], pixels[1][1][2]); - - ImageCache::destroy(imagecache); } @@ -236,8 +232,8 @@ void test_custom_threadinfo() { Strutil::print("\nTesting creating/destroying custom IC and thread info\n"); - ImageCache* imagecache = ImageCache::create(true); - auto threadinfo = imagecache->create_thread_info(); + auto imagecache = ImageCache::create(true); + auto threadinfo = imagecache->create_thread_info(); OIIO_CHECK_ASSERT(threadinfo != nullptr); imagecache->destroy_thread_info(threadinfo); imagecache->close_all(); @@ -249,7 +245,7 @@ void test_tileptr() { Strutil::print("\nTesting tile ptr things\n"); - ImageCache* imagecache = ImageCache::create(); + auto imagecache = ImageCache::create(); auto hand = imagecache->get_image_handle(checkertex); ImageCache::Tile* tile = imagecache->get_tile(hand, nullptr, 0, 0, 4, 4, 0); OIIO_CHECK_ASSERT(tile != nullptr); @@ -277,7 +273,7 @@ static void test_imagespec() { Strutil::print("\nTesting imagespec retrieval\n"); - ImageCache* ic = ImageCache::create(); + auto ic = ImageCache::create(); { // basic get_imagespec() ImageSpec spec; @@ -350,7 +346,7 @@ main(int /*argc*/, char* /*argv*/[]) test_custom_threadinfo(); test_imagespec(); - ImageCache* ic = ImageCache::create(); + auto ic = ImageCache::create(); Strutil::print("\n\n{}\n", ic->getstats(5)); ic->reset_stats(); diff --git a/src/libOpenImageIO/imagespeed_test.cpp b/src/libOpenImageIO/imagespeed_test.cpp index 6c67991e04..d929023317 100644 --- a/src/libOpenImageIO/imagespeed_test.cpp +++ b/src/libOpenImageIO/imagespeed_test.cpp @@ -34,7 +34,7 @@ static std::string output_filename; static std::string output_format; static std::vector buffer; static ImageSpec bufspec, outspec; -static ImageCache* imagecache = NULL; +static std::shared_ptr imagecache; static imagesize_t total_image_pixels = 0; static float cache_size = 0; @@ -611,6 +611,5 @@ main(int argc, char** argv) if (verbose) std::cout << "\n" << imagecache->getstats(2) << "\n"; - ImageCache::destroy(imagecache); return unit_test_failures; } diff --git a/src/libtexture/imagecache.cpp b/src/libtexture/imagecache.cpp index c048b04fd1..9c7c480135 100644 --- a/src/libtexture/imagecache.cpp +++ b/src/libtexture/imagecache.cpp @@ -51,7 +51,7 @@ spin_mutex ImageCacheImpl::m_perthread_info_mutex; namespace { // anonymous static thread_local tsl::robin_map imcache_error_messages; -static std::shared_ptr shared_image_cache; +static std::shared_ptr shared_image_cache; static spin_mutex shared_image_cache_mutex; // Make some static ustring constants to avoid strcmp's @@ -3934,53 +3934,41 @@ ImageCacheImpl::append_error(string_view message) const -ImageCache* +std::shared_ptr ImageCache::create(bool shared) { if (shared) { // They requested a shared cache. If a shared cache already // exists, just return it, otherwise record the new cache. spin_lock guard(shared_image_cache_mutex); - if (!shared_image_cache.get()) - shared_image_cache.reset(aligned_new(), - aligned_delete); - -#if 0 - std::cerr << " shared ImageCache is " - << (void *)shared_image_cache.get() << "\n"; -#endif - return shared_image_cache.get(); + if (!shared_image_cache) + shared_image_cache = std::make_shared(); + return shared_image_cache; } // Doesn't need a shared cache - ImageCacheImpl* ic = aligned_new(); -#if 0 - std::cerr << "creating new ImageCache " << (void *)ic << "\n"; -#endif - return ic; + return std::make_shared(); } void -ImageCache::destroy(ImageCache* x, bool teardown) +ImageCache::destroy(std::shared_ptr& cache, bool teardown) { - if (!x) + if (!cache) return; spin_lock guard(shared_image_cache_mutex); - if (x == shared_image_cache.get()) { + if (cache.get() == shared_image_cache.get()) { // This is the shared cache, so don't really delete it. Invalidate // it fully, closing the files and throwing out any tiles that // nobody is currently holding references to. But only delete the // IC fully if 'teardown' is true, and even then, it won't destroy // until nobody else is still holding a shared_ptr to it. - ((ImageCacheImpl*)x)->invalidate_all(teardown); + cache->invalidate_all(teardown); if (teardown) shared_image_cache.reset(); - } else { - // Not a shared cache, we are the only owner, so truly destroy it. - aligned_delete(x); } + cache.reset(); } diff --git a/src/libtexture/texture_pvt.h b/src/libtexture/texture_pvt.h index a616c86623..b7d231c35c 100644 --- a/src/libtexture/texture_pvt.h +++ b/src/libtexture/texture_pvt.h @@ -37,7 +37,7 @@ class TextureSystemImpl final : public TextureSystem { public: typedef ImageCacheFile TextureFile; - TextureSystemImpl(ImageCache* imagecache); + TextureSystemImpl(std::shared_ptr imagecache); ~TextureSystemImpl() override; bool attribute(string_view name, TypeDesc type, const void* val) override; @@ -283,7 +283,10 @@ class TextureSystemImpl final : public TextureSystem { /// Return an opaque, non-owning pointer to the underlying ImageCache /// (if there is one). - ImageCache* imagecache() const override { return m_imagecache; } + std::shared_ptr imagecache() const override + { + return m_imagecache_sp; + } private: typedef ImageCacheTileRef TileRef; @@ -495,6 +498,7 @@ class TextureSystemImpl final : public TextureSystem { void visualize_ellipse(const std::string& name, float dsdx, float dtdx, float dsdy, float dtdy, float sblur, float tblur); + std::shared_ptr m_imagecache_sp; ImageCacheImpl* m_imagecache = nullptr; uint64_t m_id; // A unique ID for this TextureSystem Imath::M44f m_Mw2c; ///< world-to-"common" matrix diff --git a/src/libtexture/texturesys.cpp b/src/libtexture/texturesys.cpp index a29a43e2f3..8d5400ce6b 100644 --- a/src/libtexture/texturesys.cpp +++ b/src/libtexture/texturesys.cpp @@ -47,7 +47,7 @@ namespace { // anonymous // The only easy way to fix this is to make shared_texturesys be an ordinary // pointer and just let it leak (who cares? the app is done, and it only // contains a few hundred bytes). -static TextureSystemImpl* shared_texturesys = NULL; +static std::shared_ptr shared_texturesys; static spin_mutex shared_texturesys_mutex; static bool do_unit_test_texture = false; static float unit_test_texture_blur = 0.0f; @@ -81,14 +81,14 @@ static const OIIO_SIMD4_ALIGN vbool4 channel_masks[5] = { } // end anonymous namespace -TextureSystem* -TextureSystem::create(bool shared, ImageCache* imagecache) +std::shared_ptr +TextureSystem::create(bool shared, std::shared_ptr imagecache) { // Because the shared_texturesys is never deleted (by design) // we silence the otherwise useful compiler warning on newer GCC versions OIIO_PRAGMA_WARNING_PUSH #if OIIO_GNUC_VERSION > 100000 - OIIO_GCC_ONLY_PRAGMA(GCC diagnostic ignored "-Wmismatched-new-delete") + // OIIO_GCC_ONLY_PRAGMA(GCC diagnostic ignored "-Wmismatched-new-delete") #endif if (shared) { // They requested a shared texture system. If a shared one already @@ -96,11 +96,8 @@ TextureSystem::create(bool shared, ImageCache* imagecache) // as the shared one. spin_lock guard(shared_texturesys_mutex); if (!shared_texturesys) - shared_texturesys = new TextureSystemImpl(ImageCache::create(true)); -#if 0 - std::cerr << " shared TextureSystem is " - << (void *)shared_texturesys << "\n"; -#endif + shared_texturesys = std::make_shared( + ImageCache::create(true)); return shared_texturesys; } @@ -110,11 +107,8 @@ TextureSystem::create(bool shared, ImageCache* imagecache) imagecache = ImageCache::create(false); own_ic = true; } - TextureSystemImpl* ts = new TextureSystemImpl(imagecache); + auto ts = std::make_shared(imagecache); ts->m_imagecache_owner = own_ic; -#if 0 - std::cerr << "creating new ImageCache " << (void *)ts << "\n"; -#endif OIIO_PRAGMA_WARNING_POP return ts; } @@ -122,25 +116,20 @@ TextureSystem::create(bool shared, ImageCache* imagecache) void -TextureSystem::destroy(TextureSystem* x, bool teardown_imagecache) +TextureSystem::destroy(std::shared_ptr& ts, + bool teardown_imagecache) { - // std::cerr << "Destroying TS " << (void *)x << "\n"; - if (!x) + if (!ts) return; - TextureSystemImpl* impl = (TextureSystemImpl*)x; + TextureSystemImpl* impl = (TextureSystemImpl*)ts.get(); if (teardown_imagecache) { if (impl->m_imagecache_owner) - ImageCache::destroy(impl->m_imagecache, true); + ImageCache::destroy(impl->m_imagecache_sp, true); impl->m_imagecache = nullptr; + impl->m_imagecache_sp.reset(); } - spin_lock guard(shared_texturesys_mutex); - if (impl == shared_texturesys) { - // This is the shared TS, so don't really delete it. - } else { - // Not a shared cache, we are the only owner, so truly destroy it. - delete x; - } + ts.reset(); } @@ -324,10 +313,11 @@ texture_type_name(TexFormat f) -TextureSystemImpl::TextureSystemImpl(ImageCache* imagecache) +TextureSystemImpl::TextureSystemImpl(std::shared_ptr imagecache) : m_id(++txsys_next_id) { - m_imagecache = (ImageCacheImpl*)imagecache; + m_imagecache_sp = imagecache; + m_imagecache = (ImageCacheImpl*)m_imagecache_sp.get(); init(); } @@ -3120,7 +3110,7 @@ TextureSystem::unit_test_hash() Strutil::print("Testing hashing with {} files of {}x{} with {}x{} tiles:", nfiles, res, res, tilesize, tilesize); - ImageCache* imagecache = ImageCache::create(); + auto imagecache = ImageCache::create(); // Set up the ImageCacheFiles outside of the timing loop using OIIO::pvt::ImageCacheFile; @@ -3129,8 +3119,8 @@ TextureSystem::unit_test_hash() std::vector icf; for (int f = 0; f < nfiles; ++f) { ustring filename = ustring::fmtformat("{:06}.tif", f); - icf.push_back( - new ImageCacheFile(*(ImageCacheImpl*)imagecache, NULL, filename)); + icf.push_back(new ImageCacheFile(*(ImageCacheImpl*)imagecache.get(), + nullptr, filename)); } // First, just try to do raw timings of the hash @@ -3208,10 +3198,7 @@ TextureSystem::unit_test_hash() max = sixteenbits[i]; } Strutil::print("16-bit hash buckets range from {} to {}\n", min, max); - Strutil::print("\n"); - - ImageCache::destroy(imagecache); #endif } diff --git a/src/maketx/maketx.cpp b/src/maketx/maketx.cpp index 3feeaf4d64..4900f41eb1 100644 --- a/src/maketx/maketx.cpp +++ b/src/maketx/maketx.cpp @@ -502,7 +502,7 @@ getargs(int argc, char* argv[], ImageSpec& configspec) if (ignore_unassoc) { configspec.attribute("maketx:ignore_unassoc", (int)ignore_unassoc); - ImageCache* ic = ImageCache::create(); // get the shared one + auto ic = ImageCache::create(); // get the shared one ic->attribute("unassociatedalpha", (int)ignore_unassoc); } @@ -532,7 +532,7 @@ main(int argc, char* argv[]) OIIO::attribute("threads", nthreads); // N.B. This will apply to the default IC that any ImageBuf's get. - ImageCache* ic = ImageCache::create(); // get the shared one + auto ic = ImageCache::create(); // get the shared one ic->attribute("forcefloat", 1); // Force float upon read ic->attribute("max_memory_MB", 1024.0); // 1 GB cache diff --git a/src/oiiotool/imagerec.cpp b/src/oiiotool/imagerec.cpp index 4ea7fc2ac5..54703f8df8 100644 --- a/src/oiiotool/imagerec.cpp +++ b/src/oiiotool/imagerec.cpp @@ -182,7 +182,7 @@ ImageRec::ImageRec(ImageBufRef img, bool copy_pixels) ImageRec::ImageRec(const std::string& name, const ImageSpec& spec, - ImageCache* imagecache) + std::shared_ptr imagecache) : m_name(name) , m_elaborated(true) , m_pixels_modified(true) diff --git a/src/oiiotool/oiiotool.h b/src/oiiotool/oiiotool.h index 48706faedb..8f05bf58d5 100644 --- a/src/oiiotool/oiiotool.h +++ b/src/oiiotool/oiiotool.h @@ -140,7 +140,7 @@ class Oiiotool { ImageRecRef curimg; // current image std::vector image_stack; // stack of previous images std::map image_labels; // labeled images - ImageCache* imagecache = nullptr; // back ptr to ImageCache + std::shared_ptr imagecache; // back ptr to ImageCache ColorConfig colorconfig; // OCIO color config Timer total_runtime; // total_readtime is the amount of time for direct reads, and does not @@ -434,7 +434,7 @@ class SubimageRec { /// and potentially MIPmap levels for each subimage. class ImageRec { public: - ImageRec(const std::string& name, ImageCache* imagecache) + ImageRec(const std::string& name, std::shared_ptr imagecache) : m_name(name) , m_imagecache(imagecache) { @@ -471,7 +471,7 @@ class ImageRec { // Initialize an ImageRec with the given spec. ImageRec(const std::string& name, const ImageSpec& spec, - ImageCache* imagecache); + std::shared_ptr imagecache); ImageRec(const ImageRec& copy) = delete; // Disallow copy ctr @@ -618,7 +618,7 @@ class ImageRec { std::vector m_subimages; std::time_t m_time; //< Modification time of the input file TypeDesc m_input_dataformat; - ImageCache* m_imagecache = nullptr; + std::shared_ptr m_imagecache; mutable std::string m_err; std::unique_ptr m_configspec; diff --git a/src/python/py_imagecache.cpp b/src/python/py_imagecache.cpp index 5b2d69db09..6d88e48416 100644 --- a/src/python/py_imagecache.cpp +++ b/src/python/py_imagecache.cpp @@ -9,10 +9,7 @@ namespace PyOpenImageIO { // Make a special wrapper to help with the weirdo way we use create/destroy. class ImageCacheWrap { public: - struct ICDeleter { - void operator()(ImageCache* p) const { ImageCache::destroy(p); } - }; - std::unique_ptr m_cache; + std::shared_ptr m_cache; ImageCacheWrap(bool shared = true) : m_cache(ImageCache::create(shared)) @@ -23,7 +20,7 @@ class ImageCacheWrap { ~ImageCacheWrap() {} // will call the deleter on the IC static void destroy(ImageCacheWrap* x, bool teardown = false) { - ImageCache::destroy(x->m_cache.release(), teardown); + ImageCache::destroy(x->m_cache, teardown); } py::object get_pixels(const std::string& filename, int subimage, int miplevel, int xbegin, int xend, int ybegin, diff --git a/src/python/py_texturesys.cpp b/src/python/py_texturesys.cpp index 3699f8745b..b51e1bc10e 100644 --- a/src/python/py_texturesys.cpp +++ b/src/python/py_texturesys.cpp @@ -44,14 +44,11 @@ class TextureOptWrap : public TextureOpt { // Make a special wrapper to help with the weirdo way we use create/destroy. class TextureSystemWrap { public: - struct TSDeleter { - void operator()(TextureSystem* p) const { TextureSystem::destroy(p); } - }; - std::unique_ptr m_texsys; + std::shared_ptr m_texsys; TextureSystemWrap(bool shared = true) - : m_texsys(TextureSystem::create(shared, nullptr)) + : m_texsys(TextureSystem::create(shared)) { } TextureSystemWrap(const TextureSystemWrap&) = delete; @@ -59,7 +56,7 @@ class TextureSystemWrap { ~TextureSystemWrap() {} // will call the deleter on the m_texsys static void destroy(TextureSystemWrap* x) { - TextureSystem::destroy(x->m_texsys.release()); + TextureSystem::destroy(x->m_texsys); } }; diff --git a/src/testtex/testtex.cpp b/src/testtex/testtex.cpp index 95bce60338..95eebb43f7 100644 --- a/src/testtex/testtex.cpp +++ b/src/testtex/testtex.cpp @@ -56,7 +56,7 @@ static bool test_construction = false; static bool test_gettexels = false; static bool test_getimagespec = false; static bool filtertest = false; -static TextureSystem* texsys = NULL; +static std::shared_ptr texsys; static std::string searchpath; static bool batch = false; static bool nowarp = false; @@ -1649,7 +1649,7 @@ test_icwrite(int testicwrite) // The global "shared" ImageCache will be the same one the // TextureSystem uses. - ImageCache* ic = ImageCache::create(); + std::shared_ptr ic = ImageCache::create(); // Set up the fake file and add it int tw = 64, th = 64; // tile width and height From 61810dfe7eb09ad64c2ece409a424f80d689ea25 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Wed, 14 Aug 2024 11:25:34 -0700 Subject: [PATCH 2/3] Use std::move to avoid extra ref count incr/decr Signed-off-by: Larry Gritz --- src/idiff/idiff.cpp | 2 +- src/libOpenImageIO/imagebuf.cpp | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/idiff/idiff.cpp b/src/idiff/idiff.cpp index a4b927e221..567e1ca532 100644 --- a/src/idiff/idiff.cpp +++ b/src/idiff/idiff.cpp @@ -419,7 +419,7 @@ main(int argc, char* argv[]) } imagecache->invalidate_all(true); - imagecache.reset(); + imagecache.reset(); // Be sure to free cache before the shutdown below shutdown(); return ret; } diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index 883f388925..25a4c5b94e 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -402,7 +402,8 @@ ImageBufImpl::ImageBufImpl(string_view filename, int subimage, int miplevel, } else if (filename.length() > 0) { // filename being nonempty means this ImageBuf refers to a file. OIIO_DASSERT(buffer == nullptr); - reset(filename, subimage, miplevel, imagecache, config, ioproxy); + reset(filename, subimage, miplevel, std::move(imagecache), config, + ioproxy); } else { OIIO_DASSERT(buffer == nullptr); } @@ -506,9 +507,9 @@ ImageBuf::ImageBuf() ImageBuf::ImageBuf(string_view filename, int subimage, int miplevel, std::shared_ptr imagecache, const ImageSpec* config, Filesystem::IOProxy* ioproxy) - : m_impl(new ImageBufImpl(filename, subimage, miplevel, imagecache, - nullptr /*spec*/, nullptr /*buffer*/, config, - ioproxy), + : m_impl(new ImageBufImpl(filename, subimage, miplevel, + std::move(imagecache), nullptr /*spec*/, + nullptr /*buffer*/, config, ioproxy), &impl_deleter) { } @@ -748,7 +749,7 @@ ImageBufImpl::reset(string_view filename, int subimage, int miplevel, } m_current_subimage = subimage; m_current_miplevel = miplevel; - m_imagecache = imagecache; + m_imagecache = std::move(imagecache); if (config) m_configspec.reset(new ImageSpec(*config)); m_rioproxy = ioproxy; @@ -772,7 +773,8 @@ ImageBuf::reset(string_view filename, int subimage, int miplevel, std::shared_ptr imagecache, const ImageSpec* config, Filesystem::IOProxy* ioproxy) { - m_impl->reset(filename, subimage, miplevel, imagecache, config, ioproxy); + m_impl->reset(filename, subimage, miplevel, std::move(imagecache), config, + ioproxy); } From 7ac76448073270109ce6af2c45f4fe3a0f625cfb Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Wed, 14 Aug 2024 11:31:11 -0700 Subject: [PATCH 3/3] Remove unnecessary IC reset Signed-off-by: Larry Gritz --- src/idiff/idiff.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/idiff/idiff.cpp b/src/idiff/idiff.cpp index 567e1ca532..b7aadef496 100644 --- a/src/idiff/idiff.cpp +++ b/src/idiff/idiff.cpp @@ -419,7 +419,6 @@ main(int argc, char* argv[]) } imagecache->invalidate_all(true); - imagecache.reset(); // Be sure to free cache before the shutdown below shutdown(); return ret; }