Luminance Matte Node Fix #47212

Closed
opened 2016-01-20 08:04:26 +01:00 by Troy Sobotka · 22 comments

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

luminance-matte-patch.diff

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)
Author

Changed status to: 'Open'

Changed status to: 'Open'
Author

Added subscriber: @troy_s

Added subscriber: @troy_s
Author

Added subscribers: @LukasStockner, @Sergey, @brecht

Added subscribers: @LukasStockner, @Sergey, @brecht
Author

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

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

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.

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
Author

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.
Author

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);

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.
Author

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.

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.
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
Author

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.

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?

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?
Author

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.
Author

@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.

@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.

Added subscriber: @mont29

Added subscriber: @mont29
Author

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.
Author

Bump. Sergey? 2.8 please. kthxbai.

Bump. Sergey? 2.8 please. kthxbai.

Added subscriber: @zuggamasta

Added subscriber: @zuggamasta

This issue was referenced by add580beee

This issue was referenced by add580beeea0e1d5bd8be2336c8a5b25bc8c601a

This issue was referenced by 0e59f2b256

This issue was referenced by 0e59f2b256fbf81145fd26f9f37f46b07c9e54b4

Changed status from 'Open' to: 'Resolved'

Changed status from 'Open' to: 'Resolved'
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#47212
No description provided.