Bug 359909 - Color rendering intent should be user-selectable, not hardcoded to Perceptual
Summary: Color rendering intent should be user-selectable, not hardcoded to Perceptual
Status: RESOLVED FIXED
Alias: None
Product: gwenview
Classification: Applications
Component: general (show other bugs)
Version: Other (add details in bug description)
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: ---
Assignee: Gwenview Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-29 01:00 UTC by DrSlony
Modified: 2018-01-30 23:57 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 18.04


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description DrSlony 2016-02-29 01:00:27 UTC
I am happy to see that Gwenview uses my monitor's color profile, but unfortunately it appears to be using the perceptual rendering intent and I see no way of changing that. Please add an option to change the rendering intent, or at the very least hard-code it to use relative colorimetric, not perceptual!

To help you test the change, you can download my monitor color profile here, it includes gamut mappings for perceptual and saturation intents.
http://filebin.net/211bsvrbjo

Reproducible: Always
Comment 1 DrSlony 2016-02-29 01:00:54 UTC
Oh, version 15.12.1.
Comment 2 DrSlony 2016-12-19 20:34:18 UTC
Hello

I wrote below that Gwenview used my monitor color profile. I have no idea how it did that, and now I'm on a new system and don't know how to get it do use my monitor color profile (ICC file). How do I do that, and has the rendering intent bug (because its a bug to use perceptual by default) been fixed since then?
Comment 3 DrSlony 2016-12-19 21:54:50 UTC
Patch to fix default rendering intent from perceptual to relative colorimetric:

diff --git a/lib/documentview/rasterimageview.cpp b/lib/documentview/rasterimageview.cpp
index 02e18a6..95f7245 100644
--- a/lib/documentview/rasterimageview.cpp
+++ b/lib/documentview/rasterimageview.cpp
@@ -108,7 +108,7 @@ struct RasterImageViewPrivate
         }
         mDisplayTransform = cmsCreateTransform(profile->handle(), cmsFormat,
                                                monitorProfile->handle(), cmsFormat,
-                                               INTENT_PERCEPTUAL, cmsFLAGS_BLACKPOINTCOMPENSATION);
+                                               INTENT_RELATIVE_COLORIMETRIC, cmsFLAGS_BLACKPOINTCOMPENSATION);
         mApplyDisplayTransform = true;
     }
Comment 4 Jan Kundrát 2016-12-20 10:37:36 UTC
TL;DR: Could you please explain why gwenview should use relative colorimetric rendering intent?

I understand the reason for providing an option for the rendering intent selection.

What I do not understand is your reasoning to use relative colorimetric rendering intent by default for all users. I should probably add that I had to look up the definitions of these two on a random website and I won't pretend that I know much about colorimetry. (Stuff at http://www.cambridgeincolour.com/tutorials/color-space-conversion.htm looks usable.)

As a random data point, I asked what Darktable (my favorite RAW photo editor) uses. It seems that by default they do absolute colorimetric for display. When a setting to use LCMS is enabled, it defaults to perceptual.
Comment 5 DrSlony 2016-12-20 15:57:47 UTC
"TL;DR"? What an attitude!

"Could you please explain why gwenview should use relative colorimetric rendering intent?"
And run the risk of it being TL and you doing DR? To make it short, perceptual screws the colors up, all of them. Not all people will experience this because not all ICC profiles support perceptual. You'd only want perceptual's complete gamut compression in rare cases.
Comment 6 Jan Kundrát 2016-12-20 16:57:30 UTC
(In reply to DrSlony from comment #5)
> "TL;DR"? What an attitude!

That was meant as a TL;DR summary of my comment for your convenience. It was not a TL;DR reaction to the bugreport of course (note the colon). Apologies for that ambiguity.

> perceptual screws the colors up, all of them. Not all people will experience
> this because not all ICC profiles support perceptual.

Yes, that's true. However, a similar argument can me made against relative colorimetric rendering intent and in favor of perceptual: "relative colorimetric rendering intent hides fine detail".

> You'd only want perceptual's complete gamut compression in rare cases.

The resources I was able to find suggest that there are reasons for both relative colorimetric and perceptual when it comes to displaying photos (which, I believe, is the main use case for Gwenview). I'm all for a patch which makes this user-configurable for those who care, and I'm also interested in a discussion about which one is better by default for photographs.
Comment 7 DrSlony 2016-12-20 17:44:11 UTC
Then it was a big misunderstanding, apologies for me being upset. I will reply with the proper reply you deserve soon, I have to tend to some cooking first.
Comment 8 DrSlony 2016-12-20 19:56:39 UTC
I agree that a user-selectable intent would be best, but I am not a Qt programmer and to do this properly involves much more than just adding a combo. To not mislead users, I believe this combo should only show the intents which the source-target profiles actually support (can be done using LCMS2). As I can't code that, the next best thing is to use a default intent which does not distort colors for those people who have a profile which is capable of using the perceptual intent. Longer explanation follows.




Most consumer screens are not capable of fully reproducing even the sRGB gamut. For example my high end laptop can only reproduce 75% of the sRGB gamut - it cannot show the extreme reds of sRGB. Despite this, I still want reds which it can show to be correctly red, and blues which it can show to be correctly blue.

The colorimetric intents perfectly preserve colors of the source gamut which do fit within the target gamut, and those colors which are too red or too blue to be shown by the screen will be rounded off to the nearest red or blue that can be shown - colors which fall outside the target gamut will be rounded off nearest reproducible color instead.

The perceptual intent will shrink the source gamut to fit inside the target gamut, in one of several ways, by changing all colors. How much it changes the hues depends on how much it needs to shrink the gamut. That is what makes perceptual a very bad choice as a default in an image viewer - what most people want in an image viewer is to see photos with accurate colors, and perceptual will change all of them. For example the reds could turn more pink, deep blue will turn purple, etc. This is useful for example if you want to adjust saturation of colors which your screen cannot reproduce - it allows you to "see" them, even though their hues are wrong, but at that moment you don't care about the hue, you just want to not over-saturate. When viewing a normal photo though you want relative colorimetric.

All rendering intents are not supported by all profiles. Matrix profiles only support the colorimetric intents. LUT profiles support perceptual intents, but it's up to the program making the LUT profile whether one will be enabled by default, and how it will work. Keep this in mind, as some people could say that perceptual looks fine, because they don't know their profile does not even support it and what they're actually seeing is relative colorimetric. This is the leading cause of why you haven't had more bug reports about this - most people are oblivious about color management and don't have ICC profiles for their monitors, of those who do only some will have LUT profiles, and of those who have LUT profiles only some will have a perceptual mapping table in their profile.

When it comes to showing an image on screen, not for the purpose of matching it to a print but jus to enjoy it, which is what Gwenview's main task is, relative colorimetric is the right choice for everyone who just wants to see the photo correctly without shifted hues. For those few of us who have (and need) a profile which supports both perceptual and relative colorimetric, having Gwenview force perceptual shoots us in the foot.
Comment 9 Nate Graham 2017-11-09 19:31:26 UTC
What you say makes a lot of sense, DrSlony. We should consider exposing this as a user setting somewhere, and I can work on that, but for now you've convinced me that relative should be the default. Please feel free to submit your patch to phabricator.kde.org, and we can discuss it more there.

Please put "BUG: 359909" on its own line in the summary section when you make the revision there.
Comment 10 DrSlony 2017-11-10 07:49:31 UTC
Hi Nate

I tried to login or register at https://phabricator.kde.org/ but failed, it says the username and password are invalid, but how can they be invalid if I'm trying to register...
https://filebin.net/028sa1gtyam07hws/imgur-2017_11_10-08_47_34.png

I tried new credentials, as well as these bugzilla credentials.

And this page says, "Registration Failed, There are no configured default registration providers.".
https://secure.phabricator.com/auth/register/

Could you commit the patch?
Comment 11 null 2017-11-10 09:38:33 UTC
> I tried to login or register at https://phabricator.kde.org/ but failed,
> it says the username and password are invalid, but how can they be invalid
> if I'm trying to register...
Yeah, sorry for this experience. That's pretty bad and might deter potential contributors. It should just redirect you to https://identity.kde.org/index.php?r=registration/index. Please go there to register your account, then come back to Phab. See also https://community.kde.org/Infrastructure/Phabricator.

On Phab we'll hopefully find someone with knowledge about color profiles to review the patch (I cannot really comment whether the patch is right or not without looking deeper into the topic).

> bugzilla credentials.
Sadly, bugzilla still uses separate credentials from the rest of all of KDE's infrastructure.

> https://secure.phabricator.com/auth/register/
That's just where Phabricator (the software itself) is developed, KDE only runs the software produced by them on KDE's infrastructure. There is no roaming of user accounts configured.
Comment 12 DrSlony 2017-11-11 16:59:46 UTC
I registered at Phabricator and I found my way to the code in question:
https://phabricator.kde.org/source/gwenview/browse/master/lib/documentview/rasterimageview.cpp;b55420b2ac3dc72ebffdb89dbb9e662d64950ecd$111

Now how do I do what it is that you want me to do?
Comment 13 null 2017-11-11 17:07:08 UTC
That's great :) In Phab, now click on the star on top, then on "Create Review Request". Paste the diff from Comment 3 and select Gwenview as the repo.

(BTW, as you may have noticed, sysadmin promptly fixed the login page to include at least a reference to identity.kde.org)
Comment 14 DrSlony 2017-11-11 17:34:27 UTC
Done, https://phabricator.kde.org/D8763
Comment 15 DrSlony 2017-12-03 13:29:24 UTC
Status?
Comment 16 Nate Graham 2017-12-03 16:19:09 UTC
The patch is undergoing review, but it seems that not many of us have much of a background in color profiles, so it could take a while.
Comment 17 DrSlony 2017-12-03 16:25:26 UTC
The red tape here seriously discourages further contribution to Gwenview. The patch is banal, the concept is simple, and Gwenview's current behavior breaks colors.

We will soon be celebrating the patch's first anniversary.
Comment 18 Nate Graham 2018-01-29 22:43:15 UTC
Git commit 338ffeee4deea6c9bf8d240593b5bc375ae50dd7 by Nathaniel Graham.
Committed on 29/01/2018 at 22:42.
Pushed by ngraham into branch 'master'.

Add a user-facing control to choose the ICC color rendering intent

Summary:
FIXED-IN: 18.04

Allow the user to choose the ICC color rendering intent, instead of hardcoding INTENT_PERCEPTUAL

Supersedes D8763

Test Plan:
The GUI control appears and seems to work as intended.

{F5680320}

- Tested that the default value of Perceptual is used when there is no value in ~/.config/gwenviewrc
- Tested that the value gets set in ~/.config/gwenviewrc
- Tested that removing the value in ~/.config/gwenviewrc reverts the GUI setting to Perceptual
- Tested that toggling the setting back and forth actually has an impact when using a display with an active color profile. Here's an example:

Perceptual:
{F5677933}

Relative Colorimetric:
{F5677990}

Reviewers: DrSlony, rkflx

Reviewed By: rkflx

Subscribers: muhlenpfordt, rempt, behrmann

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

M  +96   -3    app/advancedconfigpage.ui
M  +8    -0    app/configdialog.cpp
M  +1    -0    app/configdialog.h
M  +12   -1    lib/documentview/rasterimageview.cpp
M  +15   -0    lib/gwenviewconfig.kcfg
A  +47   -0    lib/renderingintent.h     [License: GPL]

https://commits.kde.org/gwenview/338ffeee4deea6c9bf8d240593b5bc375ae50dd7
Comment 19 null 2018-01-30 23:57:16 UTC
Git commit 899423ad41611b94848ed8fbdc877bd49f9cd408 by Henrik Fehlauer.
Committed on 30/01/2018 at 23:57.
Pushed by rkflx into branch 'master'.

Update view after changing rendering intent config

Summary:
This is a follow-up patch to D10076, so after changing the rendering
intent in Gwenview's config dialog the displayed image is updated
immediately after clicking on {nav OK} or {nav Apply}. Before, it was
necessary to {nav Reload} the image manually or pan around.

Currently this is only implemented for raster images. In case someone
figures out colour management for SVGs, this patch should be extended to
SVGs too.

Depends on {D10076}.

Test Plan:
Set up an ICC profile as described in D10076#197598 and open an
out-of-gamut image. Change the rendering intent in Gwenview's config
dialog, click {nav Apply} and observe that the image's colours change
immediately. Saving to the config file still works.

Reviewers: ngraham, muhlenpfordt

Reviewed By: ngraham

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

M  +11   -14   lib/documentview/rasterimageview.cpp
M  +2    -0    lib/documentview/rasterimageview.h
M  +1    -0    lib/documentview/rasterimageviewadapter.cpp
M  +4    -8    lib/renderingintent.h

https://commits.kde.org/gwenview/899423ad41611b94848ed8fbdc877bd49f9cd408