From 7eb3d4fa26ee1f7f5c745e01d837d83dc62226ef Mon Sep 17 00:00:00 2001 From: "cuneyt.ozdas" Date: Thu, 9 Jan 2025 10:12:36 -0800 Subject: [PATCH 1/4] Ticket #2111 - Do not use config proxy for absolute paths while computing file hash or loading LUT data. - Added the unit test provided in the ticket. Signed-off-by: cuneyt.ozdas --- src/OpenColorIO/PathUtils.cpp | 12 ++++++------ src/OpenColorIO/transforms/FileTransform.cpp | 2 +- tests/cpu/OCIOZArchive_tests.cpp | 12 ++++++++++++ 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/OpenColorIO/PathUtils.cpp b/src/OpenColorIO/PathUtils.cpp index bd9fb11351..709416aa64 100644 --- a/src/OpenColorIO/PathUtils.cpp +++ b/src/OpenColorIO/PathUtils.cpp @@ -88,20 +88,20 @@ std::string GetFastFileHash(const std::string & filename, const Context & contex fileHashResultPtr->ready = true; std::string h = ""; - if (!context.getConfigIOProxy()) + if (!pystring::os::path::isabs(filename) && context.getConfigIOProxy()) { - // Default case. - h = g_hashFunction(filename); + // Case for when ConfigIOProxy is used (callbacks mechanism). + h = context.getConfigIOProxy()->getFastLutFileHash(filename.c_str()); } else { - // Case for when ConfigIOProxy is used (callbacks mechanism). - h = context.getConfigIOProxy()->getFastLutFileHash(filename.c_str()); + // Default case + h = g_hashFunction(filename); } fileHashResultPtr->hash = h; } - + hash = fileHashResultPtr->hash; } diff --git a/src/OpenColorIO/transforms/FileTransform.cpp b/src/OpenColorIO/transforms/FileTransform.cpp index faf1d343e3..abcec4b924 100755 --- a/src/OpenColorIO/transforms/FileTransform.cpp +++ b/src/OpenColorIO/transforms/FileTransform.cpp @@ -190,7 +190,7 @@ std::unique_ptr getLutData( const std::string & filepath, std::ios_base::openmode mode) { - if (config.getConfigIOProxy()) + if (!pystring::os::path::isabs(filepath) && config.getConfigIOProxy()) { std::vector buffer = config.getConfigIOProxy()->getLutData(filepath.c_str()); std::stringstream ss; diff --git a/tests/cpu/OCIOZArchive_tests.cpp b/tests/cpu/OCIOZArchive_tests.cpp index 1f23550000..cd54911ad9 100644 --- a/tests/cpu/OCIOZArchive_tests.cpp +++ b/tests/cpu/OCIOZArchive_tests.cpp @@ -331,6 +331,18 @@ OCIO_ADD_TEST(OCIOZArchive, context_test_for_search_paths_and_filetransform_sour auto testPaths = [&mat](const OCIO::ConfigRcPtr & cfg, const OCIO::ContextRcPtr ctx) { + { + const std::string filePath = OCIO::GetTestFilesDir() + "/matrix_example4x4.ctf"; + OCIO::FileTransformRcPtr transform = OCIO::FileTransform::Create(); + transform->setSrc(filePath.c_str()); + OCIO::ConstProcessorRcPtr processor = cfg->getProcessor(transform); + OCIO::ConstTransformRcPtr tr = processor->createGroupTransform()->getTransform(0); + auto mtx = OCIO::DynamicPtrCast(tr); + OCIO_REQUIRE_ASSERT(mtx); + mtx->getMatrix(mat); + OCIO_CHECK_EQUAL(mat[0], 3.24); + } + { // This is independent of the context. OCIO::ConstProcessorRcPtr processor = cfg->getProcessor(ctx, "shot1_lut1_cs", "reference"); From dcfdeffd777324f2d008651b8e0ee325675311c6 Mon Sep 17 00:00:00 2001 From: "cuneyt.ozdas" Date: Fri, 10 Jan 2025 18:35:23 -0800 Subject: [PATCH 2/4] - Changing the logic so that for abs paths we first try the configProxy and if that fails fall back to file system. For relative paths, we don't fall back to file system though, proxy is expected to handle those. - Removed the unnecessary closeLutStream() function. We're using unique pointers, that means RAII is in place. The whole idea behind RAII is we don't need to worry about the cleanup or the type of the object wrapped by the RAII handler (unique_ptr in this case). - Cleaned up some unnecessary conversions, type shuffling and copies around the code I touched. - Cleaned up some unsafe type casts which are prone to dereferencing null pointers. Signed-off-by: cuneyt.ozdas --- src/OpenColorIO/PathUtils.cpp | 8 ++- src/OpenColorIO/transforms/FileTransform.cpp | 74 ++++++++------------ 2 files changed, 35 insertions(+), 47 deletions(-) diff --git a/src/OpenColorIO/PathUtils.cpp b/src/OpenColorIO/PathUtils.cpp index 709416aa64..3a68d366b4 100644 --- a/src/OpenColorIO/PathUtils.cpp +++ b/src/OpenColorIO/PathUtils.cpp @@ -88,10 +88,16 @@ std::string GetFastFileHash(const std::string & filename, const Context & contex fileHashResultPtr->ready = true; std::string h = ""; - if (!pystring::os::path::isabs(filename) && context.getConfigIOProxy()) + if (context.getConfigIOProxy()) { // Case for when ConfigIOProxy is used (callbacks mechanism). h = context.getConfigIOProxy()->getFastLutFileHash(filename.c_str()); + + // For absolute paths, if the proxy does not provide a hash, try the file system. + if (h.empty() && pystring::os::path::isabs(filename)) + { + h = g_hashFunction(filename); + } } else { diff --git a/src/OpenColorIO/transforms/FileTransform.cpp b/src/OpenColorIO/transforms/FileTransform.cpp index abcec4b924..7e59130bec 100755 --- a/src/OpenColorIO/transforms/FileTransform.cpp +++ b/src/OpenColorIO/transforms/FileTransform.cpp @@ -190,37 +190,35 @@ std::unique_ptr getLutData( const std::string & filepath, std::ios_base::openmode mode) { - if (!pystring::os::path::isabs(filepath) && config.getConfigIOProxy()) + if (config.getConfigIOProxy()) { - std::vector buffer = config.getConfigIOProxy()->getLutData(filepath.c_str()); - std::stringstream ss; - ss.write(reinterpret_cast(buffer.data()), buffer.size()); - - return std::unique_ptr( - new std::stringstream(std::move(ss)) - ); - } - - // Default behavior. Return file stream. - return std::unique_ptr( - new std::ifstream(Platform::filenameToUTF(filepath).c_str(), mode) - ); -} + std::vector buffer; + // Try to open through proxy. + try + { + buffer = config.getConfigIOProxy()->getLutData(filepath.c_str()); + } + catch (const std::exception&) + { + // If the path is absolute, we'll try the file system. but otherwise + // nothing to do. + if (!pystring::os::path::isabs(filepath)) + throw; + } -// Close stream returned by getLutData -void closeLutStream(const Config & config, const std::istream & istream) -{ - // No-op when it is using ConfigIOProxy since getLutData returns a vector. - if (config.getConfigIOProxy() == nullptr) - { - // The std::istream is coming from a std::ifstream. - // Pointer cast to ifstream and then close it. - std::ifstream * pIfStream = (std::ifstream *) &istream; - if (pIfStream->is_open()) + // if the buffer is empty, we'll try the file system for abs paths. + if (!buffer.empty() || !pystring::os::path::isabs(filepath)) { - pIfStream->close(); + auto pss = std::make_unique(); + pss->write(reinterpret_cast(buffer.data()), buffer.size()); + + return pss; } } + + // Default behavior. Return file stream. + return std::make_unique( + Platform::filenameToUTF(filepath), mode); } bool CollectContextVariables(const Config &, @@ -635,9 +633,8 @@ void LoadFileUncached(FileFormat * & returnFormat, filepath, tryFormat->isBinary() ? std::ios_base::binary : std::ios_base::in ); - auto & filestream = *pStream; - if (!filestream.good()) + if (!pStream || !pStream->good()) { std::ostringstream os; os << "The specified FileTransform srcfile, '"; @@ -647,7 +644,7 @@ void LoadFileUncached(FileFormat * & returnFormat, throw Exception(os.str().c_str()); } - CachedFileRcPtr cachedFile = tryFormat->read(filestream, filepath, interp); + CachedFileRcPtr cachedFile = tryFormat->read(*pStream, filepath, interp); if(IsDebugLoggingEnabled()) { @@ -660,17 +657,10 @@ void LoadFileUncached(FileFormat * & returnFormat, returnFormat = tryFormat; returnCachedFile = cachedFile; - closeLutStream(config, filestream); - return; } catch(std::exception & e) { - if (pStream) - { - closeLutStream(config, *pStream); - } - primaryErrorText += " '"; primaryErrorText += tryFormat->getName(); primaryErrorText += "' failed with: "; @@ -712,9 +702,8 @@ void LoadFileUncached(FileFormat * & returnFormat, filepath, altFormat->isBinary() ? std::ios_base::binary : std::ios_base::in ); - auto& filestream = *pStream; - if (!filestream.good()) + if (!pStream || !pStream->good()) { std::ostringstream os; os << "The specified FileTransform srcfile, '"; @@ -725,7 +714,7 @@ void LoadFileUncached(FileFormat * & returnFormat, throw Exception(os.str().c_str()); } - cachedFile = altFormat->read(filestream, filepath, interp); + cachedFile = altFormat->read(*pStream, filepath, interp); if(IsDebugLoggingEnabled()) { @@ -738,17 +727,10 @@ void LoadFileUncached(FileFormat * & returnFormat, returnFormat = altFormat; returnCachedFile = cachedFile; - closeLutStream(config, filestream); - return; } catch(std::exception & e) { - if (pStream) - { - closeLutStream(config, *pStream); - } - if(IsDebugLoggingEnabled()) { std::ostringstream os; From 23b3e4dcfb49a2d3fcc70d75969d1d7716266210 Mon Sep 17 00:00:00 2001 From: "cuneyt.ozdas" Date: Fri, 10 Jan 2025 20:12:20 -0800 Subject: [PATCH 3/4] - Ah! make_unique is a c++14 feature and we support C++11. I wonder why windows build is configured to use c++14+ while other platforms use C++11. Replacing make_unique with the new syntax to make the other platforms happy too. Signed-off-by: cuneyt.ozdas --- src/OpenColorIO/transforms/FileTransform.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenColorIO/transforms/FileTransform.cpp b/src/OpenColorIO/transforms/FileTransform.cpp index 7e59130bec..90af424e5a 100755 --- a/src/OpenColorIO/transforms/FileTransform.cpp +++ b/src/OpenColorIO/transforms/FileTransform.cpp @@ -209,7 +209,7 @@ std::unique_ptr getLutData( // if the buffer is empty, we'll try the file system for abs paths. if (!buffer.empty() || !pystring::os::path::isabs(filepath)) { - auto pss = std::make_unique(); + auto pss = std::unique_ptr(new std::stringstream); pss->write(reinterpret_cast(buffer.data()), buffer.size()); return pss; @@ -217,8 +217,8 @@ std::unique_ptr getLutData( } // Default behavior. Return file stream. - return std::make_unique( - Platform::filenameToUTF(filepath), mode); + return std::unique_ptr(new std::ifstream( + Platform::filenameToUTF(filepath), mode)); } bool CollectContextVariables(const Config &, From 9bf4f932bba88162876e8467783476f90ef0078d Mon Sep 17 00:00:00 2001 From: "cuneyt.ozdas" Date: Mon, 13 Jan 2025 10:32:09 -0800 Subject: [PATCH 4/4] - Minor cleanup - Added a test for absolute path to inexistent file. Signed-off-by: cuneyt.ozdas --- src/OpenColorIO/transforms/FileTransform.cpp | 6 ++-- tests/cpu/OCIOZArchive_tests.cpp | 35 +++++++++++++------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/OpenColorIO/transforms/FileTransform.cpp b/src/OpenColorIO/transforms/FileTransform.cpp index 90af424e5a..33da5f3077 100755 --- a/src/OpenColorIO/transforms/FileTransform.cpp +++ b/src/OpenColorIO/transforms/FileTransform.cpp @@ -200,13 +200,13 @@ std::unique_ptr getLutData( } catch (const std::exception&) { - // If the path is absolute, we'll try the file system. but otherwise + // If the path is absolute, we'll try the file system, but otherwise // nothing to do. if (!pystring::os::path::isabs(filepath)) throw; } - // if the buffer is empty, we'll try the file system for abs paths. + // If the buffer is empty, we'll try the file system for abs paths. if (!buffer.empty() || !pystring::os::path::isabs(filepath)) { auto pss = std::unique_ptr(new std::stringstream); @@ -217,7 +217,7 @@ std::unique_ptr getLutData( } // Default behavior. Return file stream. - return std::unique_ptr(new std::ifstream( + return std::unique_ptr(new std::ifstream( Platform::filenameToUTF(filepath), mode)); } diff --git a/tests/cpu/OCIOZArchive_tests.cpp b/tests/cpu/OCIOZArchive_tests.cpp index cd54911ad9..bc1d2c497a 100644 --- a/tests/cpu/OCIOZArchive_tests.cpp +++ b/tests/cpu/OCIOZArchive_tests.cpp @@ -331,18 +331,6 @@ OCIO_ADD_TEST(OCIOZArchive, context_test_for_search_paths_and_filetransform_sour auto testPaths = [&mat](const OCIO::ConfigRcPtr & cfg, const OCIO::ContextRcPtr ctx) { - { - const std::string filePath = OCIO::GetTestFilesDir() + "/matrix_example4x4.ctf"; - OCIO::FileTransformRcPtr transform = OCIO::FileTransform::Create(); - transform->setSrc(filePath.c_str()); - OCIO::ConstProcessorRcPtr processor = cfg->getProcessor(transform); - OCIO::ConstTransformRcPtr tr = processor->createGroupTransform()->getTransform(0); - auto mtx = OCIO::DynamicPtrCast(tr); - OCIO_REQUIRE_ASSERT(mtx); - mtx->getMatrix(mat); - OCIO_CHECK_EQUAL(mat[0], 3.24); - } - { // This is independent of the context. OCIO::ConstProcessorRcPtr processor = cfg->getProcessor(ctx, "shot1_lut1_cs", "reference"); @@ -447,6 +435,29 @@ OCIO_ADD_TEST(OCIOZArchive, context_test_for_search_paths_and_filetransform_sour mtx->getMatrix(mat); OCIO_CHECK_EQUAL(mat[0], 4.); } + + { + // File transform source is an absolute path, not in the archive. + const std::string filePath = + OCIO::GetTestFilesDir() + "/matrix_example4x4.ctf"; + OCIO::FileTransformRcPtr transform = OCIO::FileTransform::Create(); + transform->setSrc(filePath.c_str()); + OCIO::ConstProcessorRcPtr processor = cfg->getProcessor(transform); + OCIO::ConstTransformRcPtr tr = + processor->createGroupTransform()->getTransform(0); + auto mtx = OCIO::DynamicPtrCast(tr); + OCIO_REQUIRE_ASSERT(mtx); + mtx->getMatrix(mat); + OCIO_CHECK_EQUAL(mat[0], 3.24); + } + + { + // File transform source is an abs path but doesn't exist anywhere. + const std::string filePath = OCIO::GetTestFilesDir() + "/missing.ctf"; + OCIO::FileTransformRcPtr transform = OCIO::FileTransform::Create(); + transform->setSrc(filePath.c_str()); + OCIO_CHECK_THROW(cfg->getProcessor(transform), OCIO::Exception); + } }; testPaths(cfgWindowsArchive, ctxWindowsArchive);