File interpolation v1 compatibility (#1215)

Signed-off-by: Bernard Lefebvre <bernard.lefebvre@autodesk.com>
(cherry picked from commit 30a6b0f39acb3304950e7c70e46327206acb433f)

Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>

include/OpenColorIO/OpenColorTransforms.h

@@ -999,6 +999,12 @@ class OCIOEXPORT FileTransform : public Transform
      */
     void setCDLStyle(CDLStyle);
 
+    /**
+     * The file parsers that care about interpolation (LUTs) will try to make use of the requested
+     * interpolation method when loading the file.  In these cases, if the requested method could
+     * not be used, a warning is logged.  If no method is provided, or a method cannot be used,
+     * INTERP_DEFAULT is used.
+     */
     Interpolation getInterpolation() const;
     void setInterpolation(Interpolation interp);
 

src/OpenColorIO/OCIOYaml.cpp

@@ -1442,7 +1442,7 @@ inline void load(const YAML::Node& node, FileTransformRcPtr& t)
     }
 }
 
-inline void save(YAML::Emitter& out, ConstFileTransformRcPtr t)
+inline void save(YAML::Emitter& out, ConstFileTransformRcPtr t, unsigned int majorVersion)
 {
     out << YAML::VerbatimTag("FileTransform");
     out << YAML::Flow << YAML::BeginMap;
@@ -1456,11 +1456,20 @@ inline void save(YAML::Emitter& out, ConstFileTransformRcPtr t)
     {
         out << YAML::Key << "cdl_style" << YAML::Value << CDLStyleToString(t->getCDLStyle());
     }
-    if (t->getInterpolation() != INTERP_UNKNOWN && t->getInterpolation() != INTERP_DEFAULT)
+    Interpolation interp = t->getInterpolation();
+    if (majorVersion == 1 && interp == INTERP_DEFAULT)
+    {
+        // The DEFAULT method is not available in a v1 library.  If the v1 config is read by a v1
+        // library and the file is a LUT, a missing interp would end up set to UNKNOWN and a
+        // throw would happen when the processor is built.  Setting to LINEAR to provide more
+        // robust compatibility.
+        interp = INTERP_LINEAR;
+    }
+    if (interp != INTERP_DEFAULT)
     {
         out << YAML::Key << "interpolation";
         out << YAML::Value;
-        save(out, t->getInterpolation());
+        save(out, interp);
     }
 
     EmitBaseTransformKeyValues(out, t);
@@ -2356,7 +2365,7 @@ inline void save(YAML::Emitter & out, ConstGradingToneTransformRcPtr t)
 // GroupTransform
 
 void load(const YAML::Node& node, TransformRcPtr& t);
-void save(YAML::Emitter& out, ConstTransformRcPtr t);
+void save(YAML::Emitter& out, ConstTransformRcPtr t, unsigned int majorVersion);
 
 inline void load(const YAML::Node& node, GroupTransformRcPtr& t)
 {
@@ -2412,7 +2421,7 @@ inline void load(const YAML::Node& node, GroupTransformRcPtr& t)
     }
 }
 
-inline void save(YAML::Emitter& out, ConstGroupTransformRcPtr t)
+inline void save(YAML::Emitter& out, ConstGroupTransformRcPtr t, unsigned int majorVersion)
 {
     out << YAML::VerbatimTag("GroupTransform");
     out << YAML::BeginMap;
@@ -2426,7 +2435,7 @@ inline void save(YAML::Emitter& out, ConstGroupTransformRcPtr t)
     out << YAML::BeginSeq;
     for(int i = 0; i < t->getNumTransforms(); ++i)
     {
-        save(out, t->getTransform(i));
+        save(out, t->getTransform(i), majorVersion);
     }
     out << YAML::EndSeq;
 
@@ -3217,7 +3226,7 @@ void load(const YAML::Node& node, TransformRcPtr& t)
     }
 }
 
-void save(YAML::Emitter& out, ConstTransformRcPtr t)
+void save(YAML::Emitter& out, ConstTransformRcPtr t, unsigned int majorVersion)
 {
     if(ConstAllocationTransformRcPtr Allocation_tran = \
         DynamicPtrCast<const AllocationTransform>(t))
@@ -3243,7 +3252,7 @@ void save(YAML::Emitter& out, ConstTransformRcPtr t)
     else if(ConstFileTransformRcPtr File_tran = \
         DynamicPtrCast<const FileTransform>(t))
         // TODO: if (File_tran->getCDLStyle() != CDL_TRANSFORM_DEFAULT) should throw with config v1.
-        save(out, File_tran);
+        save(out, File_tran, majorVersion);
     else if (ConstExposureContrastTransformRcPtr File_tran = \
         DynamicPtrCast<const ExposureContrastTransform>(t))
         save(out, File_tran);
@@ -3261,7 +3270,7 @@ void save(YAML::Emitter& out, ConstTransformRcPtr t)
         save(out, GT_tran);
     else if(ConstGroupTransformRcPtr Group_tran = \
         DynamicPtrCast<const GroupTransform>(t))
-        save(out, Group_tran);
+        save(out, Group_tran, majorVersion);
     else if(ConstLogAffineTransformRcPtr Log_tran = \
         DynamicPtrCast<const LogAffineTransform>(t))
         save(out, Log_tran);
@@ -3419,7 +3428,7 @@ inline void load(const YAML::Node& node, ColorSpaceRcPtr& cs)
     }
 }
 
-inline void save(YAML::Emitter& out, ConstColorSpaceRcPtr cs)
+inline void save(YAML::Emitter& out, ConstColorSpaceRcPtr cs, unsigned int majorVersion)
 {
     out << YAML::VerbatimTag("ColorSpace");
     out << YAML::BeginMap;
@@ -3465,14 +3474,14 @@ inline void save(YAML::Emitter& out, ConstColorSpaceRcPtr cs)
     if(toref)
     {
         out << YAML::Key << (isDisplay ? "to_display_reference" : "to_reference") << YAML::Value;
-        save(out, toref);
+        save(out, toref, majorVersion);
     }
 
     ConstTransformRcPtr fromref = cs->getTransform(COLORSPACE_DIR_FROM_REFERENCE);
     if(fromref)
     {
         out << YAML::Key << (isDisplay ? "from_display_reference" : "from_reference") << YAML::Value;
-        save(out, fromref);
+        save(out, fromref, majorVersion);
     }
 
     out << YAML::EndMap;
@@ -3533,7 +3542,7 @@ inline void load(const YAML::Node& node, LookRcPtr& look)
     }
 }
 
-inline void save(YAML::Emitter& out, ConstLookRcPtr look)
+inline void save(YAML::Emitter& out, ConstLookRcPtr look, unsigned int majorVersion)
 {
     out << YAML::VerbatimTag("Look");
     out << YAML::BeginMap;
@@ -3545,14 +3554,14 @@ inline void save(YAML::Emitter& out, ConstLookRcPtr look)
     {
         out << YAML::Key << "transform";
         out << YAML::Value;
-        save(out, look->getTransform());
+        save(out, look->getTransform(), majorVersion);
     }
 
     if(look->getInverseTransform())
     {
         out << YAML::Key << "inverse_transform";
         out << YAML::Value;
-        save(out, look->getInverseTransform());
+        save(out, look->getInverseTransform(), majorVersion);
     }
 
     out << YAML::EndMap;
@@ -3694,7 +3703,7 @@ inline void load(const YAML::Node & node, ViewTransformRcPtr & vt)
     }
 }
 
-inline void save(YAML::Emitter & out, ConstViewTransformRcPtr & vt)
+inline void save(YAML::Emitter & out, ConstViewTransformRcPtr & vt, unsigned int majorVersion)
 {
     out << YAML::VerbatimTag("ViewTransform");
     out << YAML::BeginMap;
@@ -3723,14 +3732,14 @@ inline void save(YAML::Emitter & out, ConstViewTransformRcPtr & vt)
     if (toref)
     {
         out << YAML::Key << (isDisplay ? "to_display_reference" : "to_reference") << YAML::Value;
-        save(out, toref);
+        save(out, toref, majorVersion);
     }
 
     ConstTransformRcPtr fromref = vt->getTransform(VIEWTRANSFORM_DIR_FROM_REFERENCE);
     if (fromref)
     {
         out << YAML::Key << (isDisplay ? "from_display_reference" : "from_reference") << YAML::Value;
-        save(out, fromref);
+        save(out, fromref, majorVersion);
     }
 
     out << YAML::EndMap;
@@ -4878,7 +4887,7 @@ inline void save(YAML::Emitter & out, const Config & config)
         for(int i = 0; i < config.getNumLooks(); ++i)
         {
             const char* name = config.getLookNameByIndex(i);
-            save(out, config.getLook(name));
+            save(out, config.getLook(name), configMajorVersion);
         }
         out << YAML::EndSeq;
         out << YAML::Newline;
@@ -4895,7 +4904,7 @@ inline void save(YAML::Emitter & out, const Config & config)
         {
             auto name = config.getViewTransformNameByIndex(i);
             auto vt = config.getViewTransform(name);
-            save(out, vt);
+            save(out, vt, configMajorVersion);
         }
         out << YAML::EndSeq;
     }
@@ -4933,7 +4942,7 @@ inline void save(YAML::Emitter & out, const Config & config)
         out << YAML::Value << YAML::BeginSeq;
         for (const auto & cs : displayCS)
         {
-            save(out, cs);
+            save(out, cs, configMajorVersion);
         }
         out << YAML::EndSeq;
     }
@@ -4945,7 +4954,7 @@ inline void save(YAML::Emitter & out, const Config & config)
         out << YAML::Value << YAML::BeginSeq;
         for (const auto & cs : sceneCS)
         {
-            save(out, cs);
+            save(out, cs, configMajorVersion);
         }
         out << YAML::EndSeq;
     }

src/OpenColorIO/transforms/FileTransform.cpp

@@ -97,13 +97,8 @@ void FileTransform::validate() const
         throw Exception("FileTransform: empty file path");
     }
 
-    if (getInterpolation() == INTERP_UNKNOWN)
-    {
-        std::ostringstream oss;
-        oss << "FileTransform can't use unknown interpolation. ";
-        oss << "File: '" << std::string(getImpl()->m_src) << "'.";
-        throw Exception(oss.str().c_str());
-    }
+    // NB: Not validating interpolation since v1 configs such as the spi examples use
+    // interpolation=unknown.  So that is a legal usage, even if it makes no sense.
 }
 
 const char * FileTransform::getSrc() const

tests/cpu/Config_tests.cpp

@@ -162,7 +162,7 @@ OCIO_ADD_TEST(Config, simple_config)
         "        children:\n"
         "          - !<FileTransform>\n"
         "            src: diffusemult.spimtx\n"
-        "            interpolation: best\n"
+        "            interpolation: unknown\n"
         "          - !<ColorSpaceTransform>\n"
         "            src: raw\n"
         "            dst: lnh\n"
@@ -393,7 +393,7 @@ OCIO_ADD_TEST(Config, serialize_group_transform)
     "    from_reference: !<GroupTransform>\n"
     "      children:\n"
     "        - !<FileTransform> {src: \"\"}\n"
-    "        - !<FileTransform> {src: \"\"}\n"
+    "        - !<FileTransform> {src: \"\", interpolation: unknown}\n"
     "        - !<FileTransform> {src: \"\", interpolation: best}\n"
     "        - !<FileTransform> {src: \"\", interpolation: nearest}\n"
     "        - !<FileTransform> {src: \"\", interpolation: cubic}\n"
@@ -4333,6 +4333,55 @@ OCIO_ADD_TEST(Config, file_transform_serialization)
     OCIO_CHECK_EQUAL(oss.str(), str);
 }
 
+OCIO_ADD_TEST(Config, file_transform_serialization_v1)
+{
+    OCIO::ConfigRcPtr cfg;
+    OCIO_CHECK_NO_THROW(cfg = OCIO::Config::Create());
+    OCIO_REQUIRE_ASSERT(cfg);
+    cfg->setMajorVersion(1);
+    auto ft = OCIO::FileTransform::Create();
+    ft->setSrc("file");
+    auto cs = OCIO::ColorSpace::Create();
+    // Note that ft has no interpolation set.  In a v2 config, this is not a problem and is taken
+    // to mean default interpolation.  However, in this case the config version is 1 and if the
+    // config were read by a v1 library (rather than v2), this could cause a failure.  So the
+    // interp is set to linear during serialization to avoid problems.
+    cs->setTransform(ft, OCIO::COLORSPACE_DIR_TO_REFERENCE);
+    ft->setSrc("other");
+    ft->setInterpolation(OCIO::INTERP_TETRAHEDRAL);
+    cs->setTransform(ft, OCIO::COLORSPACE_DIR_FROM_REFERENCE);
+    cs->setName("cs");
+    cfg->addColorSpace(cs);
+    std::ostringstream os;
+    cfg->serialize(os);
+    OCIO_CHECK_EQUAL(os.str(), R"(ocio_profile_version: 1
+
+search_path: ""
+strictparsing: true
+luma: [0.2126, 0.7152, 0.0722]
+
+roles:
+  {}
+
+displays:
+  {}
+
+active_displays: []
+active_views: []
+
+colorspaces:
+  - !<ColorSpace>
+    name: cs
+    family: ""
+    equalitygroup: ""
+    bitdepth: unknown
+    isdata: false
+    allocation: uniform
+    to_reference: !<FileTransform> {src: file, interpolation: linear}
+    from_reference: !<FileTransform> {src: other, interpolation: tetrahedral}
+)" );
+}
+
 OCIO_ADD_TEST(Config, add_color_space)
 {
     // The unit test validates that the color space is correctly added to the configuration.

tests/cpu/transforms/FileTransform_tests.cpp

@@ -8,6 +8,7 @@
 
 #include "ContextVariableUtils.h"
 #include "testutils/UnitTest.h"
+#include "UnitTestLogUtils.h"
 #include "UnitTestUtils.h"
 
 namespace OCIO = OCIO_NAMESPACE;
@@ -296,16 +297,58 @@ OCIO_ADD_TEST(FileTransform, validate)
     tr->setSrc("lut3d_17x17x17_32f_12i.clf");
     OCIO_CHECK_NO_THROW(tr->validate());
 
-    tr->setInterpolation(OCIO::INTERP_UNKNOWN);
+    tr->setSrc("");
     OCIO_CHECK_THROW_WHAT(tr->validate(), OCIO::Exception,
-                          "FileTransform can't use unknown interpolation");
+                          "FileTransform: empty file path");
+}
+
+OCIO_ADD_TEST(FileTransform, interpolation_validity)
+{
+    OCIO::ConfigRcPtr cfg;
+    OCIO_CHECK_NO_THROW(cfg = OCIO::Config::CreateRaw()->createEditableCopy());
+    cfg->setSearchPath(OCIO::GetTestFilesDir().c_str());
+    OCIO_CHECK_NO_THROW(cfg->validate());
+
+    OCIO::FileTransformRcPtr tr = OCIO::FileTransform::Create();
+    tr->setSrc("lut1d_1.spi1d");
 
-    tr->setInterpolation(OCIO::INTERP_BEST);
     OCIO_CHECK_NO_THROW(tr->validate());
 
-    tr->setSrc("");
-    OCIO_CHECK_THROW_WHAT(tr->validate(), OCIO::Exception,
-                          "FileTransform: empty file path");
+    // File transform with format requiring a valid interpolation using default interpolation.
+    OCIO_CHECK_NO_THROW(cfg->getProcessor(tr));
+
+    // UNKNOWN can't be used by a LUT file, so the interp on the LUT is set to DEFAULT and a
+    // warning is logged. 
+
+    tr->setInterpolation(OCIO::INTERP_UNKNOWN);
+    OCIO_CHECK_NO_THROW(tr->validate());
+    {
+        OCIO::LogGuard log;
+        OCIO::SetLoggingLevel(OCIO::LOGGING_LEVEL_WARNING);
+        OCIO_CHECK_NO_THROW(cfg->getProcessor(tr));
+        OCIO_CHECK_EQUAL(log.output(), "[OpenColorIO Warning]: Interpolation specified by "
+                                       "FileTransform 'unknown' is not allowed with the "
+                                       "given file: 'lut1d_1.spi1d'.\n");
+    }
+
+    // TETRAHEDRAL can't be used for Spi1d, default is used instead (and a warning is logged).
+
+    tr->setInterpolation(OCIO::INTERP_TETRAHEDRAL);
+    {
+        OCIO::LogGuard log;
+        OCIO::SetLoggingLevel(OCIO::LOGGING_LEVEL_WARNING);
+        OCIO_CHECK_NO_THROW(cfg->getProcessor(tr));
+        OCIO_CHECK_EQUAL(log.output(), "[OpenColorIO Warning]: Interpolation specified by "
+                                       "FileTransform 'tetrahedral' is not allowed with the "
+                                       "given file: 'lut1d_1.spi1d'.\n");
+    }
+
+    // Matrices ignore interpolation, so UNKNOWN is ignored and not even logged.  Note that the
+    // spi example configs use interpolation=unknown for matrix files.
+
+    tr->setInterpolation(OCIO::INTERP_UNKNOWN);
+    tr->setSrc("camera_to_aces.spimtx");
+    OCIO_CHECK_NO_THROW(cfg->getProcessor(tr));
 }
 
 OCIO_ADD_TEST(FileTransform, context_variables)

tests/python/CDLTransformTest.py

@@ -241,8 +241,14 @@ def test_createfromfile_cdl(self):
         # Try env var first to get test file path.
         test_file = '%s/cdl_test1.cdl' % TEST_DATAFILES_DIR
 
+        # Mute warnings being logged.
+        curLogLevel = OCIO.GetLoggingLevel()
+        OCIO.SetLoggingLevel(OCIO.LOGGING_LEVEL_NONE)
+
         # Test a specified id member of the cdl file.
         cdl = OCIO.CDLTransform.CreateFromFile(test_file, 'cc0003')
+        OCIO.SetLoggingLevel(curLogLevel)
+
         self.assertEqual(cdl.getID(), 'cc0003')
         self.assertEqual(cdl.getDescription(), 'golden')
         self.assertListEqual(cdl.getSlope(), [1.2, 1.1, 1.0])