AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.76k stars 434 forks source link

v2.X.X Regression/bug with cccid handling in FileTransforms #1854

Closed KevinJW closed 11 months ago

KevinJW commented 11 months ago

Investigating a change in behaviour seen when trying to migrate a config from v1 to v2 being unable to load a ColorCorrection file, generating the error message when running ociocheck:

The transform file: test_config/master.cdl failed while building ops with this error: You must specify a valid cccid to load from the ccc file (either by name or index). id='' is not found in the file, and is not parsable as an integer index.

Points of note:

Expected:

Based on historical behaviour in v1 we should continue to try decode the file based on the contents of the file and not the file extension. we should also follow the v2 documentation accepting the file as master.cdl and default to using the first (only) colour correction found in the file, without needing users to add an explicit cccid to the config.

This should work for files named .ccc as well as .cdl

As a thought experiment I'm unclear if this should be considered as several bugs:

  1. Should the FileTransform continue to try load the file as a CC file after failing as a CDL/CCC?
  2. Is the empty string bug found in both FileFormatCCC.cpp and FileFormat.CDL.cpp

Reproduction files...

minimal.ocio:

ocio_profile_version: 1
description: Minimal
search_path: .

roles:
  default: basic
  scene_linear: basic
  data: basic
  reference: basic
  compositing_log: basic
  color_timing: basic
  color_picking: basic
  texture_paint: basic
  matte_paint: basic
  rendering: basic
  aces_interchange: basic
  cie_xyz_d65_interchange: basic

displays:
  display:
    - !<View> {name: basic, colorspace: basic }
    - !<View> {name: cdl, colorspace: basic_cdl }

colorspaces:
  - !<ColorSpace>
    name: basic

  - !<ColorSpace>
    name: basic_cdl
    from_reference: !<FileTransform> { src: master.cdl }

master.cdl:

<?xml version="1.0" encoding="UTF-8"?>
<ColorCorrection>
   <SOPNode>
      <Description>No Op CDL</Description>
      <Slope>1.00000 1.00000 1.00000</Slope>
      <Offset>0.00000 0.00000 0.00000</Offset>
      <Power>1.00000 1.00000 1.00000</Power>
   </SOPNode>
   <SatNode>
      <Saturation>1.00000</Saturation>
   </SatNode>
</ColorCorrection>
KevinJW commented 11 months ago

Possible option for fixing FileFormat handlers

diff --git a/src/OpenColorIO/fileformats/FileFormatCCC.cpp b/src/OpenColorIO/fileformats/FileFormatCCC.cpp
index 3670c57b..bad7723e 100755
--- a/src/OpenColorIO/fileformats/FileFormatCCC.cpp
+++ b/src/OpenColorIO/fileformats/FileFormatCCC.cpp
@@ -217,7 +217,7 @@ void LocalFileFormat::buildFileOps(OpRcPtrVec & ops,
     {
         // Use 0 for empty string.
         int cccindex=0;
-        if (StringToInt(&cccindex, cccid.c_str(), true))
+        if (cccid.empty() || StringToInt(&cccindex, cccid.c_str(), true))
         {
             int maxindex = ((int)cachedFile->m_transformVec.size())-1;
             if (cccindex<0 || cccindex>maxindex)
diff --git a/src/OpenColorIO/fileformats/FileFormatCDL.cpp b/src/OpenColorIO/fileformats/FileFormatCDL.cpp
index 3a687a1c..a2ff5e95 100755
--- a/src/OpenColorIO/fileformats/FileFormatCDL.cpp
+++ b/src/OpenColorIO/fileformats/FileFormatCDL.cpp
@@ -247,7 +247,7 @@ LocalFileFormat::buildFileOps(OpRcPtrVec & ops,
     {
         // Use 0 for empty string.
         int cccindex=0;
-        if (StringToInt(&cccindex, cccid.c_str(), true))
+        if (cccid.empty() || StringToInt(&cccindex, cccid.c_str(), true))
         {
             int maxindex = ((int)cachedFile->m_transformVec.size())-1;
             if (cccindex<0 || cccindex>maxindex)