ICC Color Correction Effect
Needs RevisionPublic

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

Details

Reviewers
zzag
Summary

Hi, I want to submit a simple full-screen ICC color correction effect that I made.
I'm using it with 5.14.5.
It doesn't have multi-monitor support yet, but I would like to submit it first and I can try to add the multi-monitor feature.
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 configure color correction is to build a color profile for your screen using ArgyllCMS or DisplayCAL GUI wrapper and then select it in the effect configuration as target profile. Also you can select another source profile, for example Adobe RGB.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
vitaliyf created this revision.May 7 2019, 11:28 PM
Restricted Application added a subscriber: kwin. · View Herald TranscriptMay 7 2019, 11:28 PM
vitaliyf requested review of this revision.May 7 2019, 11:28 PM

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
21–31
33

Is it a good idea to hardcode this value though?

39–41
  • Use nullptr instead NULL
  • Prefer default member initialization
49–52
110
  • Use QVector.
  • Use camelCase not snake_case :-)
112–113
127

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

130–132

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)

139–144

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

176

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

211

// namespace KWin

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

effects/icc/icc.h
24–25

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

35

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

36–37

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

class ICCEffect : public Effect
{
42

~ICCEffect() override;

45
47

Missing override keyword.

53

Delete it.

55

Huh?

65

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

67–68

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
53

Huh?

78–79

Too many empty lines.

effects/icc/icc_config.h
34

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
31

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