Details

Reviewers

Bastien Montagne (mont29)
Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)
Troy Sobotka (sobotka)

Maniphest Tasks

T43600: Repair legacy luminance / greyscale conversions

Summary

This is the first pass to repair the legacy luminance conversions happening via the various hard coded math routines.

Problems with current mechanisms in math_color_inline.c:

Many of the functions break colormanagement. That is, they are ignorant of the reference space primaries and the results are therefore broken.
In particular, the luminance weights are all over the map. This stems from what appears to be a mixture of malformed color knowledge and, in some cases, an attempt to hack linearization and luminance into singular values. Both approaches are incorrect.

This was loosely started when @Kévin Dietrich (kevindietrich) began work on D517. This work attempts to properly glean correct luminance values from the OCIO configuration.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Troy Sobotka (sobotka) added a reviewer: Antonis Ryakiotakis (psy-fi).Feb 8 2015, 10:51 PM

Troy Sobotka (sobotka) added a project: BF Blender.

Troy Sobotka (sobotka) added a task: T43600: Repair legacy luminance / greyscale conversions.

Troy Sobotka (sobotka) updated this object.Feb 8 2015, 11:38 PM

Troy Sobotka (sobotka) added reviewers: Sergey Sharybin (sergey), Bastien Montagne (mont29).

Troy Sobotka (sobotka) updated this object.

Troy Sobotka (sobotka) added a subscriber: Kévin Dietrich (kevindietrich).

We should also make sure to not forget about glsl, for now luminance there is based on rgb_to_bw(), see https://developer.blender.org/diffusion/B/browse/master/source/blender/gpu/shaders/gpu_shader_material.glsl;6b107daf23c5b8b6b7b9aebfa7384e103928a8d3$725

Great spot @Kévin Dietrich (kevindietrich), which is exactly why I added you as a subscriber. I also saw one in Freestyle I believe, which is a Python file IIRC.

Working on the byte conversions next.

Troy Sobotka (sobotka) set the repository for this revision to rB Blender.Feb 9 2015, 1:11 AM

Added and tested byte conversions as used by vertex_paint.c. Removed corresponding duplicated function.

Tests show greater accuracy than previous lighten / darken.

Hi, looks semi OK to me, I'm not 100% certain if all the operations here are in display or linear space. I think compositor is linear, but image (divers.c) and imagefile (png.c) might not be.

Sergey Sharybin (sergey) requested changes to this revision.Feb 9 2015, 4:52 PM

Sergey Sharybin (sergey) edited edge metadata.

Sergey Sharybin (sergey) added inline comments.

intern/opencolorio/fallback_impl.cc
167	No need to give constants in comment, you can easily see them in the next line anyway.
170	`*/` should be in the new line.
172	Do you really want to mention the standard 3 times for each of the components and mention which component you're witting into array?
intern/opencolorio/ocio_impl.cc
281	When exactly exception happens and whats gonna to happen in the CM pipeline when it happens?
source/blender/blenlib/intern/math_color_inline.c
30	Don't include non functional changes.
source/blender/compositor/operations/COM_CalculateStandardDeviationOperation.h
28 ↗	(On Diff #3412)	Same as above.
source/blender/editors/sculpt_paint/paint_intern.h
125 ↗	(On Diff #3412)	I don't really see other areas using this functions. Why do you need them public?
source/blender/editors/sculpt_paint/paint_vertex.c
751	That is usually not a great idea to un-inline function which was explicitly marked for force-inline. What's the reason for that?
source/blender/imbuf/intern/colormanagement.c
93	First of all guess it should be default coefficients here. So things keeps working if there's no coefficients in the config.ocio. Also, make it float values and avoid double promotion.
551	Sentence starts with capital letter and ends with fullstop.
1232	Why do you need to dump it here?
1291	Indentation seems broken.
1295	Fullstop. Plus `*/` in this case belongs to the same line.
1300	Indentation. Plus casts seems weird?

This revision now requires changes to proceed.Feb 9 2015, 4:52 PM

@Antonis Ryakiotakis (psy-fi)

I'm not 100% certain if all the operations here are in display or linear space. I think compositor is linear, but image (divers.c) and imagefile (png.c) might not be.

It doesn't matter. Luminance weights are uniform. That is, if we think about a luminance averaged value being applied to sRGB curved values, the weights are always identical to the values we would apply to display linear or scene linear sRGB values. RGB in sRGB at values 0.7, 0.5, 0.4 are multiplied by exactly the same weights a scene linear set of values of 12.4, 11.3, and 20.1 for example.

Luminance is always the Y position of the primaries for each channel.

Thanks @Sergey Sharybin (sergey) for your eyes. Will make full changes. See notes for further questions that need resolving.

Thanks again folks.

intern/opencolorio/ocio_impl.cc
281	What do you suggest?
source/blender/blenlib/intern/math_color_inline.c
30	This was because got detected a code change. How can one suppress these sorts of changes when doing a commit?
source/blender/editors/sculpt_paint/paint_intern.h
125 ↗	(On Diff #3412)	This is because the original function was BLI_INLINED. The options were thus: Make the two BLI_INLINE functions implicated public. BLI_INLINE the luminance function. I chose the former to limit impact. Should I BLI_INLINE the luminance function instead?
source/blender/editors/sculpt_paint/paint_vertex.c
751	See above. Open to options.

Updated patch.

Fixes @Sergey Sharybin (sergey)'s issues.
Left mcol* as-is on advice of @Antonis Ryakiotakis (psy-fi) regarding performance hit. Only two functions in patch that were inlined.
Removed final broken bits of luminance work from the math_color_inline.c

Remaining outstandings:

What to do in the catch() block? Under what circumstances would that catch block be reached and what is reasonable?

Kévin Dietrich (kevindietrich) mentioned this in D517: Code cleanup: de-duplicate RGB to Grayscale conversion.Feb 9 2015, 8:28 PM

Bar some nitpicks that looks OK to me now, probably some old files may need some kind of version checking though? I don't mind breaking compatibility here because this will be a den of snakes to keep all things compatible. Thoughts?

source/blender/editors/sculpt_paint/paint_intern.h
125 ↗	(On Diff #3432)	Not needed at all now I guess?
source/blender/editors/sculpt_paint/paint_vertex.c
751	same
785	same

Troy Sobotka (sobotka) updated this revision to Diff 3457.Feb 10 2015, 7:30 PM

Troy Sobotka (sobotka) edited edge metadata.

This comment was removed by Troy Sobotka (sobotka).

Hopefully fixes the oopsied last diff.

Addresses Psy-Fi's comments in D1082#18878.

Answering older thing about mcol_lighten. Even without new function being inlined mcol_lighten should just work. Plus i would also keep new function inlined as well so there's no performance impact.

Now, even tho code seems almost fine now, did anybody looked into how much impact it'll have on existing files?

intern/opencolorio/fallback_impl.cc
173	Floats, not doubles.
source/blender/imbuf/intern/colormanagement.c
93	Just standard name is enough, we can totally see values in the actual code.
1234	I'm still not sure why it's helpful to see this knowledge dump in here.
1292	Indentation is weird. Plus it seems you can use dot-product from BLI_math
1300	You can't just cast because of machine epsilon. Use FTOCHAR instead.

This revision now requires changes to proceed.Feb 11 2015, 3:50 PM

Regarding legacy files, I believe the worst case weight mangling was approximately 15% for green. I am unsure how to proceed in the color cleanups regarding legacy files, and advice would be appreciated. Versioning strikes me as a potentially problematic implementation?

intern/opencolorio/fallback_impl.cc
173	Good spot. Will fix.
source/blender/imbuf/intern/colormanagement.c
551	Tried to match existing comment style by file's author. Fixed.
1234	There were several comments in the previous implementations that showed a good deal of confusion regarding luminance values. I thought it would be future friendly to help get a developer up to speed on the correct values, and how to obtain them without merely referencing Poynton, and perhaps prevent the multiple / incorrect implementations from appearing in the future. Happy to remove it if desired.
1292	Noted. Will adjust. Indentation is on fours, which looks to misalign the second and third lines. I could cheat the second and third back a space, but that too seems odd?
1300	Good spot on my mistake.
1300	Noted. Will adjust.

Also, sorry for spam, but BLI_INLINE doesn't work according to Bishop and testing, without the luminance function being BLI_INLINEd as well.

Sergey Sharybin (sergey) added inline comments.Feb 11 2015, 4:05 PM

source/blender/imbuf/intern/colormanagement.c
1234	IMO this belongs to wiki, not the code. You can link the wiki page from the comment tho.
1292	I'd make next lines indented with single tab and space to keep words aligned. But really consider using a dot-product.

Addresses Sergey's remaining concerns.

Troy Sobotka (sobotka) added inline comments.Feb 13 2015, 6:58 PM

source/blender/imbuf/intern/colormanagement.c
93	Changed.
1234	Changed.
1292	Changed.
1300	Changed according to existing standards in colortools.c.

Campbell Barton (campbellbarton) requested changes to this revision.Feb 17 2015, 11:18 PM

Campbell Barton (campbellbarton) added a reviewer: Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) added inline comments.

source/blender/blenlib/BLI_math_color.h
83–89	Disagree with completely removing rgb-greyscale functions, There ware times when its reasonable to do quick color operations outside the color pipeline - When handing theme colors, onscreen/opengl colors, which checks if one on-screen color is darker than another.

This revision now requires changes to proceed.Feb 17 2015, 11:18 PM

Troy Sobotka (sobotka) added inline comments.Feb 17 2015, 11:29 PM

source/blender/blenlib/BLI_math_color.h
83–89	The problem with this is two fold, and I can prove this with reasonable certainty: The most important issue is that if these functions remain available, developers use them. This creeps in breakages where a developer may think it is fine to do so where it is not. See how Freestyle and GLSL has borrowed the incorrect weights and propagated more problems to the color pipeline. This also assumes erroneously that all displays are sRGB and it entirely breaks correction. Even sRGB theoretical to sRGB displays need correction. This problem is exacerbated when the display isn't sRGB, but perhaps a wide gamut display as is prevalent in smaller studios. Ultimately even the theme colors should probably be color managed. I could cite the dozens of threads on incorrect color handling in UIs that make for some rather nasty themes on wide gamut displays, or even bad color handling on browsers such as Chrome. On a more practical level, it simply leaves vastly too many holes open for breakages. It has been shown historically that if someone is provided an API function that it will get used, and it is impossible to predict how and why a developer may be using it.

to be clear, in the interface_*.c files, there is no need to use color-management.

re:

Ultimately even the theme colors should probably be color managed.

This seems to be unnecessarily over-complicating things.
Color management is great for the color pipeline, for screen color, we can use simple approximations.

I'm also against using color management in the GUI. I don't really see a benefit from it, only more work and more complicated code. Looking at the way your patch implements it in the interface_*.c files, seems like it's recalculated on each redraw which is also a big point against it (just had a quick look, didn't investigate deeper).

Seriously folks, it uses a global value. There is el-zippo performance issue here.

Further, with much respect, RGB is a relative color model.

You can't send a value to my display and tell me how it will look. You really can't.

But anyways, I restate:

There is no performance hit. There is no overcomplication.

It is one function now as opposed to five. With a default config, it does exactly what the previous functions did, only accurately.

I don't mean to say that we'll have any performance issues here, but we do have some calculations running on each redraw that are not necessary IMHO. And in general we should try to avoid any unnecessary calculations for frequent redraws. Looking at the simple calculations that are done here, I agree that this is only a really minor point, but still it is one ;)

Besides from that, I did some research and didn't find any software that promotes to have color corrected themes/a color corrected UI. So I still don't see a need to have that and until you can't provide any good points I'll stay with my opinion.

Don't want to sound rude or so, and I'm also not totally against the idea, it's just the question if it's worth it. If we decided to color correct the GUI, that would mean we always had to ensure this is considered.

Where are we at on this?

Endless mailing list thread?

I would love to carry on fixing the color management related issues, but can't exactly move forward until this most basic bit of progress lands.

Bcon1, I will commit this and follow the screams if someone complains.

intern/opencolorio/ocio_impl.h
150	No need to revirtualize in subclasses

Antonis Ryakiotakis (psy-fi) mentioned this in rB42aac45d7f0e: Part 1 of D1082 by Troy Sobotka, add API in OCIO for luminance calculation..Mar 17 2015, 2:15 PM

Antonis Ryakiotakis (psy-fi) mentioned this in rBdd38dce7f0ae: Part 2 of D1082 by Troy Sobotka, remove our functions that do luma calculations….Mar 17 2015, 3:21 PM

So, I committed this in two parts, first is OCIO API, then there's a separate commit that plugs this in to blender. Let's hope we get no "my render looks slightly wrong" complaints now.

Antonis Ryakiotakis (psy-fi) commandeered this revision.Mar 17 2015, 3:22 PM

Antonis Ryakiotakis (psy-fi) abandoned this revision.

Antonis Ryakiotakis (psy-fi) edited reviewers, added: Troy Sobotka (sobotka); removed: Antonis Ryakiotakis (psy-fi).

Antonis Ryakiotakis (psy-fi) mentioned this in rBS42aac45d7f0e: Part 1 of D1082 by Troy Sobotka, add API in OCIO for luminance calculation..Mar 25 2015, 12:26 PM

Antonis Ryakiotakis (psy-fi) mentioned this in rBSdd38dce7f0ae: Part 2 of D1082 by Troy Sobotka, remove our functions that do luma calculations….

Aaron Carlisle (Blendify) mentioned this in D2982: Fix T47212: Use proper coefficients for Luminance Matte Node.Jan 1 2018, 12:09 AM

Color Managed Luminance Repair Work
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 3513

intern/opencolorio/fallback_impl.cc

intern/opencolorio/ocio_capi.h

intern/opencolorio/ocio_capi.cc

intern/opencolorio/ocio_impl.h

intern/opencolorio/ocio_impl.cc

release/datafiles/colormanagement/config.ocio

source/blender/blenkernel/intern/colortools.c

source/blender/blenlib/BLI_math_color.h

source/blender/blenlib/intern/math_color_inline.c

source/blender/compositor/operations/COM_CalculateMeanOperation.cpp

source/blender/compositor/operations/COM_CalculateStandardDeviationOperation.cpp

source/blender/compositor/operations/COM_ColorCorrectionOperation.cpp

source/blender/compositor/operations/COM_ConvertOperation.cpp

source/blender/compositor/operations/COM_GlareThresholdOperation.cpp

source/blender/compositor/operations/COM_TonemapOperation.cpp

source/blender/editors/interface/interface_draw.c

source/blender/editors/interface/interface_regions.c

source/blender/editors/interface/interface_widgets.c

source/blender/editors/sculpt_paint/paint_image_2d.c

source/blender/editors/sculpt_paint/paint_image_proj.c

source/blender/editors/sculpt_paint/paint_vertex.c

source/blender/editors/space_sequencer/sequencer_scopes.c

source/blender/imbuf/IMB_colormanagement.h

source/blender/imbuf/intern/colormanagement.c

source/blender/imbuf/intern/divers.c

source/blender/imbuf/intern/png.c

source/blender/nodes/shader/nodes/node_shader_valToRgb.c

source/blender/nodes/texture/nodes/node_texture_valToRgb.c

source/blender/render/intern/source/render_texture.c

source/blender/render/intern/source/shadeoutput.c

source/blender/render/intern/source/volumetric.c

Color Managed Luminance Repair WorkAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3513

intern/opencolorio/fallback_impl.cc

intern/opencolorio/ocio_capi.h

intern/opencolorio/ocio_capi.cc

intern/opencolorio/ocio_impl.h

intern/opencolorio/ocio_impl.cc

release/datafiles/colormanagement/config.ocio

source/blender/blenkernel/intern/colortools.c

source/blender/blenlib/BLI_math_color.h

source/blender/blenlib/intern/math_color_inline.c

source/blender/compositor/operations/COM_CalculateMeanOperation.cpp

source/blender/compositor/operations/COM_CalculateStandardDeviationOperation.cpp

source/blender/compositor/operations/COM_ColorCorrectionOperation.cpp

source/blender/compositor/operations/COM_ConvertOperation.cpp

source/blender/compositor/operations/COM_GlareThresholdOperation.cpp

source/blender/compositor/operations/COM_TonemapOperation.cpp

source/blender/editors/interface/interface_draw.c

source/blender/editors/interface/interface_regions.c

source/blender/editors/interface/interface_widgets.c

source/blender/editors/sculpt_paint/paint_image_2d.c

source/blender/editors/sculpt_paint/paint_image_proj.c

source/blender/editors/sculpt_paint/paint_vertex.c

source/blender/editors/space_sequencer/sequencer_scopes.c

source/blender/imbuf/IMB_colormanagement.h

source/blender/imbuf/intern/colormanagement.c

source/blender/imbuf/intern/divers.c

source/blender/imbuf/intern/png.c

source/blender/nodes/shader/nodes/node_shader_valToRgb.c

source/blender/nodes/texture/nodes/node_texture_valToRgb.c

source/blender/render/intern/source/render_texture.c

source/blender/render/intern/source/shadeoutput.c

source/blender/render/intern/source/volumetric.c

Color Managed Luminance Repair Work
AbandonedPublic
Actions

Revision Contents
Changeset List