Bug 401154

For comparison, gimp, imagemagick, and showfoto do not suffer from this issue.

How does oyranos work in Plasma? As far as I know, it can only set gamma ramps, but not apply correct color transformations.

Dunno. I imagine it sets some "system-wide ICC profile", whatever that is, which applies to the whole screen. 

Presumably gwenview then retrieves that profile and applies that again. This line is suspect:
lib/cms/cmsprofile.cpp:224:        static Atom icc_atom = XInternAtom(QX11Info::display(), "_ICC_PROFILE", True);

Does this mean "retrieve the ICC profile of the current display"? If so, I think this might be a mistake - there should be no reason for gwenview to know this profile. It is already being applied to the whole screen, you shouldn't be applying it again.

I'm also not the first person to notice this:
https://forum.kde.org/viewtopic.php?f=66&t=139430
https://superuser.com/questions/1159722/why-do-some-image-viewers-display-color-differently
http://www.lieberbiber.de/2018/02/24/open-source-color-management-is-broken/

Would you like to try your hand at producing a patch, Misha?

Documentation is located at:
- https://community.kde.org/Get_Involved/development
- https://community.kde.org/Infrastructure/Phabricator#Posting_a_Patch_using_the_website

Here's a patch against 49a52e748904ae41b4991f6828ce50de9c9e05c9 (system is too old to compile tip sadly). Obviously you'll need to adjust it.

I actually like the double-profile behaviour in about 10% of cases (when the images are really dark), and I'm not sure how color management works on other systems, so I made it a config option.

diff --git a/app/generalconfigpage.ui b/app/generalconfigpage.ui
index d874d8a5..1efe9d63 100644
--- a/app/generalconfigpage.ui
+++ b/app/generalconfigpage.ui
@@ -25,6 +25,13 @@
      </property>
     </widget>
    </item>
+   <item row="0" column="1">
+    <widget class="QCheckBox" name="kcfg_NoICC">
+     <property name="text">
+       <string>No ICC</string>
+     </property>
+    </widget>
+   </item>
    <item row="1" column="1">
     <spacer name="verticalSpacer">
      <property name="orientation">
diff --git a/lib/cms/cmsprofile.cpp b/lib/cms/cmsprofile.cpp
index 56f38d5a..eec7b6ef 100644
--- a/lib/cms/cmsprofile.cpp
+++ b/lib/cms/cmsprofile.cpp
@@ -28,6 +28,8 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Cambridge, MA 02110-1301, USA
 #include <iodevicejpegsourcemanager.h>
 #include <jpegerrormanager.h>
 
+#include <lib/gwenviewconfig.h>
+
 extern "C" {
 #include <cms/iccjpeg.h>
 }
@@ -212,7 +214,7 @@ Profile::Ptr Profile::getMonitorProfile()
     // Get the profile from you config file if the user has set it.
     // if the user allows override through the atom, do this:
 #ifdef HAVE_X11
-    if (QX11Info::isPlatformX11()) {
+    if (QX11Info::isPlatformX11() && !GwenviewConfig::noICC()) {
         // get the current screen...
         int screen = -1;
 
diff --git a/lib/gwenviewconfig.kcfg b/lib/gwenviewconfig.kcfg
index 417b4ad0..f991e945 100644
--- a/lib/gwenviewconfig.kcfg
+++ b/lib/gwenviewconfig.kcfg
@@ -222,6 +222,9 @@
         <entry name="ListVideos" type="Bool">
             <default>true</default>
         </entry>
+        <entry name="NoICC" type="Bool">
+            <default>false</default>
+        </entry>
     </group>
 
     <group name="Print">

Thanks for the patch! It might be a good starting point. However...

> I actually like the double-profile behaviour in about 10% of cases
> (when the images are really dark)
This seems conceptually incorrect, even if the result looks good.

Is there some programmatic way for Gwenview to check and see whether a color profile is already in use?

Yeah, I agree it is conceptually incorrect. 

> Is there some programmatic way for Gwenview to check and see whether a color profile is already in use?

My guess is as good as yours I'm afraid. Try asking oyranos devs perhaps?

One thing that is still puzzling is that if I set Lab.icc (the one with mad colours) to be the system profile, gwenview doesn't seem to apply it again in full. You can see it if you change Lab.icc back to sRGB - the image in gwenview remains a bit brighter, but not mad.

Suffering from a similar problem. In my case its DisplayCal + ArgyllCMS. JPEG are being shown correctly, but TIFF and PNG images suffer from being oversaturated. In such cases I use showFoto, but it's inconvenient. Any progress on fixing this?

I'm using colord with colord-kde and I have the same issue. Should Gwenview even use color correction on it's own? Shouldn't it be decided systemwide? I think that's the issue here.

No, it needs to stay in Gwenview, because every image could have a different color profile. Gwenview should convert it to the current monitor's profile (or just sRGB, in case none is set).

Someone familiar with Linux color management needs to check if Gwenview tries to get the current profile, or if more code needs to be added.

> Gwenview should convert it to the current monitor's profile (or just sRGB, in case none is set).

I might be misunderstanding how color management works, but is applying a profile a composable operation? If so, is it then correct to say that the job of gwenview is to apply the image profile and the job of X is to apply the system profile? If that is correct then why does gwenview need to know the system profile?

Both conversions (from image to CIEXYZ, and from CIEXYZ to monitor) are merged into a single transformation inside liblcms2. Applications don't transfer CIEXYZ data to the X11 server; they only transfer (hardware) (s)RGB.
The purpose of the CMS server is to ensure that applications that are not aware of the CMS are treated as sRGB input (usually, but that is changeable), and then the server can apply the monitor color space transformations according to its profile.

If this was a two step transformation, then you would effectively need to go from image color space to sRGB, then from sRGB to the monitor space, and this could cause loss of color data, because both the image and the monitor could be wider than sRGB.

If CMSs were a deal when X11 was invented, it would have required that applications send CIEXYZ data, instead of RGB data. But alas...

Got it. So if I understand correctly, gwenview tells the system (presumably X) "I do my own colour management, please don't modify my output", but the system is ignoring that? Perhaps the thing that modifies output here is not X, but some component that oyranos relies on? In fact, looks like there's a bug report to that extent:
https://github.com/oyranos-cms/oyranos/issues/60

Maybe worth taking a look at showFoto app? It's handling color in a correct way.

I tried to understand what's going on and this is a lot of information so I might have gotten some thing wrong.
There's a thing called "_ICC_PROFILE" X11 atom (simply a preference) that is supposed to keep monitor ICC profile in X. It can be used by any app that wants do to some color correction. If someone uses colord or oyranos then the profile in X11 was set by it and it also makes sure that all programs that keep this 
Window Property are color-corrected. So it seems to me, that maybe a window can decide if it wants to be corrected? Food for thought.
Some apps want to do their own color correction. this is from cmsprofile.cpp getMonitorProfile() function in gwenview: 

```
 static Atom icc_atom = XInternAtom(QX11Info::display(), "_ICC_PROFILE", True);

        if (XGetWindowProperty(QX11Info::display(),
                               QX11Info::appRootWindow(screen),
                               icc_atom,
                               0,
                               INT_MAX,
                               False,
                               XA_CARDINAL,
                               &type,
                               &format,
                               &nitems,
                               &bytes_after,
                               (unsigned char **) &str) == Success
                ) {
            hProfile = cmsOpenProfileFromMem((void*)str, nitems);
```

It checks if there's a profile on a window and if it is, then it is saved to hProfile and, later, used. There's problem with that logic. If there's a profile, then most likely the windows is already being color-corrected by system-wide cms. I don't know if setting global "_ICC_PROFILE" X11 atom and leaving it up to apps to actually correct themselves is a common workflow, but I doubt it.

If there's no profile, the generic sRGB one is created. In rasterimageview.cpp you can find that monitor and image profiles are merged (doesn't matter if monitor actually has any profile, bc now it uses generic sRGB)

All in all, I think gwenview should not care about monitor profile, let it be applied by system-wide cms and just apply image profile on top of it.

(In reply to Misha Aizatulin from comment #14)
> Got it. So if I understand correctly, gwenview tells the system (presumably
> X) "I do my own colour management, please don't modify my output", but the
> system is ignoring that? Perhaps the thing that modifies output here is not
> X, but some component that oyranos relies on? In fact, looks like there's a
> bug report to that extent:
> https://github.com/oyranos-cms/oyranos/issues/60

I don't thinks that's the case. I don't know exactly how color management in X works but I think Gwenview doesn't message the rest of the system about it's cms. It probably can disable cms by removing icc profile from it's window property? Never checked that, did anyone?

I think it's simply that there are two cms engines working at once lcms in Gwenview and whatever system-wide cms is put in place. They get their monitor profile from the same source so it's used two times. I don't know any way to make them know about each other

So I made a simple fix to check my theory and replaced whole getMonitorProfile() function with just return getSRgbProfile(); 
Then, I used pictures from http://www.color.org/version4html.xalter to check if image profiles are correctly applied and everything seems to be all right. System-wide cms (kolord) takes care of my monitor profile and gwenview applies image profiles.

Fix from comment 18 will make transformation a two-step process, instead of a single transformation, with the possible color loss as indicated in comment 13. If there is no way to tell the system wide CMS that the contents is already color corrected, it is probably the only possible workaround. If you prepare a patch, I suggest to propose it for review via phabricator.

I really hope Wayland is ICC ready by design, not as an afterthought.

(In reply to Christoph Feck from comment #19)
> Fix from comment 18 will make transformation a two-step process, instead of
> a single transformation, with the possible color loss as indicated in
> comment 13. If there is no way to tell the system wide CMS that the contents
> is already color corrected, it is probably the only possible workaround. If
> you prepare a patch, I suggest to propose it for review via phabricator.

I would gladly take care of this issue, but I need to know what is a proper behaviour. In comment #15 someone said that showFoto works well, but from my colord-kde experience, every time some app tries to apply color correction from monitor, it gets applied twice. Some apps, like Gimp or Darktable allow you to only set image color correction (by setting in-app monitor profile to sRGB). 

Actually ShowFoto has a problem that correction isn't applied the first time it's changed in settings so maybe that's why it looked correct at first.

It looks to me, that as long as there is system-wide correction the transformation will always be a two-step process. 

I would love if people using oyranos, colord-gtk or any other system-wide cms would check how all those apps (gimp, ShowFoto, Gwenview, Darktable, Eye of Gnome, etc) behave and if they produce correct images for them.

From my perspective the simplest fix is to make Gwenview forget that monitor ICC profile ever existed. Is there a legitimate use case that requires someone to use color correction of images, but no full screen (remembering that Gwenview is not a photo editor)?

Created attachment 127667 [details]
Make configurable whether ICC is used or not

Here's the change I use which just adds a configuration option.

So, my personal breakdown:
GIMP - works correctly, when asked to convert to working profile I refuse, keeping the integrated one;
RawTherapee - the smoothest experience, I am using it as a reference;
showFoto - works correctly, when asked to convert to monitor's profile I refuse, keeping the integrated one.
I would switch to showFoto (though I still find Gwenview more convenient), but you can't shut up this annoying question on whether you want to convert color profile or not :-|
Adding a configuration switch to Gwenview sounds nice!

(In reply to Misha Aizatulin from comment #21)
> Created attachment 127667 [details]
> Make configurable whether ICC is used or not

I should have said that I already posted my patch: https://phabricator.kde.org/D28918

It's based on Your work, but I added more comments and made the image auto-update after applying the profile (It didn't change unless it was moved or zoomed).

Git commit b76047f541340df561c2d7b91b825814923843b5 by Nate Graham, on behalf of Karol Bieńkowski.
Committed on 29/05/2020 at 16:37.
Pushed by ngraham into branch 'master'.

Make using monitor color profile optional

Summary: BUG: 401154

Reviewers: #gwenview, #vdg, ngraham

Reviewed By: #gwenview, #vdg, ngraham

Subscribers: ngraham

Tags: #gwenview

Differential Revision: https://phabricator.kde.org/D28918

M  +163  -95   app/advancedconfigpage.ui
M  +2    -0    app/configdialog.cpp
M  +4    -1    lib/cms/cmsprofile.cpp
M  +6    -0    lib/documentview/rasterimageview.cpp
M  +1    -0    lib/documentview/rasterimageview.h
M  +1    -0    lib/documentview/rasterimageviewadapter.cpp
M  +4    -0    lib/gwenviewconfig.kcfg

https://invent.kde.org/graphics/gwenview/commit/b76047f541340df561c2d7b91b825814923843b5