Refine colorInteropID support - #2560
Conversation
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
|
|
doug-walker
left a comment
doug-walker
left a comment
There was a problem hiding this comment.
Here are some comments that might be helpful to reviewers of this PR.
| // New in OpenEXR v3.4 | ||
|
|
||
| IMF_STD_ATTRIBUTE_DEF (colorInteropID, ColorInteropID, std::string) | ||
|
|
There was a problem hiding this comment.
Updated the links and moved this up to be next to chromaticities.
| For RGB images, specifies the CIE (x,y) chromaticities of the | ||
| primaries and the white point. | ||
| primaries and the white point. Note: For most purposes, this | ||
| attribute has now been superseded by the colorInteropID. |
There was a problem hiding this comment.
Was trying to figure out the right language here to capture the fact that the CIF OpenEXR document recommends that people switch over to using colorInteropID rather than chromaticities for most purposes.
|
|
||
| </td> | ||
| </tr> | ||
|
|
There was a problem hiding this comment.
Updated the links and moved this up to be next to chromaticities.
| Application developers are asked to take care when writing OpenEXR | ||
| files to avoid writing or propagating color space metadata which may | ||
| be incorrect. The colorInteropID should be omitted or set to ``unknown`` | ||
| unless the application is confident in the value being written. |
There was a problem hiding this comment.
Rewrote this based on colorInteropID rather than chromaticities.
In particular, you will notice that I removed the sentence: "If a file doesn't have a chromaticities attribute, display software should assume that the file's primaries and the white point match Rec. ITU-R BT.709-3." This was intentional for several reasons:
- It contradicts the new Color Interop Forum OpenEXR recommendation.
- It should probably be up to the application and/or color management system to determine the default, rather than making a blanket statement in OpenEXR.
- I feel this is partially responsible for application developers sometimes setting the chromaticities to Rec.709 even if they have no idea what the color space actually is, thereby breaking trust in the chromaticities attribute.
And I removed the part about CIE XYZ since it was misleading. If anyone did want to write XYZ values into an EXR file, it would typically not be using illuminant E (1/3, 1/3) as the white point.
There was a problem hiding this comment.
I think it would be worth retaining some description of the chromaticities attribute, even if it is described as deprecated/legacy/ACES-only, since such files will continue to exist and need to be understood.
I'm also not clear if there's still a use case for using it to store completely arbitrary chromaticities, which the recommendation allows as long as no colorInteropID is set.
The Rec,709 and XYZ bits can go, though!
| Color Interop ID. Provides a mechanism to identify the color space of the RGB images. | ||
| See `An ID for Color Interop <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/03_ColorInteropID/ColorInteropID.md>`_ | ||
| and `Identifying the Color Space of OpenEXR Files <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/04_OpenEXRFiles/OpenEXRFiles.md>`_ for details. | ||
| New in OpenEXR v3.4. |
There was a problem hiding this comment.
Updated the links and moved it up to be next to chromaticities.
| // ColorInteropID -- leave a part's existing "data" tag alone, since | ||
| // it marks image data that is intentionally not color managed. | ||
| // | ||
| if (!hasColorInteropID (dst) || colorInteropID (dst) != "data") |
There was a problem hiding this comment.
Does this enforce that if the first part is data, then all parts must be data, but not the other way round? Is that intentional? (or should the clause also be skipped if src has a colorInteropID set to data, and the checkSharedAttributesvalues function have a similar clause?)
In any case, if parts are written with different colorinteropIDs, this function will delete that information from other parts, causing the reader to treat them as being in the wrong space. I would prefer either that the library always checked for colorInteropID consistency on MultiPartOutputFile with no option to bypass test or automatically "fix" attribute, or else all the colorInteropID logic was dropped from the SharedAttribute code, making the multipart rules of the Recommendation less strict.
There was a problem hiding this comment.
You're right, it should not try to modify an existing value. Is it okay if I just handle it the same as chromaticities then?
There was a problem hiding this comment.
I think the existing handling of chromaticities is a little dangerous, since it silently overrides potentially accurate metadata, without changing the pixels.
I'd be tempted leave out the colorInteropID from all this verification, which would allow files that don't strictly follow the recommendation. I certainly don't think anything should modify the metadata - it should either ignore problems or report an error.
One option for now would be to extend the API with a verification method that takes a list of headers and checks the attributes conform (including conflicts between chromaticities and colorInteropID attributes in the same part). That could be an optional method available for reading and writing files. A future update could call that by default on writing files, so preventing non-conforming files.
There was a problem hiding this comment.
I've added verification functions.
|
What are your thoughts about adding support for colorInteropID names to the Chromaticities? Perhaps the spaces listed in the recommendation (lin_rec709_scene, lin_ap0_scene, lin_ap1_scene, lin_p3d65_scene, lin_rec2020_scene, lin_adobergb_scene) could be added as static Chromaticities variables, and a function added to turn those strings into chromaticities? For lightweight utilities that aren't intended to be part of a VFX pipeline so don't need OpenColorIO, some kind of built-in support to help convert images to and from known space could encourage adoption of the attribute. |
|
Peter, yes, I'll add helper functions to convert between chromaticities and colorInteropID and vice versa. But I feel that adding color space conversion functions should be out of scope for this PR. |
The Chromaticities class has RGBtoXYZ and XYZtoRGB that return 4x4 matrices. With the colorInteropID to chromaticities addition, it will be possible to construct a matrix that converts between colorInteropID (or between a colorInteropID and arbitrary chromaticities) as long as white point adaptation isn't required. A future "nice first issue" might be to move the adaptation code in ImfACESFile.cpp into ImfChromaticities, making it part of the API, and also adding a method that applies a matrix to the specified scanlines of a given FrameBuffer to do in-place color conversion |
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
|
@peterhillman , I've made some of the changes you requested:
After further study of exr2aces and the ImfAcesFile, I noticed that it doesn't even write acesImageContainerFlag, so it is just totally broken. My recommendation is that we remove exr2aces and deprecate ImfAcesFile. And I realized that acesImageContainerFlag is not even a standard attribute, so I added that. When overrideSharedAttributes is on, I decided to continue to have it modify mismatched values. IMHO, that is simply obeying what that option is supposed to do (and it is opt-in). I've requested time on the TSC meeting agenda this Thursday to get feedback from the group about some of the questions I have. |
| {0.6400f, 0.3300f}, | ||
| {0.3000f, 0.6000f}, | ||
| {0.1500f, 0.0600f}, | ||
| {0.3127f, 0.3290f}}, |
There was a problem hiding this comment.
Would it be helpful to do something like#define D65_WHITE {0.3127f, 0.3290f} and #define ACES_WHITE {0.32168f, 0.33767f} and use those macros in the list for the white points to make it easier to read the code?
| // lin_rec709_scene lin_ap0_scene lin_ap1_scene | ||
| // lin_p3d65_scene lin_rec2020_scene lin_adobergb_scene | ||
| // | ||
| // See <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/ |
There was a problem hiding this comment.
The comment could mention this list is merely the ones the library itself defines, and that other values are colorInteropIDs are legal and should be expected by software reading OpenEXR files.
Is it possible this list would ever expand? If future releases could add extra spaces, it might be nice to expose the list through the API, (e.g. int getNumKnownInteropIDS(), char* getKnownInteropID(int index) ) then tools that use this functionality can present the list to a user without having to hardcode it
(Also I'd mention OpenColorIO here too, as a more complete approach for handling colorInteropIDs)
There was a problem hiding this comment.
I added a comment, but I'm hesitant to expose the list of IDs to the API. For example, the IDs themselves are not intended to be user-facing strings.
| // | ||
| // Any previous contents of warnings are cleared. Warnings are only | ||
| // collected when runCoreCheck is false; the OpenEXRCore (C) API path does | ||
| // not report them. |
There was a problem hiding this comment.
A tool that checks the colorspaces is good to have except I think exrcheck is the wrong place to put it. The main purpose of exrcheck is to assist with searching for vulnerabilities in the OpenEXR library, e.g. with files generated in a fuzzing process. It is a very bad idea to run it on untrusted files. It is also only installed if OPENEXR_INSTALL_DEVELOPER_TOOLS is set, so won't be generally available.
Maybe exrinfo (or exrheader) would be better places to report the colorspace of a file and any warnings?
| Rec.709 chromaticities even if the image is not actually that. In other cases, | ||
| applications report that the image is linear Rec.709, even if the chromaticities | ||
| is not present. This situation is one of the main reasons why the colorInteropID | ||
| is now the recommended way of identifying the color space in OpenEXR files. |
There was a problem hiding this comment.
This could mention the colorInteropID to Chromatcities conversion as a path for migration to the new way
Is some comment about linear images merited here? OpenEXR says that images should be scene linear, but colorInteropID allows specification of a space with a nonlinear encoding.
To avoid heated discussions, this might simply say "since OpenEXR images are supposed to be linear, the colorInteropID should refer to a linear space (e.g. one that starts lin_) but the library does not require that to be the case"
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
|
I've updated the PR to move the color metadata validation from exrcheck to exrinfo, per the TSC discussion. There are separate functions added as well for client programs to call. I updated the description at the top of the PR based on the recent commits. |
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
|
@lgritz , @zachlewis -- Do you think the new functions for validating the color space header metadata would be useful in OpenImageIO? |
2 participants
Now that the ASWF Color Interop Forum has published the Recommendations for the Color Interop ID and for identifying the color space of OpenEXR files, I've made an attempt to build on Cary's earlier PR and integrate the standard colorInteropID attribute more fully into the library.
The exr2aces command-line tool and ImfAcesFile classes are obsolete and never actually implemented SMPTE ST-2065-4. They are unmodified by this PR and will be removed/deprecated in a separate PR, per TSC discussion.
Multi-part File Handling
The CIF Recommendation states that in a multi-part file, the colorInteropID of later parts should not be different than the first part. The allowed exceptions are if the colorInteropID is missing in the later part, or set to 'data'. Here's how that is handled via this PR:
This is looser than how chromaticities are handled. Neither the C++ or core APIs are able to read a multi-part file where the chromaticities are different in each part. This is regardless of how strictHeaderValidation is set. The C++ API will not allow creation of such a file, although the core API will (I added a TODO regarding this as it seems undesirable to have the library able to produce files which may not be read).
This PR was assisted by Claude Code Sonnet 5.