Adding new attributes to color spaces. #2

cozdas · 2025-05-28T18:26:54Z

Adding AMF transform ID, interop id and icc profile name fields to color spaces with phyton bindings, java bindings, unit test
With the newly added unit tests discovered crasher issues some of the "char *" taking functions and fixed the crashers.
Also hardened Platform::Strcasecmp() and Platoform::Strncasecmp() against nullptr.

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

- adding python docs for AMF transform IDs and interop ID. - With the newly added unit tests discovered crasher issues some of the "char *" taking functions and fixed the crashers - Also hardened Platform::Strcasecmp() and Platoform::Strncasecmp() against nullptr. Signed-off-by: cuneyt.ozdas <cuneyt.ozdas@autodesk.com>

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

doug-walker

We need to decide what version checks should be added to Config.cpp. It looks like the current code will read and write these attributes for any config version. For writing, we don't want to allow them in configs with version less than 2.5 since old readers would not handle them. I guess we could always read them from any version?

include/OpenColorIO/OpenColorIO.h

doug-walker · 2025-05-29T05:00:43Z

docs/api/python/frozen/pyopencolorio_colorspace.rst


      Specify the transform for the appropriate direction. Setting the transform to null will clear it.




Did you generate this manually rather than via the process specified here?
https://opencolorio.readthedocs.io/en/latest/guides/contributing/contributing.html

I reverted the changes but failed to build the rst locally both on Windows and Mac.

src/OpenColorIO/Platform.cpp

src/bindings/java/JNIColorSpace.cpp

src/bindings/java/org/OpenColorIO/ColorSpace.java

doug-walker · 2025-05-29T05:18:21Z

tests/cpu/ColorSpace_tests.cpp

+    // Test setting to nullptr
+    cs->setDescription(nullptr);
+    cs->setAmfTransformIDs(nullptr);
+    OCIO_CHECK_EQUAL(std::string(cs->getAmfTransformIDs()), "");


Would be a more interesting test if the value was not already "".

tests/cpu/ColorSpace_tests.cpp

src/OpenColorIO/ColorSpace.cpp

src/OpenColorIO/OCIOYaml.cpp

doug-walker · 2025-05-29T05:39:47Z

Please add some Python tests, though they do not have to be as thorough as the CPU tests.

- Reverting the manual changes to python frozen docs - capitalizing the acronyms in the functions names, adding underscores to the yaml keywords - changing the new keyword's ordering in the yaml file - bumping the config minor version number for v2 to 5 - new fields are now restricted to v2.5 and higher (checkVersionConsistency() is updated to detect violations) - Fixing the version check logic error in Config::validate() - Added basic pyhton tests - fixed / extended the color space cpu tests Signed-off-by: cuneyt.ozdas <cuneyt.ozdas@autodesk.com>

…y added fields. Signed-off-by: cuneyt.ozdas <cuneyt.ozdas@autodesk.com>

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

cozdas · 2025-06-05T01:24:12Z

src/OpenColorIO/ColorSpace.cpp

+    str = cs.getAMFTransformIDs();
+    if (!str.empty())
+    {
+        os << ", amf_transform_ids=" << str;


Note that we're not parsing the string value that AMFTransformIDs field holds. Since the formatting stayed the same, that is multiple entities are separated with the newline character, this may create EOLs in the ostream.

Is this OK? Or should we try to create a list from each entity here (that requires tokenizing the string with /n/r separators).

Description has the same problem. Maybe for both we should just add something like "=(non-empty)" rather than trying to include the whole string? Or a hash value?

I'll leave those as-is for now, we can decide/change if a real need arises.

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

cozdas · 2025-06-05T01:34:47Z

src/bindings/python/PyColorSpace.cpp

+        .def("getAmfTransformIDs", &ColorSpace::getAmfTransformIDs,
+             DOC(ColorSpace, getAmfTransformIDs))
+        .def("setAmfTransformIDs", &ColorSpace::setAmfTransformIDs, "amfTransformIDs"_a,
+             DOC(ColorSpace, setAmfTransformIDs))


Do we want to add any of these newly added fields as an Init() function parameter which is defined in line 80 above?

Sure, as long as they go at the end and have default values so as not to break existing code.

cozdas · 2025-06-05T01:36:55Z

src/OpenColorIO/Config.cpp



        // Check for interchange roles requirements - scene-referred and display-referred.
-        if (getMajorVersion() >= 2 && getMinorVersion() >= 2)


This would work only until v3.0

Good catch. But you could use GetVersionHex() rather than making your own.

I saw that but that's a global function returning the library's version, not the config's current version.
I can add a function to Config class but that may break ABI.

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

doug-walker · 2025-06-05T03:52:30Z

src/OpenColorIO/ColorSpace.cpp

+    str = cs.getAMFTransformIDs();
+    if (!str.empty())
+    {
+        os << ", amf_transform_ids=" << str;


Description has the same problem. Maybe for both we should just add something like "=(non-empty)" rather than trying to include the whole string? Or a hash value?

doug-walker · 2025-06-05T03:54:15Z

src/OpenColorIO/Config.cpp



        // Check for interchange roles requirements - scene-referred and display-referred.
-        if (getMajorVersion() >= 2 && getMinorVersion() >= 2)


Good catch. But you could use GetVersionHex() rather than making your own.

doug-walker · 2025-06-05T04:15:49Z

src/OpenColorIO/OCIOYaml.cpp

            aliases.push_back(cs->getAlias(aidx));
        }
        out << YAML::Flow << YAML::Value << aliases;
    }


When reading, there is no need for version checks here since checkVersionConsistency will be called after the load. However, when saving, it seems like we need to avoid writing if the version is less than 2.5. Otherwise we would produce a config that won't load in earlier libraries, right? Validate is not called before a serialize.

Although, looking at the ColorSpace loader, maybe it only logs a warning on an unrecognized key. So perhaps we have an option?

We're calling the checkVersionConsistency() when saving too. It's in the Config::serialize() function.

doug-walker · 2025-06-05T04:29:38Z

tests/python/ColorSpaceTest.py

+
+        # Test setting None (should convert to empty string)
+        self.colorspace.setICCProfileName(None)
+        self.assertEqual(self.colorspace.getICCProfileName(), '')


The value should be initialized to something other than an empty string to be a valid test.

good catch.

doug-walker · 2025-06-05T04:37:44Z

include/OpenColorIO/OpenColorIO.h

+     * 
+     * The interop ID is a standardized identifier for commonly used color spaces.
+     * These IDs are defined by the Academy Software Foundation's ColorInterop project
+     * to standardize color space naming across the industry.


Suggest:
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.

doug-walker · 2025-06-05T04:46:47Z

tests/cpu/ColorSpace_tests.cpp

+    OCIO_CHECK_NO_THROW(cs->setInteropID(nullptr));
+    OCIO_CHECK_EQUAL(std::string(cs->getInteropID()), "");
+
+    // Test copy constructor preserves ICC profile name


Update comment.

And minor, but these comments should all end in a period.

Looks like the Cursor is also having difficulty sticking to this unusual comment syntax guideline even though the rest of the code has periods in the comments. :)

doug-walker · 2025-06-05T04:49:34Z

tests/cpu/ColorSpace_tests.cpp

+        OCIO_CHECK_EQUAL(cfgString, os.str());
+    }
+
+    // Test that the interop_id can't be used in v2.0 config


Is this significantly different than the check below (2.0 vs. 2.4 is probably not worth a separate test)?

Tests here are using the v2 yaml "Start" snippet above to create from a stream (de-serialization)
Tests below using v2.4 are exercising the other, serialization direction.

cozdas · 2025-06-06T05:07:02Z

tests/python/ColorSpaceTest.py

        aliases = cs.getAliases()
        self.assertEqual(len(aliases), 2)
-        self.assertEqual(aliases[0], 'alias1')
-        self.assertEqual(aliases[1], 'alias2')


Not sure why this is deleted. Restoring.

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

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

I was unable to build this locally so modified the CI workflow to upload the docs folder as an artifact in a separate branch. Even that was not readily consumable, I had to manually pick the differences and the remove the duplicate PyOpenColorIO in the entity names (i.e. PyOpenColorIO.PyOpenColorIO.ColorSpace) Signed-off-by: cuneyt.ozdas <cuneyt.ozdas@autodesk.com>

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

doug-walker

LGTM

cozdas added 3 commits May 25, 2025 21:17

- Added the getInteropID() field. Unit tests need cleanup

193e33a

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

- Adding color space ICC profile name field.

8bf083e

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

doug-walker suggested changes May 29, 2025

View reviewed changes

cozdas added 3 commits June 4, 2025 15:07

updated the ColorSpace << operator and the unit test to have the newl…

295f965

…y added fields. Signed-off-by: cuneyt.ozdas <cuneyt.ozdas@autodesk.com>

- unbreaking the doc builds by fixing the function name cases in the …

1e08d6b

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

cozdas commented Jun 5, 2025

View reviewed changes

removing the rest of the changes in the frozen docs.

99af000

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

cozdas commented Jun 5, 2025

View reviewed changes

cozdas added 2 commits June 4, 2025 18:47

removing the leftover java changes

0d3b302

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

Minor code and test cleanup

4e24d32

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

doug-walker suggested changes Jun 5, 2025

View reviewed changes

cozdas commented Jun 6, 2025

View reviewed changes

cozdas added 7 commits June 5, 2025 22:23

Addressing the code review requests.

8b9b0e7

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

- testing CI artifact uploading.

b85a71c

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

Another attempt for finding the correct frozen doc format.

6b3c761

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

something was not right. retry

f1f8920

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

- reverting the ci_workflow.yml, I'll continue my experiments on anot…

945e494

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

Minor fixes to python tests.

c2f65f5

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

doug-walker approved these changes Jun 7, 2025

View reviewed changes


		Specify the transform for the appropriate direction. Setting the transform to null will clear it.



		// Check for interchange roles requirements - scene-referred and display-referred.
		if (getMajorVersion() >= 2 && getMinorVersion() >= 2)

Adding new attributes to color spaces. #2

Are you sure you want to change the base?

Adding new attributes to color spaces. #2

Uh oh!

Conversation

cozdas commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doug-walker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doug-walker commented May 29, 2025

Uh oh!

cozdas Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-walker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cozdas commented May 28, 2025 •

edited

Loading

cozdas Jun 5, 2025 •

edited

Loading