From fbb91d6ab08122f321edacec1e34e41b1682d964 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Wed, 15 Mar 2023 21:44:13 -0400 Subject: [PATCH 1/3] Fix error message in DisplayViewTransform Signed-off-by: Doug Walker --- .../transforms/DisplayViewTransform.cpp | 55 ++++++--- .../transforms/DisplayViewTransform_tests.cpp | 113 ++++++++++++++++++ 2 files changed, 153 insertions(+), 15 deletions(-) diff --git a/src/OpenColorIO/transforms/DisplayViewTransform.cpp b/src/OpenColorIO/transforms/DisplayViewTransform.cpp index fd2b95c0ce..629008cf3b 100644 --- a/src/OpenColorIO/transforms/DisplayViewTransform.cpp +++ b/src/OpenColorIO/transforms/DisplayViewTransform.cpp @@ -302,6 +302,9 @@ void BuildDisplayOps(OpRcPtrVec & ops, // a view_transform plus display_colorspace. It is permitted to substitute a named_transform // for either the colorspace or the view_transform. + // Most of the error checking below is also done in Config::validate(), but it's useful to + // do here since there is no guarantee that validate will be called. + // Validate src color space. const std::string srcColorSpaceName = displayViewTransform.getSrc(); ConstColorSpaceRcPtr srcColorSpace = config.getColorSpace(srcColorSpaceName.c_str()); @@ -353,39 +356,59 @@ void BuildDisplayOps(OpRcPtrVec & ops, } // Get the color space associated to the (display, view) pair. + // (Returns an empty string if the view does not exist. This is trapped below.) const char * csName = config.getDisplayViewColorSpaceName(display.c_str(), view.c_str()); - // A shared view containing a view transform may set the color space to USE_DISPLAY_NAME, + // A shared view containing a view transform may set the color space to , // in which case we look for a display color space with the same name as the display. const std::string displayColorSpaceName = View::UseDisplayName(csName) ? display : csName; + + // At this point, displayColorSpaceName is typically one of the following strings: + // 1. The "colorspace" attribute of the View, if there is no "view_transform". + // 2. The name of the View's display, if it's a shared_view and the "display_colorspace" + // is "". It is expected this will also be the name of a display + // color space in the config. + // 3. Else, the "display_colorspace" string if it's a View with a "view_transform". + // + // (Though, in the implementation, both the "colorspace" and "display_colorspace" of + // a View are stored in the same variable and there is currently no validation to ensure + // for example that "display_colorspace" is not used if there is no view transform. + // So it's really the presence or absence of the "view_transform" that determines how + // the View's color space variable is interpreted.) + ConstColorSpaceRcPtr displayColorSpace = config.getColorSpace(displayColorSpaceName.c_str()); - ConstNamedTransformRcPtr displayNamedTransform; + ConstNamedTransformRcPtr csNamedTransform; if (!displayColorSpace) { if (displayColorSpaceName.empty()) { std::ostringstream os; - os << "DisplayViewTransform error."; - os << " Display color space name is unspecified."; + os << "DisplayViewTransform error." << " The display '"; + os << display << "' does not have view '" << view << "'."; throw Exception(os.str().c_str()); } - // If there is a view transform, the display color space can't be a named transform. + // Note: If there is a view transform, the display color space isn't allowed to be a + // named transform, hence the error message only says "color space". if (viewTransform || viewNamedTransform) { - // Config::validate catch this. std::ostringstream os; os << "DisplayViewTransform error." << " The view '"; - os << viewTransformName << "' refers to a display color space '"; + os << view << "' refers to a display color space '"; os << displayColorSpaceName << "' that can't be found."; throw Exception(os.str().c_str()); } - displayNamedTransform = config.getNamedTransform(displayColorSpaceName.c_str()); - if (!displayNamedTransform) + + // At this point, there is no view transform, so displayColorSpaceName is typically + // from the "colorspace" attribute of the View, not the "display_colorspace". The + // former may be a named transform but the latter may not (since a view transform + // is present.) + csNamedTransform = config.getNamedTransform(displayColorSpaceName.c_str()); + if (!csNamedTransform) { std::ostringstream os; os << "DisplayViewTransform error."; - os << " Cannot find color space or named transform, named '"; + os << " Cannot find color space or named transform with name '"; os << displayColorSpaceName << "'."; throw Exception(os.str().c_str()); } @@ -426,11 +449,11 @@ void BuildDisplayOps(OpRcPtrVec & ops, BuildLookOps(ops, currentCS, false, config, context, looks); } - if (displayNamedTransform) + if (csNamedTransform) { // Ignore currentCS. The forward direction NamedTransform is used for the forward // direction DisplayViewTransform. - auto transform = NamedTransform::GetTransform(displayNamedTransform, + auto transform = NamedTransform::GetTransform(csNamedTransform, TRANSFORM_DIR_FORWARD); BuildOps(ops, config, context, transform, TRANSFORM_DIR_FORWARD); } @@ -446,7 +469,9 @@ void BuildDisplayOps(OpRcPtrVec & ops, } else { - // Apply the conversion from the current color space to the display color space. + // This is the simple case where the View only had the "colorspace" attribute, + // which is referred to here as displayColorSpace. Apply the conversion to there + // from the current color space (which may vary if a look was applied). BuildColorSpaceOps(ops, config, context, currentCS, displayColorSpace, dataBypass); } break; @@ -465,10 +490,10 @@ void BuildDisplayOps(OpRcPtrVec & ops, vtSourceCS = config.getColorSpace(csRes); } - if (displayNamedTransform) + if (csNamedTransform) { // Ignore currentCS. - auto transform = NamedTransform::GetTransform(displayNamedTransform, + auto transform = NamedTransform::GetTransform(csNamedTransform, TRANSFORM_DIR_INVERSE); BuildOps(ops, config, context, transform, TRANSFORM_DIR_FORWARD); } diff --git a/tests/cpu/transforms/DisplayViewTransform_tests.cpp b/tests/cpu/transforms/DisplayViewTransform_tests.cpp index e222aa7c4a..d4a0ab3953 100644 --- a/tests/cpu/transforms/DisplayViewTransform_tests.cpp +++ b/tests/cpu/transforms/DisplayViewTransform_tests.cpp @@ -1219,6 +1219,119 @@ ocio_profile_version: 2 groupProc = groupProc->getOptimizedProcessor(OCIO::BIT_DEPTH_F32, OCIO::BIT_DEPTH_F32, OCIO::OPTIMIZATION_DEFAULT); OCIO_CHECK_ASSERT(groupProc->isNoOp()); + + // + // Check that the correct error message is thrown in various scenarios. + // + + dt->setSrc("displayCSIn"); + dt->setDisplay("display"); + dt->setView("view"); + + // Empty arguments get handled in DisplayViewTransform::validate. + + // Display name is empty. + dt->setDisplay(""); + OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform: empty display name." + ); + dt->setDisplay("display"); + + // View name is empty. + dt->setView(""); + OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform: empty view name." + ); + dt->setView("view"); + + // Source CS is empty. + dt->setSrc(""); + OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform: empty source color space name." + ); + + // More detailed error handling is in DisplayViewTransform::BuildDisplayOps. + + // Source CS doesn't exist in the config. + dt->setSrc("missing cs"); + OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform error. Cannot find source color space named 'missing cs'." + ); + dt->setSrc("displayCSIn"); + + // Display doesn't exist in the config. + dt->setDisplay("missing display"); + OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform error. Display 'missing display' not found." + ); + dt->setDisplay("display"); + + OCIO::ConfigRcPtr e_config = config->createEditableCopy(); + + // View references a view transform that doesn't exist in the config. + OCIO_CHECK_NO_THROW(e_config->addDisplayView("display", "bad_view", "missing vt", + "displayCSOut", "", "", "")); + dt->setView("bad_view"); + OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform error. The view transform 'missing vt' is neither a view transform nor a named transform." + ); + + // View doesn't exist in the config. + dt->setView("missing view"); + OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform error. The display 'display' does not have view 'missing view'." + ); + + // View references a "display_colorspace" that doesn't exist in the config. + OCIO_CHECK_NO_THROW(e_config->addDisplayView("display", "bad_view", "display_vt", + "missing cs", "", "", "")); + dt->setView("bad_view"); + OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform error. The view 'bad_view' refers to a display color space 'missing cs' that can't be found." + ); + // 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 refers to a color space or a named transform, 'missing cs', which is not defined." + ); + + // View references a "colorspace" that doesn't exist in the config. + OCIO_CHECK_NO_THROW(e_config->addDisplayView("display", "bad_view", "missing cs", "")); + dt->setView("bad_view"); + OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), + OCIO::Exception, + "DisplayViewTransform error. Cannot find color space or named transform with name 'missing cs'." + ); + + // Check a few more scenarios. + + // Missing look. + OCIO_CHECK_NO_THROW(e_config->addDisplayView("display", "bad_view", "display_vt", + "displayCSOut", "missing look", "", "")); + dt->setView("bad_view"); + OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), + OCIO::Exception, + "RunLookTokens error. The specified look, 'missing look', cannot be found. (looks: look)." + ); + dt->setView("view"); + + // Missing viewing rule does not currently throw when getting a processor. + OCIO_CHECK_NO_THROW(e_config->addDisplayView("display", "bad_view", "display_vt", + "displayCSOut", "", "missing rule", "desc: foo")); + OCIO_CHECK_NO_THROW(e_config->getProcessor(dt)); + // But validation fails. + OCIO_CHECK_THROW_WHAT(e_config->validate(), + OCIO::Exception, + "Config failed validation. Display 'display' has a view 'bad_view' refers to a viewing rule, 'missing rule', which is not defined." + ); } OCIO_ADD_TEST(DisplayViewTransform, context_variables) From 34de7946c5ca1c85c943768df7465c1545034825 Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Wed, 15 Mar 2023 21:58:17 -0400 Subject: [PATCH 2/3] Update test in Config_tests.cpp Signed-off-by: Doug Walker --- tests/cpu/Config_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cpu/Config_tests.cpp b/tests/cpu/Config_tests.cpp index 76b668fc8e..362c69fc55 100644 --- a/tests/cpu/Config_tests.cpp +++ b/tests/cpu/Config_tests.cpp @@ -1672,7 +1672,7 @@ OCIO_ADD_TEST(Config, context_variable_with_display_view) OCIO_CHECK_THROW_WHAT(config->getProcessor("cs1", "disp1", "view1", OCIO::TRANSFORM_DIR_FORWARD), OCIO::Exception, "DisplayViewTransform error. Cannot find color space or " - "named transform, named '$ENV1'."); + "named transform with name '$ENV1'."); } } From a93f1dd271c3f47a10069391de4b97587990e9ff Mon Sep 17 00:00:00 2001 From: Doug Walker Date: Tue, 21 Mar 2023 22:15:26 -0400 Subject: [PATCH 3/3] Improve indentation Signed-off-by: Doug Walker --- .../transforms/DisplayViewTransform_tests.cpp | 55 +++++++++++-------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/tests/cpu/transforms/DisplayViewTransform_tests.cpp b/tests/cpu/transforms/DisplayViewTransform_tests.cpp index d4a0ab3953..a29eaf1dc9 100644 --- a/tests/cpu/transforms/DisplayViewTransform_tests.cpp +++ b/tests/cpu/transforms/DisplayViewTransform_tests.cpp @@ -1233,24 +1233,24 @@ ocio_profile_version: 2 // Display name is empty. dt->setDisplay(""); OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform: empty display name." + OCIO::Exception, + "DisplayViewTransform: empty display name." ); dt->setDisplay("display"); // View name is empty. dt->setView(""); OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform: empty view name." + OCIO::Exception, + "DisplayViewTransform: empty view name." ); dt->setView("view"); // Source CS is empty. dt->setSrc(""); OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform: empty source color space name." + OCIO::Exception, + "DisplayViewTransform: empty source color space name." ); // More detailed error handling is in DisplayViewTransform::BuildDisplayOps. @@ -1258,16 +1258,16 @@ ocio_profile_version: 2 // Source CS doesn't exist in the config. dt->setSrc("missing cs"); OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform error. Cannot find source color space named 'missing cs'." + OCIO::Exception, + "DisplayViewTransform error. Cannot find source color space named 'missing cs'." ); dt->setSrc("displayCSIn"); // Display doesn't exist in the config. dt->setDisplay("missing display"); OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform error. Display 'missing display' not found." + OCIO::Exception, + "DisplayViewTransform error. Display 'missing display' not found." ); dt->setDisplay("display"); @@ -1278,15 +1278,17 @@ ocio_profile_version: 2 "displayCSOut", "", "", "")); dt->setView("bad_view"); OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform error. The view transform 'missing vt' is neither a view transform nor a named transform." + OCIO::Exception, + "DisplayViewTransform error. The view transform 'missing vt' is neither " + "a view transform nor a named transform." ); // View doesn't exist in the config. dt->setView("missing view"); OCIO_CHECK_THROW_WHAT(config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform error. The display 'display' does not have view 'missing view'." + OCIO::Exception, + "DisplayViewTransform error. The display 'display' does not have " + "view 'missing view'." ); // View references a "display_colorspace" that doesn't exist in the config. @@ -1294,21 +1296,24 @@ ocio_profile_version: 2 "missing cs", "", "", "")); dt->setView("bad_view"); OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform error. The view 'bad_view' refers to a display color space 'missing cs' that can't be found." + OCIO::Exception, + "DisplayViewTransform error. The view 'bad_view' refers to a display " + "color space 'missing cs' that can't be found." ); // 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 refers to a color space or a named transform, 'missing cs', which is not defined." + OCIO::Exception, + "Config failed validation. Display 'display' has a view 'bad_view' that " + "refers to a color space or a named transform, 'missing cs', which is not defined." ); // View references a "colorspace" that doesn't exist in the config. OCIO_CHECK_NO_THROW(e_config->addDisplayView("display", "bad_view", "missing cs", "")); dt->setView("bad_view"); OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), - OCIO::Exception, - "DisplayViewTransform error. Cannot find color space or named transform with name 'missing cs'." + OCIO::Exception, + "DisplayViewTransform error. Cannot find color space or named transform " + "with name 'missing cs'." ); // Check a few more scenarios. @@ -1318,8 +1323,9 @@ ocio_profile_version: 2 "displayCSOut", "missing look", "", "")); dt->setView("bad_view"); OCIO_CHECK_THROW_WHAT(e_config->getProcessor(dt), - OCIO::Exception, - "RunLookTokens error. The specified look, 'missing look', cannot be found. (looks: look)." + OCIO::Exception, + "RunLookTokens error. The specified look, 'missing look', cannot be " + "found. (looks: look)." ); dt->setView("view"); @@ -1329,8 +1335,9 @@ ocio_profile_version: 2 OCIO_CHECK_NO_THROW(e_config->getProcessor(dt)); // But validation fails. OCIO_CHECK_THROW_WHAT(e_config->validate(), - OCIO::Exception, - "Config failed validation. Display 'display' has a view 'bad_view' refers to a viewing rule, 'missing rule', which is not defined." + OCIO::Exception, + "Config failed validation. Display 'display' has a view 'bad_view' refers " + "to a viewing rule, 'missing rule', which is not defined." ); }