feat(openexr): Follow CIF for color interop ID in multi-part files - #5422
feat(openexr): Follow CIF for color interop ID in multi-part files#5422brechtvl wants to merge 2 commits into
Conversation
On read, a part without a colorInteropID now gets it from the first part, if there is any. There is no validation, EXR files that do not follow the CIF recommendation are still accepted. On write, give an error when the colorInteropID in later parts is different than the first part, except if is missing or set to "data". Upcoming versions of OpenEXR will throw an exception in this case, this applies the same logic for existing versions of OpenEXR already. Note this also means writing parts in order like this is now an error: * "lin_ap0_scene", "lin_ap1_scene" * "data", "lin_ap0_scene" * missing, "lin_ap0_scene" It also means "data" <missing> becomes "data" "data" on read. I considered automatically reordering headers to accept cases where a "data" part happens to come first. However attributes in the first header have a special meaning, and reordering attributes other than colorInteropID to later headers would be problematic. Ref AcademySoftwareFoundation/openexr#2560 Signed-off-by: Brecht Van Lommel <brecht@blender.org>
|
This is more strict than I would have chosen myself, as I explained in the OpenEXR PR. But even if the rules were relaxed a bit, it still remains a breaking change that should probably be done sooner than later in 3.2. |
Signed-off-by: Brecht Van Lommel <brecht@blender.org>
doug-walker
left a comment
doug-walker
left a comment
There was a problem hiding this comment.
This looks good Brecht, it seems to follow the CIF Recommendation.
You have raised a valid point that this will potentially refuse to write some files that would have been written before. Personally, I don't think that's undesirable since the parts would not be well-organized, but I'd be interested to hear from OIIO users if this is problematic for them.
|
|
||
| // Follow the color interop forum recommendation for OpenEXR files, | ||
| // where the colorInteropID in later parts must match the first part, | ||
| // except when "data" or missing. |
There was a problem hiding this comment.
You could add a note here, suggesting to replace this function with checkColorMetadata from the OpenEXR library, once that's available. It does some additional checks, such as validating acesImageContainer flag and chromaticities. (Though I realize that OIIO will need its own copy for a while due to the need to support older EXR versions.)
|
Converting to a draft for now since discussion in the OpenEXR PR is still ongoing, so best not to land this before a decision is made there. |
2 participants
Description
On read, a part without a colorInteropID now gets it from the first part, if there is any. There is no validation, EXR files that do not follow the CIF recommendation are still accepted.
On write, give an error when the colorInteropID in later parts is different than the first part, except if is missing or set to "data". Upcoming versions of OpenEXR will throw an exception in this case, this applies the same logic for existing versions of OpenEXR already.
Note this means writing parts in order like this is now an error:
"lin_ap0_scene" "lin_ap1_scene""data" "lin_ap0_scene"missing "lin_ap0_scene"It also means
"data" missingbecomes"data" "data"on read.I considered automatically reordering headers to accept cases where a "data" part happens to come first. However attributes in the first header have a special meaning, and reordering attributes other than colorInteropID to later headers would be problematic.
Ref AcademySoftwareFoundation/openexr#2560
Tests
New test added for this.
Checklist:
and if I used AI coding assistants, I have an
Assisted-by: TOOL / MODELline in the pull request description above.
behavior.
PR, by pushing the changes to my fork and seeing that the automated CI
passed there. (Exceptions: If most tests pass and you can't figure out why
the remaining ones fail, it's ok to submit the PR and ask for help. Or if
any failures seem entirely unrelated to your change; sometimes things break
on the GitHub runners.)
fixed any problems reported by the clang-format CI test.
corresponding Python bindings. If altering ImageBufAlgo functions, I also
exposed the new functionality as oiiotool options.