Adsk Contrib - Data bypass fix (#1229)

* Data bypass fix Signed-off-by: Bernard Lefebvre <bernard.lefebvre@autodesk.com> * beta 2 Signed-off-by: Bernard Lefebvre <bernard.lefebvre@autodesk.com> * Adjust some comments Signed-off-by: Bernard Lefebvre <bernard.lefebvre@autodesk.com> Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>
AcademySoftwareFoundation · Dec 5, 2020 · a712243 · a712243
1 parent e412d8a
commit a712243
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 15 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -34,7 +34,7 @@ project(OpenColorIO
     LANGUAGES CXX C)
 
 # "dev", "beta1", rc1", etc or "" for an official release.
-set(OpenColorIO_VERSION_RELEASE_TYPE "beta1")
+set(OpenColorIO_VERSION_RELEASE_TYPE "beta2")
 
 enable_testing()
 

diff --git a/src/OpenColorIO/Config.cpp b/src/OpenColorIO/Config.cpp
@@ -3622,17 +3622,21 @@ void Config::setFileRules(ConstFileRulesRcPtr fileRules)
 
 const char * Config::getColorSpaceFromFilepath(const char * filePath) const
 {
-    return getImpl()->m_fileRules->getImpl()->getColorSpaceFromFilepath(*this, filePath);
+    return getImpl()->m_fileRules->getImpl()->getColorSpaceFromFilepath(*this,
+                                                                        filePath ? filePath : "");
 }
 
 const char * Config::getColorSpaceFromFilepath(const char * filePath, size_t & ruleIndex) const
 {
-    return getImpl()->m_fileRules->getImpl()->getColorSpaceFromFilepath(*this, filePath, ruleIndex);
+    return getImpl()->m_fileRules->getImpl()->getColorSpaceFromFilepath(*this,
+                                                                        filePath ? filePath : "",
+                                                                        ruleIndex);
 }
 
 bool Config::filepathOnlyMatchesDefaultRule(const char * filePath) const
 {
-    return getImpl()->m_fileRules->getImpl()->filepathOnlyMatchesDefaultRule(*this, filePath);
+    return getImpl()->m_fileRules->getImpl()->filepathOnlyMatchesDefaultRule(*this,
+                                                                             filePath ? filePath : "");
 }
 
 

diff --git a/src/OpenColorIO/Processor.cpp b/src/OpenColorIO/Processor.cpp
@@ -583,7 +583,9 @@ void Processor::Impl::setColorSpaceConversion(const Config & config,
         throw Exception("Internal error: Processor should be empty");
     }
 
-    BuildColorSpaceOps(m_ops, config, context, srcColorSpace, dstColorSpace, false);
+    // Default behavior is to bypass data color space. ColorSpaceTransform can be used to not bypass
+    // data color spaces.
+    BuildColorSpaceOps(m_ops, config, context, srcColorSpace, dstColorSpace, true);
 
     std::ostringstream desc;
     desc << "Color space conversion from " << srcColorSpace->getName()

diff --git a/src/OpenColorIO/transforms/LookTransform.cpp b/src/OpenColorIO/transforms/LookTransform.cpp
@@ -289,8 +289,9 @@ void RunLookTokens(OpRcPtrVec & ops,
             if (!skipColorSpaceConversion &&
                 currentColorSpace.get() != processColorSpace.get())
             {
+                // Default behavior is to bypass data color space.
                 BuildColorSpaceOps(ops, config, context,
-                                   currentColorSpace, processColorSpace, false);
+                                   currentColorSpace, processColorSpace, true);
                 currentColorSpace = processColorSpace;
             }
 
@@ -352,8 +353,8 @@ void BuildLookOps(OpRcPtrVec & ops,
     // If current color space is already the dst space skip the conversion.
     if (!skipColorSpaceConversion && currentColorSpace.get() != dst.get())
     {
-        BuildColorSpaceOps(ops, config, context,
-                           currentColorSpace, dst, false);
+        // Default behavior is to bypass data color space.
+        BuildColorSpaceOps(ops, config, context, currentColorSpace, dst, true);
     }
 }
 

diff --git a/src/apps/ociochecklut/main.cpp b/src/apps/ociochecklut/main.cpp
@@ -218,8 +218,8 @@ int main (int argc, const char* argv[])
         {
             // What are the allowed formats?
             std::cout << "Formats supported:" << std::endl;
-            const auto nbFromats = OCIO::FileTransform::GetNumFormats();
-            for (int i = 0; i < nbFromats; ++i)
+            const auto nbFormats = OCIO::FileTransform::GetNumFormats();
+            for (int i = 0; i < nbFormats; ++i)
             {
                 std::cout << OCIO::FileTransform::GetFormatNameByIndex(i);
                 std::cout << " (." << OCIO::FileTransform::GetFormatExtensionByIndex(i) << ")";

diff --git a/tests/cpu/FileRules_tests.cpp b/tests/cpu/FileRules_tests.cpp
@@ -489,6 +489,10 @@ OCIO_ADD_TEST(FileRules, rules_filepattern)
     OCIO_CHECK_EQUAL(rulePosition, 1); // Default rule.
     config->getColorSpaceFromFilepath("/An/Arbitrary.exr/Path/MyFileexr", rulePosition);
     OCIO_CHECK_EQUAL(rulePosition, 1); // Default rule.
+    config->getColorSpaceFromFilepath("", rulePosition);
+    OCIO_CHECK_EQUAL(rulePosition, 1); // Default rule.
+    config->getColorSpaceFromFilepath(nullptr, rulePosition);
+    OCIO_CHECK_EQUAL(rulePosition, 1); // Default rule.
 
     OCIO_CHECK_NO_THROW(rules->setPattern(0, "gamma"));
     config->setFileRules(rules);
@@ -986,7 +990,11 @@ strictparsing: true
     auto rules = config->getFileRules();
     OCIO_REQUIRE_ASSERT(rules);
     OCIO_REQUIRE_EQUAL(rules->getNumEntries(), 1);
-    // File defined default role is preserved.
+    OCIO_CHECK_EQUAL(std::string(rules->getName(0)), OCIO::FileRuleUtils::DefaultName);
+    OCIO_CHECK_EQUAL(std::string(rules->getColorSpace(0)), "cs1");
+    OCIO_CHECK_EQUAL(std::string(config->getColorSpaceFromFilepath("anything")), "cs1");
+
+    // The color space of the default role is preserved.
     auto cs = config->getColorSpace(OCIO::ROLE_DEFAULT);
     OCIO_CHECK_EQUAL(std::string("raw"), cs->getName());
 }

diff --git a/tests/cpu/transforms/ColorSpaceTransform_tests.cpp b/tests/cpu/transforms/ColorSpaceTransform_tests.cpp
@@ -137,12 +137,27 @@ OCIO_ADD_TEST(ColorSpaceTransform, build_colorspace_ops)
                                                      *cst, OCIO::TRANSFORM_DIR_FORWARD));
         OCIO_CHECK_NO_THROW(ops.validate());
         OCIO_CHECK_EQUAL(ops.size(), 0);
+        ops.clear();
+
+        OCIO::ConstProcessorRcPtr proc;
+        config->setProcessorCacheFlags(OCIO::PROCESSOR_CACHE_OFF); // Cache disabled
+        OCIO_CHECK_NO_THROW(proc = config->getProcessor(cst));
+        OCIO_CHECK_EQUAL(proc->getNumTransforms(), 0);
 
         cst->setDataBypass(false);
         OCIO_CHECK_NO_THROW(OCIO::BuildColorSpaceOps(ops, *config, config->getCurrentContext(),
                                                      *cst, OCIO::TRANSFORM_DIR_FORWARD));
         OCIO_CHECK_NO_THROW(ops.validate());
         OCIO_CHECK_EQUAL(ops.size(), 4);
+        ops.clear();
+
+        // Some of the ops are no-ops, processor has 2 transforms.
+        OCIO_CHECK_NO_THROW(proc = config->getProcessor(cst));
+        OCIO_CHECK_EQUAL(proc->getNumTransforms(), 2);
+
+        // Similar test with color space, data by-pass can't be controlled.
+        OCIO_CHECK_NO_THROW(proc = config->getProcessor(src.c_str(), dst.c_str()));
+        OCIO_CHECK_EQUAL(proc->getNumTransforms(), 0);
 
         // Restore default data flags.
         csSceneToRef->setIsData(false);
@@ -153,17 +168,28 @@ OCIO_ADD_TEST(ColorSpaceTransform, build_colorspace_ops)
         csSceneFromRef->setIsData(true);
         config->addColorSpace(csSceneFromRef);
 
-        ops.clear();
         OCIO_CHECK_NO_THROW(OCIO::BuildColorSpaceOps(ops, *config, config->getCurrentContext(),
                                                      *cst, OCIO::TRANSFORM_DIR_FORWARD));
         OCIO_CHECK_NO_THROW(ops.validate());
         OCIO_CHECK_EQUAL(ops.size(), 0);
+        ops.clear();
+
+        OCIO_CHECK_NO_THROW(proc = config->getProcessor(cst));
+        OCIO_CHECK_EQUAL(proc->getNumTransforms(), 0);
 
         cst->setDataBypass(false);
         OCIO_CHECK_NO_THROW(OCIO::BuildColorSpaceOps(ops, *config, config->getCurrentContext(),
                                                      *cst, OCIO::TRANSFORM_DIR_FORWARD));
         OCIO_CHECK_NO_THROW(ops.validate());
         OCIO_CHECK_EQUAL(ops.size(), 4);
+        ops.clear();
+
+        OCIO_CHECK_NO_THROW(proc = config->getProcessor(cst));
+        OCIO_CHECK_EQUAL(proc->getNumTransforms(), 2);
+
+        // Similar test with color space, data bypass can't be controlled.
+        OCIO_CHECK_NO_THROW(proc = config->getProcessor(src.c_str(), dst.c_str()));
+        OCIO_CHECK_EQUAL(proc->getNumTransforms(), 0);
 
         // Restore default data flags.
         csSceneFromRef->setIsData(false);

diff --git a/tests/cpu/transforms/LookTransform_tests.cpp b/tests/cpu/transforms/LookTransform_tests.cpp
@@ -624,7 +624,7 @@ search_path: luts
 
     OCIO::OpRcPtrVec ops;
     OCIO_CHECK_NO_THROW(BuildColorSpaceOps(ops, *config, config->getCurrentContext(),
-                                           srcColorSpace, dstColorSpace, false));
+                                           srcColorSpace, dstColorSpace, true));
     OCIO_CHECK_NO_THROW(ops.validate());
     OCIO_REQUIRE_EQUAL(ops.size(), 11);
     OCIO::ConstOpRcPtr op = ops[0];
@@ -665,7 +665,7 @@ search_path: luts
     // Test in inverse direction.
     ops.clear();
     OCIO_CHECK_NO_THROW(BuildColorSpaceOps(ops, *config, config->getCurrentContext(),
-                                           dstColorSpace, srcColorSpace, false));
+                                           dstColorSpace, srcColorSpace, true));
     OCIO_REQUIRE_EQUAL(ops.size(), 11);
     OCIO_CHECK_NO_THROW(ops.validate());
     op = ops[0];
@@ -711,7 +711,7 @@ search_path: luts
 
     OCIO::OpRcPtrVec ops2;
     OCIO_CHECK_NO_THROW(BuildColorSpaceOps(ops2, *config, config->getCurrentContext(),
-                                           dstColorSpaceInv, srcColorSpace, false));
+                                           dstColorSpaceInv, srcColorSpace, true));
     OCIO_REQUIRE_EQUAL(ops2.size(), ops.size());
     OCIO_CHECK_NO_THROW(ops2.validate());
     for (std::size_t i = 0; i < ops2.size(); ++i)