Add ARRI contributions to OCIO-ACES CLFs

[scoopxyz]

This PR supplies the inital ARRI CLFs for the ACES config. This includes:

• ARRI LogC3 (EI800)
• ARRI LogC4

Points of conversation/review:

• Success in compliance with CLF recommended practice
• Plan for inclusion of further LogC3 transforms
• Code organization and generation of cameraLogToLin parameters
• OCIO Transform IDs and naming conventions

Special thanks to @doug-walker for his assistance.

Tagging @JGoldstone for visibility and his helpful input.

This will also be a test of the CLA system on ARRI's side which may need some tweaks.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: scoopxyz / name: Sean Cooper (31d1fbe, 543e722, 5e35362, 3bb25d1, 3512b01, 309ed4a, 8a61227, a5256c7)

[JGoldstone]

I thank you for thinking of me when it comes to pedantic, compulsive review: The EI check at line 64 is done differently then the unsupported EI error message at line 65. Do IDEs that help the user on hover of a function call put up everything in the Python comment, or only the function parameters? If there are cases when it might be the latter, then the EI restriction should be expressed not in the main body of the comment, but as part of the documentation of the ei argument on line 50. Taken in isolation line 45 could imply that you can use any EI value ≤ 1280, but in fact you can’t have an EI < 160. Why clutter someone’s debug variable pane with values that aren’t used? Turn lines 94 and 95 into comments, if those values aren’t used. Similarly comment out lines 104 and 105 in the debug output. You leave the ei argument to the generate_logc3 function in place on line 38, and on line 45 you imply it can be one of several values, as you do in line 50. In line 65 you provide for putting the supplied EI into the error message. But when you generate the clf_id on line 142, you hardcode EI 800. The same can be sed for the contents of the Description and InputDescriptor child elements you create on lines 146 and 147. You do the right thing on line 157 when forming the frame; you should do the same for the above cases. Supports continuous range of EIs but the camera does not generate a continuous range. Worth noting what the supported ones are, so that no one makes one that corresponds to something the camera doesn’t produce. If the prior LogC3 IDT generator handled EIs 1600, 1280 and 3200 the ‘right way’, with more complex math generating 1D LUTs, then I would find the last tagged release that included that code, and put in a comment referring anyone who wanted to generate IDTs for those EIs to that code — but just for those EIs. For EIs < 1600, they should use the code you’re providing here. In general, though, looks great. Thanks for the work.

…

On Jun 2, 2022, at 12:47, Sean Cooper ***@***.***> wrote: This PR supplies the inital ARRI CLFs for the ACES config. This includes: ARRI LogC3 (EI800) ARRI LogC4 Points of conversation/review: Success in compliance with CLF recommended practice Plan for inclusion of further LogC3 transforms Code organization and generation of cameraLogToLin parameters OCIO Transform IDs and naming conventions Special thanks to @doug-walker <https://github.com/doug-walker> for his assistance. Tagging @JGoldstone <https://github.com/JGoldstone> for visibility and his helpful input. This will also be a test of the CLA system on ARRI's side which may need some tweaks. You can view, comment on, or merge this pull request online at: #54 <#54> Commit Summary 765bf40 <765bf40> add ARRI contributions to OCIO-ACES CLFs File Changes (3 files <https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54/files>) A opencolorio_config_aces/clf/transforms/arri/generate.py <https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54/files#diff-141e076f755146d4b151fe28756fcc16484fb6733738660483d80a044feddba2> (263) A opencolorio_config_aces/clf/transforms/arri/input/ARRI.LogC3_EI800_to_ACES2065-1.clf <https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54/files#diff-53dc7a819ad5dfbcdca7e569cdf0cdfeade994c52812e14343c9fa9ae0ed0dc1> (19) A opencolorio_config_aces/clf/transforms/arri/input/ARRI.LogC4_to_ACES2065-1.clf <https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54/files#diff-961404223b9c70dd17a9935a2108807db57f63bde7aa3e37a46fbffcfa1ab0f5> (19) Patch Links: https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54.patch <https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54.patch> https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54.diff <https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/pull/54.diff> — Reply to this email directly, view it on GitHub <#54>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIVB7P5E7UOE5CMRJFD7I3VNEFWFANCNFSM5XWA7WRQ>. You are receiving this because you were mentioned.

[doug-walker]

This is great, thanks Sean!

The PR raises a number of points that we should probably discuss at our next working group meeting:

1. Matrix precision. OCIO processing is done as floats but it stores and works with matrices as doubles. This can be important, for example when the optimizer is deciding whether a pair of matrices is close enough to an identity that it may be omitted. All of the camera vendor white papers seem to present a set of chromaticity values and a 3x3 matrix for converting to CIE 1931 XYZ and/or ACES2065-1, but in double precision calculations, only one of those is really exact. My preference is to treat the chromaticities as the exact values rather than the matrices. This is how the OCIO Built-in Transforms, including the ones for LogC3 work. It's also how the aces-dev CTL for the LogC3 CSC transform works. Would you be open to do this via Thomas's colour library in this code (as we do elsewhere) to calculate the matrix values rather than using the values you copied in from the white paper?
2. You have some options for EI and clamping. The only EI we had planned to include in the Studio Config for LogC3 was EI 800. Does that match ARRI's expectations? Regarding clamping, I think we would want that off for the Studio Config. Are you envisioning a scenario in which we want it on? Even if we decide that the Studio Config is always 800/no clamp, I think it's fine to leave these in the code if only to make people aware of the issues involved. Someone may find these helpful for generating their own configs for special situations.
3. I see you repeated "ARRI" in the "Transform name" part of the CLF ID string. The formatting guide does not repeat the vendor name there, but I'd like to hear more about your rationale for putting it there too. We should probably try to be consistent across vendors.
4. When I added the RED transforms I did not follow the directory hierarchy that is in the formatting guide. I believe I was following a discussion we had at a working group meeting (although maybe I got it wrong). But my rationale for RED was that, for a half page of code, it is not worth creating separate directories for each vendor. Perhaps in ARRI's case, there is somewhat more code and so it should be separate. But let's discuss this again at the working group to make sure we're all on the same page. I put a comment in the formatting guide to remind us to update it, if necessary.
5. Your Log op for LogC3 has more significant digits than the OCIO Builtin Transform for LogC3. As with the Matrix op, these are stored in OCIO as doubles. While the optimizer for the Matrix op does allow for some small amount of tolerance, the Log op is currently set up for strict comparison. I'm thinking we should maybe add some tolerance, but in any case, I want to make sure that the transform here and the Built-in Transform give the same results, or at least close enough to optimize out. I'm certainly open to discussing which side to make the change on.
6. We should add a Built-in transform for LogC4, and ideally it should be exactly the same as what is done here.
7. We should discuss what the Studio config will look like for color spaces where there is a Built-in Transform available. In cases where a Lut is involved, we should clearly use the Built-in. For other cases, there are pros and cons for using the Built-in or adding explicit Matrix and Log ops. (Either way, I think that having the database of CLFs has value beyond OCIO, so I'm not opposed to generating the CLF files, even if only the metadata gets used.)

Thanks again for the PR!

[scoopxyz]

Matrix precision. OCIO processing is done as floats but it stores and works with matrices as doubles. This can be important, for example when the optimizer is deciding whether a pair of matrices is close enough to an identity that it may be omitted. All of the camera vendor white papers seem to present a set of chromaticity values and a 3x3 matrix for converting to CIE 1931 XYZ and/or ACES2065-1, but in double precision calculations, only one of those is really exact. My preference is to treat the chromaticities as the exact values rather than the matrices. This is how the OCIO Built-in Transforms, including the ones for LogC3 work. Would you be open to use Thomas's colour library in this code (as we do elsewhere) to calculate the matrix values rather than using the values you copied in from your white paper?

There are separate reasons for LogC3 and LogC4. For LogC3, this is a legacy consistency issue. I presented the CLF in a manner which represents the current state of the aces-dev repo. If this is not the goal of ACES config, then there is a different conversation to have.

For LogC4, you are correct we hold the primaries to be fully precise (by definition). Our internal matrix derivation and Thomas' code can both be validated against SMPTE RP 177, and both use CAT02, so I don't see the benefit of using external code to derive them. I would prefer float/double literals(decimal or hex), and not have the conversion vary based on intended/unintended changes of upstream software. If you need the values to be fully specified for double precision than I can provide this but will update the aces-dev PR and our White Paper to reflect this. I would like consistency across the public definitions of LogC4.

If we can discuss what is the correct and proper way to communicate color space primaries and conversion matricies then I am open ears. The SMPTE RP 177 deserves an update in this respect, as it does not satisfy the current industry's precision requirements.

You have some options for EI and clamping. The only EI we had planned to include in the Studio Config for LogC3 was EI 800. Does that match ARRI's expectations? Regarding clamping, I think we would want that off for the Studio Config. Are you envisioning a scenario in which we want it on? Even if we decide that the Studio Config is always 800/no clamp, I think it's fine to leave these in the code if only to make people aware of the issues involved. Someone may find these helpful for generating their own configs for special situations.

My understanding was that only including EI800 was a temporary measure to speed up creation of the configs. I think it is required to support all EI to enable correct linearization of LogC3 material.

Clamping is available to ensure consistency with the aces-dev CTL, and as an informative note that this is the behavior of the aces-dev CTL.

I see you repeated "ARRI" in the "Transform name" part of the CLF ID string. The formatting guide does not repeat the vendor name there, but I'd like to hear more about your rationale for putting it there too. We should probably try to be consistent across vendors.

This is because the fully qualified name of the color space is "ARRI LogC4", "LogC4" is the short-hand name.

When I added the RED transforms I did not follow the directory hierarchy that is in the formatting guide. I believe I was following a discussion we had at a working group meeting (although maybe I got it wrong). But my rationale for RED was that, for a half page of code, it is not worth creating separate directories for each vendor. Perhaps in ARRI's case, there is somewhat more code and so it should be separate. But let's discuss this again at the working group to make sure we're all on the same page. I put a comment in the formatting guide to remind us to update it, if necessary.

Open ears here. Tell the convention you prefer and I will follow.

Your Log op for LogC3 has more significant digits than the OCIO Builtin Transform for LogC3. As with the Matrix op, these are stored in OCIO as doubles. While the optimizer for the Matrix op does allow for some small amount of tolerance, the Log op is currently set up for strict comparison. I'm thinking we should maybe add some tolerance, but in any case, I want to make sure that the transform here and the Built-in Transform give the same results, or at least close enough to optimize out. I'm certainly open to discussing which side to make the change on.

What was the source of the current float literals used in the Built-In-Transform?

I present them in this way because I wanted to be able to derive the parameters from the code in the public aces-dev IDT, to connect both worlds. The precision is merely a result of the computation to derive them, becaues they are not implemented as float-literals in this form on our end. Higher precision would be preferred, but could be overridden by compatibilty concerns. Are the LogOps single precision, unlike the matrix?

We should add a Built-in transform for LogC4, and ideally it should be exactly the same as what is done here.

Yes agreed, I can put forth a PR in the coming weeks.

We should discuss what the Studio config will look like for color spaces where there is a Built-in Transform available. In cases where a Lut is involved, we should clearly use the Built-in. For other cases, there are pros and cons for using the Built-in or adding explicit Matrix and Log ops. (Either way, I think that having the database of CLFs has value beyond OCIO, so I'm not opposed to generating the CLF files, even if only the metadata gets used.)

No strong opinion here, provided results through either path are reasonably equivalent.

[scoopxyz]

Thanks for the comments, and catching the string formatting bug @JGoldstone. I've implemented most of your changes.

Why clutter someone’s debug variable pane with values that aren’t used? Turn lines 94 and 95 into comments, if those values aren’t used. Similarly comment out lines 104 and 105 in the debug output.

I will keep these for verboseness and education. Allows quick validation against White Paper values.

[doug-walker]

Thanks for the detailed reply Sean. I'll try to summarize where we stand on the seven points I raised above:

1. Matrix precision
   Please keep in mind that there are two EI 800 LogC3 transforms in aces-dev: one under IDTs and one under CSCs and they are slightly different. For its built-in transform OCIO followed the CSC one, partly because it is more recent and partly because it more closely followed the style of many other transforms in aces-dev. So I propose that the new LogC3 CLF match the aces-dev CSC code and the OCIO built-in code. I have no objection to hard-coding the matrix coefficients in the Python implementation, if you prefer, as long as they are at least double precision.

2. EI 800 only? Clamping on or off?
   Currently the Studio config spreadsheet only has the EI 800 one, so that's why I wanted to bring this to your attention. You will need to make the case if you want more EI variants added. It sounds like we both agree that keeping the clamping option in the Python code is worthwhile. You have it off for the official CLF in the PR, so I think we agree it should be off for the Studio config. My view is that it should be off to avoid problems with LogC values that have been graded (or had a CDL applied) in Log space.

3. ARRI in the CS name
   Let's discuss the naming more. There are two questions: first, should "ARRI" be repeated in both the family and color space, and second, should the primaries be included? You have "ARRI LogC3 (EI800)" and "ARRI LogC4" in the CLFs. I often see naming, both for ARRI color spaces and other vendors, that include some mention of the primaries. For example, "LogC (v3-EI800) AlexaWideGamut". For LogC3 there was more than one gamut, not sure if that applies to LogC4? The "ARRI" prefix question is related to the discussion in Issue Prefix/suffix for color space names in the new configs #57.

4. Directory organization
   The group decided during last week's meeting to leave it as you have it for now.

5. Log Op precision
   The Log parameters in OCIO are double precision. Your LogC4 CLF is perfect. As I wrote for point no.1, my preference would be to follow the LogC3 implementation of the aces-dev CSC and the OCIO built-in transform, which has slightly different constants than your proposed CLF. But they do agree to more than single-precision, so I will not object if you feel strongly about leaving it as is.

6. Add LogC4 as a built-in transform to OCIO?
   We are in agreement that this should be done.

7. For CLFs that are available as OCIO built-ins, what should the config look like?
   This was more of a question for Thomas and the group. To be discussed at the meeting. My preference would be to use the built-ins in the config, where available.

[carolalynn]

Hi @scoopxyz - hoping to get an update on this PR? I know we talked through several small changes last time we all spoke. We are planning on releasing the first version of the Studio config at SIGGRAPH in... a week and a half! Would be great to have this included. Please let us know if we can help!

[scoopxyz]

@KelSolaar it looks like CI is failing because the remote Excel sheet has bumped to v0.2.0 but the local file on main is v0.1.0

[doug-walker]

Thanks for the update Sean! I made some minor suggestions below but will approve now since I think it's close enough to be merged for beta testing the next version of the config.

The log transform values for LogC3 are very slightly different than what the OCIO BuiltinTransform uses, which will currently prevent adjacent fwd/inv transform pairs from being optimizing out. However, I intend to loosen the comparison on the OCIO side, so it will be ok.

The matrix transform values for AWG4 are very slightly different from what I think colour would give. But the difference is below what would prevent OCIO from optimizing out adjacent fwd/inv matrices, so it's ok.

[doug-walker]

Thanks for the updates Sean!

[KelSolaar]

This is a bit anti-pattern, following-up the entry-point, i.e. _main, we get into this definition generate_clf_arri, there we can see the _generate_logc3_transforms and _generate_logc4_transforms definition assigning to the same variable.

If you don't look in details at the signature of the _generate_logc*_transforms definitions you would think that this is a mistake.

Ideally the definitions should either:

1. Update the passed variable in place, in which case they should not return anything.
2. Not take an argument and return the generated transforms.

My preference would be for 2 because:

• This makes the definitions pure.
• It makes the usage simpler: We could remove the weirdly name transforms_set argument which is actually not a set but a dict.
• It is similar to other CLF definitions.

Happy to merge and do those changes in a subsequent PR!

[scoopxyz]

Agreed. Simply didn't think of a nice way to merge dicts.

Fixed.

[KelSolaar]

Awesome, thank you!

[scoopxyz]

@dp-arri for visibility.

[KelSolaar]

LGTM! Thanks al lot @scoopxyz!