From 0e5ea4e91f1201baa500007b31ab2ed65aedf697 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Fri, 25 Aug 2023 01:04:26 -0400 Subject: [PATCH] Fix named transform validation issue Signed-off-by: Doug Walker --- src/OpenColorIO/Config.cpp | 61 +++++++++-------- tests/cpu/Config_tests.cpp | 17 +++-- tests/cpu/NamedTransform_tests.cpp | 66 +++++++++++++++++++ .../transforms/DisplayViewTransform_tests.cpp | 4 +- tests/python/ConfigTest.py | 6 +- 5 files changed, 117 insertions(+), 37 deletions(-) diff --git a/src/OpenColorIO/Config.cpp b/src/OpenColorIO/Config.cpp index 305db0596a..8276149be0 100644 --- a/src/OpenColorIO/Config.cpp +++ b/src/OpenColorIO/Config.cpp @@ -219,7 +219,7 @@ StringUtils::StringVec GetViewNames(const ViewPtrVec & views) std::ostringstream GetDisplayViewPrefixErrorMsg(const std::string & display, const View & view) { std::ostringstream oss; - oss << "Config failed validation. "; + oss << "Config failed display view validation. "; if (display.empty()) { oss << "Shared "; @@ -709,7 +709,7 @@ class Config::Impl if (viewIt != viewsOfDisplay.end()) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed view validation. "; os << "The display '" << display << "' "; os << "contains a shared view '" << sharedView; os << "' that is already defined as a view."; @@ -722,7 +722,7 @@ class Config::Impl if (sharedViewIt == m_sharedViews.end()) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed view validation. "; os << "The display '" << display << "' "; os << "contains a shared view '" << sharedView; os << "' that is not defined."; @@ -740,7 +740,7 @@ class Config::Impl if (!displayCS) { std::ostringstream os; - os << "Config failed validation. The display '" << display << "' "; + os << "Config failed view validation. The display '" << display << "' "; os << "contains a shared view '" << (*sharedViewIt).m_name; os << "' which does not define a color space and there is " "no color space that matches the display name."; @@ -750,7 +750,7 @@ class Config::Impl if (displayCS->getReferenceSpaceType() != REFERENCE_SPACE_DISPLAY) { std::ostringstream os; - os << "Config failed validation. The display '" << display << "' "; + os << "Config failed view validation. The display '" << display << "' "; os << "contains a shared view '" << (*sharedViewIt).m_name; os << "that refers to a color space, '" << display << "', "; os << "that is not a display-referred color space."; @@ -1400,7 +1400,7 @@ void Config::validate() const if (!cs) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed color space validation. "; os << "The color space at index " << i << " is null."; getImpl()->m_validationtext = os.str(); throw Exception(getImpl()->m_validationtext.c_str()); @@ -1413,7 +1413,7 @@ void Config::validate() const if (getMajorVersion() >= 2 && ContainsContextVariableToken(name)) { std::ostringstream oss; - oss << "Config failed sanitycheck. " + oss << "Config failed color space validation. " << "A color space name '" << name << "' cannot contain a context variable reserved token i.e. % or $."; @@ -1426,7 +1426,7 @@ void Config::validate() const if (numAliases && getMajorVersion() < 2) { std::ostringstream oss; - oss << "Config failed sanitycheck. " + oss << "Config failed color space validation. " << "Aliases may not be used in a v1 config. Color space name: '" << name << "'."; getImpl()->m_validationtext = oss.str(); @@ -1458,7 +1458,7 @@ void Config::validate() const if (getMajorVersion() >= 2 && ContainsContextVariableToken(iter->first)) { std::ostringstream oss; - oss << "Config failed sanitycheck. " + oss << "Config failed role validation. " << "A role name '" << iter->first << "' cannot contain a context variable reserved token i.e. % or $."; @@ -1469,7 +1469,7 @@ void Config::validate() const if(!getImpl()->hasColorSpace(iter->second.c_str())) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed role validation. "; os << "The role '" << iter->first << "' "; os << "refers to a color space, '" << iter->second << "', "; os << "which is not defined."; @@ -1637,7 +1637,7 @@ void Config::validate() const if(views.empty() && sharedViews.empty()) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed display validation. "; os << "The display '" << display << "' "; os << "does not define any views."; getImpl()->m_validationtext = os.str(); @@ -1662,7 +1662,7 @@ void Config::validate() const if (numdisplays == 0) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed display validation. "; os << "No displays are specified."; getImpl()->m_validationtext = os.str(); throw Exception(getImpl()->m_validationtext.c_str()); @@ -1795,7 +1795,7 @@ void Config::validate() const if (ContainsContextVariables(name.c_str())) { std::ostringstream oss; - oss << "Config failed sanitycheck. " + oss << "Config failed transform validation. " << "This config references a color space '" << name << "' using an unknown context variable."; @@ -1807,17 +1807,23 @@ void Config::validate() const const char * csname = LookupRole(getImpl()->m_roles, name); std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed transform validation. "; os << "This config references a color space, '"; if (!csname || !*csname) { - os << name << "', which is not defined."; - getImpl()->m_validationtext = os.str(); - throw Exception(getImpl()->m_validationtext.c_str()); + // It's not a role, check to see if it's a named transform. + if (!getImpl()->getNamedTransform(name.c_str())) + { + // It's not a color space, a role, or a named transform. + os << name << "', which is not defined."; + getImpl()->m_validationtext = os.str(); + throw Exception(getImpl()->m_validationtext.c_str()); + } } else if(!getImpl()->hasColorSpace(csname)) { + // It's a role, but the color space it points to doesn't exist. os << csname << "' (for role '" << name << "'), which is not defined."; getImpl()->m_validationtext = os.str(); throw Exception(getImpl()->m_validationtext.c_str()); @@ -1835,7 +1841,7 @@ void Config::validate() const if (!lookName || !*lookName) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed Look validation. "; os << "The look at index '" << i << "' "; os << "does not specify a name."; getImpl()->m_validationtext = os.str(); @@ -1846,7 +1852,7 @@ void Config::validate() const if (!processSpace || !*processSpace) { std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed Look validation. "; os << "The look '" << lookName << "' "; os << "does not specify a process space."; getImpl()->m_validationtext = os.str(); @@ -1859,7 +1865,7 @@ void Config::validate() const const char * csname = LookupRole(getImpl()->m_roles, processSpace); std::ostringstream os; - os << "Config failed validation. "; + os << "Config failed Look validation. "; os << "The look '" << lookName << "' "; os << "specifies a process color space, '"; @@ -1948,14 +1954,14 @@ void Config::validate() const if (!files.empty()) { bool foundOne = false; - std::string errMsg("Config failed sanitycheck."); + std::string errMsg("Config failed search path validation."); for (int idx = 0; idx < getImpl()->m_context->getNumSearchPaths(); ++idx) { const char * path = getImpl()->m_context->getSearchPath(idx); if (!path || !*path) { - errMsg += " The search_path is empty."; + errMsg += " The search_path must not be an empty string if there are FileTransforms."; continue; } @@ -1984,6 +1990,10 @@ void Config::validate() const // After looping over all the search paths, none of them can be successfully resolved. if (!foundOne) { + if (getImpl()->m_context->getNumSearchPaths() == 0) + { + errMsg += " The search_path must not be empty if there are FileTransforms."; + } getImpl()->m_validationtext = errMsg; throw Exception(errMsg.c_str()); } @@ -1999,8 +2009,8 @@ void Config::validate() const if (resolvedFile.empty() || ContainsContextVariables(resolvedFile)) { std::ostringstream oss; - oss << "Config failed sanitycheck. "; - oss << "The file Transform source cannot be resolved: '"; + oss << "Config failed validation expanding file transform paths. "; + oss << "The file transform source cannot be resolved: '"; if (file != resolvedFile) { @@ -2029,9 +2039,6 @@ void Config::validate() const { const char * name = nt->getName(); - // AddColorSpace, addNamedTransform & setRole already check there is not name - // conflict. - if (getLook(name)) { std::ostringstream os; diff --git a/tests/cpu/Config_tests.cpp b/tests/cpu/Config_tests.cpp index 95e798fe0f..31e9747d51 100644 --- a/tests/cpu/Config_tests.cpp +++ b/tests/cpu/Config_tests.cpp @@ -1344,7 +1344,7 @@ OCIO_ADD_TEST(Config, context_variable_with_sanity_check) OCIO_CHECK_THROW_WHAT(cfg->validate(), OCIO::Exception, - "The file Transform source cannot be resolved: '$CS2'."); + "The file transform source cannot be resolved: '$CS2'."); OCIO_CHECK_THROW_WHAT(cfg->getProcessor("cs1", "disp1", "view1", OCIO::TRANSFORM_DIR_FORWARD), OCIO::Exception, "The specified file reference '$CS2' could not be located"); @@ -1356,16 +1356,23 @@ OCIO_ADD_TEST(Config, context_variable_with_sanity_check) // Several faulty cases for the 'search_path'. OCIO_CHECK_NO_THROW(cfg->clearSearchPaths()); + OCIO_CHECK_THROW_WHAT(cfg->validate(), + OCIO::Exception, + "The search_path must not be empty if there are FileTransforms."); + + OCIO_CHECK_NO_THROW(cfg->clearSearchPaths()); + // NB: Not sure this is desirable, but setting a nullptr is the same as setting "". + // In this case, getNumSearchtPaths == 1, which is potentially confusing. OCIO_CHECK_NO_THROW(cfg->setSearchPath(nullptr)); OCIO_CHECK_THROW_WHAT(cfg->validate(), OCIO::Exception, - "The search_path is empty"); + "The search_path must not be an empty string if there are FileTransforms."); OCIO_CHECK_NO_THROW(cfg->clearSearchPaths()); OCIO_CHECK_NO_THROW(cfg->setSearchPath("")); OCIO_CHECK_THROW_WHAT(cfg->validate(), OCIO::Exception, - "The search_path is empty"); + "The search_path must not be an empty string if there are FileTransforms."); OCIO_CHECK_NO_THROW(cfg->clearSearchPaths()); OCIO_CHECK_NO_THROW(cfg->setSearchPath("$MYPATH")); @@ -1441,7 +1448,7 @@ OCIO_ADD_TEST(Config, context_variable_with_colorspacename) OCIO_CHECK_NO_THROW(cfg = OCIO::Config::CreateFromStream(iss)->createEditableCopy()); OCIO_CHECK_THROW_WHAT(cfg->validate(), OCIO::Exception, - "The file Transform source cannot be resolved: '$VAR3'."); + "The file transform source cannot be resolved: '$VAR3'."); // Set $VAR3 and check again. @@ -5220,7 +5227,7 @@ OCIO_ADD_TEST(Config, remove_color_space) // As discussed only validation traps the issue. OCIO_CHECK_THROW_WHAT(config->validate(), OCIO::Exception, - "Config failed validation. The role 'default' refers to"\ + "Config failed role validation. The role 'default' refers to"\ " a color space, 'raw', which is not defined."); } diff --git a/tests/cpu/NamedTransform_tests.cpp b/tests/cpu/NamedTransform_tests.cpp index 3ffae1db3f..3730fd046a 100644 --- a/tests/cpu/NamedTransform_tests.cpp +++ b/tests/cpu/NamedTransform_tests.cpp @@ -1216,6 +1216,72 @@ active_views: [] } } +OCIO_ADD_TEST(Config, colorspace_transform_named_transform) +{ + // Validate Config::validate() on config with ColorSpace or DisplayView Transforms, + // or ViewTransforms that reference a Named Transform. + + constexpr const char * OCIO_CONFIG{ R"( +ocio_profile_version: 2 + +file_rules: + - ! {name: Default, colorspace: raw} + +displays: + sRGB: + - ! {name: Raw, colorspace: raw} + Rec.2100-PQ - Display: + - ! {name: test_view, view_transform: vt, display_colorspace: Rec.2100-PQ - Display} + +view_transforms: + - ! + name: vt + from_scene_reference: ! {src: nt, dst: cs2} + +display_colorspaces: + - ! + name: Rec.2100-PQ - Display + isdata: false + from_display_reference: ! {style: DISPLAY - CIE-XYZ-D65_to_REC.2100-PQ} + +colorspaces: + - ! + name: raw + isdata: true + + - ! + name: cs2 + isdata: false + from_scene_reference: ! {matrix: [ 2.041587903811, -0.565006974279, -0.344731350778, 0, -0.969243636281, 1.875967501508, 0.041555057407, 0, 0.013444280632, -0.118362392231, 1.015174994391, 0, 0, 0, 0, 1 ]} + + - ! + name: cs3 + isdata: false + from_scene_reference: ! {src: nt_alias, dst: cs2} + + - ! + name: cs4 + isdata: false + from_scene_reference: ! {src: nt_alias, display: Rec.2100-PQ - Display, view: test_view} + +named_transforms: + - ! + name: nt + aliases: [nt_alias] + transform: ! + children: + - ! {matrix: [1.49086870465701, -0.268712979082956, -0.222155725704626, 0, -0.0792372106028327, 1.1793685831111, -0.100131372460806, 0, 0.00277810076707935, -0.0304336146315336, 1.02765551391237, 0, 0, 0, 0, 1]} +)" }; + + const std::string configStr = std::string(OCIO_CONFIG); + + std::istringstream is; + is.str(configStr); + + OCIO::ConstConfigRcPtr config; + OCIO_CHECK_NO_THROW(config = OCIO::Config::CreateFromStream(is)); + OCIO_CHECK_NO_THROW(config->validate()); +} namespace { diff --git a/tests/cpu/transforms/DisplayViewTransform_tests.cpp b/tests/cpu/transforms/DisplayViewTransform_tests.cpp index a29eaf1dc9..50ae88ec12 100644 --- a/tests/cpu/transforms/DisplayViewTransform_tests.cpp +++ b/tests/cpu/transforms/DisplayViewTransform_tests.cpp @@ -1303,7 +1303,7 @@ ocio_profile_version: 2 // As with most of these, validation also fails. OCIO_CHECK_THROW_WHAT(e_config->validate(), OCIO::Exception, - "Config failed validation. Display 'display' has a view 'bad_view' that " + "Config failed display view validation. Display 'display' has a view 'bad_view' that " "refers to a color space or a named transform, 'missing cs', which is not defined." ); @@ -1336,7 +1336,7 @@ ocio_profile_version: 2 // But validation fails. OCIO_CHECK_THROW_WHAT(e_config->validate(), OCIO::Exception, - "Config failed validation. Display 'display' has a view 'bad_view' refers " + "Config failed display view validation. Display 'display' has a view 'bad_view' refers " "to a viewing rule, 'missing rule', which is not defined." ); } diff --git a/tests/python/ConfigTest.py b/tests/python/ConfigTest.py index ebc170c3df..4f25904b0f 100644 --- a/tests/python/ConfigTest.py +++ b/tests/python/ConfigTest.py @@ -1468,7 +1468,7 @@ def test_virtual_display_exceptions(self): cfg.addVirtualDisplaySharedView('sview2') with self.assertRaises(OCIO.Exception) as cm: cfg.validate() - self.assertEqual(str(cm.exception), "Config failed validation. " + + self.assertEqual(str(cm.exception), "Config failed view validation. " + "The display 'virtual_display' contains a shared " + "view 'sview2' that is not defined.") @@ -1484,7 +1484,7 @@ def test_virtual_display_exceptions(self): cfg.addVirtualDisplayView('Raw1', 'Film', 'raw1') with self.assertRaises(OCIO.Exception) as cm: cfg.validate() - self.assertEqual(str(cm.exception), "Config failed validation. " + + self.assertEqual(str(cm.exception), "Config failed display view validation. " + "Display 'virtual_display' has a " + "view 'Raw1' that refers to a color space" + " or a named transform, 'raw1', which is not defined.") @@ -1495,7 +1495,7 @@ def test_virtual_display_exceptions(self): cfg.addVirtualDisplayView('Raw1', 'Film', 'raw1', 'look') with self.assertRaises(OCIO.Exception) as cm: cfg.validate() - self.assertEqual(str(cm.exception), "Config failed validation. " + + self.assertEqual(str(cm.exception), "Config failed display view validation. " + "Display 'virtual_display' has a view 'Raw1' that " + "refers to a color space or a named transform, " + "'raw1', which is not defined.")