OCIO metadata serialization

[michdolan]

Continuing discussion from #1154 ...

It was discovered recently (and maybe this was already known) that CDLTransform setID and setDescription are not persistent as they are not currently serialized to config YAML, though they are advertised as such in the header:

    //!rst:: **Metadata**
    //
    // These do not affect the image processing, but
    // are often useful for pipeline purposes and are
    // included in the serialization.

    /// Unique Identifier for this correction.
    virtual const char * getID() const = 0;
    virtual void setID(const char * id) = 0;

    /**
     * Deprecated. Use `getFormatMetadata`.
     * First textual description of color correction (stored
     * on the SOP). If there is already a description, the setter will
     * replace it with the supplied text.
     */
    virtual const char * getDescription() const = 0;
    /// Deprecated. Use `getFormatMetadata`.
    virtual void setDescription(const char * desc) = 0;

This led to a discussion around metadata serialization across the library. Currently all transforms will serialize METADATA_NAME to the config, but no other key is supported.

I understand that format metadata is intended to support serialization to CLF/CTF specifically, but think the metadata not being persistent to a config is problematic since it limits reproducibility. Consider a case where you use a config to drive the creation of some critical CLF files that are handed off to a client; right now you would need a Python script which builds a config on the fly and bakes from that. You would not be able to just load the config and bake as any intended metadata would have been lost (except for name).

We should discuss whether this is an issue and how it could be resolved if so. It's possible others disagree here. I think the ability to store custom metadata in a config per-transform would be useful personally, rather than overloading a ColorSpace description attribute for this purpose.

Whatever the outcome of that discussion, I think we should remove, or very clearly deprecate, any public API setters that are not persistent in the config.

[zachlewis]

Thanks for bringing this up.

I think the ability to store custom metadata in a config per-transform would be useful personally, rather than overloading a ColorSpace description attribute for this purpose.

I wholeheartedly agree. I was hoping for such a feature, especially for the scarier looking, less easily-identifiable transforms, like Matrix or CameraLog. At minimum, it would really help to keep CLF/CTFs intelligible, and to reduce user-error when manually editing configs; and would be useful for generating filenames or identifying / swapping out specific transforms in arbitrary Processes. Also, I guess it would be one way to get a cache id --> description map "for free" sometimes.

Perhaps InputDescriptor and OutputDescriptor CLF/CTF metadata could be generated when creating Processors from ColorSpace or DisplayView transforms (if that doesn't happen already)?

[doug-walker]

The "serialization" comment from v1 was talking about serializing to the XML .cc file, not to the YAML.

I think the confusion here is due to the fact that in v1 the CDLTransform class is where the XML read/write was located. So the class needed the get/setID and get/setDescription for usage with the get/setXML methods. In other words, FileFormatCC/CCC/CDL were built on CDLTransform. Also, it was not possible to have private methods in the Transform interface only for library use. Furthermore, the sharing of data between the Transform, the Op, and the Parsers was less well-defined. Finally, there was not a generic way of handling transform metadata for all Transforms.

We've tried to address most of those issues in v2. The CDL parser was enhanced and is now located under the fileformat directory, it is now possible to have private methods, there is an OpData class that is used by the various clients to store the data, and there is a formatMetadata class that all Transforms are able to use.

However, we did not remove the original CDLTransform methods for fear of breaking the API. However, it's clear this is causing confusion and so I propose we remove the get/setID, get/setDescription, and get/setXML methods from CDLTransform. The new way to get/set the ID and Description is via the formatMetadata interface that all the transforms (including CDLTransform) already have. The only missing piece would be a replacement for getXML, but I propose we enhance the write method on the Processor class to allow this. That is the preferred way of doing this in v2. (We also have to document the fact that the current getDescription works on the SOP level, not the overall CDL level.)

Yes, if anyone was using these calls in v1 they will have to update their code, but it should be an easy change. I think the clarity and consistency of the API is more important than trying to avoid these minor breaks to existing code.

The other question is to what extent this metadata should be put in the config/YAML. We did it for "name" since we kept running into cases where we had a GroupTransform and we wanted a way to put a string to describe what each piece did, to improve readability of the config. (We used "name" since that neatly maps to an attribute in CLF.) We could implement ID in the same way. The problem with Description is that CLF (and hence formatMetadata) allow an aribtrary number of description strings and it's not clear it is worth trying to come up with an equivalent for that in YAML. Likewise the other CLF related strings in formatMetadata such as InputDescription and OutputDescription don't easily map to a config since that information is encoded in another way (for example, the to_reference Input is the color space itself and the Output is the reference space).

Keep in mind that if it may be in both the file formats (.cc, .clf, etc) as well as the YAML, the code will need to decide what takes priority if both are present.

I'd like to better understand the use-case for how the descriptive Transform-level metadata (beyond "name") would be used in a config. Is it basically getting a Processor from src to dst and then calling write on it? How would Descriptions in the YAML help this, and how would you combine the src and dst? If not, what is it in the config that is going to be serialized?

Currently, after a GroupTransform is built up and we then want to serialize it as a CLF we take the first InputDescriptor, the last OutputDescriptor, and take all of the Description elements (see CTFTransform.cpp:fromMetadata). (This is for the header -- if each Transform also has Descriptions, these go under each ProcessNode and don't need to be combined.) This is hopefully better than nothing, but intelligently combining metadata from input to output is a difficult problem. The API also allows setting this manually before writing, but it's not clear how people would want the config-level metadata to be used if it were present. Is the use-case for the header level or individual operator level metadata?

[michdolan]

I'm in agreement with @doug-walker 's proposal on removing API which is confusing, and extending Processor write support to CDL, Would that only occur when a processor is exclusively a CDLOp instance? Perhaps the getXML method could be retained and shortcut to that implementation for convenience. We can raise this for discussion at the next TSC meeting also.

For transform metadata, I propose not limiting it to any subset of metadata, but just writing whatever key/value pairs are present to the YAML for persistence. It is then up to the config author to determine how much or little metadata they care to have. In practice this would likely be name or nothing, but it could be more if desired. The only time a clash would occur is if metadata keys present in an external file were also used on a FileTransform, but in most cases explicit metadata won't be as useful for a FileTransform since the filename is often used to clearly communicate what the underlying transform does. If there were a clash I think the file should take precedence.

As far as what metadata to write to written Processors, that should be limited to whatever metadata keys are supported by the respective format. Without retention of metadata in the YAML, even supported keys for CLF/CTF would only be useful when set with API calls to memory before writing, vs. creating a config from a file and just writing it. And note that I am not suggesting here to write metadata from a dependent file to its FileTransform when authoring a config, but maybe that's the issue you are raising, that they can't be easily separated since the metadata exists on the same underlying object.

The point I am raising though is that FormatMetadata is not only useful for writing CTF/CLF, but also for purposes of config organization. I think many in the community would suggest supporting metadata at the ColorSpace level too for similar reasons, which is more manageable than description in many cases. TBH I was not even aware of name being serialized to the config until investigating the reported CDLTransform issue. Prior to this discussion I thought FormatMetadata was tied exclusively to file formats in memory, for querying metadata from a LUT via the API. But since a config can generate a CTF/CLF, it makes sense that this goes beyond this scope.

Metadata in the config could look like this:

- !<ColorSpace>
    name: xyz16
    family: xyz
    equalitygroup: 
    bitdepth: 16ui
    description: |
      xyz16 :Conversion for  DCP creation.
    isdata: false
    allocation: uniform
    metadata:
        key1: value1
        key2: value2
    from_reference: !<GroupTransform>
      children:
        - !<ColorSpaceTransform>:
            src: lnf
            dst: p3dci8
            metadata:
                key1: value1
                key2:  value2
        - !<ExponentTransform> {value: [2.6, 2.6, 2.6, 1]}
        - !<FileTransform> {src: p3_to_xyz16_corrected_wp.spimtx, interpolation: unknown}
        - !<ExponentTransform>:
            value: [2.6, 2.6, 2.6, 1]
            direction: inverse
            metadata:
                key1: value1

All said, this is a topic that should have no action until the community agrees on a solution. This would have the potential to be very useful or to itself be confusing, so should be considered carefully.

[doug-walker]

Would that only occur when a processor is exclusively a CDLOp instance?

Currently the write method throws if there are any ops that may not be losslessly written to the selected format and suggests using the baker instead. So if the format is cc/ccc/cdl, it would throw if the op is anything other than a single CDL. Strictly speaking, those formats do not have a way of indicating whether clamping is desired (as CLF is able to) since the ASC spec always clamps. But for the sake of compatibility with common industry workflows we should proceed with the write even for the no-clamp case.