-
Notifications
You must be signed in to change notification settings - Fork 696
Refine colorInteropID support #2560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| #include "ImfOutputPartData.h" | ||
| #include "ImfOutputStreamMutex.h" | ||
| #include "ImfPartType.h" | ||
| #include "ImfStandardAttributes.h" | ||
| #include "ImfStdIO.h" | ||
| #include "ImfThreading.h" | ||
| #include "ImfTiledOutputFile.h" | ||
|
|
@@ -377,6 +378,18 @@ MultiPartOutputFile::Data::overrideSharedAttributesValues ( | |
| dst.insert ("chromaticities", *chromaticities); | ||
| else | ||
| dst.erase ("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") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this enforce that if the first part is 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| { | ||
| if (hasColorInteropID (src)) | ||
| addColorInteropID (dst, colorInteropID (src)); | ||
| else | ||
| dst.erase ("colorInteropID"); | ||
| } | ||
| } | ||
|
|
||
| bool | ||
|
|
@@ -447,6 +460,23 @@ MultiPartOutputFile::Data::checkSharedAttributesValues ( | |
| } | ||
| } | ||
|
|
||
| // | ||
| // ColorInteropID -- a part after the first may omit it (inheriting | ||
| // the first part's value), or set it to "data" to mark image data | ||
| // that should not be color managed. Any other value that differs | ||
| // from the first part's is a conflict. | ||
| // | ||
| if (hasColorInteropID (dst) && colorInteropID (dst) != "data") | ||
| { | ||
| if ((hasColorInteropID (src) && | ||
| colorInteropID (src) != colorInteropID (dst)) || | ||
| (!hasColorInteropID (src))) | ||
| { | ||
| conflict = true; | ||
| conflictingAttributes.push_back ("colorInteropID"); | ||
| } | ||
| } | ||
|
|
||
| return conflict; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -600,6 +600,26 @@ IMF_STD_ATTRIBUTE_DEF (reelName, ReelName, std::string) | |
|
|
||
| IMF_STD_ATTRIBUTE_DEF (chromaticities, Chromaticities, Chromaticities) | ||
|
|
||
| // | ||
| // ColorInteropID -- the Color Interop Forum ID that describes the color space of the RGB images. | ||
| // | ||
| // This is a specially designed string used to communicate the color space of | ||
| // the image in an interoperable manner. The contents of the string is described | ||
| // in the specification "An ID for Color Interop": | ||
| // <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/03_ColorInteropID/ColorInteropID.md> | ||
| // | ||
| // Guidance to application developers is provided in "Identifying the Color Space of OpenEXR Files": | ||
| // <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/04_OpenEXRFiles/OpenEXRFiles.md> | ||
| // | ||
| // Please adhere to the following usage guidance: | ||
| // 1. If the color space of an image is modified, do not forward the stale attribute value. | ||
| // 2. Do not set this to a default color space. If the actual color space is not known, omit | ||
| // the attribute or set it to "unknown". | ||
| // | ||
| // New in OpenEXR v3.4 | ||
|
|
||
| IMF_STD_ATTRIBUTE_DEF (colorInteropID, ColorInteropID, std::string) | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the links and moved this up to be next to chromaticities. |
||
| // | ||
| // whiteLuminance -- for RGB images, defines the luminance, in Nits | ||
| // (candelas per square meter) of the RGB value (1.0, 1.0, 1.0). | ||
|
|
@@ -724,25 +744,4 @@ IMF_STD_ATTRIBUTE_DEF_DEPRECATED ( | |
|
|
||
| IMF_STD_ATTRIBUTE_DEF (idManifest, IDManifest, CompressedIDManifest) | ||
|
|
||
| // | ||
| // ColorInteropID -- the Color Interop Forum ID that describes the color space of the RGB images. | ||
| // | ||
| // This is a specially designed string used to communicate the color space of | ||
| // the image in an interoperable manner. The contents of the string is described | ||
| // in the specification "An ID for Color Interop": | ||
| // <https://docs.google.com/document/d/1T94lYbis9uCskL_ZEMxGBF2JryLfZnjxlEoNgRHZzBE/edit?usp=sharing> | ||
| // | ||
| // Guidance to application developers is provided in "Identifying the Color Space of OpenEXR Files": | ||
| // <https://docs.google.com/document/d/1MTH1bq2L67ifvdDf64Amhzg4AbkIM5LG6yPHrB96Vwo/edit?usp=sharing> | ||
| // | ||
| // Please adhere to the following usage guidance: | ||
| // 1. If the color space of an image is modified, do not forward the stale attribute value. | ||
| // 2. Do not set this to a default color space. If the actual color space is not known, omit | ||
| // the attribute or set it to "unknown". | ||
| // | ||
| // New in OpenEXR v3.4 | ||
|
|
||
|
|
||
| IMF_STD_ATTRIBUTE_DEF (colorInteropID, ColorInteropID, std::string) | ||
|
|
||
| #endif | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a file has acesContainerFlag and Chromaticities set to ACES, but the colorInteropID set to something else? Should the input side error, or change the colorInteropID on reading? Arguably, SMPTE 2065-4 would say that correct behavior is to ignore the colorInteropID, so it "doesn't exist" in ACES files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImfAcesFile does not seem to do any validation of the other inputs, so I did not add any for colorInteropID either. AcesOutputFile seems to ignore the chromaticities of the provided header and write the ACES ones, so I did the same with colorInteropID.
AcesInputFile is more confusing than I first realized. When I looked at ImfAcesFile.cpp as I was doing the PR, I mistakenly thought the color conversion was part of the AcesOutpuFile because it converts from the file header color space to ACES2065-1, which would be reasonable on output. However, it's actually part of AcesInputFile, and converts from the file's color space to ACES2065-1.
I had thought that ImfAcesFile was intended to read and write ST 2065-4 files, but maybe I'm wrong about what the input side is intended for. It seems that AcesInputFile is actually reading OpenEXR files with arbitrary chromaticities and converting the image buffer to the ACES2065-1 color space. Is that really what it's supposed to do? I would guess that almost no one would want that! The more typical workflow is to use ACES2065-1 as the storage space and ACEScg as the working/image processing space.
Please let me know the actual intent of this class and I will update the PR accordingly. Thank you for your review Peter!