AgX-Step5: Add AgX and its components #111099
No reviewers
Brecht Van Lommel
Nathan Vegdahl
Sergey Sharybin
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
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
7 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#111099
Loading…
Reference in New Issue
No description provided.
Delete Branch ":AgX-Step5"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This is the step 5 of the AgX implementation as discussed in the original AgX PR
Original plan was to also shrink Filmic LUT without quality loss, but after discussion, it was moved to its separate step 6.
This PR adds the following:
Negative value handling (lower Guard Rails):
All of the above are hidden from the colorspace list UI by listing them under
inactive_colorspacesDisplay Devices
AgX Log
AgX View Transforms for the different display devices
AgX False Color for the different display devices (replacing Filmic's False Color)
All the false color spaces are also hidden from colorspace list UI by listing under
inactive_colorspacesAgX Looks, adding view name prefix to separate from Filmic Looks.
Change default startup to use AgX instead of Filmic by default.
Also attaching to this PR the ICC profiles associated with the display encoding we support, just in case of future needs, e.g. support embedding/assigning ICC profiles to saved image files. The profiles are generated using RawTherapee, under CC0 licence.
2b3207dd456d66172ba7@blender-bot build
@Eary Thanks for the patch. I'll go over it in details in a bit.
Quick question: I see change in the
startup.blend. Is it for making the AgX the default view? If so, I think current preferred way is to go viaversioning_defaults.c. I can help with that.Oh didn't know that, yes it's for making the view the startup default. Thanks.
@blender-bot package
Package build started. Download here when ready.
@ -2,4 +2,1 @@## Based on aces, nuke-default and spi configurations from OpenColorIO-Config## Filmic Dynamic Range LUT configuration crafted by Troy James Sobotka withI don't it is fair to leave information about Filmic out. It is still present in the configuration file for the compatibility reasons.
The aces I am not sure. We do have aces color spaces, but i am not sure if they are coming from the OpenColorIO-Config.
The parts nuke-default and spi I think are indeed no longer present in our configuration.
On a user level I think it is working all fine.
On a technical side think there are, unfortunately, two main concerns.
One of them is the file size of the lookup tables.
While it is on par with the Filmic files, it feels a bit worrysome to ship Blender with 100s of megabytes of lookup tables. I am not sure if there is anything can be done as part of this change to improve it without loosing precision.
The compressed size is not too bad, so personally I am not so much worried about this point.
Another one is the heavily increased CPU side of the color management: saving 4k render as PNG with Filmic takes 84.3 ms, with AgX it takes 490.7 ms. Measured on MacBook M2. This is quite a huge impact.
From the quick thinking maybe some things could be baked more into LUTs, i.e. it seems to be a typical pattern of how the
guard_rail_higher.cubeis applied (inverse Guard_Rail_Higher_Shaper_EOTF, then the cube, then the Guard_Rail_Higher_Shaper_EOTF). Although, would need to be careful with precision perhaps?In any case, such a performance penalty is not something that feels comfortable to accept without trying our best to avoid it.
Quick note: I'd like a chance to test and review this before it lands, and I likely won't have time to do that until this Wednesday. I don't want to block it or anything, but if you guys can be a bit patient I'd really appreciate it!
@nathanvegdahl Performance is kind of blocking anyway :( And it would indeed be cool if you have a chance to have a look into this PR!
@Eary Are you available to help looking into the performance aspects?
It doesn't seem to an easy way to log time of color management step of image saving. So what I can do is to commit a temporary line of code to
IMB_colormanagement_imbuf_for_writeand trigger a build here, so that it'll be easier for you to investigate.Yes I have been trying to optimize the LUTs, and yes some info about time cost can be helpful.
@blender-bot package
Package build started. Download here when ready.
I've committed a timing information. The build should be ready soon.
To see the timing you'd need to start Blender from Terminal, and save some 4k image with "Save as Render", and use format like PNG. You'll see a message like
Timer 'IMB_colormanagement_imbuf_for_write' took 90.2 msin the terminal. The AgX timing will be much higher.Just updated the optimized LUTs. All LUTs are sized down, and some have changed shaper curve to optimize for precision. AgX view transforms now each use their own packed LUT. Although I would like to keep the "single image flow" design from Base Rec.2020 to Guard Railed other display encodings, it just seems 3D LUT is incapable of doing so gracefully. Therefore I baked all into one single LUT for individual view transforms.
Green InkandPunchyLook suffered from precision issue after the LUT change,Green Inkhas been complained in recent user feedback that it's too stylized, and too case-specific, generally useless. So I might as well take the chance to remove it.Punchylook has been moved toAgX Logprocess space, which gives better precision.I was unable to see the timer info from terminal though, not sure why.
Thanks for the update!
The timing i now 96 ms for Filmic and 146 for AgX. Definitely getting close!
This is a bit odd. With the latest build of this patch, or build of the branch there should be logging. I've verified the build on macOS.
Just to be sure, the terminal I mean the OS level terminal (cmd.exe on Windows, iTerm or Term on macOS, etc), not the built-in Python console in Blender. Sometimes we get confused about what terminal we are talking about, so nice to get this clarified.
Also, I've attached a .blend file is used for timings. The steps I do are:
Now, I was looking closer into the config itself, and there is something strange I've noticed. A lot of color spaces define both
from_scene_referenceandto_scene_reference. They should only define one direction, otherwise at a best it is confusing and fragile (i.e. theGuard Rail sRGBwhere conversion steps are so "asymmetric"), but might also prevent OCIO from performing some short-circuiting optimizations at worst.P.S. Also, is only now I've noticed it for the
Filmic Login the main branch. I guess we can simply removeto_scene_referencefromFilmic Login the main branch?This is by design since some LUTs are not reversible.
No since the Filmic desat LUT is not really reversible.
The new packed LUTs I updated are also not reversible, which is why I added a dedicated LUT for the inverse.
And nope I still don't see the info:
@blender-bot package
Package build started. Download here when ready.
@Eary Apparently, the buildbot silently ignored the windows build. Lets try again.
For the display-only color spaces it is fine to have them defined only in one direction.
If a color space is not a display-only, it is fine to have both
to_scene_referenceandfrom_scene_reference, but if they do not pass the round-trip test (my_color == to_scene_reference(from_scene_reference(my_color))) then I do not see how it could work reliably.Inverse of view transform is a hack by nature anyways. And sometimes it also doesn't make sense to require inverse, the False Color for example. Hiding Guard Rails and False Color from the colorspace UI was partially because of that.
Another example could be, say if we have a colorspace that turns things into Greyscale, how should that
Greyscalespace be invertible? The view transforms are kind of similar, since it attenuates the colors to white as the intensity go up, it can be also thought to be a special kind of "greyscaling".Those standard colorimetry interop spaces are defined single-direction so it should be fine. Only those view-transform-related ones are defined both directions to prevent OCIO inverting something that is not invertible. I tried once with Filmic's False Color and it crashed Blender.
If a view transform are attempted to be inverted it sounds like a bug somewhere.
Not bug, like how Filmic sRGB was "added as texture colorspace" some time ago by removing its
family: display, it is a hack by nature but it works in a lot of cases. Though it's important to understand it is a hack.EDIT: Upon double check, it seems AgX Log doesn't need to be defined both direction (was needed some versions back, I guess I forgot to remove the inverse direction when they were changed to be identical)
@blender-bot package
Package build started. Download here when ready.
I think the main purpose of selecting the display space as an input was to be able to use an input display-space image like PNG (which is not HDR) and combine it with HDR content, but keep the final look of that image the same.
This means that for spaces like Guard Rail the more fitting way is to choose corresponding sRGB/Rec.2020 space. So there is no need to have
to_scene_referencefor those spaces. Removing them will solve the un-ease of having such asymmetric space definition.The AgX Base spaces I think are defined fine. At least, from reading the config I don't have any un-ease with them :) It follows the way how OCIO documentation allows you do define transform in both directions by providing a manually inversed 3D LUT. The only thing which is not very clear to me is the "Base". For artists, what does it mean? :)
P.S. The windows build with timing report should finally be available!
@ -272,0 +312,4 @@children:- !<ColorSpaceTransform> {src: Linear FilmLight E-Gamut, dst: Linear Rec.2020, direction: inverse}- !<AllocationTransform> {allocation: lg2, vars: [-12.47393, 12.5260688117]}- !<FileTransform> {src: luminance_compensation_bt2020.cube, interpolation: best}This is something counter-intuitive to me. Shouldn't it be an inverse table?
The LUT is not really invertible, it is like the "smart clip" as how it was called in another discussion. You cannot just "undo clip" something after they are clipped. This "smart clip" is used in AgX Log, since people can color grade in AgX Log, I was trying to prevent things going outside of Rec.2020 during the process. Therefore this is "smart clipping it when encoding to AgX Log", and then "smart clipping it when decoding from AgX Log".
Should I stop doing this? This would involve restoring AgX Log's inverse definition and make sure it skips the inverse of this "smart clipping".
As I was mentioning the use of Filmic (and other Display spaces) is aimed for use on non-HDR inputs, which were clipped already before saving. In a lame terms the intended usecase is:
The suzanne in the composited output should look same/very close to how it looked in its original rendered PNG.
For supporting this usecase I think you can skip un-clipping.
If it is something that becomes tricky, it is also fine to say that AgX Log is just a display space for now, and can't be used as input.
@Eary Do I understand correctly then that this LUT only does clipping, and doesn't alter colors in any other way?
It calculates luminance of the RGB values, offsets target RGB values upwards globally until there is no negative, and then restore the luminance. So it should be a chromaticity-linear "clipping" whithout changing anything else..
Got it. Yeah, it doesn't make any sense to invert, then. So I think it makes sense to just remove the
to_scene_referencetransform entirely. Which it looks like you've done already.@ -272,0 +371,4 @@- !<FileTransform> {src: Guard_Rail_Shaper_EOTF.spi1d, interpolation: linear}- !<ColorSpaceTransform> {src: Linear Rec.2020, dst: Rec.2020}- !<RangeTransform> {max_in_value: 1, max_out_value: 1}to_scene_reference: !<GroupTransform>I would leave this
to_scene_referenceout, and not allow using it as an input color space.Artists can choose the
Rec.2020as input space.P.S. Only later during checking the code and actual build I've noticed it is not possible to select those Guard Rail spaces as an input. So, does not seem we need these
to_scene_referencefor Guard Rail at all.@ -272,0 +391,4 @@- !<FileTransform> {src: Guard_Rail_Shaper_EOTF.spi1d, interpolation: linear}- !<ColorSpaceTransform> {src: Linear Rec.709, dst: sRGB}- !<RangeTransform> {max_in_value: 1, max_out_value: 1}to_scene_reference: !<GroupTransform>Similarly to above: I'd leave it out and let artists to choose sRGB input space.
@ -276,0 +408,4 @@children:- !<ColorSpaceTransform> {src: Linear CIE-XYZ E, dst: Guard Rail sRGB}- !<ColorSpaceTransform> {src: sRGB, dst: Rec.1886}to_scene_reference: !<GroupTransform>Same as above.
@ -276,0 +428,4 @@- !<FileTransform> {src: Guard_Rail_Shaper_EOTF.spi1d, interpolation: linear}- !<ColorSpaceTransform> {src: Linear DCI-P3 D65, dst: Display P3}- !<RangeTransform> {max_in_value: 1, max_out_value: 1}to_scene_reference: !<GroupTransform>And again :)
@ -282,0 +445,4 @@from_scene_reference: !<GroupTransform>children:- !<ColorSpaceTransform> {src: Linear CIE-XYZ E, dst: Luminance Compensation Rec.2020}# rotate = [3.0, -1, -2.0], inset = [0.4, 0.22, 0.13]More like to ensure I understand it correct: this is comment about what the matrix consists of?
Will make more sense in a context of the comment below as well :)
Yes this is what the matrix is for, it's a rotation + inset.
@ -282,0 +463,4 @@- !<ColorSpaceTransform> {src: Linear CIE-XYZ E, dst: Linear FilmLight E-Gamut}- !<AllocationTransform> {allocation: lg2, vars: [-12.47393, 12.5260688117]}- !<FileTransform> {src: AgX_Base_Rec2020.cube, interpolation: best}# outset = [0.4, 0.22, 0.04]I am a bit confused what this refers to. Is it something that left from previous versions of the config? Or is some extra transform is missing here? Or does it refer to some existing transform stop from above?
There was supposed to be another matrix after the LUT, and then apply display device's Guard Rail. But now they are all baked inside of this one single LUT.
The comment can be removed then? :)
As long as the AgX parameters are documented somewhere, I'm fine with these comments being removed. It would probably make sense to post documentation about how the transform LUTs were generated somewhere else, and then link to that at the top of the configuration where it talks about AgX. (Assuming @Eary is okay with sharing.)
@ -282,3 +576,3 @@looks:- !<Look>name: Very High Contrastname: Filmic - Very High ContrastSuch rename breaks compatibility. Can we just leave the current names as-is, and only prefix the new ones with
AgX -?No because then when the view is AgX, the look menu will feature both AgX's and Filmic's Look. You will have two
High Contrasts in the menu, this will be super confusing.I see. Working on a couple of patches which should solve this, together with the issue of "invalid" look being used when changing the view transform.
Would be nice to not open the compatibility issues just yet, and have more time on focusing on what's left for the AgX transform for 4.0.
I'll let you know when those patches land.
Here's my high-level review. A review of the configuration nitty-gritty will come after.
(Edit: attached the EXRs for the two test images I used in this post.)
Guard Rail
I'll get my main concern out of the way first, which is Guard Rail.
Guard Rail seems overly complex to me. Here are some comparisons of different approaches we could take:
(*These are not exact, because I quickly tossed them together for illustrative purposes. In particular, the chroma-preserving clip should never blow out. Actual implementations in the OCIO config would be more careful.)
In other threads you mentioned that Guard Rail is basically supposed to replace
Standard. But I would expect something with that goal to exhibit either the luminance-preserving behavior or the chroma-preserving behavior (with a personal preference for the luminance-preserving). These are the kinds of simple mappings I would expect from an alternative toStandard.Guard Rail, on the other hand, has a significant shoulder, which is more tone-mapping-like and seems unnecessary. If the user wants tone-mapping-like behavior, that's what AgX is for.
Moreover, if we stick with simple gamut clipping (either luminance- or chroma-preserving) that lets the transforms be much simpler. In the case of chroma-preserving clipping, I believe we can do it with just three built-in OCIO transforms. And in the case of luminance-preserving clipping we can use a fairly low-resolution 3D LUT with cells aligned to the closed-domain gamut boundaries.
(Having said all that, I do think there's a place for a tone mapper that leaves colors within a protected zone un-touched, basically as a viewport "working tone mapper" for texture/shading artists. But that would need to make different trade offs than Guard Rail here.)
AgX
General notes
This is a significant improvement over the original AgX configuration!
The red hues don't shift towards magenta as they blow out anymore, and relative luminance relationships are preserved much better. There also isn't nearly as much of a global desaturation effect in yours. Awesome work! This is an excellent tone mapper.
But there are a couple things I want to double-check:
First, reds globally shift a bit towards orange:
Since it's a fairly global shift, and it's not extreme, that seems totally reasonable to me. And I'm tentatively guessing there's a trade off with the Abney effect involved there...?
So I assume this is just a known trade off, but I wanted to double-check.
Second, as highly saturated reds blow out, they don't quite keep a consistent relative luminance with surrounding colors:
(Pay special attention the faces.)
I assume this is also a known trade off, to e.g. look better in other situations. Because this is admittedly an extreme case, and the effect is subtle. But again, just wanted to double-check.
Certainly neither of these are blockers IMO. But if you do want to make more tweaks, that should ideally happen before merging, I think.
Relationship to spectral
In the source repo for this, you mention that this is targeting spectral Cycles. And I have a question about that.
It looks like AgX uses FilmLight's E-Gamut as its working color gamut, to do its transforms. However, that doesn't cover all the colors that can be produced by a spectral renderer when rendering to a CIE XYZ observer. So some kind of gamut clipping/compression seems necessary to bring those colors inside of E-Gamut before further processing.
So, am I correct that the
luminance_compensation_*.cubeLUTs include such gamut mapping, before proceeding with the AgX transforms? And if not, is there somewhere else I'm missing that is doing appropriate gamut mapping before the AgX transforms?Open source transforms
Is this version of AgX still exclusively using the AgX transforms as published by Troy, just with well-tweaked constants? Or are there other things layered on top? For example:
The luminance compensation transforms seem like an additional thing.
In the source repo you write:
So those
BT.2020 -> othertransforms also sound like something additional.The reason I ask is simply because in the spirit of open source, it would be nice for everything to be documented and reproducible, and all the techniques involved known. LUTs are a bit like compiled binaries, and it would be nice to have the "source code", so to speak. I don't think this needs to be a blocker, but it would be very appreciated.
If we end up going with your version of Guard Rail, this request also applies to that.
If you've already documented these things or published the source code for generating the LUTs, awesome. I just wasn't able to find anything like that either here or in the source repo, so figured I'd ask.
Aside from what @Sergey has already commented about, and a few nit picks/questions, over-all the configuration looks pretty reasonable to me.
(I do think some things could be simplified even more by switching to a simpler Guard Rail, since that would also have knock-on simplifications like the
Luminance Compensation *color spaces no longer being necessary. But I talked about Guard Rail in my high-level review already.)@ -47,2 +51,2 @@color_timing: Filmic Logcompositing_log: Filmic Logcolor_timing: AgX Logcompositing_log: AgX LogSince these aren't used in Blender anyway, not a big deal. But I wonder if it would make sense to switch to something a little more... agnostic? (For lack of a better word.) Not sure what that would be. And, again, it's academic anyway since Blender explicitly doesn't use these.
@ -56,3 +61,2 @@- !<View> {name: Filmic, colorspace: Filmic sRGB}- !<View> {name: Filmic Log, colorspace: Filmic Log}- !<View> {name: False Color, colorspace: False Color}- !<View> {name: AgX Log,colorspace: AgX Log}This very well could just be a blind spot for me, but it's not clear to me what the purpose of including
Agx Logas a view transform is. (And it wasn't clear to me the purpose of includingFilmic Logbefore this, either.)I just thought we should have something to replace
Filmic Log, but if you confirm we should not include eitherAgX LogorFilmic LogI am fine with removing it.I have a hard time thinking what the use case is for it. It's not a useful transform for viewing purposes, and it's not an interchange standard (and people should just use something like ACES EXR files for that use case anyway).
Unless someone else has a compelling use case, let's remove it as a view transform, and just leave it as an internal implementation detail of AgX.
@ -272,0 +305,4 @@children:- !<ColorSpaceTransform> {src: Linear CIE-XYZ E, dst: Linear FilmLight E-Gamut}- !<AllocationTransform> {allocation: lg2, vars: [-12.47393, 12.5260688117]}- !<FileTransform> {src: luminance_compensation_bt2020.cube, interpolation: best}Rather than
best, please explicitly name the interpolation method. For 3D LUTs I believebestis equivalent totetrahedral.In practice I doubt OCIO is going to change what
bestmeans, so it doesn't practically matter in terms of functionality. But I think it makes things clearer.@ -282,0 +523,4 @@children:- !<ColorSpaceTransform> {src: Linear CIE-XYZ E, dst: Linear FilmLight E-Gamut}- !<AllocationTransform> {allocation: lg2, vars: [-12.47393, 12.5260688117]}- !<FileTransform> {src: AgX_Base_sRGB.cube, interpolation: best}Am I correct that the transfer function is baked into the 3D LUT in this case? (Hence why the other transforms apply a 2.4 exponent transform before applying their own transfer functions?)
Yes there is a power 2.4 curve baked in, the reason was more for precision, as tested it seems outputting linear values causes lower precision.
@ -335,1 +629,4 @@- !<FileTransform> {src: filmic_to_0-70_1-03.spi1d, interpolation: linear, direction: inverse}- !<Look>name: AgX - PunchyWhat is the purpose of
Punchycompared to the contrast looks? The results are admittedly not identical, but it feels like they address similar use cases, and in that sensePunchyis a bit redundant.The contrast Looks are pivoted on linear 0.18 middle grey, but some user doesn't necessary like that behavior. The punchy look is a different kind of contrast as it darkens the entire image. As tested, it seems to also somehow work as a dim surround compenstation as well, not what it was intended to be but it seems to work.
If other people think it's useful to keep, then I'm fine with it. Just seems like a gamma adjustment is more of a post processing thing. But I don't feel strongly about it, and the list of looks is long regardless.
Ok I just stayed up the entire night addressing the things pointed out.
I shortened the "shoulder" now, not exactly the same as what your example shows, but I think it should be fine for our purpose:
Yes, it's for Abney etc. It's caused by the rotation in AgX Log's matrix.
Also fixed now:
No, it actually uses Rec.2020 as the image formation working space.
Actually, I originally used CIE-XYZ as the LUT input encoding to make sure the
luminance_compensation_bt2020LUT can handle the negative values in our Rec.2020 working space, but later someone provided an EXR encoded in E Gmaut, with lots of negative luminance values and I realized my version back then didn't work at all with negative luminance values, as all negatives are clipped by entering LUT, in order to have a better handling of negative values, those values need to at least make it inside the LUT.Since the provided EXR was encoded in E Gamut, I tried switching to E Gamut as LUT encoding and it works fine. I tried some other encodings including ACES2065-1 but none of them reaches downwards as much as E Gamut. And upon testing, using E Gamut doesn't cause significant issue for spectral data, so I just went with it.
So the use of E Gamut in the first place was to make sure we get wider "gamut" values mapped back to Rec.2020 working space.
Yes, that's correct.
There are a lot of things added on top, but most things like the entire idea of "Guard Rail" etc. were given by Troy. He rarely just tells us what to do though, he often just gave some vague ideas and let us do our own stuff, so there are indeed a lot of things I modify myself. I also got help from the people listed on the top comment.
The current state of the scripts are messy and full of debug codes, I can share them but I need more time to clean them up before I can comfortably do so.
Just think I should mention, the Guard Rail script I am using was modified from this script shared by Sakari Kapanen, I got their approval to include it in my version so I have been just using it. The general principles behind the lower and higher Guard Rail is generally unchanged.
Ok it's 8:35 am right now, need to get some quick rest before I need to go do what I need to do today.
@Eary Wow, much appreciate the dedication! But we shouldn't be burned-out after being done with this project!
From quick looking into the update it seems we are really close. We are still working with Brecht to land some patches to avoid the backward/forward compatibility issues with Filmic. I would really like the AgX to land during bcon1, so want to avoid opening more cans of worms than it is strictly needed...
@Eary I've committed some fixes and improvements. Most notable is the #111229. So now you should be able to remove "Filmic -" prefix from views. This will solve compatibility concerns, but without affecting the way you expect looks to work for the AgX.
I do see some confusing with the way how Filmic looks are defined/used currently, and maybe we'll need to revisit this topic. Just preferable after the AgX is landed.
From the configuration itself i think the renaming of the Filmic looks back to their original name is the only remaining thing which I'd like to happen. The rest seems fine to me. I'll go through it again tomorrow with a fresh mind, to ensure we did not miss anything obvious.
@blender-bot package
Package build started. Download here when ready.
@Eary
Oh god! You definitely didn't need to do that.
In any case, thanks for your answers to my questions. And awesome work on the updated AgX! I wasn't able to find any problematic areas with it (and the previous version was already very good).
Yeah, no rush at all! But it sounds like you're happy to share once you have a chance to clean them up? That's fantastic to hear. Thanks so much!
I guess I just don't understand the purpose of having a shoulder at all in this case. In general, I can see two reasons to involve a shoulder in a mapping:
But I don't believe either of these apply to the goal of replacing
Standard. Moreover, both goals are impossible to consistently achieve without touching in-gamut colors anyway, because of grays (try playing with guard rail on a gray scale image). And even if that were possible (which it isn't), there's still another first-derivative discontinuity where the colors first start blowing out to white anyway, which is also unavoidable without touching in-gamut colors.So to me it seems like the shoulder is a fool's errand. There's just no way to make a gamut-preserving mapping good in the ways that a shoulder otherwise could. And I'd rather have a simple, consistent mapping rather than a Frankenstein mapping that's trying to look good but failing.
To me, the goals of a
Standardreplacement are just the following:Standarddoes).That's it. It's still not going to look good, because it can't.
And as mentioned before, I believe a simpler mapping could also simplify the configuration.
If this still needs more discussion (which is fine, if so!), maybe it's best to split Guard Rail off into another PR so it doesn't block merging AgX.
(Edit: upon re-reading this the following morning, I realize all the italics come across a bit aggressive, which was not my intention. Sorry about that. I was just using them as emphasis.)
@ -269,3 +295,2 @@- !<ColorSpace>name: False Colorfamily: Filmicname: Luminance Compensation Rec.2020Based on what you said in the other comment, I think these transforms and their LUT files could be named something like "Luminance Preserving Gamut Clip". Feel free to come up with something better than that, but the current name did not suggest what it actually does to me.
Maybe just call it "Lower Guard Rail"? It's split from the Guard Rail (lower + higher) from the first place.
Honestly, "Guard Rail" is not the most obvious name. Currently that feels like a, like, brand name (as does AgX) - it doesn't really tell people what it does by the name alone.
I mean it sorta does I suppose, if you are thinking about guard rails as a thing you can't get past in the context of gamuts. But it'd not be the clearest thing in the world without an explanation, I think.
What the "Lower Guard Rail" does is dealing with negative-luminance colors, right? So ideally it should be a name that invokes this.
(The corresponding "Upper Guard Rail", meanwhile, deals with above-luminance-1 colors and does so in a way that doesn't touch already-in-target-gamut colors, so you can basically first do the "lower" and then the "upper" and end up with exclusively colors in the target-gamut. I.e. for "Luminance Compensation Rec.2020" in Rec.2020)
Oh, interesting. In my own color tools I call those "open domain clip" and "closed domain clip", respectively. (The first is a fairly accurate name, but the second is a little misleading because like Upper Guard Rail it only clips the > 1.0 values, and has to be combined with the first to do a full gamut clip.)
Anyway, as a user-facing name I think Guard Rail is about as good a name as any. But for the non-user-facing transforms and the LUT file names, I think it would be better to call them something a little more technically descriptive.
Having said that, I don't feel super strongly about it. It's just that it wasn't at all clear to me what those transforms/luts did from their current names.
Strictly speaking "open domain clip" and "closed domain clip" is not mathematically less ambiguous than "Luminance Compensation". Such name does not indicate the boundary itself, so an extra explanation is required anyway. The way how clipping is done is also important.
To improve clarity to me it would suffice to add a comment briefly explaining what exactly is going on. I.e.
Ensure Rec.2020 colors are within [0 .. inf) range by <using some smart gamut-projection algorithm>..Just adding a comment sounds good to me. @Eary If you also want to rename it to "Lower Guard Rail" I think that would further provide clarity, but I don't feel strongly as long as there's a comment.
@nathanvegdahl For the clarity, is it the shoulder in the guard rail which is a stopper here?
Correct. AgX itself is great, and is ready to land from my side.
But I'm not comfortable accepting Guard Rail with a shoulder unless a good case can be made for it. This is an area where I think we should keep things as simple and straightforward as we reasonably can.
I think the intended use case for Guard Rail is a situation where somebody wants very precise colors (say, for a logo or product) and still wants at least some sort of graceful tone mapping of blown-out areas. I.e. stuff should go to white and hues should avoid the Notorious Six problem.
At least that was the use case people lamented AgX (and also Filmic) fails to fulfill, which iirc got us started on the Guard Rail route in the first place.
I suppose the best thing to do here would be to test GuardRail against the two other options you @nathanvegdahl provided (Luminance- and Chrominance-presevering clips respectively), or something in between those (I think you can basically smoothly trade off those two extremes, right?) and see what people like best of these.
We'd need a collection of test images for that, probably. Regular photo or "photorealistic" .exrs aren't gonna help us there. Really, we'd have to convene with the NPR crowd to really nail this down.
If that were the necessary route to take, to fully justify this (and @Eary I'm not saying it isn't: if you have some better ideas, please share!) then I'd agree that perhaps Guardrail can wait in favor of bringing in AgX sooner rather than later.
I am fine with leaving the upper Guard Rail and the
Guard Railview transforms out, especially since I have baked the AgX views into single LUTs, now the components are not as interwoven together.I'd like to kindly remind to keep better scoping and presentation of the patch.
While I agree the
Filmic LogandAgX Logare rather confusing, I do not believe removal ofFilmic Logshould be included into the patch which is advertized as "Adding AgX view transform".For this PR it would mean undoing the
Filmic Logpart.If people feel strongly about removal of the
Filmic Logit should be submitted and motivated separately.It does feel a bit weird to have Filmic Log but not AgX Log. IMO there should either be both or neither. - Any potential use case Filmic Log might have is likely improved by an AgX Log after all.
My reasoning is that Troy's original config didn't have AgX Log as view transform anyways. And I also have concerns about different forks for AgX don't have interchangeable AgX Logs, there is going to be different primaries, different inset, different rotation, different Log2 range or even straight up a different Log curve. IIRC Troy's Resolve version now uses ARRI LogC3 curve for example. So it didn't really make sense anyways. It just seems AgX Log in nature is just an implementation detail of AgX view.
What you are saying to me sounds like effectively a reason to remove Filmic Log over adding AgX Log. Which is a fine outcome. I think a corresponding PR should work?
Exactly, separate PR with its own motivational part.
Let's not forget who Guard Rail was made for, people who use Standard because they think it makes their renders too washed out. Unfortunately some will just see that their renders look more appealing out of the box with a single option change, and will just keep using it without doing proper research.
So for me Guard Rail's usecase is simply the same as Standard's, but when you don't want saturated specular highlights or emissive textures to turn into flat color blobs.
@nathanvegdahl Wasn't the case for it made already #106355 (comment) ?
To me there is also a difference between having
Guard Rail, and having it as a replacement ofStandard. As in, to me it does seem that in some cases one will want to prefer its look, but it does not mean we have to replace Standard as default view.@blender-bot package
Package build started. Download here when ready.
The hue skewing issue demonstrated there is caused by clamping the individual RGB channels (as in
Standard), and is avoided by the other approaches I mentioned as well. It's not related to the shoulder.The luminance-preserving and chroma-preserving clips both avoid the Notorious Six problem as well:
(*Again, these are not exact, just quickly tossed together approximations for illustration purposes.)
And I don't believe there's a good way to do graceful tone mapping in terms of luminance. You'll always hit harsh cut offs to white as the source colors approach the achromatic axis:
(*Again, approximate.)
Guard Rail is able to give a tone-mapping like fall off in the highly saturated source image. But in the desaturated image it literally can't, because that would require touching in-gamut colors. Admittedly, gray scale is the most extreme case, but you progressively approach that result with less and less saturated colors. So in a scene with a combination of more and less saturated colors, you're going to get weird relative-luminance flips etc. as things blow out. And this problem is intrinsic to trying to add a shoulder while leaving in-gamut colors untouched.
Doing a simple luminance-based clip, on the other hand, keeps everything blowing out consistently with each other. In some sense it might not look as nice, but it is at least consistent in its treatment of colors.
I definitely think there's room for an NPR-targeted view transform. But indeed we'll need to specifically design for that, and I'm skeptical (though much less confident in that particular skepticism) that Guard Rail as-is is well tuned for that use case either.
In any case, I'm not ready to accept Guard Rail as-is. So let's split Guard Rail off into a separate PR for further discussion, and then we can merge AgX. Because @Eary's work on AgX is excellent, and I'm eager to get that in.
I guess it depends on what's meant by "graceful". It's certainly true that you cannot possibly, mathematically, have any sort of continuity better than C0 while demanding both no change at all in the in-gamut and in-dynamic-range interval, and a hard limit of 0 and 1 below and above the interval (at least when that hard limit is set to precisely the boundaries of the interval, so there is no leeway to be had for a shoulder of sorts)
You could still attempt to give "the closest reasonable color" by that measure though.
Personally, I'd prefer the take on red xmas that Guard Rail accomplishes here over the Luminance preserving clip even as the desaturated version doesn't look great (and presumably wouldn't look great in the Luminance preserving version either)
At the very least you can see some approximation of what's going on there, rather than all detail being blown out.
But anyways, I'd agree in terms of this being a separate issue from AgX, and in the spirit of getting AgX in asap, a separate PR sounds reasonable to me
Aside from adding comments explaining the
Luminance Compensation *transforms, I think this is ready to land.I did write it in the space's description though.
Offset the negative values in BT.2020 and compensate for luminance, ensuring there is no negative values in Rec.2020Ah! Sorry! I somehow missed that.
The description still feels a bit vague to me. In particular, it's not clear to me what "compensate for luminance" means here. It leaves me wondering if this is more than just a gamut clip, and if so in what way.
Having said that, if you're planning to release the scripts used to generate the LUTs, then I don't feel too strongly about addressing this right now. And admittedly, it can be difficult to concisely describe color transformations, particularly when they're not already in common use and don't already have a common term associated with them. So I don't want to block on this.
This looks ready to land to me. I still have some minor nits, but none of them are functional or user-facing, and can be easily addressed later.
@blender-bot build
Great to see we are all happy with this PR!
I'll be taking care of the last double-checks, release notes, landing the patch etc now.
@Eary We discussed the Guard Rail a bit with Nathan. It feels a bit of an open topic to me still, so it might be good to have some place where you and Nathan can agree on the following steps for it: is it Guard Rail? with some tweaks? something completely different? Some smart clipping algorithm we can upstream to OCIO?
I am not sure what is the best platform for it would be. I guess we can start with a dedicated PR for the Guard Rail, and if/when Nathan has some alternatives he had in mind we can revisit whether we keep discussion here, or move it to the devtalk.
Having a PR seems like a good idea nevertheless, as it allows to easily make testable builds.
How does it sound to both of you? :)
After giving some thoughts, I feel the need to do more exploration before making any more PRs regarding Guard Rails. I think maybe we can have 4.0 without the upper Guard Rail.
@Eary For this PR: can we rename "AgX False Color" to "False Color" ? We don't have multiple false colors, and having better backwards compatibility never hurts.
Makes sense.
Main point is: feel welcome to kick-start re-iteration on the Guard Rail story.
Sounds good! I think starting a topic on devtalk, and pointing to PRs of alternatives with builds available, would be a good way to go. But I'm happy to wait until @Eary has a chance to explore and work on things.
@Eary @nathanvegdahl I have a specific use case where
guard railmight come in handy. Is there any further development going on already?