From 7b245f49e70f3a79af65dfbf2baf5ac0b886a302 Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Wed, 25 Sep 2024 12:07:56 -0700 Subject: [PATCH 1/9] first draft of the frontend refactor for issue #4436 Signed-off-by: Basile Fraboni --- src/iconvert/iconvert.cpp | 3 +- src/include/OpenImageIO/imagecache.h | 77 ++++++++------- src/include/OpenImageIO/texture.h | 4 +- src/libOpenImageIO/imagebuf.cpp | 8 +- src/libOpenImageIO/imagecache_test.cpp | 15 +-- src/libOpenImageIO/imagespeed_test.cpp | 2 +- src/libtexture/imagecache.cpp | 125 +++++++++++++++++++------ src/libtexture/imagecache_pvt.h | 18 ++-- src/libtexture/texture_pvt.h | 4 +- src/libtexture/texturesys.cpp | 18 ++-- src/oiiotool/imagerec.cpp | 8 +- src/python/py_imagecache.cpp | 19 +++- src/testtex/testtex.cpp | 10 +- 13 files changed, 202 insertions(+), 109 deletions(-) diff --git a/src/iconvert/iconvert.cpp b/src/iconvert/iconvert.cpp index 239b74a11c..0c68fe190e 100644 --- a/src/iconvert/iconvert.cpp +++ b/src/iconvert/iconvert.cpp @@ -364,8 +364,7 @@ convert_file(const std::string& in_filename, const std::string& out_filename) if (nsubimages > 1) { subimagespecs.resize(nsubimages); for (int i = 0; i < nsubimages; ++i) { - ImageSpec inspec = *imagecache->imagespec(ufilename, i, 0, - true /*native*/); + ImageSpec inspec = *imagecache->imagespec(ufilename, i); subimagespecs[i] = inspec; adjust_spec(in.get(), out.get(), inspec, subimagespecs[i]); } diff --git a/src/include/OpenImageIO/imagecache.h b/src/include/OpenImageIO/imagecache.h index 54671c5926..f36209fe40 100644 --- a/src/include/OpenImageIO/imagecache.h +++ b/src/include/OpenImageIO/imagecache.h @@ -724,42 +724,29 @@ class OIIO_API ImageCache { int miplevel, ustring dataname, TypeDesc datatype, void* data); - /// Copy the ImageSpec associated with the named image (the first - /// subimage & miplevel by default, or as set by `subimage` and - /// `miplevel`). + /// Copy the ImageSpec associated with the named image file. /// /// @param filename /// The name of the image, as a UTF-8 encoded ustring. /// @param spec /// ImageSpec into which will be copied the spec for the /// requested image. - /// @param subimage/miplevel - /// The subimage and MIP level to query. - /// @param native - /// If `false` (the default), then the spec retrieved will - /// accurately describe the image stored internally in the - /// cache, whereas if `native` is `true`, the spec retrieved - /// will reflect the contents of the original file. These - /// may differ due to use of certain ImageCache settings - /// such as `"forcefloat"` or `"autotile"`. + /// @param subimage + /// The subimage to query. /// @returns /// `true` upon success, `false` upon failure failure (such /// as being unable to find, open, or read the file, or if - /// it does not contain the designated subimage or MIP - /// level). - bool get_imagespec(ustring filename, ImageSpec& spec, int subimage = 0, - int miplevel = 0, bool native = false); + /// it does not contain the designated subimage. + bool get_imagespec(ustring filename, ImageSpec& spec, int subimage = 0); /// A more efficient variety of `get_imagespec()` for cases where you /// can use an `ImageHandle*` to specify the image and optionally have a /// `Perthread*` for the calling thread. bool get_imagespec(ImageHandle* file, Perthread* thread_info, - ImageSpec& spec, int subimage = 0, int miplevel = 0, - bool native = false); + ImageSpec& spec, int subimage = 0); - /// Return a pointer to an ImageSpec associated with the named image - /// (the first subimage & MIP level by default, or as set by `subimage` - /// and `miplevel`) if the file is found and is an image format that can - /// be read, otherwise return `nullptr`. + /// Return a pointer to an ImageSpec associated with the named image file. + /// If the file is found and is an image format that can be read, + /// otherwise return `nullptr`. /// /// This method is much more efficient than `get_imagespec()`, since it /// just returns a pointer to the spec held internally by the ImageCache @@ -770,27 +757,45 @@ class OIIO_API ImageCache { /// /// @param filename /// The name of the image, as a UTF-8 encoded ustring. - /// @param subimage/miplevel - /// The subimage and MIP level to query. - /// @param native - /// If `false` (the default), then the spec retrieved will - /// accurately describe the image stored internally in the - /// cache, whereas if `native` is `true`, the spec retrieved - /// will reflect the contents of the original file. These - /// may differ due to use of certain ImageCache settings - /// such as `"forcefloat"` or `"autotile"`. + /// @param subimage + /// The subimage to query. /// @returns /// A pointer to the spec, if the image is found and able to /// be opened and read by an available image format plugin, - /// and the designated subimage and MIP level exists. - const ImageSpec* imagespec(ustring filename, int subimage = 0, - int miplevel = 0, bool native = false); + /// and the designated subimage exists. + const ImageSpec* imagespec(ustring filename, int subimage = 0); /// A more efficient variety of `imagespec()` for cases where you can /// use an `ImageHandle*` to specify the image and optionally have a /// `Perthread*` for the calling thread. const ImageSpec* imagespec(ImageHandle* file, Perthread* thread_info, - int subimage = 0, int miplevel = 0, - bool native = false); + int subimage = 0); + + /// Copy the image dimensions (x, y, z, width, height, depth, full*, + /// nchannels, format) and data types associated with the named image + /// cache file for the specified subimage and miplevel. It does *not* + /// copy arbitrary named metadata or channel names (thus, for an + /// `ImageSpec` with lots of metadata, it is much less expensive than + /// copying the whole thing with `operator=()`). + /// + /// @param filename + /// The name of the image, as a UTF-8 encoded ustring. + /// @param spec + /// ImageSpec into which will be copied the dimensions + /// for the requested image. + /// @param subimage/miplevel + /// The subimage and mip level to query. + /// @returns + /// `true` upon success, `false` upon failure failure (such + /// as being unable to find, open, or read the file, or if + /// it does not contain the designated subimage or mip level. + bool get_cache_dimensions(ustring filename, ImageSpec& spec, + int subimage = 0, int miplevel = 0); + /// A more efficient variety of `get_cache_dimensions()` for cases where + /// you can use an `ImageHandle*` to specify the image and optionally + /// have a `Perthread*` for the calling thread. + bool get_cache_dimensions(ImageHandle* file, Perthread* thread_info, + ImageSpec& spec, int subimage = 0, + int miplevel = 0); /// Copy into `thumbnail` any associated thumbnail associated with this /// image (for the first subimage by default, or as set by `subimage`). diff --git a/src/include/OpenImageIO/texture.h b/src/include/OpenImageIO/texture.h index 9f59b67280..f51e7c92f8 100644 --- a/src/include/OpenImageIO/texture.h +++ b/src/include/OpenImageIO/texture.h @@ -1433,12 +1433,12 @@ class OIIO_API TextureSystem { /// as being unable to find, open, or read the file, or if /// it does not contain the designated subimage or MIP /// level). - bool get_imagespec(ustring filename, int subimage, ImageSpec& spec); + bool get_imagespec(ustring filename, ImageSpec& spec, int subimage = 0); /// A more efficient variety of `get_imagespec()` for cases where you /// can use a `TextureHandle*` to specify the image and optionally have /// a `Perthread*` for the calling thread. bool get_imagespec(TextureHandle* texture_handle, Perthread* thread_info, - int subimage, ImageSpec& spec); + ImageSpec& spec, int subimage = 0); /// Return a pointer to an ImageSpec associated with the named texture /// if the file is found and is an image format that can be read, diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index 8304295439..257a11113e 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -1076,9 +1076,11 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel, m_imagecache->get_image_info(m_name, subimage, miplevel, s_fileformat, TypeString, &fmt); m_fileformat = ustring(fmt); - m_imagecache->get_imagespec(m_name, m_spec, subimage, miplevel); - m_imagecache->get_imagespec(m_name, m_nativespec, subimage, miplevel, - true); + + m_imagecache->get_imagespec(m_name, m_nativespec, subimage); + m_spec = m_nativespec; + m_imagecache->get_cache_dimensions(m_name, m_spec, subimage, miplevel); + 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/libOpenImageIO/imagecache_test.cpp b/src/libOpenImageIO/imagecache_test.cpp index f766d88ca4..c532545d3b 100644 --- a/src/libOpenImageIO/imagecache_test.cpp +++ b/src/libOpenImageIO/imagecache_test.cpp @@ -313,17 +313,18 @@ test_imagespec() ic->geterror()); } { // imagespec() for out of range subimage - const ImageSpec* spec = ic->imagespec(checkertex, 10, 0); - OIIO_CHECK_ASSERT(spec == nullptr && ic->has_error()); - Strutil::print("imagespec() out-of-range subimage:\n {}\n", - ic->geterror()); - } - { // imagespec() for out of range mip level - const ImageSpec* spec = ic->imagespec(checkertex, 0, 100); + const ImageSpec* spec = ic->imagespec(checkertex, 10); OIIO_CHECK_ASSERT(spec == nullptr && ic->has_error()); Strutil::print("imagespec() out-of-range subimage:\n {}\n", ic->geterror()); } + //! TODO: get_cache_dimensions tests + // { // imagespec() for out of range mip level + // const ImageSpec* spec = ic->imagespec(checkertex, 0, 100); + // OIIO_CHECK_ASSERT(spec == nullptr && ic->has_error()); + // Strutil::print("imagespec() out-of-range subimage:\n {}\n", + // ic->geterror()); + // } } diff --git a/src/libOpenImageIO/imagespeed_test.cpp b/src/libOpenImageIO/imagespeed_test.cpp index 24b7bcd661..dc8a03968e 100644 --- a/src/libOpenImageIO/imagespeed_test.cpp +++ b/src/libOpenImageIO/imagespeed_test.cpp @@ -496,7 +496,7 @@ main(int argc, char** argv) imagesize_t maxpelchans = 0; for (auto&& fn : input_filename) { ImageSpec spec; - if (!imagecache->get_imagespec(fn, spec, 0, 0, true)) { + if (!imagecache->get_imagespec(fn, spec)) { std::cout << "File \"" << fn << "\" could not be opened.\n"; return -1; } diff --git a/src/libtexture/imagecache.cpp b/src/libtexture/imagecache.cpp index 46ab9f1173..e53051f784 100644 --- a/src/libtexture/imagecache.cpp +++ b/src/libtexture/imagecache.cpp @@ -3014,10 +3014,9 @@ ImageCacheImpl::get_image_info(ImageCacheFile* file, bool -ImageCacheImpl::get_imagespec(ustring filename, ImageSpec& spec, int subimage, - int miplevel, bool native) +ImageCacheImpl::get_imagespec(ustring filename, ImageSpec& spec, int subimage) { - const ImageSpec* specptr = imagespec(filename, subimage, miplevel, native); + const ImageSpec* specptr = imagespec(filename, subimage); if (specptr) { spec = *specptr; return true; @@ -3031,11 +3030,9 @@ ImageCacheImpl::get_imagespec(ustring filename, ImageSpec& spec, int subimage, bool ImageCacheImpl::get_imagespec(ImageCacheFile* file, ImageCachePerThreadInfo* thread_info, - ImageSpec& spec, int subimage, int miplevel, - bool native) + ImageSpec& spec, int subimage) { - const ImageSpec* specptr = imagespec(file, thread_info, subimage, miplevel, - native); + const ImageSpec* specptr = imagespec(file, thread_info, subimage); if (specptr) { spec = *specptr; return true; @@ -3047,8 +3044,7 @@ ImageCacheImpl::get_imagespec(ImageCacheFile* file, const ImageSpec* -ImageCacheImpl::imagespec(ustring filename, int subimage, int miplevel, - bool native) +ImageCacheImpl::imagespec(ustring filename, int subimage) { ImageCachePerThreadInfo* thread_info = get_perthread_info(); ImageCacheFile* file = find_file(filename, thread_info, nullptr); @@ -3056,15 +3052,73 @@ ImageCacheImpl::imagespec(ustring filename, int subimage, int miplevel, error("Image file \"{}\" not found", filename); return NULL; } - return imagespec(file, thread_info, subimage, miplevel, native); + return imagespec(file, thread_info, subimage); } const ImageSpec* ImageCacheImpl::imagespec(ImageCacheFile* file, - ImageCachePerThreadInfo* thread_info, int subimage, - int miplevel, bool native) + ImageCachePerThreadInfo* thread_info, int subimage) +{ + if (!file) { + error("Image file handle was NULL"); + return NULL; + } + if (!thread_info) + thread_info = get_perthread_info(); + file = verify_file(file, thread_info, true); + if (file->broken()) { + if (file->errors_should_issue()) + error("Invalid image file \"{}\": {}", file->filename(), + file->broken_error_message()); + return NULL; + } + if (file->is_udim()) { + error("Cannot retrieve ImageSpec of a UDIM-like virtual file"); + return NULL; // UDIM-like files don't have an ImageSpec + } + if (subimage < 0 || subimage >= file->subimages()) { + if (file->errors_should_issue()) + error("Unknown subimage {} (out of {})", subimage, + file->subimages()); + return NULL; + } + + //! force mip level to zero for now + //! TODO: store nativespec in SubImageInfo, and extra overrides in LevelInfo + constexpr int miplevel = 0; + if (miplevel < 0 || miplevel >= file->miplevels(subimage)) { + if (file->errors_should_issue()) + error("Unknown mip level {} (out of {})", miplevel, + file->miplevels(subimage)); + return NULL; + } + return &file->nativespec(subimage, miplevel); +} + + + +bool +ImageCacheImpl::get_cache_dimensions(ustring filename, ImageSpec& spec, + int subimage, int miplevel) +{ + ImageCachePerThreadInfo* thread_info = get_perthread_info(); + ImageCacheFile* file = find_file(filename, thread_info, nullptr); + if (!file) { + error("Image file \"{}\" not found", filename); + return NULL; + } + return get_cache_dimensions(file, thread_info, spec, subimage, miplevel); +} + + + +bool +ImageCacheImpl::get_cache_dimensions(ImageCacheFile* file, + ImageCachePerThreadInfo* thread_info, + ImageSpec& spec, int subimage, + int miplevel) { if (!file) { error("Image file handle was NULL"); @@ -3095,9 +3149,12 @@ ImageCacheImpl::imagespec(ImageCacheFile* file, file->miplevels(subimage)); return NULL; } - const ImageSpec* spec = native ? &file->nativespec(subimage, miplevel) - : &file->spec(subimage, miplevel); - return spec; + //! copy dimensions from mip level ImageSpec + //! TODO: store nativespec in SubImageInfo, and extra overrides in LevelInfo + //! and simply copy extra overrides here + const ImageSpec& mipspec = file->spec(subimage, miplevel); + spec.copy_dimensions(mipspec); + return true; } @@ -4219,36 +4276,50 @@ ImageCache::get_image_info(ImageHandle* file, Perthread* thread_info, bool -ImageCache::get_imagespec(ustring filename, ImageSpec& spec, int subimage, - int miplevel, bool native) +ImageCache::get_imagespec(ustring filename, ImageSpec& spec, int subimage) { - return m_impl->get_imagespec(filename, spec, subimage, miplevel, native); + return m_impl->get_imagespec(filename, spec, subimage); } bool ImageCache::get_imagespec(ImageHandle* file, Perthread* thread_info, - ImageSpec& spec, int subimage, int miplevel, - bool native) + ImageSpec& spec, int subimage) { - return m_impl->get_imagespec(file, thread_info, spec, subimage, miplevel, - native); + return m_impl->get_imagespec(file, thread_info, spec, subimage); } const ImageSpec* -ImageCache::imagespec(ustring filename, int subimage, int miplevel, bool native) +ImageCache::imagespec(ustring filename, int subimage) { - return m_impl->imagespec(filename, subimage, miplevel, native); + return m_impl->imagespec(filename, subimage); } const ImageSpec* -ImageCache::imagespec(ImageHandle* file, Perthread* thread_info, int subimage, - int miplevel, bool native) +ImageCache::imagespec(ImageHandle* file, Perthread* thread_info, int subimage) +{ + return m_impl->imagespec(file, thread_info, subimage); +} + + + +bool +ImageCache::get_cache_dimensions(ustring filename, ImageSpec& spec, + int subimage, int miplevel) +{ + return m_impl->get_cache_dimensions(filename, spec, subimage, miplevel); +} + + +bool +ImageCache::get_cache_dimensions(ImageHandle* file, Perthread* thread_info, + ImageSpec& spec, int subimage, int miplevel) { - return m_impl->imagespec(file, thread_info, subimage, miplevel, native); + return m_impl->get_cache_dimensions(file, thread_info, spec, subimage, + miplevel); } diff --git a/src/libtexture/imagecache_pvt.h b/src/libtexture/imagecache_pvt.h index f8f8b33284..3d5b7665f5 100644 --- a/src/libtexture/imagecache_pvt.h +++ b/src/libtexture/imagecache_pvt.h @@ -921,18 +921,22 @@ class ImageCacheImpl { /// of its specification in spec and return true. Return false if /// the file was not found or could not be opened as an image file /// by any available ImageIO plugin. - bool get_imagespec(ustring filename, ImageSpec& spec, int subimage = 0, - int miplevel = 0, bool native = false); + bool get_imagespec(ustring filename, ImageSpec& spec, int subimage = 0); bool get_imagespec(ImageCacheFile* file, ImageCachePerThreadInfo* thread_info, ImageSpec& spec, - int subimage = 0, int miplevel = 0, bool native = false); + int subimage = 0); - const ImageSpec* imagespec(ustring filename, int subimage = 0, - int miplevel = 0, bool native = false); + const ImageSpec* imagespec(ustring filename, int subimage = 0); const ImageSpec* imagespec(ImageCacheFile* file, ImageCachePerThreadInfo* thread_info = NULL, - int subimage = 0, int miplevel = 0, - bool native = false); + int subimage = 0); + + bool get_cache_dimensions(ustring filename, ImageSpec& spec, + int subimage = 0, int miplevel = 0); + bool get_cache_dimensions(ImageCacheFile* file, + ImageCachePerThreadInfo* thread_info, + ImageSpec& spec, int subimage = 0, + int miplevel = 0); ImageCacheFile* resolve_udim(ImageCacheFile* udimfile, Perthread* thread_info, int utile, int vtile); diff --git a/src/libtexture/texture_pvt.h b/src/libtexture/texture_pvt.h index 66c47dc0bf..5149af00cf 100644 --- a/src/libtexture/texture_pvt.h +++ b/src/libtexture/texture_pvt.h @@ -199,9 +199,9 @@ class TextureSystemImpl { int subimage, ustring dataname, TypeDesc datatype, void* data); - bool get_imagespec(ustring filename, int subimage, ImageSpec& spec); + bool get_imagespec(ustring filename, ImageSpec& spec, int subimage); bool get_imagespec(TextureHandle* texture_handle, Perthread* thread_info, - int subimage, ImageSpec& spec); + ImageSpec& spec, int subimage); const ImageSpec* imagespec(ustring filename, int subimage = 0); const ImageSpec* imagespec(TextureHandle* texture_handle, diff --git a/src/libtexture/texturesys.cpp b/src/libtexture/texturesys.cpp index 300f1b038b..772c5b4515 100644 --- a/src/libtexture/texturesys.cpp +++ b/src/libtexture/texturesys.cpp @@ -323,18 +323,18 @@ TextureSystem::get_texture_info(TextureHandle* texture_handle, bool -TextureSystem::get_imagespec(ustring filename, int subimage, ImageSpec& spec) +TextureSystem::get_imagespec(ustring filename, ImageSpec& spec, int subimage) { - return m_impl->get_imagespec(filename, subimage, spec); + return m_impl->get_imagespec(filename, spec, subimage); } bool TextureSystem::get_imagespec(TextureHandle* texture_handle, - Perthread* thread_info, int subimage, - ImageSpec& spec) + Perthread* thread_info, ImageSpec& spec, + int subimage) { - return m_impl->get_imagespec(texture_handle, thread_info, subimage, spec); + return m_impl->get_imagespec(texture_handle, thread_info, spec, subimage); } @@ -1001,8 +1001,8 @@ TextureSystemImpl::get_texture_info(TextureHandle* texture_handle, bool -TextureSystemImpl::get_imagespec(ustring filename, int subimage, - ImageSpec& spec) +TextureSystemImpl::get_imagespec(ustring filename, ImageSpec& spec, + int subimage) { bool ok = m_imagecache->get_imagespec(filename, spec, subimage); if (!ok) { @@ -1017,8 +1017,8 @@ TextureSystemImpl::get_imagespec(ustring filename, int subimage, bool TextureSystemImpl::get_imagespec(TextureHandle* texture_handle, - Perthread* thread_info, int subimage, - ImageSpec& spec) + Perthread* thread_info, ImageSpec& spec, + int subimage) { bool ok = m_imagecache->get_imagespec((ImageCache::ImageHandle*)texture_handle, diff --git a/src/oiiotool/imagerec.cpp b/src/oiiotool/imagerec.cpp index 54703f8df8..f732ae292f 100644 --- a/src/oiiotool/imagerec.cpp +++ b/src/oiiotool/imagerec.cpp @@ -282,9 +282,11 @@ ImageRec::read(ReadPolicy readpolicy, string_view channel_set) // relying on the cache to read their frames on demand rather // than reading the whole movie up front, even though each frame // individually would be well below the threshold. - imagesize_t imgbytes - = m_imagecache->imagespec(uname, s, m)->image_bytes(); - bool forceread = (s == 0 && m == 0 + ImageSpec spec; + m_imagecache->get_imagespec(uname, spec, s); + m_imagecache->get_cache_dimensions(uname, spec, s, m); + imagesize_t imgbytes = spec.image_bytes(); + bool forceread = (s == 0 && m == 0 && imgbytes * subimages < 50 * 1024 * 1024); ImageBufRef ib( new ImageBuf(name(), s, m, m_imagecache, configspec())); diff --git a/src/python/py_imagecache.cpp b/src/python/py_imagecache.cpp index 6d88e48416..395ce2fcb1 100644 --- a/src/python/py_imagecache.cpp +++ b/src/python/py_imagecache.cpp @@ -120,14 +120,23 @@ declare_imagecache(py::module& m) .def( "get_imagespec", [](const ImageCacheWrap& ic, const std::string& filename, - int subimage, int miplevel, bool native) { + int subimage) { ImageSpec spec; - ic.m_cache->get_imagespec(ustring(filename), spec, subimage, - miplevel, native); + ic.m_cache->get_imagespec(ustring(filename), spec, subimage); return spec; }, - "filename"_a, "subimage"_a = 0, "miplevel"_a = 0, - "native"_a = false) + "filename"_a, "subimage"_a = 0) + .def( + "get_cache_dimensions", + [](const ImageCacheWrap& ic, const std::string& filename, + int subimage, int miplevel) { + ImageSpec spec; + ic.m_cache->get_imagespec(ustring(filename), spec, subimage); + ic.m_cache->get_cache_dimensions(ustring(filename), spec, + subimage, miplevel); + return spec; + }, + "filename"_a, "subimage"_a = 0, "miplevel"_a = 0) // .def("get_thumbnail", &ImageCacheWrap::get_thumbnail, // "subimage"_a=0) .def("get_pixels", &ImageCacheWrap::get_pixels, "filename"_a, diff --git a/src/testtex/testtex.cpp b/src/testtex/testtex.cpp index a1aa94b75b..1c42409fe2 100644 --- a/src/testtex/testtex.cpp +++ b/src/testtex/testtex.cpp @@ -624,7 +624,7 @@ map_env_latlong(const int& x, const int& y, Imath::Vec3& R, float dphi_dy = float(M_PI) * dv_dy; // dphi_dx = 0 R = Imath::Vec3(fast_sin(phi) * fast_sin(theta), fast_cos(phi), - -fast_sin(phi) * fast_cos(theta)); + -fast_sin(phi) * fast_cos(theta)); dRdx = Imath::Vec3(fast_sin(phi) * fast_cos(theta) * dtheta_dx, float(0.0f), fast_sin(phi) * fast_sin(theta) * dtheta_dx); @@ -1350,7 +1350,7 @@ test_getimagespec_gettexels(ustring filename) { ImageSpec spec; int miplevel = 0; - if (!texsys->get_imagespec(filename, 0, spec)) { + if (!texsys->get_imagespec(filename, spec, 0)) { Strutil::print(std::cerr, "Could not get spec for {}\n", filename); std::string e = texsys->geterror(); if (!e.empty()) @@ -1425,12 +1425,12 @@ do_tex_thread_workout(int iterations, int mythread) ImageSpec spec0; if (texsys->is_udim(filenames[0])) { auto th = texsys->resolve_udim(filenames[0], 0.5f, 0.5f); - if (!th || !texsys->get_imagespec(th, nullptr, 0, spec0)) { + if (!th || !texsys->get_imagespec(th, nullptr, spec0, 0)) { Strutil::print(std::cerr, "Unexpected error with {}: {}\n", filenames[0], texsys->geterror()); } } else { - bool ok = texsys->get_imagespec(filenames[0], 0, spec0); + bool ok = texsys->get_imagespec(filenames[0], spec0, 0); if (!ok) { Strutil::print(std::cerr, "Unexpected error: {}\n", texsys->geterror()); @@ -1854,7 +1854,7 @@ main(int argc, const char* argv[]) if (test_getimagespec) { ImageSpec spec; for (int i = 0; i < iters; ++i) { - texsys->get_imagespec(filenames[0], 0, spec); + texsys->get_imagespec(filenames[0], spec, 0); } iters = 0; } From e9cc3d5ec35cdeaeb154ad3f7fd5bca0e363f267 Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Wed, 25 Sep 2024 14:19:24 -0700 Subject: [PATCH 2/9] fix return typo Signed-off-by: Basile Fraboni --- src/libtexture/imagecache.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libtexture/imagecache.cpp b/src/libtexture/imagecache.cpp index e53051f784..b71aba7e06 100644 --- a/src/libtexture/imagecache.cpp +++ b/src/libtexture/imagecache.cpp @@ -3107,7 +3107,7 @@ ImageCacheImpl::get_cache_dimensions(ustring filename, ImageSpec& spec, ImageCacheFile* file = find_file(filename, thread_info, nullptr); if (!file) { error("Image file \"{}\" not found", filename); - return NULL; + return false; } return get_cache_dimensions(file, thread_info, spec, subimage, miplevel); } @@ -3122,7 +3122,7 @@ ImageCacheImpl::get_cache_dimensions(ImageCacheFile* file, { if (!file) { error("Image file handle was NULL"); - return NULL; + return false; } if (!thread_info) thread_info = get_perthread_info(); @@ -3131,23 +3131,23 @@ ImageCacheImpl::get_cache_dimensions(ImageCacheFile* file, if (file->errors_should_issue()) error("Invalid image file \"{}\": {}", file->filename(), file->broken_error_message()); - return NULL; + return false; } if (file->is_udim()) { error("Cannot retrieve ImageSpec of a UDIM-like virtual file"); - return NULL; // UDIM-like files don't have an ImageSpec + return false; // UDIM-like files don't have an ImageSpec } if (subimage < 0 || subimage >= file->subimages()) { if (file->errors_should_issue()) error("Unknown subimage {} (out of {})", subimage, file->subimages()); - return NULL; + return false; } if (miplevel < 0 || miplevel >= file->miplevels(subimage)) { if (file->errors_should_issue()) error("Unknown mip level {} (out of {})", miplevel, file->miplevels(subimage)); - return NULL; + return false; } //! copy dimensions from mip level ImageSpec //! TODO: store nativespec in SubImageInfo, and extra overrides in LevelInfo From 1b8cde5667b25a7c98dc8d2816760325594fe7e4 Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Wed, 25 Sep 2024 15:05:34 -0700 Subject: [PATCH 3/9] add backward compatible inline methods; documentation update Signed-off-by: Basile Fraboni --- src/include/OpenImageIO/imagecache.h | 47 ++++++++++++++++++++++++++-- src/include/OpenImageIO/texture.h | 12 +++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/include/OpenImageIO/imagecache.h b/src/include/OpenImageIO/imagecache.h index f36209fe40..50c96b476a 100644 --- a/src/include/OpenImageIO/imagecache.h +++ b/src/include/OpenImageIO/imagecache.h @@ -724,8 +724,15 @@ class OIIO_API ImageCache { int miplevel, ustring dataname, TypeDesc datatype, void* data); - /// Copy the ImageSpec associated with the named image file. - /// + /// Copy the ImageSpec that describes the named image file. + /// + /// Note that the spec returned describes the file as it exists in the + /// file, at the base (highest-resolution) MIP level of that subimage. + /// Certain aspects of the in-cache representation may differ from the + /// file (due to ImageCache implementation strategy or options like + /// `"forcefloat"` or `"autotile"`). If you really need to know the + /// in-cache data type, tile size, or how the resolution or tiling changes + /// on a particular MIP level, you should use `get_cache_dimensions()`. /// @param filename /// The name of the image, as a UTF-8 encoded ustring. /// @param spec @@ -744,6 +751,21 @@ class OIIO_API ImageCache { bool get_imagespec(ImageHandle* file, Perthread* thread_info, ImageSpec& spec, int subimage = 0); + /// DEPRECATED old API. Note that the miplevel and native parameters are ignored: + /// it will always get the native spec of miplevel 0. We recommend switching to + /// the new API. + bool get_imagespec(ustring filename, ImageSpec& spec, int subimage, + int miplevel, bool native = false) + { + return get_imagespec(filename, spec, subimage); + } + bool get_imagespec(ImageHandle* file, Perthread* thread_info, + ImageSpec& spec, int subimage, int miplevel, + bool native = false) + { + return get_imagespec(file, thread_info, spec, subimage); + } + /// Return a pointer to an ImageSpec associated with the named image file. /// If the file is found and is an image format that can be read, /// otherwise return `nullptr`. @@ -755,6 +777,13 @@ class OIIO_API ImageCache { /// (even other threads) calls `invalidate()` on the file, or /// `invalidate_all()`, or destroys the ImageCache. /// + /// Note that the spec returned describes the file as it exists in the + /// file, at the base (highest-resolution) MIP level of that subimage. + /// Certain aspects of the in-cache representation may differ from the + /// file (due to ImageCache implementation strategy or options like + /// `"forcefloat"` or `"autotile"`). If you really need to know the + /// in-cache data type, tile size, or how the resolution or tiling changes + /// on a particular MIP level, you should use `get_cache_dimensions()`. /// @param filename /// The name of the image, as a UTF-8 encoded ustring. /// @param subimage @@ -770,6 +799,20 @@ class OIIO_API ImageCache { const ImageSpec* imagespec(ImageHandle* file, Perthread* thread_info, int subimage = 0); + /// DEPRECATED old API. Note that the miplevel and native parameters are ignored: + /// it will always get the native spec of miplevel 0. We recommend switching to + /// the new API. + const ImageSpec* imagespec(ustring filename, int subimage, int miplevel, + bool native = false) + { + return imagespec(filename, subimage); + } + const ImageSpec* imagespec(ImageHandle* file, Perthread* thread_info, + int subimage, int miplevel, bool native = false) + { + return imagespec(file, thread_info, subimage); + } + /// Copy the image dimensions (x, y, z, width, height, depth, full*, /// nchannels, format) and data types associated with the named image /// cache file for the specified subimage and miplevel. It does *not* diff --git a/src/include/OpenImageIO/texture.h b/src/include/OpenImageIO/texture.h index f51e7c92f8..0972b41723 100644 --- a/src/include/OpenImageIO/texture.h +++ b/src/include/OpenImageIO/texture.h @@ -1440,6 +1440,18 @@ class OIIO_API TextureSystem { bool get_imagespec(TextureHandle* texture_handle, Perthread* thread_info, ImageSpec& spec, int subimage = 0); + /// DEPRECATED old API. Note that the spec and subimage parameters are + /// inverted. We recommend switching to the new API. + bool get_imagespec(ustring filename, int subimage, ImageSpec& spec) + { + return get_imagespec(filename, spec, subimage); + } + bool get_imagespec(TextureHandle* texture_handle, Perthread* thread_info, + int subimage, ImageSpec& spec) + { + return get_imagespec(texture_handle, thread_info, spec, subimage); + } + /// Return a pointer to an ImageSpec associated with the named texture /// if the file is found and is an image format that can be read, /// otherwise return `nullptr`. From 8e70fc18be67fc618c0979fa7db5de78887b32bd Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Wed, 25 Sep 2024 15:09:52 -0700 Subject: [PATCH 4/9] documentation update Signed-off-by: Basile Fraboni --- src/include/OpenImageIO/imagecache.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/include/OpenImageIO/imagecache.h b/src/include/OpenImageIO/imagecache.h index 50c96b476a..f8ae5cc6cc 100644 --- a/src/include/OpenImageIO/imagecache.h +++ b/src/include/OpenImageIO/imagecache.h @@ -766,7 +766,7 @@ class OIIO_API ImageCache { return get_imagespec(file, thread_info, spec, subimage); } - /// Return a pointer to an ImageSpec associated with the named image file. + /// Return a pointer to an ImageSpec that describes the named image file. /// If the file is found and is an image format that can be read, /// otherwise return `nullptr`. /// @@ -814,11 +814,13 @@ class OIIO_API ImageCache { } /// Copy the image dimensions (x, y, z, width, height, depth, full*, - /// nchannels, format) and data types associated with the named image + /// nchannels, format) and data types that describes the named image /// cache file for the specified subimage and miplevel. It does *not* /// copy arbitrary named metadata or channel names (thus, for an /// `ImageSpec` with lots of metadata, it is much less expensive than - /// copying the whole thing with `operator=()`). + /// copying the whole thing with `operator=()`). The associated + /// metadata and channels names can be retrieved with `imagespec()` + /// or `get_imagespec`. /// /// @param filename /// The name of the image, as a UTF-8 encoded ustring. From 90a297e7c1601a8a4e7bef2329f0b9a0fcc30914 Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Wed, 25 Sep 2024 15:19:09 -0700 Subject: [PATCH 5/9] fix clang-format CI Signed-off-by: Basile Fraboni --- src/testtex/testtex.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/testtex/testtex.cpp b/src/testtex/testtex.cpp index 1c42409fe2..e49dcab078 100644 --- a/src/testtex/testtex.cpp +++ b/src/testtex/testtex.cpp @@ -624,7 +624,7 @@ map_env_latlong(const int& x, const int& y, Imath::Vec3& R, float dphi_dy = float(M_PI) * dv_dy; // dphi_dx = 0 R = Imath::Vec3(fast_sin(phi) * fast_sin(theta), fast_cos(phi), - -fast_sin(phi) * fast_cos(theta)); + -fast_sin(phi) * fast_cos(theta)); dRdx = Imath::Vec3(fast_sin(phi) * fast_cos(theta) * dtheta_dx, float(0.0f), fast_sin(phi) * fast_sin(theta) * dtheta_dx); From 2ed839c9c218bed771f97d28f0de12b682c369a6 Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Fri, 27 Sep 2024 12:06:55 -0700 Subject: [PATCH 6/9] remove redundant imagespec() unit test; add new get_cache_dimensions() unit tests Signed-off-by: Basile Fraboni --- src/libOpenImageIO/imagecache_test.cpp | 64 ++++++++++++++++++++------ 1 file changed, 51 insertions(+), 13 deletions(-) diff --git a/src/libOpenImageIO/imagecache_test.cpp b/src/libOpenImageIO/imagecache_test.cpp index c532545d3b..cb02590f33 100644 --- a/src/libOpenImageIO/imagecache_test.cpp +++ b/src/libOpenImageIO/imagecache_test.cpp @@ -300,12 +300,6 @@ test_imagespec() Strutil::print("imagespec() of non-existant file:\n {}\n", ic->geterror()); } - { // imagespec() for nonexistant file - const ImageSpec* spec = ic->imagespec(ustring("noexist.exr")); - OIIO_CHECK_ASSERT(spec == nullptr && ic->has_error()); - Strutil::print("imagespec() of non-existant file:\n {}\n", - ic->geterror()); - } { // imagespec() for null handle const ImageSpec* spec = ic->imagespec(nullptr, nullptr); OIIO_CHECK_ASSERT(spec == nullptr && ic->has_error()); @@ -318,13 +312,56 @@ test_imagespec() Strutil::print("imagespec() out-of-range subimage:\n {}\n", ic->geterror()); } - //! TODO: get_cache_dimensions tests - // { // imagespec() for out of range mip level - // const ImageSpec* spec = ic->imagespec(checkertex, 0, 100); - // OIIO_CHECK_ASSERT(spec == nullptr && ic->has_error()); - // Strutil::print("imagespec() out-of-range subimage:\n {}\n", - // ic->geterror()); - // } +} + + + +static void +test_get_cache_dimensions() +{ + Strutil::print("\nTesting cache dimensions retrieval\n"); + auto ic = ImageCache::create(); + + { // basic get_cache_dimensions() + ImageSpec spec; + OIIO_CHECK_ASSERT(ic->get_cache_dimensions(checkertex, spec)); + OIIO_CHECK_EQUAL(spec.width, 256); + } + { // basic get_cache_dimensions() with handle + auto hand = ic->get_image_handle(checkertex); + ImageSpec spec; + OIIO_CHECK_ASSERT(ic->get_cache_dimensions(hand, nullptr, spec)); + OIIO_CHECK_EQUAL(spec.width, 256); + } + + { // get_cache_dimensions() for nonexistant file + ImageSpec spec; + OIIO_CHECK_FALSE(ic->get_cache_dimensions(ustring("noexist.exr"), spec)); + OIIO_CHECK_ASSERT(ic->has_error()); + Strutil::print("get_cache_dimensions() of non-existant file:\n {}\n", + ic->geterror()); + } + { // get_cache_dimensions() for null handle + ImageSpec spec; + const bool valid = ic->get_cache_dimensions(nullptr, nullptr, spec); + OIIO_CHECK_ASSERT(!valid && ic->has_error()); + Strutil::print("get_cache_dimensions(handle) of non-existant file:\n {}\n", + ic->geterror()); + } + { // get_cache_dimensions() for out of range subimage + ImageSpec spec; + const bool valid = ic->get_cache_dimensions(checkertex, spec, 10); + OIIO_CHECK_ASSERT(!valid && ic->has_error()); + Strutil::print("get_cache_dimensions() out-of-range subimage:\n {}\n", + ic->geterror()); + } + { // get_cache_dimensions() for out of range mip level + ImageSpec spec; + const bool valid = ic->get_cache_dimensions(checkertex, spec, 0, 100); + OIIO_CHECK_ASSERT(!valid && ic->has_error()); + Strutil::print("get_cache_dimensions() out-of-range miplevel:\n {}\n", + ic->geterror()); + } } @@ -346,6 +383,7 @@ main(int /*argc*/, char* /*argv*/[]) test_get_pixels_errors(); test_custom_threadinfo(); test_imagespec(); + test_get_cache_dimensions(); auto ic = ImageCache::create(); Strutil::print("\n\n{}\n", ic->getstats(5)); From ac0015286f13fae3237e7062f71919993bd0939d Mon Sep 17 00:00:00 2001 From: Basile Fraboni Date: Fri, 27 Sep 2024 12:27:39 -0700 Subject: [PATCH 7/9] fix clang-format Signed-off-by: Basile Fraboni --- src/libOpenImageIO/imagecache_test.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libOpenImageIO/imagecache_test.cpp b/src/libOpenImageIO/imagecache_test.cpp index cb02590f33..e1a9854d4b 100644 --- a/src/libOpenImageIO/imagecache_test.cpp +++ b/src/libOpenImageIO/imagecache_test.cpp @@ -336,7 +336,8 @@ test_get_cache_dimensions() { // get_cache_dimensions() for nonexistant file ImageSpec spec; - OIIO_CHECK_FALSE(ic->get_cache_dimensions(ustring("noexist.exr"), spec)); + OIIO_CHECK_FALSE( + ic->get_cache_dimensions(ustring("noexist.exr"), spec)); OIIO_CHECK_ASSERT(ic->has_error()); Strutil::print("get_cache_dimensions() of non-existant file:\n {}\n", ic->geterror()); @@ -345,8 +346,9 @@ test_get_cache_dimensions() ImageSpec spec; const bool valid = ic->get_cache_dimensions(nullptr, nullptr, spec); OIIO_CHECK_ASSERT(!valid && ic->has_error()); - Strutil::print("get_cache_dimensions(handle) of non-existant file:\n {}\n", - ic->geterror()); + Strutil::print( + "get_cache_dimensions(handle) of non-existant file:\n {}\n", + ic->geterror()); } { // get_cache_dimensions() for out of range subimage ImageSpec spec; From 6426f14d67d97ecaf12cc522e82ae4b3a865d323 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Sat, 28 Sep 2024 12:53:56 -0700 Subject: [PATCH 8/9] Update src/include/OpenImageIO/imagecache.h Slight fix to comment formatting Signed-off-by: Larry Gritz --- src/include/OpenImageIO/imagecache.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/include/OpenImageIO/imagecache.h b/src/include/OpenImageIO/imagecache.h index f8ae5cc6cc..81ecab3eac 100644 --- a/src/include/OpenImageIO/imagecache.h +++ b/src/include/OpenImageIO/imagecache.h @@ -733,6 +733,7 @@ class OIIO_API ImageCache { /// `"forcefloat"` or `"autotile"`). If you really need to know the /// in-cache data type, tile size, or how the resolution or tiling changes /// on a particular MIP level, you should use `get_cache_dimensions()`. + /// /// @param filename /// The name of the image, as a UTF-8 encoded ustring. /// @param spec From dc26c841710a59e39c9f3b8cd4a3b038b7ce0126 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Sat, 28 Sep 2024 12:54:07 -0700 Subject: [PATCH 9/9] Update src/include/OpenImageIO/imagecache.h Slight fix to comment formatting Signed-off-by: Larry Gritz --- src/include/OpenImageIO/imagecache.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/include/OpenImageIO/imagecache.h b/src/include/OpenImageIO/imagecache.h index 81ecab3eac..bdf35f9bd6 100644 --- a/src/include/OpenImageIO/imagecache.h +++ b/src/include/OpenImageIO/imagecache.h @@ -785,6 +785,7 @@ class OIIO_API ImageCache { /// `"forcefloat"` or `"autotile"`). If you really need to know the /// in-cache data type, tile size, or how the resolution or tiling changes /// on a particular MIP level, you should use `get_cache_dimensions()`. + /// /// @param filename /// The name of the image, as a UTF-8 encoded ustring. /// @param subimage