feat(openexr): ACES Container hint for exr outputs

[Glowies]

Closes #4791

Description

This PR introduces the oiio:ACESContainer hint for OpenEXR outputs.
The hint can take one of the following values: none (default), strict, or relaxed. If not none, the spec will be checked to see if it is compliant with the ACES Container format defined in ST 2065-4. If it is, chromaticities will be set to the ACES AP0 ones, and the acesImageContainerFlag attribute will be set to 1. In strict mode, if the spec is non-compliant, the output will throw an error and avoid writing the image. While in relaxed mode, if the spec in non-compliant, only a warning will be printed and the attributes mentioned above will not be written to the spec.

Tests

I've added several tests under openexr-suite. These use oiiotool to attempt writing several EXRs with the oiio:ACESContainer hint.

Checklist:

• I have read the contribution guidelines.
• I have updated the documentation, if applicable. (Check if there is no
  need to update the documentation, for example if this is a bug fix that
  doesn't change the API.)
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• If I added or modified a C++ API call, I have also amended the
  corresponding Python bindings (and if altering ImageBufAlgo functions, also
  exposed the new functionality as oiiotool options).
• My code follows the prevailing code style of this project. If I haven't
  already run clang-format before submitting, I definitely will look at the CI
  test that runs clang-format and fix anything that it highlights as being
  nonconforming.

[lgritz]

Other than my fairly minor comments about attribute name and unnecessary work related to channel order, etc, this all looks right to me. I'll ask Anton for a review as well.

[lgritz]

Would appreciate a review from @antond-weta as the person who filed the issue this is based on (as well as being more knowledgeable than me about ACES containers.)

[lgritz]

@Glowies would you mind changing the description to say specifically

Closes #4907

or

Fixes #4907 

those are the magic words, and I think anything else doesn't trigger GitHub to link the issue to be automatically closed by the merge of the PR.

[antond-weta]

Looks good!
I've raised a point about setting the chromaticities in the "relaxed" mode automatically.

[JGoldstone]

The documentation reference should be changed to https://pub.smpte.org/pub/st2065-4/st2065-4-2023.pdf, as minor changes were made to the spec and the publication was updated.

On p. 5 there's a list of changes, and since they are short enough I'll just list them here:

• The definitions and usages of string types were clarified.
• The lensFirmwareVersion attribute was added.
• The usage of the aperture attribute was clarified.
• The definition of the timecodeRate attribute was clarified.
• The recommended supported size of data window to readers was increased.

Nothing in this changes the definition of an ACES container.

In the future (there is a five-year review of this document, only a couple years away), if the recent addition to OpenEXR of HTJ2K, a compression type that is normatively specified seems to be beneficial to the community, I could see that one particular compression type being allowed in ACES containers; I would lobby for it (if people are using it). But for now, compression remains disallowed, not because it's undesirable, but because ST 2065-4 is a normative international standard, and can only normatively reference other normatively specified international standards. At one point I did suggest a Google Summer of Code project to produce normative-quality documentation on piz, the DWA formats, etc. etc. and the output of which could be taken to SMPTE, but it was pointed out the phrase is "Summer of Code", not "Summer of Code and Doc".

Anyway, please update the reference to the more recent version of the normative definition of the ACES container.

[lgritz]

@Glowies There are a variety of (fairly minor) suggestions for changes. I think that once you fix those few things, we'll be ready to merge this.

[Glowies]

@Glowies There are a variety of (fairly minor) suggestions for changes. I think that once you fix those few things, we'll be ready to merge this.

Yep, thank you for the suggestions everyone! Didn't get a chance over the weekend but I'll be addressing them tomorrow.

[antond-weta]

I have another suggestion, sorry!

If the ImageBuf already contained the acesImageContainerFlag set, the writer should treat it the same way as if it had the oiio:ACESContainer hint set to "strict".

Here is an example of a use case when this might be important:
Using oiitool to read an AcesContainer file presumably retains the acesImageContainerFlag, if the image is then modified in a way that it's no longer compliant with the AcesContainer standard, attempting to save as an exr should make the writer fail, unless the flag was explicitly removed by the user.

This does change the existing behaviour, so I'm not sure if we want to make this change.

[lgritz]

If the ImageBuf already contained the acesImageContainerFlag set, the writer should treat it the same way as if it had the oiio:ACESContainer hint set to "strict".

Same as strict, or same as relaxed?

And now I'm wondering about inevitable confusion between the acesImageContainerFlag attribute and the openexr:ACESContainer hint.

Maybe the presence of acesImageContainerFlag should be the one and only request to write THIS image to a file as an ACES container, and there is a separate (self-documentingly named) hint about the policy for how strict to be? And at that point, we should consider if the policy hint should be global (set by OIIO:attribute()) or per-file (set as a config hint)?

[antond-weta]

Same as strict, or same as relaxed?
Either/or. Rephrasing my comment to be more generic: in the absence of any hints, if the ImageBuf contains the acesImageContainerFlag, but is otherwise noncompliant, the writer should either fail, or remove the flag. The question is what behaviour we prefer. I generally prefer the tools not to make any decisions implicitly (despite wanting the proposed hint as an instruction to set the ACES chromaticities implicitly), so I would prefer "strict".

A similar check could be added regarding the colour space: if the ImageBuf contains colour space metadata, and it is different from AP0, the writer should fail in either mode. I have zero experience working with colour spaces in OIIO, so I actually don't have a strong opinion on this.

[lgritz]

Let's be totally concrete about what behavior we want.

What specific combinations of

• (a) metadata in the spec
• (b) config hints to EXRImage::open()
• (c) global OIIO::attribute() settings

do we want to trigger each of the following behaviors:

1. Non-ACES-container regular exr files
2. Fully compliant ACES container file
3. Error, no file written

And ensure that all combinations of a-c must deterministically result in one of behaviors 1-3.

[antond-weta]

Here is my take:

bool global_aces_container_flag = get_global_aces_container_flag();
string aces_container_hint = get_aces_container_hint();
bool has_local_aces_container_flag = has_attribute("acesImageContainerFlag");
int local_aces_container_flag = get_attribute("acesImageContainerFlag", 0);

bool treat_as_aces_container = 
    (global_aces_container_flag == true) ||
    (aces_container_hint == "strict") ||
    (local_aces_container_flag == 1);

bool compliant = 
    check_compression_is_compliant() &&
    check_channels_are_compliant() &&
    check_chromaticities_missing_or_equal(AP0) &&
    check_colourspace_missing_or_equal(ACES2065-1) &&
    check_acesImageContainerFlag_missing_or_equal(1) &&
    check_colorInteropID_missing_or_equal("lin_ap0_scene");

if (treat_as_aces_container) {
    if (!compliant) error();
    else {
        if (has_local_aces_container_flag == false)
            set_attribute("acesImageContainerFlag", 1);
        set_chromaticities(AP0);
        write_file();
    }
}
else if (aces_container_hint == "relaxed") {
    if (compliant) {
        if (has_local_aces_container_flag == false)
            set_attribute("acesImageContainerFlag", 1);
        set_chromaticities(AP0);
        write_file();
    }
    else {
        if (has_local_aces_container_flag)
            remove_attribute("acesImageContainerFlag");
        set_chromaticities(AP0);
        set_attribute("colorInteropID", "lin_ap0_scene");
        issue_warnings();
        write_file();
    }
}
else {
    write_file();
}

[JGoldstone]

Oh, one other thing I remember us clarifying in the recent 2065-4 revision: most but for some reason not all string attributes were prohibited from being blank. E.g. you can't have a lensMake that is "". That's something to add to the list of things to be checked

[lgritz]

The CI failures that say something about ffmpeg not found are not related to this PR, just ignore them.

[lgritz]

I see, that makes sense. Ultimately, I'm happy to go with you recommendation on all matters ACES container.

If I understand you correctly, you want to rely 100% on the hint for whether or not to write it as an aces container? Which means it needs to be proactively set when a file is being written?

So consider this:

oiiotool acescontainer.exr -o out.exr

Should out.exr end up with the flag that makes it an ACES container?

[antond-weta]

So consider this:

oiiotool acescontainer.exr -o out.exr
Should out.exr end up with the flag that makes it an ACES container?

Yes it should, assuming that acescontainer.exr is a valid AcesContainer file.

In my pseudocode, if we don't want to do the global setting for now, we can just replace the top line with
bool global_aces_container_flag = false;
Then the presence of acesImageContainerFlag in the input file (we inherit it, but don't set manually!) should trigger the same logic as having the hint "openexr:ACESContainerPolicy" (I like this name) set to "strict". If the checks pass, the output is AcesContainer.

In other words, these (and only these?) cases should set the acesImageContainerFlag in the output file:

oiiotool valid_aces_container_file.exr -o out.exr
oiiotool aces_container_compliant_file.exr -sattrib openexr: ACESContainerPolicy relaxed -o out.exr
oiiotool aces_container_compliant_file.exr -sattrib openexr: ACESContainerPolicy strict -o out.exr

This case should set the ACES primaries and colorInteropID, but not acesImageContainerFlag:
oiiotool not_fully_compliant_file.exr -sattrib openexr: ACESContainerPolicy relaxed -o out.exr

[Glowies]

Heya @antond-weta! I've done another pass on my implementation, and I tried to follow the logic in your pseudo-code - just without the global attribute. I also tried to converge some of the branching in your pseudo-code since it looked like there was some common behaviours on some of the branches. I made two assumptions there that I wanted to check with you:

• I saw that the colorInteropId attribute was only set in the branch where we're in "relaxed" mode. I assumed the same should be the case for the other branch as well (the case where we're in "strict" mode or we have the acesImageContainerFlag already set)
• I also noticed that you were checking for the existence of the acesImageContainerFlag, and only assigned 1 to it if it does not exist. I removed that check, since I assume that the flag should be set to 1 on all cases where the image is compliant anyway.

Let me know if either of those assumptions are incorrect and I can fix them!

[lgritz]

I feel like this is not clear about what happens if the spec is passed with a colorInteropID that is set, but is not lin_ap0_scene. Are you saying that it will be changed to lin_ap0_scene (but pixels stay as they are)? Or is this something that will be rejected as non-compliant? Or does the behavior differ depending on the policy setting?

[lgritz]

Do we want "no color space known" to pass? Or fail? The way to make "none specified" to fail is to not supply default values to the get_string_attribute calls.

[Glowies]

I think I'd like to get @antond-weta's opinions on that. I had assumed that we want the "no color space known" case to pass the compliance check. (ie. If there is no attribute indicating a color space, it passes the ACES Container compliance check, and we just continue processing the image assuming it is in linear AP0 space)

[lgritz]

What's the best policy here?

For something like "doesn't have the right channels", the only thing to do is fail.

But for "wrong data type" or "wrong compression", we could force compliance by just fixing it.

[lgritz]

Philosophically, the concerns I have boil down to these:

1. Should we have a mode where we repair the things that are easily fixed? Things like asking for a compression format that shouldn't be in an ACES container, forcing half type even if another is used, etc.
2. Should we reject an image that doesn't say what color space the pixels are in, or should we add the color space label (without actually knowing if it's right)?

[lgritz]

I'm going to approve and merge this because I don't want it to languish any longer, and I think it's a definite step forward, although I believe we may wish to continue to iterate on this functionality in a couple of minor ways.

In my mind, the biggest unanswered questions (which I hope we can discuss, definitively answer, and amend with subsequent PRs if necessary) are:

• Should we require a colorInteropID set to the correct color space in strict mode (or any mode), or else consider it an error? I think that now, having NO color space info is just accepted and assumed to be in the right space. But an ACES file not in the main ACES color space is totally out-of-spec and is a problem waiting to happen, so maybe "no color space news" is actually "bad news?"

• For any or all of the modes, should we fix things that are easily fixed, rather than make an ACES file that doesn't conform to spec? As an example, if an unsupported compression method is requested, should we just force an ACES-supported compression to ensure compliance?

[zachlewis]

1. No. Per the CIR Interop ID proposal, we definitely should NOT set the colorInteropID here.

2. We can enforce one of the lossless compression modes in non-strict mode if we'd like. Strict mode should be uncompressed.

[zachlewis]

Oh, I misread item 1.

We should not prevent people from writing non-ACES stuff into the ACES container. Maybe we can log a warning with OCIO's logger, but we shouldn't do anything too contrived here.