- Another cleanup pass. After some deliberation, we decided to remove getColorSpaceFromInteropID() which will be done in a later pass.

Signed-off-by: cuneyt.ozdas <cuneyt.ozdas@autodesk.com>

Author: cozdas

include/OpenColorIO/OpenColorIO.h

@@ -631,7 +631,7 @@ class OCIOEXPORT Config
     /**
      * \brief Get the color space from the provided interopID.
      */
-    ConstColorSpaceRcPtr getColorSpaceFromInteropID(const char * interopID, InteropIDSearchMethod method=FullID) const;
+    ConstColorSpaceRcPtr getColorSpaceFromInteropID(const char * interopID, InteropIDSearchMethod method) const;
 
     /**
      * Accepts an alias, role name, named transform name, or color space name and returns the
@@ -1992,13 +1992,16 @@ class OCIOEXPORT ColorSpace
     void setDescription(const char * description);
 
     /**
-     * Get/Set the interop ID for the color space.
-     *
-     * The interop ID is a standardized identifier to uniquely identify commonly
-     * used color spaces. These IDs are defined by the Academy Software
-     * Foundation's Color Interop Forum project. If you create your own ID, you
-     * must prefix it with unique characters that will ensure it won't conflict
-     * with future Color Interop Forum IDs.
+     * Get/Set the interop ID for the color space. The interop ID is a
+     * structured string defined by the Color Interop Forum. It is intended to
+     * identify color spaces in a way that is portable across different configs,
+     * making it suitable for use in various file formats. The Color Interop
+     * Forum publishes ID strings for common color spaces. If you create your
+     * own IDs, they must be preceded by a namespace string. The setter will
+     * throw if the string does not follow certain rules (run ociocheck for a
+     * more complete validation). For more details, please review the interop ID
+     * white paper available from:
+     * https://github.com/AcademySoftwareFoundation/ColorInterop.
      */
     const char * getInteropID() const noexcept;
     void setInteropID(const char * interopID);
@@ -2008,7 +2011,7 @@ class OCIOEXPORT ColorSpace
      *
      * Currently supported attribute names are "amf_transform_ids" and
      * "icc_profile_name". Using any other name will throw. If the attribute is
-     * not defined, it'll return empty string. Similarly setting it to empty
+     * not defined, it'll return empty string. Setting the value to an empty
      * string will effectively delete the attribute.
      *
      * The AMF transform IDs are used to identify specific transforms in the

src/OpenColorIO/ColorSpace.cpp

@@ -241,55 +241,66 @@ void ColorSpace::setInteropID(const char * interopID)
 
     if (!id.empty())
     {
-        // Count the number of ':' characters in the string
-        // and check for non-ASCII characters
-        size_t colonCount = 0;
-        size_t lastColonPos = std::string::npos;
-        
-        for (size_t i = 0; i < id.length(); ++i)
+        // check if it only uses ASCII characters: 0-9, a-z, and the following characters (no spaces):
+        // - _ ~ / * # % ^ + ( ) [ ] |
+        auto allowed = [](char c)
+        {
+            return  (c >= '0' && c <= '9')||
+                    (c >= 'a' && c <= 'z')||
+                    c=='.'||c=='-'||c=='_'||c=='~'||c=='/'||c=='*'||c=='#'||c=='%'||
+                    c=='^'||c=='+'||c=='('||c==')'||c=='['||c==']'||c=='|'||c==':';
+        };
+
+        if (!std::all_of(id.begin(), id.end(), allowed)) 
         {
-            if (id[i] == ':')
+            std::ostringstream oss;
+            oss << "InteropID '" << id << "' contains invalid characters. "
+                "Only lowercase a-z, 0-9 and . - _ ~ / * # % ^ + ( ) [ ] | are allowed." << 
+                std::endl;
+            throw Exception(oss.str().c_str());
+        }
+
+        // Check if has a namespace.
+        size_t pos = id.find(':');
+        if (pos != std::string::npos) 
+        {
+            // Namespace found, split into namespace and color space.
+            std::string ns = id.substr(0, pos);
+            std::string cs = id.substr(pos+1);
+        
+            // both should be non-empty
+            if (ns.empty() || cs.empty()) 
             {
-                colonCount++;
-                lastColonPos = i;
+                std::ostringstream oss;
+                oss << "InteropID '" << id << "' is not valid. "
+                    "If ':' is used, both the namespace and the color space parts must be non-empty." <<
+                    std::endl;
+                throw Exception(oss.str().c_str());
             }
-            else if (static_cast<unsigned char>(id[i]) >= 0x80)
+
+            // More than one ':' is an error.
+            if (cs.find(':') != std::string::npos) 
             {
                 std::ostringstream oss;
-                oss << "InteropID '" << id << "' is invalid: only ASCII characters [0x00..0x7F] are allowed.";
+                oss << "ERROR: InteropID '" << id << "' is not valid. "
+                    "Only one ':' is allowed to separate the namespace and the color space." << 
+                    std::endl;
                 throw Exception(oss.str().c_str());
             }
         }
-        
-        // Validate: only zero or one ':' character allowed
-        if (colonCount > 1)
-        {
-            std::ostringstream oss;
-            oss << "InteropID '" << id << "' is invalid: only zero or one ':' character is allowed.";
-            throw Exception(oss.str().c_str());
-        }
-        
-        // Validate: ':' cannot be the last character
-        if (colonCount == 1 && lastColonPos == id.length() - 1)
-        {
-            std::ostringstream oss;
-            oss << "InteropID '" << id << "' is invalid: ':' character cannot be the last character.";
-            throw Exception(oss.str().c_str());
-        }
-
-        // TODO: Do we want to verify the interopID against the CIF list here?
     }
 
     getImpl()->m_interopID = id;
 }
 
 const char * ColorSpace::getInterchangeAttribute(const char* attrName) const
 {
-    std::string nameTrimmed = StringUtils::Trim(attrName);
+    std::string name = attrName ? attrName : "";
+
     for (auto& key : knownInterchangeNames)
     {
         // do case-insensitive comparison.
-        if (StringUtils::Compare(key, nameTrimmed))
+        if (StringUtils::Compare(key, name))
         {
             auto it = m_impl->m_interchangeAttribs.find(key);
             if (it != m_impl->m_interchangeAttribs.end())
@@ -301,19 +312,20 @@ const char * ColorSpace::getInterchangeAttribute(const char* attrName) const
     }
 
     std::ostringstream oss;
-    oss << "Unknown attribute name '" << attrName << "'.";
+    oss << "Unknown attribute name '" << name << "'.";
     throw Exception(oss.str().c_str());
 }
 
 void ColorSpace::setInterchangeAttribute(const char* attrName, const char* value)
 {
-    std::string nameTrimmed = StringUtils::Trim(attrName);
+    std::string name = attrName ? attrName : "";
+
     for (auto& key : knownInterchangeNames)
     {
         // Do case-insensitive comparison.
-        if (StringUtils::Compare(key, nameTrimmed))
+        if (StringUtils::Compare(key, name))
         {
-            // use key instead of nameTrim for storing in correct capitalization.
+            // use key instead of name for storing in correct capitalization.
             if (!value || !*value)
             {
                 m_impl->m_interchangeAttribs.erase(key);
@@ -327,7 +339,7 @@ void ColorSpace::setInterchangeAttribute(const char* attrName, const char* value
     }
 
     std::ostringstream oss;
-    oss << "Unknown attribute name '" << attrName << "'.";
+    oss << "Unknown attribute name '" << name << "'.";
     throw Exception(oss.str().c_str());
 }
 
@@ -336,7 +348,7 @@ std::map<std::string, std::string> ColorSpace::getInterchangeAttributes() const
     return m_impl->m_interchangeAttribs;
 }
 
-BitDepth ColorSpace::ColorSpace::getBitDepth() const noexcept
+BitDepth ColorSpace::getBitDepth() const noexcept
 {
     return getImpl()->m_bitDepth;
 }
@@ -555,7 +567,6 @@ std::ostream & operator<< (std::ostream & os, const ColorSpace & cs)
     {
         os << ", description=" << str;
     }
-    // TODO: Do we need to output the section name too?
     for (const auto& attr : cs.getInterchangeAttributes())
     {
         os << ", " << attr.first << "=" << attr.second;

src/OpenColorIO/Config.cpp

@@ -2499,8 +2499,7 @@ const char * Config::getCanonicalName(const char * name) const
 
 ConstColorSpaceRcPtr Config::getColorSpaceFromInteropID(const char * interopID, InteropIDSearchMethod method) const
 {
-    // TODO: this currently searches for roles as well. Do we want that? /coz
-
+    // NOTE: This will search color space names, aliases, and roles.
     auto cs = getColorSpace(interopID);
 
     // Fall back to name-only if allowed.
@@ -5682,11 +5681,13 @@ void Config::Impl::checkVersionConsistency() const
         }
     }
 
-    // Check for the ColorSpaces.
+    // Check ColorSpace properties.
 
     const int nbCS = m_allColorSpaces->getNumColorSpaces();
     for (int i = 0; i < nbCS; ++i)
     {
+        // Check for display color spaces.
+
         const auto & cs = m_allColorSpaces->getColorSpaceByIndex(i);
         if (m_majorVersion < 2) 
         {
@@ -5696,6 +5697,8 @@ void Config::Impl::checkVersionConsistency() const
             }
         } 
 
+        // Check for new color space attributes.
+
         if (m_majorVersion < 2) 
         {
             if (*cs->getInteropID())
@@ -5705,11 +5708,15 @@ void Config::Impl::checkVersionConsistency() const
                 os << "has non-empty InteropID and config version is less than 2.0.";
                 throw Exception(os.str().c_str());
             }
+        }
+
+        if (hexVersion < 0x02050000) 
+        {
             if (cs->getInterchangeAttributes().size()>0)
             {
                 std::ostringstream os;
                 os << "Config failed validation. The color space '" << cs->getName() << "' ";
-                os << "has non-empty interchange attributes and config version is less than 2.0.";
+                os << "has non-empty interchange attributes and config version is less than 2.5.";
                 throw Exception(os.str().c_str());
             }
         }

src/OpenColorIO/OCIOYaml.cpp

@@ -344,7 +344,7 @@ inline void loadCustomKeys(const YAML::Node& node, CustomKeysLoader & ck, const
     else
     {
         std::ostringstream ss;
-        ss << "The '" << sectionName << "' section  need to be a YAML map.";
+        ss << "Expected a YAML map in the " << sectionName << " section.";
         throwError(node, ss.str().c_str());
     }
 }
@@ -3259,7 +3259,7 @@ inline void load(const YAML::Node& node, ColorSpaceRcPtr& cs, unsigned int major
                         keyval.first.as<std::string>().c_str(), 
                         val.c_str());
                 }
-                catch (Exception*)
+                catch (Exception &)
                 {
                     LogUnknownKeyWarning(iter->second, keyval.first);
                 }
@@ -3381,13 +3381,6 @@ inline void save(YAML::Emitter& out, ConstColorSpaceRcPtr cs, unsigned int major
         }
         out << YAML::Flow << YAML::Value << aliases;
     }
-    
-    const std::string interopID{ cs->getInteropID() };
-    if (!interopID.empty())
-    {
-        out << YAML::Key << "interop_id";
-        out << YAML::Value << interopID;
-    }
 
     out << YAML::Key << "family" << YAML::Value << cs->getFamily();
 
@@ -3451,6 +3444,13 @@ inline void save(YAML::Emitter& out, ConstColorSpaceRcPtr cs, unsigned int major
         out << YAML::Key << "encoding";
         out << YAML::Value << is;
     }
+        
+    const std::string interopID{ cs->getInteropID() };
+    if (!interopID.empty())
+    {
+        out << YAML::Key << "interop_id";
+        out << YAML::Value << interopID;
+    }
 
     out << YAML::Key << "allocation" << YAML::Value;
     save(out, cs->getAllocation());

src/apps/ociocheck/main.cpp

@@ -31,6 +31,8 @@ const char * DESC_STRING = "\n\n"
 // returns true if the interopID is valid
 bool isValidInteropID(const std::string& id)
 {
+    // See https://github.com/AcademySoftwareFoundation/ColorInterop for the details.
+
     static const std::set<std::string> cifTextureIDs = {
         "lin_ap1_scene",
         "lin_ap0_scene",
@@ -50,34 +52,30 @@ bool isValidInteropID(const std::string& id)
         "unknown"
     };
 
-    // empty is fine
-    if (id.empty()) 
-        return true;
-
-    // check if it only uses ASCII characters: 0-9, a-z, and the following characters (no spaces):
-    // - _ ~ / * # % ^ + ( ) [ ] |
-    auto allowed = [](char c)
-    {
-        return (c >= '0' && c <= '9') ||
-               (c >= 'a' && c <= 'z') ||
-               c=='-'||c=='_'||c=='~'||c=='/'||c=='*'||c=='#'||c=='%'||
-               c=='^'||c=='+'||c=='('||c==')'||c=='['||c==']'||c=='|' || c==':';
+    static const std::set<std::string> cifDisplayIDs = {
+        "srgb_rec709_display",
+        "g24_rec709_display",
+        "srgb_p3d65_display",
+        "srgbx_p3d65_display",
+        "pq_p3d65_display",
+        "pq_rec2020_display",
+        "hlg_rec2020_display",
+        "g22_rec709_display",
+        "g22_adobergb_display",
+        "g26_p3d65_display",
+        "g26_xyzd65_display",
+        "pq_xyzd65_display",
     };
 
-    if (!std::all_of(id.begin(), id.end(), allowed)) 
-    {
-        std::cout << "ERROR: InteropID '" << id << "' contains invalid characters. "
-            "Only lowercase a-z, 0-9 and - _ ~ / * # % ^ + ( ) [ ] | are allowed." << 
-            std::endl;
-        return false;
-    }
+    if (id.empty()) 
+        return true;
 
     // Check if has a namespace.
     size_t pos = id.find(':');
     if (pos == std::string::npos) 
     {
-        // No namespace, so id must be in the forumID list.
-        if (cifTextureIDs.count(id) == 0)
+        // No namespace, so id must be in the Color Interop Forum ID list.
+        if (cifTextureIDs.count(id) == 0 && cifDisplayIDs.count(id)==0)
         {
             std::cout << "ERROR: InteropID '" << id << "' is not valid. "
                 "It should either be one of Color Interop Forum standard IDs or "
@@ -91,27 +89,9 @@ bool isValidInteropID(const std::string& id)
         // Namespace found, split into namespace and id.
         std::string ns = id.substr(0, pos);
         std::string cs = id.substr(pos+1);
-        
-        // both should be non-empty
-        if (ns.empty() || cs.empty()) 
-        {
-            std::cout << "ERROR: InteropID '" << id << "' is not valid. "
-                "If a namespace is used, it must be followed by a non-empty ID, e.g. 'mycompany:mycolorspace'." << 
-                std::endl;
-            return false;
-        }
 
-        // More than one ':' is an error.
-        if (cs.find(':') != std::string::npos) 
-        {
-            std::cout << "ERROR: InteropID '" << id << "' is not valid. "
-                "Only one ':' is allowed to separate the namespace and ID, e.g. 'mycompany:mycolorspace'." << 
-                std::endl;
-            return false;
-        }
-        
-        // id should not be in the cifID list.
-        if (cifTextureIDs.count(cs) > 0) 
+        // Id should not be in the Color Interop Forum ID list.
+        if (cifTextureIDs.count(cs) > 0 || cifDisplayIDs.count(cs)> 0) 
         {
             std::cout << "ERROR: InteropID '" << id << "' is not valid. "
                 "The ID part must not be one of the Color Interop Forum standard IDs when a namespace is used." << 
@@ -385,8 +365,6 @@ int main(int argc, const char **argv)
             std::cout << std::endl;
             std::cout << "** ColorSpaces **" << std::endl;
 
-
-
             const int numCS = config->getNumColorSpaces(
                 OCIO::SEARCH_REFERENCE_SPACE_ALL,   // Iterate over scene & display color spaces.
                 OCIO::COLORSPACE_ALL);              // Iterate over active & inactive color spaces.
@@ -398,11 +376,6 @@ int main(int argc, const char **argv)
                     OCIO::COLORSPACE_ALL,
                     i));
 
-                // Validate InteropID if present
-                // TODO: When the CIF list for display color spaces is ready consider 
-                // splitting this validation into two parts so that display color spaces 
-                // are tested against the display CIF list.
-
                 std::string interopID = cs->getInteropID();
                 if (!interopID.empty())
                 {