Add remaining CG config transforms

[michdolan]

The CLFs in this PR, and the method for converting them to GroupTransform were provided by @doug-walker

In addition to adding the remaining CLFs for the CG config, this PR modifies the CG config generation code to convert those CLFs into native OCIO transforms rather than reference the external files. So in this case the CLFs are being used as a template for colorspace creation, but the resulting config is 100% self-contained.

Other updates:

• Updated the code to implement NamedTransforms when called out in the CSV
• Update CLF URN handling based on the updated CLF formatting document: https://docs.google.com/document/d/1uYNnq1IlKqP8fRXnPviZHrAAu37ctvVsjJZeajOFF2A/edit?usp=sharing

Note: I removed the code which generates a JSON file alongside the OCIO config. Because this update is putting OCIO objects into ConfigData in some cases, they fail to serialize. We should discuss whether this is ok (do we need the JSON file in addition to the config?), and if not, possible solutions.

Signed-off-by: Michael Dolan michdolan@gmail.com

[michdolan]

Generated CG config: https://gist.github.com/michdolan/215b894a145eb7e5133904fb0dfa25c3

[KelSolaar]

Excellent and thanks @michdolan,

I will take a look tonight.

Because this update is putting OCIO objects into ConfigData in some cases, they fail to serialize. We should discuss whether this is ok (do we need the JSON file in addition to the config?), and if not, possible solutions.

Any traceback, jsonpickle has been able to serialize anything I threw at it so far, I would not say it is an absolute requirement but it has been extremely useful to introspect the data the config generator holds so far.

[michdolan]

Traceback when trying to serialize a GroupTransform:

Traceback (most recent call last):
  File ".../OpenColorIO-Config-ACES/opencolorio_config_aces/config/cg/generate/config.py", line 462, in <module>
    os.path.join(build_directory, 'config-aces-cg.json'))
  File "...\OpenColorIO-Config-ACES\opencolorio_config_aces\utilities\common.py", line 431, in wrapped
    return function(*args, **kwargs)
  File "...\OpenColorIO-Config-ACES\opencolorio_config_aces\config\generation\common.py", line 656, in serialize_config_data
    config_json.write(jsonpickle.encode(asdict(data), indent=2))
  File "C:\Python37\lib\dataclasses.py", line 1064, in asdict
    return _asdict_inner(obj, dict_factory)
  File "C:\Python37\lib\dataclasses.py", line 1071, in _asdict_inner
    value = _asdict_inner(getattr(obj, f.name), dict_factory)
  File "C:\Python37\lib\dataclasses.py", line 1099, in _asdict_inner
    return type(obj)(_asdict_inner(v, dict_factory) for v in obj)
  File "C:\Python37\lib\dataclasses.py", line 1099, in <genexpr>
    return type(obj)(_asdict_inner(v, dict_factory) for v in obj)
  File "C:\Python37\lib\dataclasses.py", line 1103, in _asdict_inner
    for k, v in obj.items())
  File "C:\Python37\lib\dataclasses.py", line 1103, in <genexpr>
    for k, v in obj.items())
  File "C:\Python37\lib\dataclasses.py", line 1105, in _asdict_inner
    return copy.deepcopy(obj)
  File "C:\Python37\lib\copy.py", line 169, in deepcopy
    rv = reductor(4)
TypeError: can't pickle PyOpenColorIO.GroupTransform objects

[doug-walker]

Thanks for moving this forward Michael! Here are some comments:

• This is compatible with 2.0, but the config says 2.1 since the library writes the current version number and you need to manually set it to 2.0.
• Some of the CLFs are going under clf/transforms/builtins/aces even though one could argue they are not ACES transforms (i.e., not part of aces-dev, at least right now).
• There is a script in clf/transforms/aces.py that generated the placeholder clfs that could be updated too, based on the script I wrote to make the clfs. Let's discuss at the meeting how to update that.
• The Raw colorspace should have the file-io category too.
• We could debate whether we want to add Gamma 2.4 Rec.709 and sRGB as separate spaces since the config already has Display colorspaces for these.
• Do we still want the sRGB texture named "Input" whereas the others are "Utility"?
• We could discuss what negative handling we want to use for the Exponent Transforms.
• For the Gamma 2.2 AP1 space, we should clarify in the Description what white point is expected (I'm assuming D60, but others may be thinking D65?).
• The Description for the Named Transforms should clarify that the forward direction is applying (rather than removing) the gamma-correction.
• We could discuss dropping some of the views, for simplicity (e.g. HDR cinema, 4000 nits HDR video seem to fall outside the target audience).
• We could set the equalitygroup for Linear Rec.709 and Linear sRGB (minor).
• We should set the 'working-space' category for Linear Rec.709 and Linear P3-D65.
• We should consider not setting the rendering role so that it remains a user choice.

[michdolan]

This is compatible with 2.0, but the config says 2.1 since the library writes the current version number and you need to manually set it to 2.0.

Updated to 2.0, and added support for major and minor version to config generator

Some of the CLFs are going under clf/transforms/builtins/aces even though one could argue they are not ACES transforms (i.e., not part of aces-dev, at least right now).

I moved them all to a Utilities directory and namespace, since that's what they are labeled with in the config. Open for discussion of course.

There is a script in clf/transforms/aces.py that generated the placeholder clfs that could be updated too, based on the script I wrote to make the clfs. Let's discuss at the meeting how to update that.

Good point. Let's discuss tomorrow.

The Raw colorspace should have the file-io category too.

Updated (also in the reference config)

We could debate whether we want to add Gamma 2.4 Rec.709 and sRGB as separate spaces since the config already has Display colorspaces for these.

I think it's worth keeping both for ease of use. It will be easier to use color spaces for handling textures, etc. vs. having to use an inverse display view transform.

Do we still want the sRGB texture named "Input" whereas the others are "Utility"?

I changed this to Utility, which matches naming in existing ACES configs.

We could discuss what negative handling we want to use for the Exponent Transforms.

Good point. Let's discuss tomorrow.

For the Gamma 2.2 AP1 space, we should clarify in the Description what white point is expected (I'm assuming D60, but others may be thinking D65?).

I updated the config generator to pull strings from CLF input and output descriptors to build a more detailed color space description, which includes white point.

The Description for the Named Transforms should clarify that the forward direction is applying (rather than removing) the gamma-correction.

This is also solved by the updated description mentioned above.

We could discuss dropping some of the views, for simplicity (e.g. HDR cinema, 4000 nits HDR video seem to fall outside the target audience).
We could set the equalitygroup for Linear Rec.709 and Linear sRGB (minor).

For discussion tomorrow.

Updated config: https://gist.github.com/michdolan/215b894a145eb7e5133904fb0dfa25c3

[KelSolaar]

Hey @michdolan,

In 08e0061, I generalised the embedding process for CLF transforms, I basically made it a special case of the opencolorio_config_aces.transform_factory:

• If the name is FileTransform
• If the clf_transform_to_group_transform kwarg is passed
• Then the CLF transform is embedded.

A few benefits:

• No need for a dedicated clf_transform_to_group_transform definition living in the CG generator.
• Can be used from anywhere.
• Does not break the JSON serialisation which I think does not work with OCIO objects :(.

Example of how it is serialised now:

    {
      "name": "Utility - sRGB - Texture",
      "description": "Convert ACES2065-1 to sRGB\n\nCLFtransformID: urn:aswf:ocio:transformId:1.0:OCIO:Utility:AP0_to_Rec709-sRGB:1.0",
      "from_reference": {
        "name": "FileTransform",
        "src": "/Users/kelsolaar/Documents/Development/colour-science/ampas/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/transforms/builtins/utility/AP0_to_Rec709-sRGB.clf",
        "clf_transform_to_group_transform": true
      },
      "encoding": "sdr-video",
      "categories": "file-io"
    },

[michdolan]

@KelSolaar do you want to make a PR into my branch or a PR into the main repo following merging this one?

[michdolan]

Updated config: https://gist.github.com/michdolan/215b894a145eb7e5133904fb0dfa25c3

Based on config working group discussion, the following changes were made:

• Removed extra view transforms and display color spaces. These were being added during linked_display_colorspace_style filtering. Since the reference config handles that lower level filtering for us, we only need to check the aces_transform_id for the CG config. This trimmed down the config quite a bit.
• Added namespaces to CLFs and changed transform directory from builtins to ocio
• Updated the Google sheet and re-exported the CSV from there.
• Filtered rendering and reference roles. We didn't discuss removing the reference role today, but in the UX working group we have been talking about deprecating that, so figured we could start that way with this config. We may want to roll that change back to the reference config too.
• The working-space category now has ACEScg, linear Rec.709, and linear sRGB spaces

[KelSolaar]

My PR is up at michdolan#1.

I had to move my code on my fork because I could not force-push into the feature branch on https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES. It would be great to figure out what is going on. All the branches are protected. I cannot delete a merged branch either. :(

[michdolan]

I am locked out of the settings for this repo too. I'll talk to LF about getting admin permissions.

[zachlewis]

I don't suppose I could convince you guys to rename "name" kwarg used for transform_factory_default to something more descriptive, like "transform_type" or "transform_class"? Compared to how "name" is used with the other factory methods, the term feels kind of overloaded; especially because there's a canonical "name" format metadata attribute that manifests in serialized yaml as an attribute called "name":

!<LogAffineTransform> {name: Log3G10 to Linear, log_side_slope: 0.224282, lin_side_slope: 155.975327, lin_side_offset: 2.55975327, direction: inverse}

I was thinking, maybe we could reserve "name", "id", "description", "input_descriptor" and "output_descriptor" for slapping format metadata on freshly-birthed transform instances. It's pretty nice having a way to bundle and deal with the transform parameter data and format metadata at the same points in time and space.

(I know you guys are already using "name" in a bunch of places, but I figured I should say something now if ever)

feel free to snag this method for certain format metadata to supported transforms (or to group transforms wrapping unsupported transforms):

https://gist.github.com/zachlewis/39a121d07025e2727527d0446b2b5da5

[KelSolaar]

Good point, when I looked back then, I did not see any collisions and went for shorter is better but seems like I missed the one highlighted (and possibly others). I'll fix that.

[KelSolaar]

Should be fixed in michdolan#2

[zachlewis]

This is gonna fail for "LogCameraTransform"; possibly others. The constructor requires a linSideBreak value.

On the other hand, the "LogAffineTransform" doesn't accept an initial value for base during instantiation -- you have to use the setBase instance method afterwards.

Damned if you do, damned if you don't.

[KelSolaar]

That's fine, we can handle those special cases one-by-one.

[KelSolaar]

@michdolan : I noticed that the CLF classification code now breaks because of the changes in the URN:

/Users/kelsolaar/Library/Caches/pypoetry/virtualenvs/opencolorio-config-aces-UB0f4P8F-py3.8/bin/python /Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py
Traceback (most recent call last):
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 1332, in <module>
    print_clf_taxonomy()
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 1308, in print_clf_taxonomy
    classified_clf_transforms = classify_clf_transforms(
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 1168, in classify_clf_transforms
    clf_transform = CLFTransform(
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 534, in __init__
    self._parse()
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 838, in _parse
    self._clf_transform_id = CLFTransformID(root.attrib['id'])
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 198, in __init__
    self._parse()
  File "/Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/clf/discover/classify.py", line 461, in _parse
    assert clf_transform_id.startswith(URN_CLF), (
AssertionError: urn:aswf:ocio:transformId:v1.0:ACES.OCIO.AP0_to_AP1-Gamma2pnt2.c1.v1 URN None is invalid!

[KelSolaar]

I also noted that the __main__ section of opencolorio_config_aces/config/generation/common.py also break:

/Users/kelsolaar/Library/Caches/pypoetry/virtualenvs/opencolorio-config-aces-UB0f4P8F-py3.8/bin/python /Users/kelsolaar/Documents/Development/opencolorio/OpenColorIO-Config-ACES/opencolorio_config_aces/config/generation/common.py
CRITICAL:root:Config failed validation. File rules failed with: File rules: rule named 'Default' is referencing 'default' that is neither a color space nor a named transform.

Could be unrelated and I haven't checked as to why, about to hit the sack!

[michdolan]

@KelSolaar we changed the URN format away from urn:aswf:ocio:transformId:v1.0:ACES.OCIO.AP0_to_AP1-Gamma2pnt2.c1.v1. Now it expects: urn:aswf:ocio:transformId:1.0:OCIO:Utility:AP0_to_AP1-Gamma2.2:1.0

[KelSolaar]

@KelSolaar we changed the URN format away from urn:aswf:ocio:transformId:v1.0:ACES.OCIO.AP0_to_AP1-Gamma2pnt2.c1.v1. Now it expects: urn:aswf:ocio:transformId:1.0:OCIO:Utility:AP0_to_AP1-Gamma2.2:1.0

Yeah I know, I guess I was pointing out that we did not change the code accordingly!

[michdolan]

I updated the classification code, but the error in that case is failing because the CLF being classified has the old naming, which is no longer valid. michdolan#2 reintroduced the placeholder CLF files which I had removed in an earlier commit, which are causing the CI failure.

[KelSolaar]

No worries, there is a point not too far in the future where we should write unit tests for the generator.

[doug-walker]

In the Gist of the config, the ST2084-P3-D65 colorspace has an encoding of sdr-video, which is wrong. Although here it says hdr-video, which is correct. So maybe the Gist is out of date?