Cycles: Use proper XYZ <-> Scene Linear conversion instead of assuming sRGB
Needs RevisionPublic

Authored by Lukas Stockner (lukasstockner97) on May 8 2016, 1:18 AM.

Details

Reviewers
Brecht Van Lommel (brecht)
Group Reviewers
Cycles
Summary

This is the first part of the Cycles color management implementation. It still has some ToDos
and areas to be cleaned up, but it should be ready for general review now.

More details on this patch can be found in T46860, this diff contains a cleaned-up version of
the first three patches.

Diff Detail

Branch
arcpatch-D1973
Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Use proper XYZ <-> Scene Linear conversion instead of assuming sRGB.May 8 2016, 1:18 AM
Brecht Van Lommel (brecht) requested changes to this revision.May 8 2016, 3:04 AM
Brecht Van Lommel (brecht) added inline comments.
intern/cycles/blender/blender_curves.cpp
767–768

I like the term 'color-critical' :). But yes, Blender vertex colors conventions is sRGB still.

intern/cycles/blender/blender_sync.cpp
362

This function is not defined anywhere either? Is that part of another patch or was it supposed to be included in this one?

intern/cycles/kernel/kernel_color.h
22

NUM_COLORSPACES is not defined anyway, guess you forgot to include a file?

intern/cycles/kernel/kernel_film.h
23

This is the code path in case there is no GLSL / half float support for old GPUs, which will be removed in Blender 2.8.

Don't worry about making this work with arbitrary display transforms, hardcoding sRGB here is fine.

intern/cycles/kernel/svm/svm_math_util.h
106–107

Is this a TODO or did you actually manage to simplify the code this much?

This revision now requires changes to proceed.May 8 2016, 3:04 AM

While we are at this patch, the default config should be chucked out. The ACES bits are entirely wrong, the legacy copied transforms from the SPI pack is completely wrong in this context, and a hundred other errors.

Lukas has a cleaned OCIO configuration in his GitHub branch, and it would at least be a beginning of fixing this abomination.

intern/cycles/blender/blender_curves.cpp
767–768

That looks like transfer curve only, which will be problematic displayed on anything with alternate primaries, including every new generation Apple display.

intern/cycles/kernel/kernel_film.h
23

Will it?

How about P3 or any other display such as a broadcast display?

Let's not tie a revised OCIO config into this code review, one thing at a time.

intern/cycles/blender/blender_curves.cpp
767–768

Mesh vertex colors by convention use the same primaries as e.g. material colors, but with an sRGB transfer curve to compress them to 8 bit more efficiently.

Display spaces are irrelevant in this part of the code.

intern/cycles/kernel/kernel_film.h
23

There might not even exist any GPU where this code runs now. It's an old fallback that is only used when the OCIO shader is not supported.

It seems I screwed up the splitting into different patches a bit, some stuff that was in here
actually belongs to the second patch (texture handling), while another change wasn't included.

Regarding the new Blackbody code: The new approximation comes from Wikipedia (article "Planckian locus"),
which in turn cites "Design of Advanced Color Temperature Control System for HDTV Applications" (can be
found easily on Google). I haven't verified it myself yet, but it seems to be quite accurate.

Brecht Van Lommel (brecht) requested changes to this revision.May 10 2016, 12:36 AM

The color output from the blackbody node looks quite different with this patch, even at the default temperature of 1500 K.

Maybe the RGB values in the tables in the original code can be normalized, converted from Rec.709 to XYZ, and unnormalized again? With some luck the fit is still close in XYZ space. Another option would be to add a Rec.709 to working space matrix, but would prefer not to.

Other than that it looks good to me.

intern/cycles/blender/blender_curves.cpp
767–769

So we should clarify this commit a bit to say something like:

Vertex colors are in scene linear color space, compressed to 8 bit using sRGB transfer curves. They are assumed to have the same primaries as e.g. material colors, so no extra conversion is necessary.

This revision now requires changes to proceed.May 10 2016, 12:36 AM

Okay, I just decided to kick the curve fitting approximation approach completely and use lookup tables.
This affects both the Wavelength node (which had a hardcoded table directly in the function before), which now uses the regular CIE XYZ curves, as well as the Blackbody node.
I generated the blackbody tables directly from Planck's law and the XYZ curves in Octave, so it should be as accurate as it gets. The difference between the old and new code
has a deltaE value of around 0.01 (assuming a D65 white point), which is way too low to make a real difference. For the extreme regions (below ~1500K and above ~10000K) the
difference is larger, but that's because the old code simply didn't cover that range afaics.
For indexing I just went with reciprocal Kelvins since they're way more perceptually uniform than the temperature itself. The spacing is still not perfect, the values in the
reddish ares are ~2 times denser than in the bluish areas. In the future we might use a better distribution, but my experiments didn't turn out so great yet (just squaring
the index provided more resolution in the bluish colors, but the middle area, which is the most interesting one, was even sparser - and directly evaluating the difference,
numerically inverting it and fitting a function worked extremely well with a sum of two exponentials, but the fitted function couldn't be inverted analytically...).
However, to be honest, I don't think anybody will ever notice the deviations, we're talking about a difference of ~1e-4 here.

To clean up the lookup table handling, I generalized the Beckmann lookup code a bit to make adding new tables easier. Also, the nodes now request the tables they need,
which avoids loading them in case they're not needed. For committing, I'd split that from the main patch.

Weused ot have lookup table to blackbody which then we replaced with an approximation by @lockalas in D1280, which gives quite reasonable speedup (up to 30% in a fire scene). Before going back to lookup table speed is to be investigated again and if we'll have penalty there then it's something to concern about (having slowdown due to support of a case which normal blender being almost never uses is mwua).

Do you have plot of your lookup table? Did you try to curve-fit it?

This revision now requires changes to proceed.Jul 6 2017, 2:38 AM

Any chance we can move a bit on this and T46860 before SIGGRAPH 2018?