A port of Blender's AgX to darktable

[kofa73]

An attempt to port Blender's AgX tone mapper to darktable, based on @EaryChow's https://github.com/EaryChow/AgX_LUT_Gen/blob/main/AgXBaseRec2020.py.

• The curve, look and primaries parameters are exposed as sliders, with some pickers to help users.
• Early user feedback is being collected at https://discuss.pixls.us/t/blender-agx-in-darktable-proof-of-concept/48697/
  The tinting sliders are not implemented, no one seemed to require them at pixls, and I think darktable provides plenty of controls to implement that in other modules.
• Based on @EaryChow's advice, the hue restoration has been set to a default of 0, rather than 60% (which would correspond to the 40% used in his script, because of inverse mixing logic), as he has received a lot of criticism over that.
• iop_profile.c has been altered to make the output profile available as a source of base primaries. It is not essential to the module, but I think it would be useful to keep, e.g. for future gamut compression code. If accepted, maybe it'd make sense to update sigmoid, as well, where sRGB can currently be selected as a set of base primaries, too.
• Warning: much of the more complex UI code (especially that related to the plot of the sigmoid curve) was generated using LLMs, as I don't know GTK or any other GUI.
• I have squashed my changes, I don't think my close to 150 commits of trials and attempts would provide much benefit to anyone.

(TBH, I'm scared witless. I feel like I'm in The Emperor's New Clothes, and I'm no emperor, just a clown, to be exposed to the world -- and being aware of the situation just makes it worse.)

[TurboGit]

Not yet tested, only a first pass on the code, just some QA checks.

[kofa73]

Not yet tested, only a first pass on the code, just some QA checks.

I hope I was able to take care of most formatting issues, please let me know if I missed or misunderstood any. I'll take care of the extraction of deg2rad later this week.

What's the policy regarding resolving discussions? Should I resolve them, indicating they should now be fixed, or does the person who started the conversation resolve them, indicating they are satisfied with the outcome?

[TurboGit]

What's the policy regarding resolving discussions?

If you have resolved them, then do click on the resolve button. I'll anyway do another and possible multiple pass on the code.

[kofa73]

I don't know what to do about that macOS build failure. The error is not shown in the output, and agx.c was also not part of it.

[zisoft]

See #19035, same error

The error is not shown in the output

Search for error: (with the colon) in the output.

[kofa73]

@TurboGit : maybe the tiling_callback could be removed? sigmoid.c does not have it, I assume what I copy-pasted from some other module is probably not needed. Is that correct?

[TurboGit]

@kofa73 : The CI error is not related to this PR. I have merged the fix from @victoryforce. If you rebase your PR on top of current master and push back the branch kofa73:agx it should be green now.

The tiling callback is needed for module using lot of memory. I suppose it is not the case here, so yes as for Sigmoid you could probably remove the tilling code/

[TurboGit]

A second pass on the code, still not tested.

[dterrahe]

I've really only glanced at the ui related code, because there's obviously too much to catch up on here and on pixls to fully understand what's going on.

It is hard to get a feel for the ui code quickly because it follows very different conventions from the other modules. We have standards, like a pointer to _params_t (not _user_params_t) is p, not current_params or user_params. gui_data is g. Yes, it is arbitrary and maybe originally better choices could have been made, but they are the standard, and effort has been put into making it consistent across the code base, so that it becomes easier for people to switch between different parts of it, for review or maintenance. If everything is different, it becomes harder to understand what really is different (and why; like the whole presets thing). It is up to @TurboGit whether he wants to "enforce" this, but obviously it is quite a thankless task to keep doing the same cleanup work again and again. The point of janitorial work is to create a clean base which can then be preserved, not to create permanent jobs.

Separate from whether it is advisable to duplicate widgets over several tabs, like is done when enable_curve_tab is TRUE, this would still not require duplicate widgets and the hassle of syncing them. Instead, the relevant box of widgets can be reparented when changing tabs. An example can be seen here abab321
Some care needs to be taken with sizegroups to prevent the height of the module from jumping when the box is removed from the taller tab. I'd be happy to help with this, after the duplicate sets of widgets, and the complications in the definition of gui_data_t, have been removed.

For shortcuts, it might be helpful to create some order in the long list of parameters using DT_IOP_SECTION_FOR_PARAMS. Search the code for examples.

[kofa73]

@dterrahe

Thanks for the feedback. You are right, of course, I'll update the names for _params_t and gui_data.
I'll have a look at the reparenting, thanks for the pointer (the idea came up during searches / 'conversations' with Gemini I did during development, but I didn't pursue it) . I do think having the sliders duplicated makes the UI a lot more convenient: pivot, the contrast, the toe and shoulder power are tools that people use a lot; the curve display, and the advanced params are much less frequently used, but it would be really annoying to switch back and forth between tabs.

The sizegroups I've never heard of. I'll try to check, and will rely on the offered help if I'm completely lost.

I'll have a look at the DT_IOP_SECTION_FOR_PARAMS, too.

You've given me quite a bit to work on; 'fortunately', it looks like it'll be rainy weekend. I'm sure I won't get everything done, and I'm going on vacation in a few days, cut off from any development environment (I may work a little bit on the docs, though). So if I'm not responding in a timely manner, it's not due to indifference/laziness, but lack of access.

[kofa73]

Param name clean-up, reparenting and DT_IOP_SECTION_FOR_PARAMS done. I hope I didn't make a mess of them.

I kept dt_iop_agx_params_t *preset_params only, as those are not actual processing params, but params loaded from presets to populate the primaries drop-down.

Regarding the size-groups: as we have multiple tabs as well as collapsible sections, the size of the 'current tab' can vary greatly. I don't know if it would be useful to keep the height of the module always the same, as people were already complaining about vertical space taken up by the module (that's why the curve page was introduced).

We have some other modules whose height varies, e.g. clipping (highlight reconstruction, depending on method) or colorequal (one tab has more sliders).

Anyway, I don't know how to make the sizes better behaved, so if you have some ideas, or can help with that, @dterrahe, I'd be grateful.

[kofa73]

For the primaries tab it would be nice to have shade of colors on the sliders as done in the Primaries RVB module (I know that Sigmoid don't have it, and maybe it's because it is not that easy on this context, I don't remember, maybe later anyway, that's not blocking).

Messy, see https://discuss.pixls.us/t/blender-agx-in-darktable-proof-of-concept/48697/1883

Given that AgX is about control to me the Look tab is essential why not have the look controls in a standard section (like basic curve parameters) instead of a collapsible section? This won't make the module UI higher because of the primaries tab anyway.

Now supported via plugins/darkroom/agx/look_always_visible=TRUE|FALSE, see https://discuss.pixls.us/t/blender-agx-in-darktable-proof-of-concept/48697/1884

[TurboGit]

@kofa73 : Let's forget about the colored sliders, we'll handle that later if really needed.

[TurboGit]

Tested again, works on my side. At this stage I think it is ready for integration.

Is that ok or do you plan some other minor changes?

[kofa73]

Tested again, works on my side. At this stage I think it is ready for integration.

Is that ok or do you plan some other minor changes?

Thank you. No other changes are planned.

[TurboGit]

Now merged! Congrats!

Can you propose a release note entry? TIA.

[TurboGit]

I'll update files for AgX to be translatable and will add a regression test.

[jenshannoschwalm]

Lots of congrats also from my side.

Really good stuff and good to see you fully landing in the dev team!

[victoryforce]

@kofa73 Congrats! This is a real and significant improvement to darktable as a photo processing tool.

[kofa73]

release notes

How about:

Added a new tone mapper, based on Blender's AgX display transform

[TurboGit]

@kofa73 : For such a great feature I would expect a paragraph with minimum 5 lines or so. We need to talk about the different controls, the curve, the primaries. Maybe also have some words about why it was developed. What is different compared to Filmic or Sigmoid. How does that sound to you?

[kofa73]

Err... I'm not good at this. It's like a list of the controls.

Added a new tone mapper implementation based on Blender's AgX display transform. The new module's color output is similar to that of sigmoid, but the provided set of controls is more extensive. It allows for setting exposure white and black points explicitly, similar to filmic rgb. The implementation includes a user-adjustable pivot point for the tone curve, with the default set at 18% mid-gray. Contrast around this pivot is adjustable. Controls are provided to create a section of constant contrast, and independent sliders are available to control the contrast in shadows and highlights (the "toe" and "shoulder" of the curve). The resulting output is characterized by a gradual color desaturation in highlights, similar to film, and a natural-looking color representation in midtones and highlights.

[TurboGit]

Good to me, thanks! We can improve later if needed but this make it more appealing :)