MrLixm / AgXc

Fork of Troy.S AgX, a display rendering transform available via OCIO and more
92 stars 8 forks source link

[OCIO] ACES interchange inaccuracy #16

Closed conlen-b closed 1 year ago

conlen-b commented 1 year ago

The conversion to ACES2065-1 for interchange seems to be incorrect-- I found this error originally with the following process: I have a scene-referred render from Maya + V-Ray with the Linear sRGB rendering space from your config. I bring this into a Nuke scene set up with your OCIO config with a read node with the IDT set to Linear sRGB. Using an OCIOColorSpace node, I convert Linear sRGB -> ACES2065-1. I write this out to a 16bit EXR using a write node with the ODT set to Passthrough.

In a different nuke scene using the cg-config-v1.0.0_aces-v1.3_ocio-v2.0.ocio OCIO config from https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/releases, I read in the 16bit EXR from the previous Nuke scene with the IDT set to Raw. With a second read node, I read in the original scene-referred render with the IDT set to Raw. I then use an OCIOColorSpace node to convert the second read node from Linear Rec.709 -> ACES2065-1. If I set my viewer to Raw (sRGB - Display) and compare the two, they are close but still pretty off. Liam was able to recreate this issue as well.

MrLixm commented 1 year ago

Thanks for reporting, manage to reproduce the issue on my side. Will investigate what's going on

MrLixm commented 1 year ago

Just created a new branch with a new module that allow me to reproduce the generation of the MatrixTransform I used in the OCIO config :

https://github.com/MrLixm/AgXc/blob/16-ocio-matrix-diff/dev/ocio/colorspaces.py

Executing this module gives me :

xyz_to_ap0()=[1.0604675987, 0.008384430217, -0.014338952537, 0.0, -0.486910537062, 1.370790753303, 0.093070834934, 0.0, -0.005306229727, 0.006434186849, 0.916879121543, 0.0, 0.0, 0.0, 0.0, 1.0]
srgb_to_xyz()=[0.4124, 0.3576, 0.1805, 0.0, 0.2126, 0.7152, 0.0722, 0.0, 0.0193, 0.1192, 0.9505, 0.0, 0.0, 0.0, 0.0, 1.0]
ap0_to_srgb()=[2.526838601825, -1.115756102267, -0.368990640029, 0.0, -0.297761286039, 1.376149938024, -0.084965677227, 0.0, -0.009234528688, -0.171078993092, 1.168621917667, 0.0, 0.0, 0.0, 0.0, 1.0]

The 2 first matrix corresponds to what I have in the config.

The last matrix is suppose to correspond to what is used in the ACES cg config : https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES For the Linear Rec.709 (sRGB) colorspace.

Though in the config here is the matrix specified :

[2.52168618674388, -1.13413098823972, -0.387555198504164, 0, -0.276479914229922, 1.37271908766826, -0.096239173438334, 0, -0.0153780649660342, -0.152975335867399, 1.16835340083343, 0, 0, 0, 0, 1]

Which as you can notice is not the same as what I mange to produce with my method.

Next step find why. I think I need to check if my code is using the official matrix (with rounding issues) or the derived one for the primaries.

MrLixm commented 1 year ago

Found the issue, it was of course in my matrix generation code. I mixed up the order in which I must concatenate the CAT matrix + I should also have used derived matrices (at least that what are using the official ACES configs)

You can check the fix in commit https://github.com/MrLixm/AgXc/commit/61f076ac3528115718d701d36d82491ec778c83b

I will now propagate it to the OCIO config

MrLixm commented 1 year ago

Actually no, I did not fix anything, the issue is still here as that new commit still produce the same matrix. I manage to find that the issue is on the CIE XYZ D65 to AP0 matrix (also to AP1). The sRGB > CIE XYZ D65 matrix is a perfect match with the ACES config.

MrLixm commented 1 year ago

I finally have it, the order is which is was multiplying the matrices when going from RGB to XYZ was not correct, I had to inverse it. Fix in commit 8c9a1404c352aed9534bf71c90576bf46ab85f7d

For confirming it was working I used :

import colour
import numpy

d65 = colour.CCS_ILLUMINANTS["CIE 1931 2 Degree Standard Observer"]["D65"]
ap0: colour.RGB_Colourspace = colour.RGB_COLOURSPACES["ACES2065-1"]
ap0.use_derived_transformation_matrices(True)
array = numpy.array([0.5, 0.4, 0.3])
converted = colour.XYZ_to_RGB(
    array,
    d65,
    ap0.whitepoint,
    ap0.matrix_XYZ_to_RGB,
    "Bradford",
)
print(converted)

matrix = numpy.array( [ [...], [...], [...]])
converted_w_matrix = numpy.dot(matrix, array)
print(converted_w_matrix)
MrLixm commented 1 year ago

Fix released in 0.5.1 https://github.com/MrLixm/AgXc/releases/tag/v0.5.1

Thanks again for the report, that is a pretty embarrassing issue, would have been very annoying to not get this fixed.