AgX-Step3: Refactor OCIO config to be CIE XYZ I-E based #110913

Zijun Zhou · 2023-08-08T06:12:38+02:00

Zijun Zhou commented

2023-08-08 06:12:38 +02:00

This is step 3 of the AgX implementation, as discussed in the original AgX PR

step 3. refactor the config's transform tree to be XYZ I-E based.
The design decision has been discussed in this PR, XYZ I-E is the actual CIE standard, it's to prepare for Spectral Cycles, but this will also make the next step more straight forward.

This includes changing the xyz_D65_to_E.spimtx to use the more accurate Bradford method instead of XYZ Scaling, and also adding xyz_E_to_D65.spimtx since the opposite direction uses a different matrix.

EDIT: Forgot to mention this also involved in renaming of the XYZ space, as mentioned by the comment

This is step 3 of the AgX implementation, as discussed in [the original AgX PR](https://projects.blender.org/blender/blender/pulls/106355#issuecomment-984699) >step 3. refactor the config's transform tree to be XYZ I-E based. The design decision has been discussed in this PR, XYZ I-E is the actual CIE standard, it's to prepare for Spectral Cycles, but this will also make the next step more straight forward. > >This includes changing the xyz_D65_to_E.spimtx to use the more accurate Bradford method instead of XYZ Scaling, and also adding xyz_E_to_D65.spimtx since the opposite direction uses a different matrix. EDIT: Forgot to mention this also involved in renaming of the XYZ space, as mentioned by the comment

Zijun Zhou added 3 commits 2023-08-08 06:12:49 +02:00

eb6d9bfa4a Refactor the config to be CIE-XYZ I-E based

0ff4ef2b8c Update Chromatic Adaptation method from `XYZ Scaling` to the more accurate `Bradford`

buildbot/vexp-code-patch-coordinator Build done. Details

feb692ed52 Delete release/datafiles/colormanagement/luts/srgb_to_xyz.spimtx

Zijun Zhou requested review from Sergey Sharybin 2023-08-08 06:13:51 +02:00

Zijun Zhou requested review from Brecht Van Lommel 2023-08-08 06:14:18 +02:00

Zijun Zhou referenced this pull request

2023-08-08 06:17:17 +02:00

Replace Default OCIO config with AgX (Filmic v2) #106355

Sergey Sharybin commented

2023-08-08 09:17:03 +02:00

@blender-bot build

Brecht Van Lommel reviewed 2023-08-08 10:57:59 +02:00

release/datafiles/colormanagement/config.ocio Outdated

						
				@ -97,4 +124,2 @@

				        - !<BuiltinTransform> {style: "UTILITY - ACES-AP1_to_CIE-XYZ-D65_BFD", direction: inverse}

				  - !<ColorSpace>

				    name: XYZ

Brecht Van Lommel commented

2023-08-08 10:52:15 +02:00

This is a significant change that breaks forward compatibility, but it is not mentioned in the PR description.

brecht marked this conversation as resolved

release/datafiles/colormanagement/config.ocio Outdated

						
				@ -62,2 +63,4 @@

				colorspaces:

				  - !<ColorSpace>

				    name: Linear CIE-XYZ I-E

Brecht Van Lommel commented

2023-08-08 10:48:41 +02:00

Slightly shorter name: Linear CIE-XYZ E

Slightly shorter name: `Linear CIE-XYZ E`

brecht marked this conversation as resolved

release/datafiles/colormanagement/config.ocio Outdated

						
				@ -64,0 +69,4 @@

				    equalitygroup:

				    bitdepth: 32f

				    description: |

				      1931 CIE XYZ Standard with assumed I-E white point

Brecht Van Lommel commented

2023-08-08 10:50:16 +02:00

Standard -> standard
I-E -> illuminant E

Standard -> standard I-E -> illuminant E

brecht marked this conversation as resolved

release/datafiles/colormanagement/config.ocio Outdated

						
				@ -64,0 +73,4 @@

				    isdata: false

				  - !<ColorSpace>

				    name: Linear CIE-XYZ I-D65

Brecht Van Lommel commented

2023-08-08 10:48:50 +02:00

Slightly shorter name: Linear CIE-XYZ D65

As can be seen from the aliases, it's not common to have this I- prefix and not self explanatory anyway.

Slightly shorter name: `Linear CIE-XYZ D65` As can be seen from the aliases, it's not common to have this `I-` prefix and not self explanatory anyway.

brecht marked this conversation as resolved

release/datafiles/colormanagement/config.ocio Outdated

						
				@ -64,0 +79,4 @@

				    equalitygroup:

				    bitdepth: 32f

				    description: |

				      1931 CIE XYZ with adapted I-D65 white point

Brecht Van Lommel commented

2023-08-08 10:50:06 +02:00

I-D65 -> illuminant D65, here we can actually use the full word

brecht marked this conversation as resolved

release/datafiles/colormanagement/config.ocio

						
				@ -64,0 +82,4 @@

				      1931 CIE XYZ with adapted I-D65 white point

				    isdata: false

				    from_scene_reference: !<FileTransform> {src: xyz_E_to_D65.spimtx, interpolation: linear}

				    to_scene_reference: !<FileTransform> {src: xyz_D65_to_E.spimtx, interpolation: linear}

Brecht Van Lommel commented

2023-08-08 10:54:32 +02:00

Is it necessary to provide transforms in two directions?

I wonder if that might actually affect performance negatively, if perhaps in this case OpenColorIO can no longer cancel out the transforms.

Is it necessary to provide transforms in two directions? I wonder if that might actually affect performance negatively, if perhaps in this case OpenColorIO can no longer cancel out the transforms.

brecht marked this conversation as resolved

Zijun Zhou added 1 commit 2023-08-08 11:26:35 +02:00

b6323eac14 Adress comments

Zijun Zhou added 1 commit 2023-08-08 11:28:43 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

806effe40d Fix consequence after renaming

Zijun Zhou commented

2023-08-08 11:30:39 +02:00

Comments are adressed.

Brecht Van Lommel commented

2023-08-08 12:01:04 +02:00

@blender-bot build

Brecht Van Lommel approved these changes 2023-08-08 12:34:01 +02:00

Sergey Sharybin referenced this pull request

2023-08-08 14:33:59 +02:00

Add Linear CIE-XYZ D65 as alias to XYZ color space #110924

Sergey Sharybin approved these changes 2023-08-08 14:35:06 +02:00

Sergey Sharybin left a comment

LGTM.

I've created a patch for 3.6 to keep forward compatibility as per our policy: #110924

LGTM. I've created a patch for 3.6 to keep forward compatibility as per our policy: #110924

Sergey Sharybin referenced this issue from a commit

2023-08-08 14:39:42 +02:00

Add Linear CIE-XYZ D65 as alias to XYZ color space

Sergey Sharybin added 1 commit 2023-08-08 15:10:00 +02:00

7ced631807 Remove unused matrix file

Sergey Sharybin merged commit 1bcf31a874 into main

2023-08-08 15:11:17 +02:00

Sergey Sharybin deleted branch AgX-Step3

2023-08-08 15:11:19 +02:00

Nathan Vegdahl commented

2023-08-09 10:42:42 +02:00

Sorry I'm a bit late to this. Didn't realize it would get merged so quickly.

A tiny nit: rather than Linear CIE-XYZ I-E or Linear CIE-XYZ E it can just be Linear CIE XYZ. X=Y=Z is already defined to be E by the standard, so mentioning it in the name seems redundant to me. E.g. we don't call sRGB "sRGB D65", etc. It does make sense in the D65 XYZ variant, though, since that deviates from actual CIE XYZ.

(I also wondered briefly if we should drop the Linear prefix as well, for similar reasons. But thinking a bit more, it probably makes sense to have a general linear/non-linear distinction for the spaces, and we're already using the Linear prefix for that anyway.)

Sorry I'm a bit late to this. Didn't realize it would get merged so quickly. A tiny nit: rather than `Linear CIE-XYZ I-E` or `Linear CIE-XYZ E` it can just be `Linear CIE XYZ`. X=Y=Z is already defined to be E by the standard, so mentioning it in the name seems redundant to me. E.g. we don't call sRGB "sRGB D65", etc. It does make sense in the D65 XYZ variant, though, since that deviates from actual CIE XYZ. (I also wondered briefly if we should drop the `Linear` prefix as well, for similar reasons. But thinking a bit more, it probably makes sense to have a general linear/non-linear distinction for the spaces, and we're already using the `Linear` prefix for that anyway.)

Sergey Sharybin commented

2023-08-09 10:51:00 +02:00

We can add an alias if needed, but on the interface I'd rather stick to a fully disambiguated naming. I do not think it is great to require people to know standards by heart to know which illuminant is used. When you see "Linear CIE-XYZ E" you know for sure what it is. If you see "Linear CIE-XYZ" you'll be like "holap, lemme double check my knowledge of standard".

Nathan Vegdahl commented

2023-08-09 12:45:28 +02:00

I could see that. But to expand on my perspective:

To me it's more that CIE XYZ doesn't have a white point in the traditional sense. In RGB color spaces, for example, the white point plays double-duty:

It's the last piece needed to fully define the color space as a coordinate system (by implicitly defining the relative RGB intensities at unity).
And it also defines an assumed perceptual white point for viewing colors defined in that space.

In CIE XYZ, on the other hand, X=Y=Z -> illuminant E is purely about defining the coordinate system colorimetrically, and has none of the perceptual implications usually associated with specifying a white point. It's a bit of a subtle distinction, but in my mind at least it means that CIE XYZ does not have a white point as such—it's just purely a colorimetric coordinate system. And that's also what makes it useful as the foundation upon which other color spaces define their white points.

(XYZ D65, on the other hand, does introduce a white point into XYZ. Making it no longer CIE XYZ per se.)

In any case, I don't feel strongly about it. So I'm fine leaving it as-is. It just feels a little misleading to me, I guess, in terms of what CIE XYZ is.

I could see that. But to expand on my perspective: To me it's more that CIE XYZ doesn't have a white point in the traditional sense. In RGB color spaces, for example, the white point plays double-duty: 1. It's the last piece needed to fully define the color space as a coordinate system (by implicitly defining the relative RGB intensities at unity). 2. And it *also* defines an assumed perceptual white point for viewing colors defined in that space. In CIE XYZ, on the other hand, X=Y=Z -> illuminant E is purely about defining the coordinate system colorimetrically, and has none of the perceptual implications usually associated with specifying a white point. It's a bit of a subtle distinction, but in my mind at least it means that CIE XYZ does not have a white point as such—it's just purely a colorimetric coordinate system. And that's also what makes it useful as the foundation upon which other color spaces define *their* white points. (XYZ D65, on the other hand, *does* introduce a white point into XYZ. Making it no longer CIE XYZ per se.) In any case, I don't feel strongly about it. So I'm fine leaving it as-is. It just feels a little misleading to me, I guess, in terms of what CIE XYZ is.

Brecht Van Lommel commented

2023-08-09 14:07:50 +02:00

From that argument I slightly prefer just "Linear CIE-XYZ", but don't feel very strongly about it either way.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

AgX-Step3: Refactor OCIO config to be CIE XYZ I-E based #110913