ICC Color Correction Effect
AbandonedPublic

Authored by mikeljohnson on May 7 2019, 11:28 PM.

Details

Summary

Hi, I want to submit a simple full-screen ICC color correction effect that I made.
I'm using it with <s>5.14.5</s> 5.17.5 already.
It doesn't have multi-monitor support yet, but I would like to submit it first and try to add the multi-monitor feature later.
Its configuration is intentionally simplified, it doesn't talk to colord or old kolor-manager or oyranos or anything else. The only dependency is LCMS2 which is available everywhere so it's not a problem to build.
All you have to do to configure color correction is to build a color profile for your screen using ArgyllCMS or DisplayCAL GUI wrapper, select it in the effect configuration as target profile, and set another profile (for example, Adobe RGB) as source.

Diff Detail

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

Neato!

effects/icc/icc_config.desktop
10

Don't include translated text; the translation team prefers to take care of this themselves

Thanks! :) OK, I'll remove translated text

vitaliyf updated this revision to Diff 57986.May 13 2019, 8:45 AM

Removed translation, commented code and curly braces style

zzag added a subscriber: zzag.EditedMay 13 2019, 9:27 AM

The rabbit hole goes a bit deeper. On X11, all screens are rendered in a single pass, which means that per screen color correction is a huge pita.

For what it's worth, we had OpenGL-based color correction some time ago.

Relevant links:

vitaliyf added a comment.EditedMay 13 2019, 10:38 AM

Yes, I know. I even tested it 5 or 6 years ago. I think that implementation was really clunky because it depended on Oyranos which is unavailable in most distros, and instead of being implemented as an effect it was patching shader code of other effects before compiling them. There was a whole 2 or 3 screen long function that was doing it. My implementation is much simpler and it only depends on LCMS2.

Please merge single-screen color correction first :) it's very useful at least for a lot of laptops. In fact it's a unique feature, neither Windows nor Macos have it. I think anyway it's better to have single-screen color correction that no correction at all :)

Also I see there's WindowPaintData::screen() so it's probably possible to select different shaders on a per-window basis. So it should be easy to implement multi-screen correction if it actually works.

anthonyfieroni added inline comments.
effects/icc/CMakeLists.txt
14–22

Strip out commented code.

effects/icc/icc.cpp
114–119

Don't use goto and custom allocations, use QScopedPointer on pointer type (also notice custom deleters https://doc.qt.io/qt-5/qscopedpointer.html#custom-cleanup-handlers), vector on arrays.

effects/icc/icc.h
45–48

Use override not virtual.

effects/icc/icc_config.h
39–41

override.

Lets evaluate the concept before we fuss over code details, otherwise we're doing it in the wrong order.


Whilst this patch is simpler than using colord, it comes with the obvious downside of not then sharing the relevant UI. It "solves" one problem but it creates another. Two textfields isn't going to enough longterm.

If we are going to do all windows, what does this offer over xcalib?

vitaliyf added a comment.EditedMay 13 2019, 1:42 PM

If we are going to do all windows, what does this offer over xcalib?

xcalib/dispwin don't do ICC color correction! They only load 1-dimensional LUTs ("VCGT" tag, Video Card Gamma Table) into the video card. With 1-dimensional LUTs you can basically only tune color temperature and gamma. But you can't make colors look "like they should look".

ICC profile transformation is more complex: it contains either a matrix transformation (RGB = XYZ * 3x3 matrix) - which is also insufficient for a lot of displays to make colors match sRGB - or a full 3-dimensional LUT (cLUT, i.e. (R,G,B) = f(X,Y,Z)).

So the real transformation is only possible with a shader, so the only place to put it is KWin :).

See an example here: https://yourcmc.ru/wiki/images/thumb/1/12/Venice_940z5l.jpg/347px-Venice_940z5l.jpg

This is an example of color correction which is not possible with 1D LUT and even with a matrix transformation. The image in the middle looks good if you view it on an sRGB-tuned display; the image at the top looks good on Samsung NP940Z5L (my laptop); and the image at the bottom looks on an sRGB display like it looks on NP940Z5L.

vitaliyf marked 5 inline comments as done.May 13 2019, 4:22 PM
vitaliyf added inline comments.
effects/icc/icc.cpp
114–119

OK, I changed uint8_t* to std::vector<uint8_t>.

But how to do it with cmsHPROFILE? It's probably a pointer type, but it's internal to the LCMS2 library. How to wrap it in a QScopedPointer, and are you sure it must be wrapped? I think it will only complicate the code... It's local to this exact function, so is it a big problem that it's allocated with custom code?..

vitaliyf updated this revision to Diff 58017.May 13 2019, 4:22 PM
vitaliyf marked an inline comment as done.

They only load 1-dimensional LUTs ("VCGT" tag, Video Card Gamma Table) into the video card.

It being impossible would certainly be a very compelling reason to do it at this layer instead. I would support something along these lines if that's the case.

Though your comment doesn't match the docs I've seen on the colord site. I'll poke a Mutter dev and see what they're doing.

It being impossible would certainly be a very compelling reason to do it at this layer instead. I would support something along these lines if that's the case.

Though your comment doesn't match the docs I've seen on the colord site. I'll poke a Mutter dev and see what they're doing.

https://www.freedesktop.org/software/colord/intro.html

  • Setting output gamma tables (with local brightness and adjustments) to any XRandR output.

That's what I'm talking about (VCGT), same as xcalib/dispwin. colord is only a registry of ICC profile settings that color-management aware apps can take and apply themselves. It doesn't do fullscreen color correction, there's just technically no way to do it for colord (it's not a compositor).

Yeah, you're right. Under their scheme the relevant app is meant to do it, not the compositor.
Thanks for the link.

So, can you merge it? :)

zzag requested changes to this revision.May 27 2019, 11:11 AM
zzag added a subscriber: graesslin.

I'm not sure about inclusion because:

(a) (currently) this effect doesn't work correctly on multi screen setups
(b) there is a policy about inclusion of new effects. Maybe @graesslin could weigh in (it may take some time to get response though)

I don't see any problems with shipping this effect as a third party binary effect (out-of-source tree).

effects/icc/icc.cpp
22–32
34

Is it a good idea to hardcode this value though?

40–42
  • Use nullptr instead NULL
  • Prefer default member initialization
50–53
111
  • Use QVector.
  • Use camelCase not snake_case :-)
113–114
128

Post increment in a for loop is a bug, even if it's an int.

131–133

Just do

clut_source[addr++] = 255 * r / (LUT_POINTS - 1);
clut_source[addr++] = 255 * g / (LUT_POINTS - 1);
clut_source[addr++] = 255 * b / (LUT_POINTS - 1);

and remove addr += 3 in the for loop.

(also, I wonder whether we could use reciprocals here, just a thought)

140–145

Please don't do this in C++. It would be perfectly fine C code, but not C++. Use a QScopedPointer with custom deleter instead.

177

You don't need to check whether m_valid is true. drawWindow() won't be called if isActive() returns false.

212

// namespace KWin

(though namespace comments is a bit controversial topic in KWin)

effects/icc/icc.h
25–26

Why one is <filename> and the other is "filename"?

36

In KWin, doxygen comments have to terminate with **/.

37–38

You're following the old coding style. Instead it should look like

class ICCEffect : public Effect
{
43

~ICCEffect() override;

46
48

Missing override keyword.

54

Delete it.

56

Huh?

66

I don't see why we need it. This field is used only in makeclut.

68–69

I don't see why these two have to be methods. Both can be static functions in the cpp file.

effects/icc/icc_config.cpp
54

Huh?

79–80

Too many empty lines.

effects/icc/icc_config.h
35

Use nullptr.

This revision now requires changes to proceed.May 27 2019, 11:11 AM

How to wrap it into a custom deleter? It's an internal pointer type of LCMS... Even a custom class with destructor seems simpler...

As I understand 3rdparty binary effects are harder to install, very unpopular and it will take 100 years to get it packaged in all distros... so... I still kindly ask you to merge it into the base :-) it's definitely useful. I can add multi-screen support later, I just want to try to get through your review process for the first time :-)

effects/icc/icc_config.ui
32

This at a very bare minimum needs to have a file picker dialog.

rjvbb added a subscriber: rjvbb.Aug 10 2020, 7:37 PM

Something like this should be part of any self-respecting desktop; per-screen colour management ("ColorSync") has been in Mac OS X since its very early days and even with a simple software calibration utility (SuperCal for Mac, Calibrize for MSWin) you can really improve how your display looks (whether you go for absolute correctness or a calibration that makes things look at their best for your own perception).

A competing implementation exists in Compiz, with multi-screen support in that case:
https://www.oyranos.org/compicc/index.html
https://github.com/compiz-reloaded/compicc

rjvbb requested changes to this revision.Aug 10 2020, 9:20 PM

Just tested this with KWin 5.13.3 (of which I had a build all set up; the patch applies cleanly except for the .desktop file).

It works nicely but there's 1 annoying (to me) side-effect: the ICC effect cancels the translucency effects (checked with moving windows and menus). That's a no-go for me.

At first, I very much appreciate that somebody starts working on color correction in KWin again. There is certainly a need for it. However, and although I am by no means an expert in this area, I have certain concerns about the approach.

The current state of the art in the X Windowing System world seems to be that the application either queries colord or reads the _ICC_PROFILE xatom, and then subsequently does the conversion on its own. The window manager seesm to have more a passive role.

Examples are:

  • Darktable uses either colord or the xatom (cf. here).
  • Inkscape watches for changes in the _ICC_PROFILE xatom (cf. here)
  • Gimp also reads the xatom (cf. here)

It seems that there once existed plans to offload the color conversion to the window manager, or to at least indicate the regions for which the compositor does not need to color correct [ 1 ].

Regarding wayland, I have no clue whether there is something similar worked on already. At least the example client applications listed above seem to be totally oblivious of wayland in that regard.

effects/icc/icc.cpp
114–119

FYI, we have a similar construction in poppler with std::shared_ptr and a custom deleter to handle cmsHPROFILEs, cf. https://gitlab.freedesktop.org/poppler/poppler/-/commit/5927e0b08f1ad6868d04cb209ee1e17b4ac07b70#966756b758fb5d7e46114c6835faf491d5b92582_195_195.

I like solutions that work right here right now. Waiting for all apps to do color correction on their own is utopism. Sometimes I just turn the effect off when I don't need it.
I'm not "working" on color correction, I mean it's not a continuous process :) I don't have much time to develop it further, make it fit guidelines or patterns.
I NEEDED full-screen color correction and I solved my problem with this effect. I use it 24/7.
Sometimes there are glitches, for example sometimes the taskbar flickers and so on, but these are always there with KWin as I understand so they don't bother me much.
By the way, I've updated the patch for KDE 5.17 + fixed it to work with translucency effects correctly (I didn't use "modulation" in the previous version of the patch).
It's on github: https://github.com/vitalif/kwin ... OK I'll upload it here too ...

vitaliyf updated this revision to Diff 83367.Sep 25 2020, 11:24 PM
vitaliyf edited the summary of this revision. (Show Details)

Updated for 5.17.5 + added "modulation" usage

vitaliyf edited the summary of this revision. (Show Details)Sep 25 2020, 11:32 PM
vitaliyf edited the summary of this revision. (Show Details)
rjvbb added a comment.Sep 26 2020, 8:43 AM

Apparently one can no longer contribute via email replies to the email notifications?!

The current state of the art in the X Windowing System world seems to be that the application either queries colord or reads the _ICC_PROFILE xatom

Status quo, a state of no art ;) This is undoubtedly because neither server nor WM (the more logical candidate) implemented the feature, which in turns is undoubtedly because it was considered too expensive and generally unnecessary.

So there's a status quo, and there are applications that *can* work around it. So what? The proposed KWin feature here is optional; users who activate it can disable the corresponding feature in applications that offer one.

Regarding wayland, I have no clue whether there is something similar worked on already.

KWin also provides a Wayland compositor; a colour correction plugin for KWin *could* thus work its magic under Wayland too.

rjvbb requested changes to this revision.Oct 2 2020, 4:37 PM
By the way, I've updated the patch for KDE 5.17 + fixed it to work with translucency effects correctly (I didn't use "modulation" in the previous version of the patch).

still doesn't seem to work together with KWin's translucency-when-moving effect for me (with KWin 5.13.3; patch applies cleanly except for the inexistent effects kcm). And after a few KWin restarts the correction has either "stuck" or isn't being applied anymore.

This revision now requires changes to proceed.Oct 2 2020, 4:37 PM

still doesn't seem to work together with KWin's translucency-when-moving effect for me (with KWin 5.13.3; patch applies cleanly except for the inexistent effects kcm). And after a few KWin restarts the correction has either "stuck" or isn't being applied anymore.

Something has probably changed since 5.13.3, because translucency-while-moving works fine with my KWin 5.17.5...

rjvbb added a comment.Oct 4 2020, 3:32 PM

Something has probably changed since 5.13.3, because translucency-while-moving works fine with my KWin 5.17.5...

It doesn't work either for me with KWin 5.15.5 and that doesn't seem to be because of one of the other few effects I have activated.

vitaliyf added a comment.EditedOct 4 2020, 10:55 PM

It doesn't work either for me with KWin 5.15.5 and that doesn't seem to be because of one of the other few effects I have activated.

Hm, and by not working you mean that the translucency effect gets canceled, but the correction itself works, right?
Are you sure you applied the newer patch? One with the tex *= modulation line in icc.frag? Also are you sure you installed the resulting libkwin4_effect_builtins.so correctly?

rjvbb added a comment.Oct 5 2020, 8:26 AM

Hm, and by not working you mean that the translucency effect gets canceled, but the correction itself works, right?

Yep.

Are you sure you applied the newer patch? One with the tex *= modulation line in icc.frag? Also are you sure you installed the resulting libkwin4_effect_builtins.so correctly?

I got this one: https://github.com/vitalif/kwin/commit/83d69c928574de388d8f27ec6948f307b4eab3f2.patch following the link to your github repo you posted earlier, but I see no occurence of that modulation pattern in that commit. In fact, I now see that it looks like an old commit - so where is that newer one?

Are you sure you applied the newer patch? One with the tex *= modulation line in icc.frag? Also are you sure you installed the resulting libkwin4_effect_builtins.so correctly?

I got this one: https://github.com/vitalif/kwin/commit/83d69c928574de388d8f27ec6948f307b4eab3f2.patch following the link to your github repo you posted earlier, but I see no occurence of that modulation pattern in that commit. In fact, I now see that it looks like an old commit - so where is that newer one?

It's here: https://github.com/vitalif/kwin/tree/icc-effect-5.17.5
Sorry, icc-effect branch is the older one.

rjvbb accepted this revision.Oct 5 2020, 12:58 PM

Yup, works for me now.

rjvbb added a comment.Oct 5 2020, 1:10 PM

I was maybe a little quick. Consider these differences

vs.

That's with the translucency effect for menus set just 1 tick non-opaque. The effect gets less the more opaque menus are made, but it seems wrong. My widget style and colour palette specify a white menu background and my colour correction profile clearly doesn't affect white noticeably.

That's with the translucency effect for menus set just 1 tick non-opaque. The effect gets less the more opaque menus are made, but it seems wrong. My widget style and colour palette specify a white menu background and my colour correction profile clearly doesn't affect white noticeably.

Huh. Yeah, I reproduced it. It's a mystery for me how all of it works, but I've just fixed gray menus by copy-pasting a block of shader code related to uniform float saturation from the inversion effect :-)

vitaliyf updated this revision to Diff 83369.Oct 5 2020, 9:14 PM
rjvbb added a comment.EditedOct 6 2020, 9:50 AM

As I understand 3rdparty binary effects are harder to install, very unpopular and it will take 100 years to get it packaged in all distros...

It should be possible to package this as an add-on desktop effect that people can install at will:

My guess would be that the greyscale effect has very similar requirements (it's basically a hardwired colour correction profile, no?)

Your fix seems to work, but I continue to get annoying update glitches, mostly when switching between virtual desktops (for which I don't use any transition effects):


I get them too (but much less severe) in xfwm4 (4.12.x) with composition enabled so this may be a manifestation of the apparently shitty Intel i915 driver (on this machine I have to stick to the 4.14 kernel because later versions tend to hang the GPU >:( )
But kWin also causes an update issue in Konsole when compositing is enabled, where the last line shows the previous command instead of the prompt. I do have a dynamic shell prompt set up that echoes the last command I executed whenever I hit Enter

but the prompt should still show as in this snapshot. Moving the mouse around tends to fix all those glitches so it looks like graphics operations remain pending. I hoped it could be the VSync tearing prevention but I still get the same glitches with that turned off.

Much as I appreciate the colour correction I probably will go back to xfwm4 because it glitches less (and is also much less resource hungry).

EDIT: I know this is not the place, but if someone seeing this can tell whether the output below is relevant (and suggest a fix)...

OpenGL vendor string:                   Intel Open Source Technology Center
OpenGL renderer string:                 Mesa DRI Intel(R) HD Graphics 400 (Braswell) 
OpenGL version string:                  4.5 (Core Profile) Mesa 18.3.3
OpenGL shading language version string: 4.50
Driver:                                 Intel
GPU class:                              Unknown
OpenGL version:                         4.5
GLSL version:                           4.50
Mesa version:                           18.3.3
X server version:                       1.18.3
Linux kernel version:                   4.14.23
Requires strict binding:                yes
GLSL shaders:                           yes
Texture NPOT support:                   yes
Virtual Machine:                        no
libkwinglutils: Skipping self test as it is reported to return false positive results on Mesa drivers
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glBindFramebuffer(non-gen name)
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glFramebufferTexture2D(window-system framebuffer)
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glBindFramebuffer(non-gen name)
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glFramebufferTexture2D(window-system framebuffer)
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glBindFramebuffer(non-gen name)
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glFramebufferTexture2D(window-system framebuffer)
qt.qpa.xcb: QXcbConnection: XCB error: 9 (BadDrawable), sequence: 27290, resource id: 0, major code: 14 (GetGeometry), minor code: 0
kwin_scene_opengl: 0xa: GL_INVALID_OPERATION in glBindTexture(non-gen name)
qt.qpa.xcb: QXcbConnection: XCB error: 3 (BadWindow), sequence: 64380, resource id: 119537941, major code: 19 (DeleteProperty), minor code: 0
qt.qpa.xcb: QXcbConnection: XCB error: 3 (BadWindow), sequence: 64393, resource id: 119537941, major code: 19 (DeleteProperty), minor code: 0
qt.qpa.xcb: QXcbConnection: XCB error: 3 (BadWindow), sequence: 64394, resource id: 119537941, major code: 18 (ChangeProperty), minor code: 0

This is from the session error log so I cannot be certain the XCB errors come from KWin too. They most likely do though.

As I understand 3rdparty binary effects are harder to install, very unpopular and it will take 100 years to get it packaged in all distros...

It should be possible to package this as an add-on desktop effect that people can install at will:

Is it? How does it work? It definitely requires distribution of a binary addon. Does KDE installer build binary addons?

My guess would be that the greyscale effect has very similar requirements (it's basically a hardwired colour correction profile, no?)

As I understand grayscale effect is built into KWin and all the 3rdparty addon has to do is to enable it via JS code, which is not the case with ICC.

Your fix seems to work, but I continue to get annoying update glitches, mostly when switching between virtual desktops (for which I don't use any transition effects):


I get them too (but much less severe) in xfwm4 (4.12.x) with composition enabled so this may be a manifestation of the apparently shitty Intel i915 driver (on this machine I have to stick to the 4.14 kernel because later versions tend to hang the GPU >:( )

I get similar glitches too. They don't reproduce always, but I feel they tend to "accumulate" over time. Sometimes I just restart kwin with kwin --replace...
Problem is that I have little to no knowledge about OpenGL and kwin rendering pipeline, so the code is mostly written by guess :)
The whole idea is to generate a texture and run pixels through it. I use GL_TEXTURE3...
I'd appreciate if you could give me some advice on how I should deal with that texture. Maybe I bind it incorrectly or maybe I don't clear some bindings or maybe something else...?

But kWin also causes an update issue in Konsole when compositing is enabled, where the last line shows the previous command instead of the prompt. I do have a dynamic shell prompt set up that echoes the last command I executed whenever I hit Enter

I had this problem at some point and I don't remember how exactly I solved it. I remember that it definitely wasn't caused just by my effect though.

One recent thing that I remember is Mesa 20 switching to the Intel "iris" driver by default which is still buggy and leads to all kinds of glitches with KWin. Try to switch to the older stable driver by adding MESA_LOADER_DRIVER_OVERRIDE=i965 to /etc/environment ...

rjvbb added a comment.Oct 6 2020, 1:26 PM
Is it? How does it work? It definitely requires distribution of a binary addon. Does KDE installer build binary addons?

I have no idea how it works, but I imagine you create the build locally, possibly with the help of kpackagetool, and then upload it to some KDE server.

One thing is certain: if you can do a build that works against multiple KWin versions your effect will become available to more users much quicker. Distributions like Debian may still take years to get to the version that includes your effect if it were to be accepted.

I get similar glitches too. They don't reproduce always, but I feel they tend to "accumulate" over time. Sometimes I just restart kwin with `kwin --replace`...

I restart KWin several times a day, that's how easy they can appear. I have a vague impression that they're worse with this effect enabled but maybe it just depends on how many effects you have enabled.

I'd appreciate if you could give me some advice on how I should deal with that texture. Maybe I bind it incorrectly or maybe I don't clear some bindings or maybe something else...?

It's been years since I did any OpenGL coding and I've never actually doodled in compositing, shader etc.

I had this problem at some point and I don't remember how exactly I solved it. I remember that it definitely wasn't caused just by my effect though

The weird thing is that I already disabled (blocked) compositing for Konsole, but the glitch still occurs unless I turn off the compositor entirely.

One recent thing that I remember is Mesa 20 switching to the Intel "iris" driver by default which is still buggy and leads to all kinds of glitches with KWin. Try to switch to the older stable driver by adding MESA_LOADER_DRIVER_OVERRIDE=i965 to /etc/environment ...

I'm using Mesa 1.18.3 and the i915 driver. Which isn't exactly bug free either :(

In the end, try to update KDE itself. I'm sure glitches were worse when I was using KDE 5.14. KDE 5.17 is definitely better

vitaliyf added a comment.EditedOct 6 2020, 2:04 PM

I have no idea how it works, but I imagine you create the build locally, possibly with the help of kpackagetool, and then upload it to some KDE server.

I don't know if it's possible to create a distribution-independent plugin binary. All GNU/Linuxes differ to some extent.
All current KWin effects are built in a single file - libkwin4_effect_builtins.so. Effects from the "Get Effects" KDE dialog are all JS-based. So I don't have an example of packaging.
And of course I'll appreciate it a lot more if the plugin gets merged into KWin itself - even if it's possible to package it separately :)

rjvbb added a comment.Oct 6 2020, 6:50 PM

You must be right about the greyscale effect; I don't see any actual calculation of the intensity (from the RGB values) in the code that gets installed. And that code is JavaScript (aka QtScript); up to you to determine if your effect could be implemented in that language...

And I fear this review won't be going anywhere; patch review has moved to gitlab

You must be right about the greyscale effect; I don't see any actual calculation of the intensity (from the RGB values) in the code that gets installed. And that code is JavaScript (aka QtScript); up to you to determine if your effect could be implemented in that language...

No way, I need a shader and interface to LCMS2. :-)

And I fear this review won't be going anywhere; patch review has moved to gitlab

Oh, fine. I'll resubmit it there.

mikeljohnson abandoned this revision.Nov 5 2020, 7:22 PM