From 1d3b69502eeb0f0b1d381d347efcab5b18ae9f3c Mon Sep 17 00:00:00 2001 From: tobim Date: Sun, 12 Nov 2023 02:37:47 +0100 Subject: [PATCH 1/2] Simplify the Findyaml-cpp module (#1891) This fixes compatibility with yaml-cpp 0.8, which previously failed because of a `get_property` call with the wrong target name. I took the liberty to add a few simplifications along the way. Signed-off-by: Tobias Mayer Co-authored-by: Doug Walker --- share/cmake/modules/Findyaml-cpp.cmake | 82 +++++++++++++------------- src/cmake/Config.cmake.in | 7 ++- 2 files changed, 47 insertions(+), 42 deletions(-) diff --git a/share/cmake/modules/Findyaml-cpp.cmake b/share/cmake/modules/Findyaml-cpp.cmake index 5a850f9601..907de3115b 100644 --- a/share/cmake/modules/Findyaml-cpp.cmake +++ b/share/cmake/modules/Findyaml-cpp.cmake @@ -10,10 +10,9 @@ # yaml-cpp_VERSION - Library's version # # Global targets defined by this module: -# yaml-cpp +# yaml-cpp::yaml-cpp # # For compatibility with the upstream CMake package, the following variables and targets are defined: -# yaml-cpp::yaml-cpp - Alias of the yaml-cpp target # YAML_CPP_LIBRARIES - Libraries to link against yaml-cpp # YAML_CPP_INCLUDE_DIR - Include directory # @@ -41,32 +40,39 @@ if(CMAKE_BUILD_TYPE MATCHES "[Dd][Ee][Bb][Uu][Gg]") set(BUILD_TYPE_DEBUG ON) endif() +if(yaml-cpp_FIND_QUIETLY) + set(quiet QUIET) +endif() + if(NOT OCIO_INSTALL_EXT_PACKAGES STREQUAL ALL) - set(_yaml-cpp_REQUIRED_VARS yaml-cpp_LIBRARY) + # Search for yaml-cpp-config.cmake if(NOT DEFINED yaml-cpp_ROOT) - # Search for yaml-cpp-config.cmake - find_package(yaml-cpp ${yaml-cpp_FIND_VERSION} CONFIG QUIET) + find_package(yaml-cpp ${yaml-cpp_FIND_VERSION} CONFIG ${quiet}) endif() if(yaml-cpp_FOUND) - get_target_property(yaml-cpp_LIBRARY yaml-cpp LOCATION) + # Alias target for yaml-cpp < 0.8 compatibility + if(TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp) + add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp) + endif() + + set(yaml-cpp_INCLUDE_DIR ${YAML_CPP_INCLUDE_DIR}) + get_target_property(yaml-cpp_LIBRARY yaml-cpp::yaml-cpp LOCATION) else() # As yaml-cpp-config.cmake search fails, search an installed library # using yaml-cpp.pc . - list(APPEND _yaml-cpp_REQUIRED_VARS yaml-cpp_INCLUDE_DIR yaml-cpp_VERSION) - # Search for yaml-cpp.pc - find_package(PkgConfig QUIET) - pkg_check_modules(PC_yaml-cpp QUIET "yaml-cpp>=${yaml-cpp_FIND_VERSION}") + find_package(PkgConfig ${quiet}) + pkg_check_modules(PC_yaml-cpp ${quiet} "yaml-cpp>=${yaml-cpp_FIND_VERSION}") # Try to detect the version installed, if any. if(NOT PC_yaml-cpp_FOUND) - pkg_search_module(PC_yaml-cpp QUIET "yaml-cpp") + pkg_search_module(PC_yaml-cpp ${quiet} "yaml-cpp") endif() - + # Find include directory find_path(yaml-cpp_INCLUDE_DIR NAMES @@ -91,7 +97,7 @@ if(NOT OCIO_INSTALL_EXT_PACKAGES STREQUAL ALL) # Prefer static lib names set(_yaml-cpp_STATIC_LIB_NAMES "${CMAKE_STATIC_LIBRARY_PREFIX}yaml-cpp${CMAKE_STATIC_LIBRARY_SUFFIX}") - + # Starting from 0.7.0, all platforms uses the suffix "d" for debug. # See https://github.com/jbeder/yaml-cpp/blob/master/CMakeLists.txt#L141 if(BUILD_TYPE_DEBUG) @@ -125,44 +131,40 @@ if(NOT OCIO_INSTALL_EXT_PACKAGES STREQUAL ALL) set(yaml-cpp_FIND_REQUIRED FALSE) endif() + set(YAML_CPP_INCLUDE_DIR "${yaml-cpp_INCLUDE_DIR}") + include(FindPackageHandleStandardArgs) find_package_handle_standard_args(yaml-cpp - REQUIRED_VARS - ${_yaml-cpp_REQUIRED_VARS} + REQUIRED_VARS + yaml-cpp_LIBRARY + yaml-cpp_INCLUDE_DIR + yaml-cpp_VERSION VERSION_VAR yaml-cpp_VERSION ) -endif() - -############################################################################### -### Create target -if(yaml-cpp_FOUND AND NOT TARGET yaml-cpp) - add_library(yaml-cpp UNKNOWN IMPORTED GLOBAL) - set(_yaml-cpp_TARGET_CREATE TRUE) + mark_as_advanced(yaml-cpp_INCLUDE_DIR yaml-cpp_LIBRARY yaml-cpp_VERSION) endif() ############################################################################### -### Configure target ### +### Create target -if(_yaml-cpp_TARGET_CREATE) - set_target_properties(yaml-cpp PROPERTIES +if (NOT TARGET yaml-cpp::yaml-cpp) + add_library(yaml-cpp::yaml-cpp UNKNOWN IMPORTED GLOBAL) + set_target_properties(yaml-cpp::yaml-cpp PROPERTIES IMPORTED_LOCATION ${yaml-cpp_LIBRARY} INTERFACE_INCLUDE_DIRECTORIES ${yaml-cpp_INCLUDE_DIR} ) - mark_as_advanced(yaml-cpp_INCLUDE_DIR yaml-cpp_LIBRARY yaml-cpp_VERSION) -endif() - -############################################################################### -### Set variables for compatibility ### - -if(TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp) - add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp) -endif() - -if(yaml-cpp_INCLUDE_DIR) - set(YAML_CPP_INCLUDE_DIR "${yaml-cpp_INCLUDE_DIR}") -endif() - -set(YAML_CPP_LIBRARIES yaml-cpp::yaml-cpp) + # Required because Installyaml-cpp.cmake creates `yaml-cpp::yaml-cpp` + # as an alias, and aliases get resolved in exported targets, causing the + # find_dependency(yaml-cpp) call in OpenColorIOConfig.cmake to fail. + # This can be removed once Installyaml-cpp.cmake targets yaml-cpp 0.8. + if (NOT TARGET yaml-cpp) + add_library(yaml-cpp ALIAS yaml-cpp::yaml-cpp) + endif () + + # TODO: Remove this variable and use the `yaml-cpp::yaml-cpp` target + # directly when the minimum version of yaml-cpp is updated to 0.8. + set(YAML_CPP_LIBRARIES yaml-cpp::yaml-cpp) +endif () diff --git a/src/cmake/Config.cmake.in b/src/cmake/Config.cmake.in index c122b013c7..4e2367b09b 100644 --- a/src/cmake/Config.cmake.in +++ b/src/cmake/Config.cmake.in @@ -34,15 +34,18 @@ if (NOT @BUILD_SHARED_LIBS@) # NOT @BUILD_SHARED_LIBS@ find_dependency(pystring @pystring_VERSION@) endif() - if (NOT TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp) + if (NOT TARGET yaml-cpp::yaml-cpp) find_dependency(yaml-cpp @yaml-cpp_VERSION@) + if (TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp) + add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp) + endif() endif() if (NOT TARGET ZLIB::ZLIB) # ZLIB_VERSION is available starting CMake 3.26+. # ZLIB_VERSION_STRING is still available for backward compatibility. # See https://cmake.org/cmake/help/git-stage/module/FindZLIB.html - + if (@ZLIB_VERSION@) # @ZLIB_VERSION@ find_dependency(ZLIB @ZLIB_VERSION@) else() From 52b496528ca42e0c16dfebb33a8efbef0795f55d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89ric=20Renaud-Houde?= Date: Mon, 13 Nov 2023 20:47:11 -0500 Subject: [PATCH 2/2] Skip processor concatenation if the display color space is also data. (#1896) * Skip processor concatenation if the display view transform is also data. Signed-off-by: Eric Renaud-Houde * Moved missing display color space exception before processor creation. Signed-off-by: Eric Renaud-Houde --------- Signed-off-by: Eric Renaud-Houde Co-authored-by: Doug Walker --- src/OpenColorIO/Config.cpp | 15 ++++++++++++--- tests/cpu/Config_tests.cpp | 8 ++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/OpenColorIO/Config.cpp b/src/OpenColorIO/Config.cpp index a17d1dc288..ccfb134b45 100644 --- a/src/OpenColorIO/Config.cpp +++ b/src/OpenColorIO/Config.cpp @@ -4719,20 +4719,29 @@ ConstProcessorRcPtr Config::GetProcessorFromConfigs(const ConstContextRcPtr & sr "the source color space."); } + const char* csName = dstConfig->getDisplayViewColorSpaceName(dstDisplay, dstView); + const char* displayColorSpaceName = View::UseDisplayName(csName) ? dstDisplay : csName; + ConstColorSpaceRcPtr displayColorSpace = dstConfig->getColorSpace(displayColorSpaceName); + if (!displayColorSpace) + { + throw Exception("Can't create the processor for the destination config: " + "display color space not found."); + } + auto p2 = dstConfig->getProcessor(dstContext, dstInterchangeName, dstDisplay, dstView, direction); if (!p2) { throw Exception("Can't create the processor for the destination config " - "and the destination color space."); + "and the destination display view transform."); } ProcessorRcPtr processor = Processor::Create(); processor->getImpl()->setProcessorCacheFlags(srcConfig->getImpl()->m_cacheFlags); - // If the source color spaces is a data space, its corresponding processor + // If either of the color spaces are data spaces, its corresponding processor // will be empty, but need to make sure the entire result is also empty to // better match the semantics of how data spaces are handled. - if (!srcColorSpace->isData()) + if (!srcColorSpace->isData() && !displayColorSpace->isData()) { if (direction == TRANSFORM_DIR_INVERSE) { diff --git a/tests/cpu/Config_tests.cpp b/tests/cpu/Config_tests.cpp index 5d30fda5fc..59d1472dbb 100644 --- a/tests/cpu/Config_tests.cpp +++ b/tests/cpu/Config_tests.cpp @@ -5924,6 +5924,7 @@ ocio_profile_version: 2 displayname: - ! {name: view1, colorspace: displaytest1} - ! {name: view2, view_transform: vt1, display_colorspace: display2} + - ! {name: view3, colorspace: data_space} view_transforms: - ! @@ -6166,6 +6167,13 @@ ocio_profile_version: 2 auto ff4 = OCIO_DYNAMIC_POINTER_CAST(t4); OCIO_CHECK_ASSERT(ff4); + // If one of the spaces is a data space, the whole result must be a no-op. + OCIO_CHECK_NO_THROW(p = OCIO::Config::GetProcessorFromConfigs( + config2, "test2", config1, "displayname", "view3", OCIO::TRANSFORM_DIR_FORWARD)); + OCIO_REQUIRE_ASSERT(p); + group = p->createGroupTransform(); + OCIO_REQUIRE_EQUAL(group->getNumTransforms(), 0); + constexpr const char * SIMPLE_CONFIG3{ R"( ocio_profile_version: 2