Skip to content

feat(exr): Follow CIF for color interop ID in multi-part files #5422

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.

Sign up for GitHub

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

Open
brechtvl wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(exr): Follow CIF for color interop ID in multi-part files #5422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04a9d30
feat(openexr): Follow CIF for color interop ID in multi-part files
brechtvl Aug 24, 2026
391bfaf
Improve test naming and failureok usage
brechtvl Aug 25, 2026
6f87cc0
Make write validation optional with openexr:ColorInteropIDPolicy
brechtvl Sep 5, 2026
7de64ab
Merge branch 'main' into exr-multipart-colorspace
brechtvl Sep 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/cmake/testing.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,9 @@ macro (oiio_add_all_tests)
# Self-contained decompression-bomb regression (ships its own tiny fixture);
# exercises both the C++ and C-API readers via the openexr:core attribute.
oiio_add_tests (openexr-bomb)
# Self-contained multi-part colorInteropID inheritance test (ships its own
# tiny fixture); exercises both the C++ and C-API readers.
oiio_add_tests (openexr-multipart-colorspace)
# if (NOT DEFINED ENV{${PROJECT_NAME}_CI})
# oiio_add_tests (openexr-damaged
# IMAGEDIR openexr-images
Expand Down
8 changes: 8 additions & 0 deletions src/doc/builtinplugins.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1746,6 +1746,14 @@ control aspects of the writing itself:
While in `relaxed` mode, if the spec is non-compliant, `chromaticities`
and `colorInteropID` will be set, but `acesImageContainerFlag`
will NOT.
* - ``openexr:ColorInteropIDPolicy``
- string
- One of `none` (default) or `strict`, taken from the first subimage.
Comment on lines +1749 to +1751

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that the right place to set this is in the spec of every output file when it's opened? Is it likely that people will want to use different policies for different files within an app invocation?

Or would it be more convenient for most people to set once per execution for an app-wide policy, like we do for "openexr:core"?

@brechtvl brechtvl Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following openexr:ACESContainerPolicy which is per file, but personally I have no need for that or this new policy to be that granular.

The main use cases I can think of involve oiiotool, and a new global imageoutput:strict attribute to match imageinput:strict seems most convenient to me. Since then it's only a single option to remember for all types of validation and file formats.

For a DCC, global attributes can be problematic because there's multi-threading and plug-ins and so you have to be very careful with what you enable to avoid breaking other code. But at least in Blender I have no intent of using this and would try to make EXRs valid by construction rather than trying to validate.

I'd be happy to make it either a global openexr:ColorInteropIDPolicy or imageoutput:strict attribute (or both) instead.

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a real preference between those two. I was mostly concerned with whether it would be easier to use as a global versus per-file.

In `strict` mode, if the spec is non-compliant with the ASWF Color
Interop Forum Recommendation for OpenEXR files, the output will
throw an error and avoid writing the image. For compliance, the
`colorInteropID` in all subimages must be either equal to the first
subimage, or be unspecified or set to `data`.
* - ``oiio:RawColor``
- int
- If nonzero, writing images with non-RGB color models (such as YCbCr)
Expand Down
2 changes: 2 additions & 0 deletions src/openexr.imageio/exr_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ class OpenEXRInput final : public ImageInput {
int m_miplevel; ///< What MIP level are we looking at?
std::vector<float> m_missingcolor; ///< Color for missing tile/scanline
std::string m_filename; // filename, if known
std::string m_file_color_interop_id;
ExrChunkCache m_chunkcache;

void init()
Expand All @@ -366,6 +367,7 @@ class OpenEXRInput final : public ImageInput {
m_local_io.reset();
m_missingcolor.clear();
m_filename.clear();
m_file_color_interop_id.clear();
}

// Read scanlines [ybegin,yend) out of the chunk [cbegin,cend), decoding
Expand Down
24 changes: 22 additions & 2 deletions src/openexr.imageio/exrinput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,17 @@ OpenEXRInput::valid_file(Filesystem::IOProxy* ioproxy) const



// Color space shared by all parts of the file, taken from the first part.
static std::string
file_color_interop_id(const Imf::MultiPartInputFile* multipart)
{
const Imf::StringAttribute* attr
= multipart->header(0).findTypedAttribute<Imf::StringAttribute>(
"colorInteropID");
return attr ? attr->value() : std::string();
}


bool
OpenEXRInput::open(const std::string& name, ImageSpec& newspec,
const ImageSpec& config)
Expand Down Expand Up @@ -327,6 +338,8 @@ OpenEXRInput::open(const std::string& name, ImageSpec& newspec,
m_subimage = -1;
m_miplevel = -1;

m_file_color_interop_id = file_color_interop_id(m_input_multipart);

// Set up for the first subimage ("part"). This will trigger reading
// information about all the parts.
bool ok = seek_subimage(0, 0);
Expand Down Expand Up @@ -743,8 +756,15 @@ OpenEXRInput::PartInfo::parse_header(OpenEXRInput* in,
// Try to figure out the color space for some unambiguous cases
if (spec.get_int_attribute("acesImageContainerFlag") == 1) {
spec.set_colorspace("lin_ap0_scene");
} else if (auto c = spec.find_attribute("colorInteropID", TypeString)) {
spec.set_colorspace(c->get_ustring());
} else {
// Follow the color interop forum recommendation for OpenEXR files,
// inheriting the colorInteropID from the first part.
string_view interop_id = spec.get_string_attribute("colorInteropID");
if (!interop_id.empty()) {
spec.set_colorspace(interop_id);
} else if (!in->m_file_color_interop_id.empty()) {
spec.set_colorspace(in->m_file_color_interop_id);
}
Comment on lines +759 to +767

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't understand what's going on here. The comment says "inherit from the first part", but I think what the code is doing is "use the colorInteropID of this part, but if it doesn't exist, then use the one from the first part."

Also, none of this comes into play if acesImageContainerFlag is 1. Should that also inherit (conditionally or unconditionally?) from the first part? What's supposed to happen for a file that inconsistently says it's an aces container but also has a colorInteropID that is something other than lin_ap0_scene?

@brechtvl brechtvl Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe inherit is a poor choice of words, I will more explicitly write it out in the comment the way you did.

An ACES image container only has a single part according to the specification. OpenEXR has special handling for attributes like colorInteropID, chromaticities and displayWindow in multiple parts, but nothing for acesImageContainerFlag. So I didn't do anything here either, but I don't really have an opinion on what is best here.

The CIF OpenEXR recommendation does say that the ACES container flag has precedence over the color interop ID, so that's what I implemented.

@doug-walker doug-walker Sep 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Brecht wrote, the CIF Recommendation does give the ACES container flag precedence, and likewise the inheriting from the first part is following the recommendation.

If the file says it's an ACES container but has a different colorInteropID, that is something that would be flagged by the OpenEXR checkColorMetadata function. I'm not sure if OIIO has some kind of validation function on images, but something like that could be added there as well. On the OpenEXR side, we decided not to have errors like this block reading the file, but if exrinput has something analogous to the strict mode in exroutput, that might be a useful option.

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I do understand that the ACES container flag has precedence. I was referring to whether that, too, should also have the "first part has precedence over all others" property that the interop id has.

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, true ACES containers are only one part anyway. Never mind.

}

// Squash some problematic texture metadata if we suspect it's wrong
Expand Down
30 changes: 28 additions & 2 deletions src/openexr.imageio/exrinput_c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ class OpenEXRCoreInput final : public ImageInput {
int m_nsubimages; ///< How many subimages are there?
std::vector<float> m_missingcolor; ///< Color for missing tile/scanline
std::string m_filename; // filename, if known
std::string m_file_color_interop_id;

void init()
{
Expand All @@ -226,6 +227,7 @@ class OpenEXRCoreInput final : public ImageInput {
m_local_io.reset();
m_missingcolor.clear();
m_filename.clear();
m_file_color_interop_id.clear();
m_chunkcache.clear();
}

Expand Down Expand Up @@ -349,6 +351,21 @@ OpenEXRCoreInput::valid_file_or_proxy(const std::string& filename,



// Color space shared by all parts of the file, taken from the first part.
static std::string
file_color_interop_id(exr_context_t ctxt)
{
int32_t length = 0;
const char* interop = nullptr;
if (exr_attr_get_string(ctxt, 0, "colorInteropID", &length, &interop)
!= EXR_ERR_SUCCESS
|| !interop)
return std::string();

return std::string(interop, size_t(length));
}


bool
OpenEXRCoreInput::open(const std::string& name, ImageSpec& newspec,
const ImageSpec& config)
Expand Down Expand Up @@ -448,6 +465,8 @@ OpenEXRCoreInput::open(const std::string& name, ImageSpec& newspec,
m_subimage = -1;
m_miplevel = -1;

m_file_color_interop_id = file_color_interop_id(m_exr_context);

// Set up for the first subimage ("part"). This will trigger reading
// information about all the parts.
bool ok = seek_subimage(0, 0);
Expand Down Expand Up @@ -845,8 +864,15 @@ OpenEXRCoreInput::PartInfo::parse_header(OpenEXRCoreInput* in,
// Try to figure out the color space for some unambiguous cases
if (spec.get_int_attribute("acesImageContainerFlag") == 1) {
spec.set_colorspace("lin_ap0_scene");
} else if (auto c = spec.find_attribute("colorInteropID", TypeString)) {
spec.set_colorspace(c->get_ustring());
} else {
// Follow the color interop forum recommendation for OpenEXR files,
// inheriting the colorInteropID from the first part.
string_view interop_id = spec.get_string_attribute("colorInteropID");
if (!interop_id.empty()) {
spec.set_colorspace(interop_id);
} else if (!in->m_file_color_interop_id.empty()) {
spec.set_colorspace(in->m_file_color_interop_id);
}
}

// Squash some problematic texture metadata if we suspect it's wrong
Expand Down
53 changes: 52 additions & 1 deletion src/openexr.imageio/exroutput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,9 @@ class OpenEXROutput final : public ImageOutput {
// spec a bit.
bool spec_to_header(ImageSpec& spec, int subimage, Imf::Header& header);

// Validate color interop IDs if openexr:ColorInteropIDPolicy is set.
bool validate_color_interop_ids();

// Compute an OpenEXR PixelType from an OIIO TypeDesc
Imf::PixelType imfpixeltype(TypeDesc type);

Expand Down Expand Up @@ -710,6 +713,51 @@ OpenEXROutput::open(const std::string& name, const ImageSpec& userspec,
}


bool
OpenEXROutput::validate_color_interop_ids()
{
string_view policy = m_subimagespecs[0].get_string_attribute(
"openexr:ColorInteropIDPolicy", "none");
if (policy == "none")
return true;

if (policy != "strict") {
errorfmt("Unknown openexr:ColorInteropIDPolicy \"{}\"", policy);
return false;
}
Comment on lines +723 to +727

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merits of treating the policy request that is anything other than case-sensitive "none" and "strict" as a hard error that prevents reading the file? Or should an unknown string just silently be the default "none" behavior?

@brechtvl brechtvl Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I find it helpful for it to detect when I've made a typo. But I see openexr:ACESContainerPolicy doesn't check this, so I'm fine removing it too for consistency.

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A case could be plausibly made for being quite strict on all hint names and values, but since the status quo is usually to be as forgiving as possible and only fail if there is no reasonable interpretation of how to proceed, I think that making this one more strict than almost everything else just will tend to trip people up.


// Follow the color interop forum recommendation, where the colorInteropID
// of later parts must match the first part, except when "data" or missing.
//
// In the future, checkColorMetadata added in OpenEXR 3.5 can replace this.
string_view file_interop_id;

for (size_t s = 0; s < m_headers.size(); ++s) {
const Imf::StringAttribute* attr
= m_headers[s].findTypedAttribute<Imf::StringAttribute>(
"colorInteropID");
string_view interop_id = attr ? string_view(attr->value())
: string_view();

if (s == 0) {
file_interop_id = interop_id;
continue;
}

if (interop_id.empty() || interop_id == "data"
|| interop_id == file_interop_id)
continue;

errorfmt(
"OpenEXR subimage {} has color space \"{}\", different from \"{}\" in the first subimage",
s, interop_id, file_interop_id);
return false;
}

return true;
}



bool
OpenEXROutput::open(const std::string& name, int subimages,
Expand Down Expand Up @@ -758,6 +806,9 @@ OpenEXROutput::open(const std::string& name, int subimages,
}
}

if (!validate_color_interop_ids())
return false;

Comment on lines +809 to +811

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this means that the default "none" policy will just have no complaints if the parts (invalidly?) give and propagate different color spaces.

Is there merit to having the purpose of the policy to dictate whether we issue an error versus silently fix the problem? That is, should "none" still result in all parts inheriting the color space of the first part?

@brechtvl brechtvl Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a good way to silently fix the problem. If you have invalid parts like this:

  • "lin_ap0_scene" "lin_ap1_scene"
  • "data" "lin_ap0_scene"

Then making all parts have lin_ap0_scene or data respectively seems more damaging than helpful to me.

@doug-walker doug-walker Sep 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the chromaticities attribute, OpenEXR does have a mode where the user may request that the chromaticities of later parts are changed to match the first part, and my first commits when adding interop ID support to OpenEXR did likewise. But I removed this at the suggestion of Peter. As Brecht wrote, silently overwriting a part's ID is problematic and seem inappropriate for the "none" option. Though perhaps it would be useful in a "relaxed" or other option as a convenience for people to avoid needing to manually iterate through the parts to fix errors in cases where they know some of the parts are wrong for some reason (I think that was the motivation for OpenEXR's chromaticities mode).

@lgritz lgritz Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is fine, then. The differences only matter if you're trying to write invalid files, so I'm not inclined to care much, and we can adjust later if this is causing problems.

If I'm being honest, I would have said all along that it was perfectly fine with me for different parts to have different color spaces. OIIO's conceptual view of multi-image file is that the subimages they can be arbitrary images that just happen to be in one file container, and it's a peculiarity of OpenEXR that it's more restrictive than that. So the details of exactly how OpenEXR wants to be restrictive is not something I am going to worry much about, we just go along with what the file format says.

m_spec = m_subimagespecs[0];
sanity_check_channelnames();
compute_pixeltypes(m_spec);
Expand Down Expand Up @@ -1158,7 +1209,7 @@ static ExrMeta exr_meta_translation[] = {
// user or from a file we read.
ExrMeta("YResolution"), ExrMeta("planarconfig"), ExrMeta("type"),
ExrMeta("tiles"), ExrMeta("chunkCount"), ExrMeta("maxSamplesPerPixel"),
ExrMeta("openexr:roundingmode")
ExrMeta("openexr:roundingmode"), ExrMeta("openexr:ColorInteropIDPolicy")
};


Expand Down
Loading
Loading