Luminance Matte Node Fix #47212

New Issue

Troy Sobotka · 2016-01-20T08:04:26+01:00

Troy Sobotka commented

2016-01-20 08:04:26 +01:00

Sets Luminance Matte node to use proper calculation for Luminance as opposed to YUV code path.

Sets Luminance Matte node to use proper calculation for Luminance as opposed to YUV code path. [luminance-matte-patch.diff](https://archive.blender.org/developer/F275281/luminance-matte-patch.diff)

Troy Sobotka commented

2016-01-20 08:04:26 +01:00

Changed status to: 'Open'

Troy Sobotka commented

2016-01-20 08:04:26 +01:00

Added subscriber: @troy_s

Troy Sobotka commented

2016-01-20 08:09:35 +01:00

Added subscribers: @LukasStockner, @Sergey, @brecht

Troy Sobotka commented

2016-01-20 08:09:35 +01:00

@Sergey, @brecht, or @LukasStockner care to apply this trivial one?

Sergey Sharybin commented

2016-01-20 10:04:53 +01:00

What exactly you're fixing here and why you consider something wrong here? Do you have examples which fails dramatically here?

From what i see, this is how it was originally supposed to work (see - [x]). I can only suppose that you're doing this to support luma coefficients but then proper solution should be to make YUV conversion aware of those coefficients instead.

https://developer.blender.org/diffusion/B/browse/master/source/blender/nodes/composite/nodes/node_composite_lummaMatte.c;03747781a8b1027eb78914a2b92f289c10558226$87

What exactly you're fixing here and why you consider something wrong here? Do you have examples which fails dramatically here? From what i see, this is how it was originally supposed to work (see - [x]). I can only suppose that you're doing this to support luma coefficients but then proper solution should be to make YUV conversion aware of those coefficients instead. - [x] https://developer.blender.org/diffusion/B/browse/master/source/blender/nodes/composite/nodes/node_composite_lummaMatte.c;03747781a8b1027eb78914a2b92f289c10558226$87

Troy Sobotka commented

2016-01-20 15:22:26 +01:00

Luminance will always depend on the chromaticity coordinates of the particular RGB reference.

In the case of YUV (wrong term, as YUV is an analog concept) it would be specifically referencing a particular encoding of YCbCr with particular weights relative to that encoding. Further, broadcast specifications require a scaled and offset Y to meet specifications.

All of these facets amount to the wrong result for a keying node that requires RGB luminance for the reference space.

Luminance will always depend on the chromaticity coordinates of the particular RGB reference. In the case of YUV (wrong term, as YUV is an analog concept) it would be specifically referencing a particular encoding of YCbCr with particular weights relative to that encoding. Further, broadcast specifications require a scaled and offset Y to meet specifications. All of these facets amount to the wrong result for a keying node that requires RGB luminance for the reference space.

Troy Sobotka commented

2016-01-20 18:02:58 +01:00

On subject, it appears the line in rna_nodetree.c that restricts the maximum value to 1.0 should be changed to FLT_MAX to accommodate Cycles scene referred ranges as well.

RNA_def_property_range(prop, 0.0f, 1.0f);

should be

RNA_def_property_range(prop, 0.0f, FLT_MAX);

On subject, it appears the line in rna_nodetree.c that restricts the maximum value to 1.0 should be changed to FLT_MAX to accommodate Cycles scene referred ranges as well. RNA_def_property_range(prop, 0.0f, 1.0f); should be RNA_def_property_range(prop, 0.0f, FLT_MAX);

Sergey Sharybin commented

2016-01-23 12:26:20 +01:00

Luminance indeed depends on chromaticities, that was not the point of question. To make it short: why to switch to explicit luminance calculation instead of making YUV aware of custom chromaticities? There are quite some places which depends on YUV conversion which will be wrong when using non-standard luma coefficients i guess.

And please don't mix separate things in a single path.

Luminance indeed depends on chromaticities, that was not the point of question. To make it short: why to switch to explicit luminance calculation instead of making YUV aware of custom chromaticities? There are quite some places which depends on YUV conversion which will be wrong when using non-standard luma coefficients i guess. And please don't mix separate things in a single path.

Troy Sobotka commented

2016-01-23 16:16:44 +01:00

Apologies for mixing. I will re-report two more.

YCbCr (and YUV) rely on specific weights that should never change. As they currently exist, they are somewhat confused as there are many different weight settings for YCbCr, and several for YUV.

TL;DR the weights for encoding / decoding to YCbCr / YUV do not depend on the reference space, but rather the encoding specification aimed for. Luminance, on the other hand, would explicitly be related to the reference primaries in use.

Apologies for mixing. I will re-report two more. YCbCr (and YUV) rely on specific weights that should never change. As they currently exist, they are somewhat confused as there are _many_ different weight settings for YCbCr, and several for YUV. TL;DR the weights for encoding / decoding to YCbCr / YUV do not depend on the reference space, but rather the encoding specification aimed for. Luminance, on the other hand, would explicitly be related to the reference primaries in use.

Sergey Sharybin commented

2016-01-23 19:46:44 +01:00

Ok, some more things then:

Luma coefficients are actually deprecated in OCIO - , so shall we keep using it?
The change will affect on existing setups, which is an unwanted thing to happen. It's nothing really broken here and we shall not break compatibility, IMO. Proposal could be to have a option how to calculate luminance (either via YUV or using Luma coefficients from the config, default to the new Luma, bt YUV for the existing files). Taking first point into account i'm not sure what would be the most proper way to go actually atm.

http://opencolorio.org/userguide/config_syntax.html#luma

Ok, some more things then: 1. Luma coefficients are actually deprecated in OCIO - [x], so shall we keep using it? 2. The change will affect on existing setups, which is an unwanted thing to happen. It's nothing really broken here and we shall not break compatibility, IMO. Proposal could be to have a option how to calculate luminance (either via YUV or using Luma coefficients from the config, default to the new Luma, bt YUV for the existing files). Taking first point into account i'm not sure what would be the most proper way to go actually atm. - [x] http://opencolorio.org/userguide/config_syntax.html#luma

Troy Sobotka commented

2016-01-23 20:00:12 +01:00

This is a tremendous question actually.

First, even though the ACES folks note it as depreciated, their information is based off of a message to the list from Mr. Selan. Nowhere is it actually depreciated, in code nor via your linked comment, last I looked. The comment itself can be read two ways, but I am reasonably sure that it was placed there to counter the claims that it was depreciated.

That said, I've also spoken with the lead developer of OCIO about these sorts of things as chromaticities are important for doing colour space conversions. His post house has several new features that they are putting in front of the OCIO developers soon for inclusion.

So short term we have two options, given that we have a real need to support variable reference spaces:

Leave it for now and let Blender respect the variable. Check alternate configs (I've been working on an ACEScg wide gamut for example) to use appropriate values.
Use @LukasStockner's evolutionary work on Cycles approach, which was discussed a bit with @brecht as well. We created a transform in OCIO that does a d65-sRGB from_reference transformation to XYZ, and crafted an API-friendly handle called chromaticity. The application then knows to look for the chromaticity role, and use that to glean the chromaticity of the reference space. To get the white point, 1,1,1 will return the achromatic values, and the Y position is the weights. It will frequently differ slightly from a specification's value, but it is technically the mathematical result, and is extremely accurate for our purposes.

I'd defer to the the second option currently, as Lukas' work is pretty important and is a very easy change for us in the code.

I understand on the front of changing existing setups. I'm relatively sure that many folks wouldn't notice a shift in the weights, but if this is an issue, I'd propose that we do exactly as you thought and put a toggle on, but also perhaps migrate a new node into a properly audited set so that artists are well aware that they are using "more correct" versions. I originally added the toggle actually in the node, but took it out for fear it wouldn't pass the review.

While this might seem overly-drastic, we also have to consider that the existing mix, blur, convert YCbCr, etc., nodes have many issues that do not make sense in B'ender's colour managed, linearized reference space, as well as being absolutely wrong with regard to scene referred values delivered from Cycles or any other raytracing image engine. In Mix blend modes, screen is an obvious example, with dodge and burn being wrong as well- [x]. The blur node still has a moot gamma selection. YCbCr, YUV, and a few other places make hard assumptions about reference space, and are frequently inaccurate as well.

Anyways... just some thoughts on how to move forward without being too crippled with legacy node setups. What do you think of this approach for a 2.8 migration?\

Nuke has a toggle (godawfully named "video space") that lets imagers select which variant of the formula they are using.

This is a tremendous question actually. First, even though the ACES folks note it as depreciated, their information is based off of a message to the list from Mr. Selan. Nowhere is it actually depreciated, in code nor via your linked comment, last I looked. The comment itself can be read two ways, but I am reasonably sure that it was placed there to *counter* the claims that it was depreciated. That said, I've also spoken with the lead developer of OCIO about these sorts of things as chromaticities are important for doing colour space conversions. His post house has several new features that they are putting in front of the OCIO developers soon for inclusion. So short term we have two options, given that we have a real need to support variable reference spaces: - Leave it for now and let Blender respect the variable. Check alternate configs (I've been working on an ACEScg wide gamut for example) to use appropriate values. - Use @LukasStockner's evolutionary work on Cycles approach, which was discussed a bit with @brecht as well. We created a transform in OCIO that does a d65-sRGB from_reference transformation to XYZ, and crafted an API-friendly handle called *chromaticity*. The application then knows to look for the chromaticity role, and use that to glean the chromaticity of the reference space. To get the white point, 1,1,1 will return the achromatic values, and the Y position is the weights. It will frequently differ slightly from a specification's value, but it is technically the mathematical result, and is extremely accurate for our purposes. I'd defer to the the second option currently, as Lukas' work is pretty important and is a very easy change for us in the code. I understand on the front of changing existing setups. I'm relatively sure that many folks wouldn't notice a shift in the weights, but if this is an issue, I'd propose that we do exactly as you thought and put a toggle on, but also perhaps migrate a new node into a properly audited set so that artists are well aware that they are using "more correct" versions. I originally added the toggle actually in the node, but took it out for fear it wouldn't pass the review. While this might seem overly-drastic, we also have to consider that the existing mix, blur, convert YCbCr, etc., nodes have many issues that do not make sense in B'ender's colour managed, linearized reference space, as well as being absolutely wrong with regard to scene referred values delivered from Cycles or any other raytracing image engine. In Mix blend modes, screen is an obvious example, with dodge and burn being wrong as well- [x]. The blur node still has a moot gamma selection. YCbCr, YUV, and a few other places make hard assumptions about reference space, and are frequently inaccurate as well. Anyways... just some thoughts on how to move forward without being too crippled with legacy node setups. What do you think of this approach for a 2.8 migration?\ - [x] Nuke has a toggle (godawfully named "video space") that lets imagers select which variant of the formula they are using.

Sergey Sharybin commented

2016-01-23 21:51:53 +01:00

For the time being it's fine to add a "reference" to the keying node . Bigger reconsiderations are possible, but not as a longer term projects. But even then, i still don't' see comparison demonstrating it on an artist level what was actually improved. If it's so much more correct and gives better keys and keeps everyone happy with even existing setups why not just go for it and leave all the options out?

Troy Sobotka commented

2016-01-23 21:59:07 +01:00

It is a step to getting Blender's internal logic up to date.

When the reference space changes, so too must these values. That is the rather extensive patch Lukas is working on for Cycles.

But also, and something that hasn't happened yet, the nodes need to become more scene referred aware as a whole to fully support Cycles renders, which they do not currently.

With specific attention to this node, the keys pulled would be "better" and more accurately reflect the luminance of 709 primaries that the imager's image encoding is. As it stands now, the weights are wrong, so the luminance won't be correct.

This means that semi-transparent regions will have incorrect weighting resulting in subtle fringing where none would occur with the proper weights.

TL;DR Why?

This currently is not a luminance node, despite the title.
This has no known set of primaries where this would ever be a luminance node.

The result with the changed values would be a much more accurate key pull, resulting in faster and more correct work for imagers.

It is a step to getting Blender's internal logic up to date. When the reference space changes, so too must these values. That is the rather extensive patch Lukas is working on for Cycles. But also, and something that hasn't happened yet, the nodes need to become more scene referred aware as a whole to fully support Cycles renders, which they do not currently. With specific attention to this node, the keys pulled would be "better" and more accurately reflect the luminance of 709 primaries that the imager's image encoding is. As it stands now, the weights are wrong, so the luminance won't be correct. This means that semi-transparent regions will have incorrect weighting resulting in subtle fringing where none would occur with the proper weights. TL;DR Why? - This currently is not a luminance node, despite the title. - This has no known set of primaries where this would ever be a luminance node. # The result with the changed values would be a much more accurate key pull, resulting in faster and more correct work for imagers.

Troy Sobotka commented

2016-02-01 16:29:06 +01:00

@Sergey, would you be against a global flag in User Preferences that flips on a "Legacy Mode"?

It would allow us to move forward and properly fix these sorts of issues without adding the complexity of a toggle that shouldn't be there in the first place.

@Sergey, would you be against a global flag in User Preferences that flips on a "Legacy Mode"? It would allow us to move forward and properly fix these sorts of issues without adding the complexity of a toggle that shouldn't be there in the first place.

Sergey Sharybin commented

2016-02-06 01:07:09 +01:00

@troy_s, no, that's a bad idea. If we want option for that, it should be aware of the fact that one might want to work on old projects with old settings and new projects with new settings.

Now, if the difference in the node behavior is subtle enough, we can just change it. But please, bother to get good demonstration file (like the techhead greesnscreen footage from Tears of Steel) and run the keying wit hand without the patch to show the difference in the matte output.

@troy_s, no, that's a bad idea. *If* we want option for that, it should be aware of the fact that one might want to work on old projects with old settings and new projects with new settings. Now, if the difference in the node behavior is subtle enough, we can just change it. But please, bother to get good demonstration file (like the techhead greesnscreen footage from Tears of Steel) and run the keying wit hand without the patch to show the difference in the matte output.

Bastien Montagne commented

2016-02-19 12:47:37 +01:00

Added subscriber: @mont29

Troy Sobotka commented

2016-03-27 06:49:30 +02:00

Sergey, can we just fix this? I am really hesitant to even attempt to fix anything else color related, of which there are hundreds of things, if we can't simply agree that:

It is broken and totally incorrect.
Forward looking, it is totally broken for all other reference spaces.

It is a very easy fix.

Sergey, can we just fix this? I am really hesitant to even attempt to fix anything else color related, of which there are hundreds of things, if we can't simply agree that: - It is broken and totally incorrect. - Forward looking, it is totally broken for all other reference spaces. # It is a very easy fix.

Troy Sobotka commented

2017-09-10 19:42:29 +02:00

Bump. Sergey? 2.8 please. kthxbai.