FEATURE: 359909
FIXED-IN: 18.04
Allow the user to choose the ICC color rendering intent, instead of hardcoding INTENT_PERCEPTUAL
Supersedes D8763
FEATURE: 359909
FIXED-IN: 18.04
Allow the user to choose the ICC color rendering intent, instead of hardcoding INTENT_PERCEPTUAL
Supersedes D8763
The GUI control appears and seems to work as intended.
Perceptual:
Relative Colorimetric:
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
Great to see this!
Could not test that the setting makes a visual difference because none of my images changed appearance when I changed the intent
As I explained in the original issue in Bugzilla and in the issue I was asked to open here in Phabricator, to see the difference you need to use a monitor profile which supports perceptual mapping (not common) and an image whose colors lie outside your monitor's gamut (very common). I provided a sample ICC file with perceptual mapping tables in the original issues.
The label is currently "Color rendering". The technical and universal term is "Rendering intent".
The new config option works for me, but the "Advanced Config" dialog has a scrollbar, though there are no more items below. While resizing, the "Color rendering" part moves down.
Sorry, can't say anything regarding the color profiles.
@DrSlony Yes, I chose "Color rendering" deliberately, to make it sound a bit less technical and provide a hint about what it does. But if that's flat-out wrong, I can use "Rendering intent", though I'm not a huge fan of the term.
I'll try again later today with the ICC profile you provided in the other Phabricator revision.
@muhlenpfordt, sounds like an artifact of the layout hacks I needed to apply to get thenew option to appear in the correct lcoation. Gwenview's config code is really squirrely. I'll see if I can improve the situation.
I think this is one of those cases where appearing to be more user-friendly is in fact having the opposite effect. Everyone familiar with what this does is familiar with the correct term, while "color rendering" is vague at best.
OK, "Rendering intent" it is. I guess it's not so bad since this is hidden away in the Advanced page anyway.
Thanks Nate, appreciate your patch. I'll try to find some time to review once you tested with the ICC profile and looked at the scrollbar. (Probably related: The config option shouldn't move when resizing the config dialog vertically.)
Just one comment for now: The new option will surely have some users scratch their head what this is all about, so maybe a short explanation in the docbook and/or "What's this" would be helpful ("What does this do?", "What's the difference between both options?").
OK, I figured out how to apply the profile using kcm_colord (installed from the colord-kde package) and I still can't see any difference between relative and perceptual for any of my photos (even using a version of Gwenview that hardcodes the relative profile, to rule out the possibility that my patch isn't working). Is this not the right way to apply a profile?
Not being a professional photographer or a color expert, I'm kind of flying blind here.
I am doing all of this in a VM, so maybe that's the issue?
Ideally, a program should let the user choose an ICC in one of two ways:
Currently Gwenview only supports the first option.
Auto-detection is based on checking whether the _ICC_PROFILE X11 atom is set. You can use Argyll's dispwin (or argyll-dispwin) or DisplayCAL's displaycal-apply-profiles to set the atom to point to an ICC file, and to load the calibration curves contained in the ICC file into your graphic card's gamma table. Normally users wouldn't have to worry about that, since they would use DisplayCAL to create the ICC profile using their colorimeter, and DisplayCAL sets the atom and loads the calibration curves into the VCGT automatically.
Ah, yes... That's the old way of getting the profile. For krita, I moved to checking colord.
I'm told the X11 atom doesn't/won't work in Wayland, so polling colord would be good, and it's always good to have the fool-proof way of being able to point to the ICC file manually and explicitly.
Holy shamoley, I'm sort of amazed that you don't jump ship to macOS given the crazy state of profile handling here and how good it is there. We definitely need to do some work to improve this situation. I'll add it onto my long-term strategy list, because we'll need better profile handling to attract more professional artists.
I guess Gwenview should also move to colord. Something else for the priority list... That probably explains why using colord didn't work. I'll try the X11 approach tonight and see if that works.
You should be able to nick the colord code from Krita with lots of ease, since I made it into a separate little library :-)
Oh that would be lovely. Should we maybe put that library into a framework so it doesn't have to be duplicated?
@ngraham jumping ship is not in my nature - filing bug reports and enhancement requests to improve the situation is ;)
OK, so I did this:
$ dispwin -v ~/profile.icc About to open dispwin object on the display About to set display to given calibration Calibration set About to destroy dispwin object
But then I get this:
$ displaycal-apply-profiles (displaycal-apply-profiles:8362): Gtk-WARNING **: Unable to locate theme engine in module_path: "adwaita", (displaycal-apply-profiles:8362): Gtk-WARNING **: Unable to locate theme engine in module_path: "adwaita", ================================================================================ Loading calibration curves of current display device profile... /usr/bin Argyll CMS 1.8.3 /usr/lib/python2.7/dist-packages/DisplayCAL/ICCProfile.py:443: Warning: There is no assigned profile for /org/freedesktop/ColorManager/devices/xrandr_VBOX_monitor_64358212_dev2_1001 -------------------------------------------------------------------------------- Command line: dispwin -v -d1 -L About to open dispwin object on the display About to set display to given calibration Calibration set About to destroy dispwin object
Is that okay?
For correct color management, the calibration curves must be loaded into the VCGT and the color-managed programs must find and use the ICC profile. If either is missing, or are incompatible, colors won't be correct.
I let DisplayCal install the ICC + calibration curves for me, and DisplayCal uses this command:
argyll-dispwin -v -d1 -c -I /usr/share/color/icc/monitor.icc
I'm not too familiar with doing this through CLI, but based on the documentation* it seems that the -I (install) option is responsible for setting the X11 atom. Running dispwin without -I only loads the calibration curves from the ICC file into the VCGT, so while your whole screen will look different, it does not mean that the X11 atom has been set, and if the atom has not been set then programs which support color-management will not find the ICC file and so will not show correct colors (unless they allow the user to point to the ICC file manually - that's why I asked for this feature) .
OK, the -I argument seemed to do something:
$ dispwin -v -I ~/profile.icc About to open dispwin object on the display About to install '/home/dev2/profile.icc' as display's default profile Installed '/home/dev2/profile.icc' and made it the default About to destroy dispwin object
But then displaycal-apply-profiles still says " Warning: There is no assigned profile for /org/freedesktop/ColorManager/devices/xrandr_VBOX_monitor_64358212_dev2_1001", which seems worrying:
$ displaycal-apply-profiles (displaycal-apply-profiles:8759): Gtk-WARNING **: Unable to locate theme engine in module_path: "adwaita", (displaycal-apply-profiles:8759): Gtk-WARNING **: Unable to locate theme engine in module_path: "adwaita", ================================================================================ Loading calibration curves of current display device profile... /usr/bin Argyll CMS 1.8.3 /usr/lib/python2.7/dist-packages/DisplayCAL/ICCProfile.py:443: Warning: There is no assigned profile for /org/freedesktop/ColorManager/devices/xrandr_VBOX_monitor_64358212_dev2_1001 -------------------------------------------------------------------------------- Command line: dispwin -v -d1 -L About to open dispwin object on the display About to set display to given calibration Calibration set About to destroy dispwin object
OK, gave this a whirl on my bare metal machine, just in case my VM development environment was screwing things up, and I got a reproducible test case. Looks like my patch needs some work.
No major issues found after testing and looking at the code, just some niggles.
Do you think it would be more user-friendly if the changes were applied immediately after confirming the config dialog? Currently you have to Reload the image or pan around.
We definitely need to do some work to improve this situation.
FWIW, for me kcm_colord was enough to get the ICC profile working in a VM. The UI is a bit unintuitive, though, because you have to click on Add profile twice and the Apply button is broken.
Nevertheless, we also have Bug 214856 (kio/thumbnail: icc color profiles for image preview), for which I was just about to file a duplicate by accident, haha.
app/advancedconfigpage.ui | ||
---|---|---|
154 | Not sure what the HIG says, but at least the options above end the explanatory sentences with a full stop. Apart from that, they are quite good! | |
162 | This results in app/advancedconfigpage.ui: Warning: The name 'verticalSpacer' (QSpacerItem) is already in use, defaulting to 'verticalSpacer1'. In general we should try to avoid such warnings ;) | |
182 | The term I've seen used most often is "Relative Colorimetric". | |
lib/documentview/rasterimageview.cpp | ||
111 | int → cmsUInt32Number This isn't a static, so you have to initialize it with = 0. (I know it's handled by the else eventually, but in the future someone might miss this assumption. Better to always initialize things in code not critical to performance.) | |
112 | Do you think a switch would be more readable here? At least I'd prefer it, and it'll force us to properly handle all (future) enum values, too. |
app/advancedconfigpage.ui | ||
---|---|---|
154 | Thanks! I deliberately left off the period. Seemed a bit short for that. The text above has a period because it's written as an actual full sentence. |
Yes, ideally the new rendering intent would immediately take effect upon changing the setting, but I looked into it a bit and was not able to come up with a solution that was within my skills and understanding of Gwenview's codebase. If you want to block the patch on getting that in, I'd appreciate a helping hand. Otherwise, what do you say to landing it without support for instantly applying a new rendering intent, and work on implementing that in another patch? Thanks Henrik!
Some final notes:
That's great information, @DrSlony, but it's also way too long and technical to put inside the config window (it would become a wall of text and people would skip over it). I'd really like to avoid making the inline explanatory text labels much longer than they are now. Would you be able to craft two more appropriate text strings that can each fit within about 110 characters and incorporates the details you feel are important? Thanks!
Nearly perfect from my side, thanks again Nate. Accepting for now, but let me know in case you really want to change the explanatory text again (I don't). IMO the more educational aspects are better suited for the docbook or even external resources (see also digiKam's docs).
Uh oh, I feared it would come to that ;) Implementing missing pieces myself for every patch I review isn't really sustainable in the long term, though. I'm not familiar with the code either, but looked into it a bit. I'll post a patch shortly, basically replicating parts of the code for Image View → Transparent background, which seems to take effect as soon as you hit Apply.
lib/documentview/rasterimageview.cpp | ||
---|---|---|
112 | Coding style: Add space after switch keyword. | |
lib/renderingintent.h | ||
41 | Superfluous comma. |
Thanks so much Henrik! I really wasn't expecting you to do the work; I had every intention of doing it later, honest! But I do appreciate the follow-up patch. It's always good to learn from the master!
No worries. First I spent some time researching and then I even started writing some tips for you. But in the end I thought "What the heck, let's just add a commit message and publish the Diff, so I can finally commit something myself instead of only doing review after review".