AgX-Step2: Use OCIO's built-in function for sRGB #110712

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

Zijun Zhou commented

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

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

step 2. Use OCIO's built-in functionality for sRGB, includes changing sRGB to be un-clipped

This is the step 2 of AgX implementation as discussed [in the original PR](https://projects.blender.org/blender/blender/pulls/106355#issuecomment-984699) > step 2. Use OCIO's built-in functionality for sRGB, includes changing sRGB to be un-clipped

❤️ 8 🎉 5

Zijun Zhou added 3 commits 2023-08-02 06:29:47 +02:00

e0d63999d4 Use OCIO's built-in function for sRGB

This will change the sRGB space to be unclipped.

faa57cf91b Delete release/datafiles/colormanagement/luts/srgb.spi1d

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

57dcb57668 Delete release/datafiles/colormanagement/luts/srgb_inv.spi1d

Zijun Zhou requested review from Sergey Sharybin 2023-08-02 06:30:35 +02:00

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

Zijun Zhou referenced this pull request

2023-08-02 06:31:19 +02:00

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

Nathan Vegdahl approved these changes 2023-08-02 11:40:55 +02:00

Nathan Vegdahl left a comment

Looks good to me!

Some notes for other reviewers:

The expanded version of ExponentWithLinearTransform looks like:

!<ExponentWithLinearTransform> { gamma: [r, g, b, a], offset: [r, g, b, a], style: <linear | mirror>, direction: <inverse | forward> }

Which allows different values to be used for different channels. The style parameter determines how negative values are handled.

Unfortunately, because OCIO is (extremely) poorly documented, it's difficult to look up what happens in the abbreviated form. However, I've verified that the following things are true by sifting through OCIO's source code:

The single-value forms of gamma and offset set the r, g, and b values to the provided value, but set alpha to 1.0 and 0.0, respectively. This is what we want, because alpha should be left alone.
The default value for style is linear, which continues the linear segment into the negative values without clamping. I think this is probably what we want, since this allows round-tripping when needed, and code that needs >= 0 values (i.e. image file writers) will presumably clamp it themselves if needed. Moreover, proper handling would require gamut clipping prior to the transform anyway.

Looks good to me! Some notes for other reviewers: The expanded version of `ExponentWithLinearTransform` looks like: ``` !<ExponentWithLinearTransform> { gamma: [r, g, b, a], offset: [r, g, b, a], style: <linear | mirror>, direction: <inverse | forward> } ``` Which allows different values to be used for different channels. The `style` parameter determines how negative values are handled. Unfortunately, because OCIO is (extremely) poorly documented, it's difficult to look up what happens in the abbreviated form. However, I've verified that the following things are true by sifting through OCIO's source code: - The single-value forms of `gamma` and `offset` set the r, g, and b values to the provided value, but set alpha to `1.0` and `0.0`, respectively. This is what we want, because alpha should be left alone. - The default value for `style` is `linear`, which continues the linear segment into the negative values without clamping. I think this is probably what we want, since this allows round-tripping when needed, and code that needs >= 0 values (i.e. image file writers) will presumably clamp it themselves if needed. Moreover, proper handling would require gamut clipping prior to the transform anyway.

Brecht Van Lommel commented

2023-08-02 11:55:13 +02:00

@blender-bot build

Sergey Sharybin commented

2023-08-02 12:16:24 +02:00

@blender-bot build

Sergey Sharybin commented

2023-08-02 12:28:49 +02:00

From testing locally the compositor_matte_test fails the keying matte test. It seems that the node is very sensitive to precision. I.e. it seems to give different result when a hard-wired srgb_to_linearrgb_v3_v3 is sued with and without SSE. So this I think will fall under "known side effects".

The clamping of negative values should not happen in the OCIO configuration. This will match the currently used LUT more closely (which allows negative colors between -0.125 and 0). From quick look the "extension" policy seems to also be linear in the current LUT. Since the mapping already allows negative values, I'd expect code which expects clamping is already doing so.

From testing locally the `compositor_matte_test` fails the keying matte test. It seems that the node is very sensitive to precision. I.e. it seems to give different result when a hard-wired `srgb_to_linearrgb_v3_v3` is sued with and without SSE. So this I think will fall under "known side effects". The clamping of negative values should not happen in the OCIO configuration. This will match the currently used LUT more closely (which allows negative colors between -0.125 and 0). From quick look the "extension" policy seems to also be linear in the current LUT. Since the mapping already allows negative values, I'd expect code which expects clamping is already doing so.

Sergey Sharybin commented

2023-08-02 13:01:13 +02:00

Something fishy is going on. All platforms except of Apple Silicon have some real regressions. There is an example screenshot of the result of the image_alpha_blend.blend.

I can confirm the regression on Linux box, but did not have time yet to dig into details.
And to be clear: I can not reproduce the issue on Apple M2.

Something fishy is going on. All platforms except of Apple Silicon have some real regressions. There is an example screenshot of the result of the [image_alpha_blend.blend](https://svn.blender.org/svnroot/bf-blender/trunk/lib/tests/render/image_colorspace/image_alpha_blend.blend). I can confirm the regression on Linux box, but did not have time yet to dig into details. And to be clear: I can not reproduce the issue on Apple M2.

Screenshot from 2023-08-02 12-57-12.png

90 KiB

Sergey Sharybin requested changes 2023-08-02 13:01:57 +02:00

Sergey Sharybin left a comment

Since there are real unexpected changes marking it as Requested Changes as some work needs to be done to solve those.

Sergey Sharybin commented

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

@Eary I am looking into the failure. There is something odd going on even with the current OCIO configuration. Seems like the OCIO is doing different things depending on which direction the transform is defined.

I'll post later once investigation is finished.

@Eary I am looking into the failure. There is something odd going on even with the current OCIO configuration. Seems like the OCIO is doing different things depending on which direction the transform is defined. I'll post later once investigation is finished.

👀 2

Sergey Sharybin commented

2023-08-02 18:04:33 +02:00

Turns out there are few things here.

Long story short: this PR is actually correct. It just uncovers issue which is already in Blender's codebase.
What's going on is a fiasco of Cycles trying to be too smart, unfortunately.

First aspect is the lovely piece of code around ColorSpaceManager::is_builtin_colorspace() which checks whether a colorspace seems to be sRGB or linear. It does it by comparing 256 points of colors representing in the 0 .. 1 range. For the linear space the code expects a unity conversion, and for sRGB it expects to be close to a function implemented in C color_srgb_to_linear.

One of the issues with this code is that either epsilon needs to be high enough, or it will not detect a sRGB definition as such. For example, removing to_reference: !<FileTransform> {src: srgb.spi1d, interpolation: linear} from the current config.ocio configuration will make the code to NOT detect the sRGB space as such. Similarly, using an analytical piece-wise function in the config.ocio (as this PR proposes) does the same: the color space is no longer considered to be sRGB.

Another issue with this code is that it is not very logically correct. According to the logic in this function a color space like Guard Rail color space will be considered as a built-in, because they do not modify values within the 0 .. 1 range.

The second aspect seems to be a discrepancy of how alpha is handled by the CPU-side slow OCIO code path and the render-time "fast" (also often more memory efficient due to use of uint8 storage) code path. The latter one will only un-associate alpha if the alpha socket is connected to something (and the image is not a data/non-color). The former one will un-associate alpha prior to the color space conversion based on the number of channels in the file.
I might have gotten details here a bit wrong, but the point is: the handling of alpha channel w.r.t color space conversion in "slow" and "fast" code paths is different.

Turns out there are few things here. Long story short: this PR is actually correct. It just uncovers issue which is already in Blender's codebase. What's going on is a fiasco of Cycles trying to be too smart, unfortunately. First aspect is the lovely piece of code around `ColorSpaceManager::is_builtin_colorspace()` which checks whether a colorspace seems to be sRGB or linear. It does it by comparing 256 points of colors representing in the 0 .. 1 range. For the linear space the code expects a unity conversion, and for sRGB it expects to be close to a function implemented in C `color_srgb_to_linear`. One of the issues with this code is that either epsilon needs to be high enough, or it will not detect a sRGB definition as such. For example, removing `to_reference: !<FileTransform> {src: srgb.spi1d, interpolation: linear}` from the current `config.ocio` configuration will make the code to NOT detect the sRGB space as such. Similarly, using an analytical piece-wise function in the `config.ocio` (as this PR proposes) does the same: the color space is no longer considered to be sRGB. Another issue with this code is that it is not very logically correct. According to the logic in this function a color space like Guard Rail color space will be considered as a built-in, because they do not modify values within the 0 .. 1 range. The second aspect seems to be a discrepancy of how alpha is handled by the CPU-side slow OCIO code path and the render-time "fast" (also often more memory efficient due to use of uint8 storage) code path. The latter one will only un-associate alpha if the alpha socket is connected to something (and the image is not a data/non-color). The former one will un-associate alpha prior to the color space conversion based on the number of channels in the file. I might have gotten details here a bit wrong, but the point is: the handling of alpha channel w.r.t color space conversion in "slow" and "fast" code paths is different.

Sergey Sharybin added 1 commit 2023-08-07 14:34:40 +02:00

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

daa17bdcc4 Merge branch 'main' into AgX-Step2

Sergey Sharybin commented

2023-08-07 14:34:58 +02:00

@blender-bot build

Sergey Sharybin commented

2023-08-07 14:41:40 +02:00

We relaxed the checks a little bit (#110889), so both the from_reference: !<FileTransform> {src: srgb_inv.spi1d, interpolation: linear} (without to_reference) and the from_reference: !<ExponentWithLinearTransform> {gamma: 2.4, offset: 0.055, direction: inverse} are detected as "built-in".

Would still be good to make it so the run-time and pre-processing-time alpha handling are consistent, and improve the naming of the color space check probing (to somehow indicate it is only used for the memory-optimized 8bit access or so), but it is a separate story and is not a blocker from this PR perspective.

I've poked the buildbot to verify this PR with my commit included. Hopefully it passes the check, and we can land this PR soon!

We relaxed the checks a little bit (#110889), so both the `from_reference: !<FileTransform> {src: srgb_inv.spi1d, interpolation: linear} ` (without `to_reference`) and the `from_reference: !<ExponentWithLinearTransform> {gamma: 2.4, offset: 0.055, direction: inverse}` are detected as "built-in". Would still be good to make it so the run-time and pre-processing-time alpha handling are consistent, and improve the naming of the color space check probing (to somehow indicate it is only used for the memory-optimized 8bit access or so), but it is a separate story and is not a blocker from this PR perspective. I've poked the buildbot to verify this PR with my commit included. Hopefully it passes the check, and we can land this PR soon!

👍 3

Sergey Sharybin added 1 commit 2023-08-07 15:54:42 +02:00

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

c64864d45a Merge branch 'main' into AgX-Step2

Sergey Sharybin commented

2023-08-07 15:54:51 +02:00

@blender-bot build

Sergey Sharybin added 1 commit 2023-08-07 16:47:48 +02:00

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

3c8ea03d5d Merge branch 'main' into AgX-Step2

Sergey Sharybin commented

2023-08-07 16:47:58 +02:00

@blender-bot build

Sergey Sharybin commented

2023-08-07 16:49:15 +02:00

The struggle continues, but getting closer!

Apparently, it is not that easy to match the matte node across all the platforms we support. It is so sensitive to the exact values.
So increased tolerance there as well (#110897).

The struggle continues, but getting closer! Apparently, it is not that easy to match the matte node across all the platforms we support. It is so sensitive to the exact values. So increased tolerance there as well (#110897).

Sergey Sharybin approved these changes 2023-08-07 17:48:23 +02:00

Sergey Sharybin merged commit f5e567b4a8 into main

2023-08-07 17:52:35 +02:00

Sergey Sharybin referenced this issue from a commit

2023-08-07 17:52:36 +02:00

Color Management: Use OCIO's built-in function for sRGB color space

Sergey Sharybin deleted branch AgX-Step2

2023-08-07 17:52:37 +02:00

Hannes Loeschke referenced this issue from a commit

2023-08-08 17:36:13 +02:00

Color Management: Use OCIO's built-in function for sRGB color space

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-Step2: Use OCIO's built-in function for sRGB #110712