Skip to content

Commit

Permalink
Fix named transform validation issue (#1829)
Browse files Browse the repository at this point in the history 
Signed-off-by: Doug Walker <[email protected]>
Co-authored-by: Michael Dolan <[email protected]>
  • Loading branch information
@doug-walker @michdolan
doug-walker and michdolan authored Aug 29, 2023
1 parent de6d62d commit 96f528f
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 37 deletions.
61 changes: 34 additions & 27 deletions src/OpenColorIO/Config.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,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 ";
Expand Down Expand Up @@ -711,7 +711,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.";
Expand All @@ -724,7 +724,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.";
Expand All @@ -742,7 +742,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.";
Expand All @@ -752,7 +752,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.";
Expand Down Expand Up @@ -1416,7 +1416,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());
Expand All @@ -1429,7 +1429,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 $.";
Expand All @@ -1442,7 +1442,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();
Expand Down Expand Up @@ -1474,7 +1474,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 $.";
Expand All @@ -1485,7 +1485,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.";
Expand Down Expand Up @@ -1653,7 +1653,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();
Expand All @@ -1678,7 +1678,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());
Expand Down Expand Up @@ -1811,7 +1811,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.";

Expand All @@ -1823,17 +1823,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());
Expand All @@ -1851,7 +1857,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();
Expand All @@ -1862,7 +1868,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();
Expand All @@ -1875,7 +1881,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, '";

Expand Down Expand Up @@ -1964,14 +1970,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;
}

Expand Down Expand Up @@ -2000,6 +2006,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());
}
Expand All @@ -2015,8 +2025,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)
{
Expand Down Expand Up @@ -2045,9 +2055,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;
Expand Down
17 changes: 12 additions & 5 deletions tests/cpu/Config_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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"));
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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.");
}

Expand Down
66 changes: 66 additions & 0 deletions tests/cpu/NamedTransform_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
- !<Rule> {name: Default, colorspace: raw}
displays:
sRGB:
- !<View> {name: Raw, colorspace: raw}
Rec.2100-PQ - Display:
- !<View> {name: test_view, view_transform: vt, display_colorspace: Rec.2100-PQ - Display}
view_transforms:
- !<ViewTransform>
name: vt
from_scene_reference: !<ColorSpaceTransform> {src: nt, dst: cs2}
display_colorspaces:
- !<ColorSpace>
name: Rec.2100-PQ - Display
isdata: false
from_display_reference: !<BuiltinTransform> {style: DISPLAY - CIE-XYZ-D65_to_REC.2100-PQ}
colorspaces:
- !<ColorSpace>
name: raw
isdata: true
- !<ColorSpace>
name: cs2
isdata: false
from_scene_reference: !<MatrixTransform> {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 ]}
- !<ColorSpace>
name: cs3
isdata: false
from_scene_reference: !<ColorSpaceTransform> {src: nt_alias, dst: cs2}
- !<ColorSpace>
name: cs4
isdata: false
from_scene_reference: !<DisplayViewTransform> {src: nt_alias, display: Rec.2100-PQ - Display, view: test_view}
named_transforms:
- !<NamedTransform>
name: nt
aliases: [nt_alias]
transform: !<GroupTransform>
children:
- !<MatrixTransform> {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
{
Expand Down
4 changes: 2 additions & 2 deletions tests/cpu/transforms/DisplayViewTransform_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."
);

Expand Down Expand Up @@ -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."
);
}
Expand Down
6 changes: 3 additions & 3 deletions tests/python/ConfigTest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1474,7 +1474,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.")

Expand All @@ -1490,7 +1490,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.")
Expand All @@ -1501,7 +1501,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.")

0 comments on commit 96f528f

Please sign in to comment.