Jellby wants to merge 5 commits from /u/jellby/mcomix/ to master, 7 hours ago
This implements the suggestion in #137. Unfortunately, I don't know how to do it with GdkPixbuf, so when the preference is enabled I switch the priority to PIL. Ugly but effective: it works nicely for me.
Thank you for your MR, I will have a look into it.
I am sorry for the delay. Your MR looks like a good starting point for proper color management in MComix. You seem knowledgeable when it comes to Pillow and color management in general, so it would be great if you could help us here.
I tested your MR a bit and, apparently, toggling the checkbox does not seem to work. Normally, when changing a setting in the prefs, the change is applied immediately. (To work around this issue with the current MR, re-caching images appears to be the minimum necessary for the change to be applied.) I think this is because the conversion happens in
load_pixbuf. Could you please try and move the conversion step to somewhere later so the conversion can be applied even after the images have been loaded and cached? If this seems impossible or unreasonable, you might find another (or even better!) way to fix it.For some reason, when conversion is enabled, any AVIF file becomes undisplayable. Not sure whether it's only on my system (Arch Linux), though.
The conversion also has the side effect of un-animating any animated image like GIF. Similar issues are already known which is why MComix does not transform animated images if animation is enabled in the prefs. Please have a look at
main.py:393andlens.py:148(current master) as examples of howis_animationinimage_tools.pyis used to determine whether an image is considered animated.Thanks for the feedback. Unfortunately, no, I'm not knowledgeable in either Pillow or color management... I mean, I know enough to figure out that some CMYK images are displayed wrong, track it down to images with embedded profiles and find a way to apply/use the profile "on the fly" without re-encoding the image. But I still don't understand color profiles and their quirks. I'm not even sure "convert to sRGB" is right, since sRGB is a colorspace and not a profile. I'm quite lost here. Still, for my problem images applying this hack is better than nothing.
Right, the checkbox does not work "live", you have to refresh to have it take effect. On the other hand, if you assign a keyboard shortcut it will refresh automatically. I can try to make it refresh when (un)checking the preference too. But I don't think I can do the correction later, I think it must be done before converting PIL.Image to GdkPixbuf.Pixbuf, since the profile is available in the former and not the latter.
The other problem regarding AVIF, is it maybe due to changing the provider priority? As I mentioned, I had to change it to try PIL first, otherwise the images are loaded directly with GdkPixbuf and color profile is not available (as far as I can see). I'll try to find a solution/workaround, and for the animations too.
Last edit: Jellby 6 days ago
Is it better now? Animations should now work as before (i.e. profile conversion is not available for animations), and toggling the preference should refresh all images. AVIF images do not work for me (Kubuntu 24.04) no matter what, so I cannot test that. Although they work with Gwenview and ImageMagick, so maybe it's just some Python module.
I'm having a look at it. By the way, the current MR still points to the previous commit. Not sure whether you can update the MR, but you probably should (if you can, that is).
Under those circumstances, your code looks quite impressive!
I see.
I am not an expert on that, either, more like an intermediate, but if we combine our understandings, we might be able to get that thing right enough.
Crash course (to the best of my knowledge and with simplifications here and there): Profiles, for the most part, describe a color space by means of some profile connection space (PCS), usually XYZ. As in, "the most luminous reddest red in the color space described here has the XYZ coordinates so-and-so". The most luminous reddest red of one monitor might look totally different (read: has totally different XYZ coordinates) from the reddest red of another. And by "look" I mean according to what a colorimeter (a device that is, essentially, an "objective human eye") has measured. When you profile a monitor, you measure not just the "corners" but many, many points in the color space of the monitor in question because the monitor might have some nonlinearities/surprises here and there. (Think of it as if someone took a hammer to make dents in what would otherwise be a nicely defined, three-dimensional grid.) Profiles can also contain metadata and VCGT data and whatnot, so there are many ways to create essentially broken profiles. On the other hand, there are standardized color spaces like sRGB where the most luminous reddest red has a certain XYZ coordinate by definition. So, for this to work in practice, a CMS can create a synthetic profile on the fly (or has them predefined and cached somewhere), and that synthetic profile describes how the sRGB color space looks like, for example. Thanks to relatively simple definitions, such a standardized color space doesn't have unexpected dents like real devices so a profile that describes it is comparatively small. Most users are even lazier: They do not include a profile at all in their images. For these cases (which is, like, most of the media out there), software like mpv uses heuristics. For example, if the spatial dimensions of a video are 1920x1080, it matches the definition made in BT.709, so the correct color space is probably also defined in BT.709. Using this educated guess, mpv creates a synthetic profile that describes the same color space as defined in BT.709, and uses it as if it had been embedded in the video. Now, with a profile of the input (device, video or image) and a profile of the output device (e. g., a monitor) and a common PCS like XYZ, color (as essentially defined in standards like CIE 1931) can be translated from the input to XYZ and then from XYZ to the output. But wait, what if the user doesn't have a profile for his monitor (good profiles need good colorimeters, which are expensive, after all!), you might ask. Good question! In this case, we can only use heuristics, again. Easiest way out: Since many images still use mere sRGB, just assume that the output device is also perfectly calibrated to match sRGB. (This is not true in many real cases, by the way, as many devices sold today with "better" color reproduction are closer to DCI-P3 or Display P3, for example, and many cheaper devices are only somewhat similar to sRGB but have terribly yellowish red.) This way, color translation is trivial. As in, the user doesn't have a monitor profile, so he probably doesn't care, so we don't care, either! But if we encounter images with radically different definitions like CMYK-based ones, we can now at least do something to get the colors a bit closer to what was intended by the artist. Speaking of intent, the "rendering intent" loosely describes how to bias the color mapping so you can keep what you want from a color that is outside of what your output device can handle ("out of gamut"), like trading saturation for correct hue etc. (You currently use magic constants for that but it would be better to have a drop-down menu for it as well.)
tl;dr We need a drop-down menu instead of a check box, and we most certainly need a new pref tab labeled "Color Management" where said drop-down menus should appear. More on that later, but if you want, you can already help here by temporarily moving the new option to such a new tab.
Definitely. I have a few CMYK-based images myself so I know exactly what you mean.
Yes, thanks. I think the way you handle this particular issue in the currently most-recent draft is probably good enough, even AVIF works again.
Thanks for the info. I would like to help you here but for me, it just works and I can't name a package that you might need for AVIF to work.
By the way, for experimentation, testing and learning about color management in practice, you might want to have a look at the
colordpackage which comes with files likeSwappedRedAndGreen.iccso when you use it in mpv, you can enjoy acid trips for free!Last edit: Ark 6 days ago
Thanks for the crash course. The intent, and to some extent the math, is relatively clear, but when it comes to when and how each profile should (or actually is) taken into account, I have the impression it's the wild west, and each program/library does whatever.
I have put the preference in a new tab. For what it's worth, Gwenview has, as far as I can see, only two related settings: enable/disable color management, and select rendering intent (perceptual/relative colorimetric).
I've also added a trial fix for an annoyance I've been having, where the window geometry fails to be restored apparently at random. If this is not OK I can remove it or create a separate MR.
Last edit: Jellby 6 days ago
I've figured out how to read an embedded profile from a pixbuf, so I'll try to rewrite this.
So we are on the same page, which is good.
Also on the same page, unfortunately. :-(
Thanks. My experience tells me that there are several different aspects of real-world scenarios to consider:
So, in order to cover many use cases without user interaction reasonably well, there should be several settings with reasonable defaults. I try to conceptualize how the content of the "Color Management" tab could look like to achieve that.
Without having checked the code, I guess that they either determine the monitor's profile (with sRGB as a fallback) and map everything to its color space (assuming sRGB where there is no input profile) or leave everything to the Wayland compositor (color management is a thing there since relatively recently, after all). Perceptual and relative colorimetric are the most reasonable choices as well. Looks like a good compromise if you want to keep things simple.
So, I opened a CMYK file in Gwenview and, as it turns out, the advertised conversion to sRGB only seems to work for RGB files. That being said, mpv only supports RGB profiles, and there is at least one CMYK file where GIMP segfaults when I try to open it. In other words, MComix is (thanks to your contribution!) currently the only application I know of where I can view certain image files in a meaningful way close to the artistic intent.
Is that bug related to this MR? Did that behavior occur before any of the changes in this MR?
Great, looking forward to it!
I'm afraid it won't work. Yes, I can read a profile from a pixbuf created directly from file, and yes, I can also make sure that the profile is preserved when converting a PIL image to a pixbuf. But GdkPixbuf does not support CMYK, it only supports RGB (https://docs.gtk.org/gdk-pixbuf/property.Pixbuf.colorspace.html), and a JPG (or anything else, I assume) is probably converted to RGB (ignoring any profile) when read, so that the embedded CMYK profile is now quite useless. It may work for RGB images, but I'd rather not have separate code paths for CMYK and RGB images. It might also be possible to create some pseudo-profile transformation that simulates the effect of a proper profile conversion when applied to a pre-converted-to-RGB image, but that would never be the same, and I don't think it's worth it.
So, I think we'll have to live with the hack of changing the priority of the two providers. Or maybe PIL should always be the default provider. And given that image enhancements are applied by converting pixbuf to PIL and back to pixbuf, maybe the conversion to pixbuf could be done later?
Last edit: Jellby 4 days ago
As far as I can see it's unrelated to this MR and it did occur before the changes (but I didn't experience it as much as now that I've been opening/closing MComix multiple times...) The change is so small (basically just resizing before
show()) that I thought it would be OK to just include it in this MR.No problem, and thank you for trying.
I would not mind using PIL first, given that it seems more powerful in so many ways. The only real downside I see is how (currently) the conversion to Pixbuf takes place, but that's due to a completely unrelated issue. (The issue is, instead of only converting what is actually needed for the current viewport, the entire image is first converted to a Pixbuf, if necessary, and then, the entire Pixbuf is rendered and kept in memory using the final dimensions after scaling. This becomes more and more an issue, not only memory-wise. Avoiding the PIL detour wherever possible hopefully helps a bit regarding memory. A long time ago, I rewrote the core of
lens.pyin a way that only the relevant portions of an image are processed but I didn't find the time to generalize it for the entire viewport.)By the way, another related question is how to overcome the limitations regarding animated images. Currently, all animated images are loaded using Pixbuf's capabilities only. Just the other day, I stumbled on something that might help solving that issue: https://pillow.readthedocs.io/en/stable/reference/ImageSequence.html#extracting-frames-from-an-animation Thus, if you find a way to load animated images in PIL directly, not only would color management and more become available for animations, but we could finally get rid of one of the two main reasons why the Pixbuf stuff is more or less preferred for loading images. (The other reason is the memory issue explained above.)
With that out of the way, PIL would be, at least feature-wise, clearly the preferred way to load and process images. Also, probably, fewer hoops to jump through in the image loading code (think your newly-introduced
IMAGEIO_PIL_NOANIM). Or higher compatibility with more or broken images. That is, if an image can only be loaded via Pixbuf, stuff like color profiles might not be available, but at least there would be an image at all.Anyway, what I want to say is that, if animation could be loaded using PIL only, images could be stored in-memory using PIL stuff only, while Pixbuf stuff would only be necessary to interface with GTK. Like, everything could be so much simpler, color management included!
Okay, then please move that part to a separate MR. Also, you might want to update the
ChangeLog.mdin the new MR if you think your change is worth mentioning.For the record (in case this becomes useful in the future), this is how I could read the profile:
and to preserve the profile when coverting a PIL
imageto apixbuf:At least animations can be detected (
image.is_animated), and the separate frames can be read (image.seek()). I don't know if that's enough, one would at least also need to know the delay, and maybe something else.Done. Should I add more changes here? Should I squash commits?
Thanks for your animation experiments. According to https://github.com/python-pillow/Pillow/commit/f84684931d3e3ec0c9f6e1ff263fe7e51ee24013 , if I understand it correctly, a frame has a dict-like member
infowith maybe a keyduration. Maybe that helps understanding how the animation stuff in PIL works.If you feel like improving the image loading algorithm of MComix such that PIL can be used as an additional provider for animated images. that would be great! Also, it should probably go to its own MR. I think that for now, it is okay to convert the PIL-provided animation to Pixbuf right after loading if doing so is easier to implement.
Please try to remove the commits regarding window geometry as they are essentially covered in the other MR I have merged just recently.
More on the current MR here will follow in another post.
Motivated by the scenarios described earlier, I would like to specify how I think color management should be controlled by the user. Let me first define some hypothetical GUI elements just so we can express this concisely. (How they will be implemented exactly is not important at this point.)
Now for the settings and their defaults with (hopefully) obvious semantics:
"Default output profile"
"If embedded profile is RGB"
"If embedded profile is CMYK"
"If embedded profile is of other type"
"If embedded profile is unavailable"
Each of the four cases above should also have the following four options as well:
The defaults presented here are chosen based in the idea that there are two kinds of users. Users who don't care too much about colors probably also do not have a valid profile for their monitor installed, so everything will get converted to sRGB, including CMYK. Whether the colors are actually sRGB or not (e.g. oversaturated, probably) is not important because the users don't care too much anyway. On the other hand, users who do care about colors know what color management is, probably have a display profile installed and look up the prefs either spontaneously or when they notice that the colors are unexpectedly off (for example because some CMYK images look different from other images that should look the same but don't because they are not CMYK but actually sRGB). The defaults chosen are supposed to cover most cases for most users, color enthusiasts or not.
Given the many scenarios I described earlier, I think that being merely able to enable or disable sRGB conversion is not sufficient. Imagine some user with a profiled Adobe RGB display, maybe an artist. CMYK images could be displayed nearly perfectly on such a device but no matter what the user does, it's going to be bad. If the box is unchecked, the colors are virtually useless. If the box is checked, the resulting image on the screen is, strictly speaking, not in sRGB but in some over-saturated sRGB-to-Adobe-RGB-stretched color space because the display profile is not taken into account, which is bad as well. And even if the profile is taken into account, having everything converted to actual sRGB on the screen is going to suck as well because then, CMYK images and even the images created by the artist herself in Adobe RGB will be converted to sRGB which will completely take the fun out of the picture. Literally.
Some manual switching might always be necessary, but I think it can be kept close to a minimum. The idea is to introduce something that could be called a color management "preset" that represents a combination of all the color management settings. For example, the defaults presented above could be called the "Default" preset. A preset "sRGB" could be used when everything needs to be converted to sRGB, and so on. A current preset that is not saved might be called "(current)". The user can add and remove her own presets and name them appropriately (like "Photography", "Artistic", "Print", "Monitor 1", "Monitor 2", "Projector", etc.). Then, when viewing images, the user can switch between presets using something like a drop-down list in the toolbar or keyboard shortcuts.
And there is where I think a shortcut like the one you proposed (or at least made provision for) comes into play. However, instead of toggling only one setting between two states, two shortcuts would allow iterating over all possible presets.
I am aware of the fact that this is more difficult than a checkbox hack, but at the end, it might be worth it. It would make viewing different images of very different kinds a pleasant experience. Also, not everything needs to be implemented at once. For example, the whole preset stuff can be added later.
What do you think? Any ideas, comments, suggestions?
By the way, I just checked and, apparently, Firefox also does not (yet) convert CMYK to RGB.
Another issue that just came to my mind: How to apply color management to PDF files? Maybe MuPDF has some options for that. Needs checking, but that's for later.
It sounds reasonable, The main drawback from my perspective is that I have no way of testing if whatever I implement (if you're asking me to implement it) is correct or not. I mean, I just used this
ImageCmsmodule to do something that seems sensible and that looks on my screen similar to what e.g. Gimp shows, but I don't know if it's taking into account my monitor profile (if any), or if it's clipping stuff that needn't be clipped, or something like that. The "easy" part, that is adding preferences for default target profile or rendering intent, that I can do.Hmmm... It converts to RGB, at least it displays my test images with oversaturated but "right" colors, unlike ImageMagick's
display, which apparently just maps C->R, M->G, Y->B. It does not use the embedded profile though (I don't know if it uses any other profile, I guess it just does CMYK-to-RBG with some hard-coded formula, and anything else would be applied after). See https://stackoverflow.com/questions/79322180/converting-a-cmyk-jpeg-to-rgb/79322257 and https://bugzilla.mozilla.org/show_bug.cgi?id=1989218 (there is hope).(Note: Assuming Arch and Xorg here. Not sure about Wayland but
displaycaldoes not work properly when doing stuff with a colorimeter viaxwayland. At least it used to be that way, not sure how things are today.)dispwin -I profile.icc(packageargyllcms) to set the monitor profile. If successful,xprop -len 14 -root _ICC_PROFILE(packagexorg-xprop) has something meaningful to say. There is an experimental function in PIL that detects a monitor profile (but I can't remember where it is right now). Usedispwin -U -cto undo the monitor profile installation.colord. If you want to create your own profiles, launchdisplaycal(packagedisplaycalwhich also depends onargyllcmssince it is mostly a GUI frontend for it) and go to Tools → Advanced → Create synthetic ICC profile. It can't create CMYK profiles, though.displaycal/argyllcmsis like the ultimate display calibration and profiling tool out there! It took me a while to get used to it, as it is clearly for advanced users, but the correctness, reliability and precision of the results beat virtually every commercial tool out there, according to reviews I read. Also, a good colorimeter was one of the best things I ever bought for my computer. Every display sucks unless properly (optionally) calibrated and profiled!I recommend using the aforementioned
SwappedRedAndGreen.icc(packagecolord) for testing so you can see whether it is doing anything at all. As for details like BPC, I think that currently, we do not need to worry about it. Just write code that makes sense according to the documentation, and when in doubt, leave the testing to me.By the way, in case you need details about embedded profiles,
iccdump -s image.jpg(packagecolord) might help.Any help is welcome. :-) But I need to come up with a more concrete idea for the GUI first. Right now, I'm thinking of doing away with the "Color management" tab and replace it with a new window similar to the "Edit external commands" dialog, as it already features a lot of the functionality we need as well …
Oh, I see. My bad. I wanted to say that the conversion does not even try understanding that CMYK is very different from RGB.
You might be right, who knows.
The first link is very helpful, thanks! As for the second link … well, we must get it done before they do!!
Okay, I think I have a quite concrete idea now. Also, I figured that the whole thing will be too big to fit into the normal prefs window so I think a separate window will probably be better. And since the "Edit external commands" window already features quite a lot of what we need for the design I propose here, I think it is be better to copy it, even though it means that the parts relevant for presets management will be there first. Also, this way, there is no need for a "(current)" preset.
So, if it counts as "easy", please try the following below. The explanations given there are mostly for later, so no need to implement everything perfectly right away. A mock-up should be good enough to get an impresson first.
preferences.confwith key "color management presets". The details need to be discussed later. Make the new dialog accessible in some way.The "Preview" is some kind of test image that demonstrates how colors (RGB, CMY, white, black and near-black) look like when put under the influence of the settings next to it (so the user can tell how a setting influences the result at various stages). If a conversion fails (for example, because of incompatibilities), a warning sign might appear instead. Not sure whether this can easily be implemented, but since it will be more of a gimmick than a necessity, we might just as well drop it.
Then, create copies of this grid for all the other cases ("CMYK", "other", "unavailable"). Actually, that should be done later because we first need to test and discuss whether something like that grid is usable at all. I just mention it here just to remind us of the fact that the grid will appear three more times.
If you have issues or you think you have a better idea, or comments, whatever, just let me know.