GNOME Bugzilla – Bug 325564
Use CIE LCH instead of HSL for layer mode "Color"
Last modified: 2015-06-24 05:44:36 UTC
The layer mode "Color" doesn't work much like its counterpart in Photoshop. To reproduce, take image at: http://wilber.gimp.org/~sjburges/pix/IMG_3570sm.jpg 1) Add alpha to Background layer 2) Add a layer under it, call it GrayLayer 3) Fill GrayLayer with 50% gray (127,127,127) 4) Set layer mode on Background layer to Color In Photoshop, you get: http://wilber.gimp.org/~sjburges/pix/IMG_3570sm_ps.tif In GIMP, you get: http://wilber.gimp.org/~sjburges/pix/IMG_3570sm_gimp.tif There's lots of good Photoshop tutorials out there that make use of the Color mode; this makes them pretty useless for GIMP. It'd be nice if we did a better job of emulating the Photoshop algorithm. I can do some exercises to help figure out what the Photoshop algorithm does if it would help matters.
I don't follow the argumentation that we need to emulate PS behaviour here. If you can show that the current behaviour of the Color mode is different than what it used to do in earlier GIMP versions, then it does indeed need to be fixed. Otherwise all we could do is to add a new mode that gives more satisfying results. Changing the effect of an existing layer mode is not an option since it would break loading of XCF files using this mode.
I think a pretty convincing argument can be made that gimp_rgb_to_hsl_int is broken. It is supposed to give back a result in h=[0-360], s=[0-255], v=[0-255]; however, if you'll look at the end of it: if (h < 0) h += 255; else if (h > 255) h -= 255; so we're for some reason adding/subtracting a 255 from a value that can be legally over 255? I really doubt anyone's used this mode for much of anything (since the results really aren't satisfying), but can buy the argument that we need to retain some backwards compatibility. However, I don't want to rename the current one to "broken_color" which I think is what it most accurately describes for at least the past couple of years - maybe make a color_ps? The layer modes can certainly get crowded... maybe we need to be able to select what is displayed eventually?
okay, I'll retract the brokeness comment on the function gimp_rgb_to_hsl_int; only the doc string seems obviously in error (it doesn't have a range of 0-360). It is a shame that we've got something named the same that does something completely different than the most popular graphics package out there, but I'll cope.
One possibly way of dealing with this is to add a new mode, call it Color and rename the broken one. Only the enum value must not change. Another possibility is to declare the old one broken and just fix it. From a user interface point of view this would be the preferred solution.
Removing NEEDINFO; should have been done months ago.
For what its worth... Photoshop's result is very close to the following procedure (some values are slightly different still, but its probably close enough for practical purposes): For both Top and Bottom layers: Filters->Colors->Decompose->YCbCr 470 then in one of them: Filters->Colors->Compose->YCbCr 470; choose Luma of Bottom layer, Redness and Blueness of top layer. I compared a 16M pixel (every color) image composed with a grey-127 image for testing, then did a "difference" with the result from photoshop doing the layer mode. The YCbCr 709 had less "harsh" transitions on the resulting differences, but more (practically all) of the image was different from photoshop's for this leading me to think the 470 algorithm is much closer. It could very well be rounding/colorspace compression that's causing the 470 profile to be different than Photoshop's, which could potentially be avoided if a YCbCr type were implemented in GIMP to add this layer mode (and paint mode, of course) rather than having to go through a lossy transition to a grayscale layer.
I looked up what Photoshop's Color blend mode does. According to the Photoshop documentation: Color - Creates a result color with the luminance of the base color and the hue and saturation of the blend color. This preserves the gray levels in the image and is useful for coloring monochrome images and for tinting color images. So, what Photoshop does with your example is this: it takes the luminance of the gray layer and the hue and saturation of the image layer. The result is an image that has the same luminance for every pixel. You can see this in the Photoshop histogram if you set it to Luminance: there is one spike in the middle of the histogram. GIMP's color mode works differently, you don't get a single spike in the middle of the histogram. I'm going to try and implement Photoshop's color mode in GIMP. I agree that it's best to call the new mode "Color" and rename the old mode to something else (keeping the same enum value for compatibility). Do you have a suggestion for the name of the old GIMP Color mode?
I think this is really a bug in GIMP's Color mode. I created a Photoshop PSD file with a layer set to the blend mode "Color" and loaded this image in GIMP. It looks wrong in GIMP, it uses GIMP's Color mode. So it looks like GIMP's Color mode was intended to be the same as Photoshop's Color mode, but it isn't. However, people might have used GIMP's broken Color mode in their images and if we would just fix Color mode and throw away the old, broken version, then those images will suddenly look differently. So I agree that the old, broken color mode needs to be preserved (but with a different name). Attachment: PSD file that looks different when loaded in Photoshop and in GIMP.
Created attachment 99613 [details] PSD file which shows that GIMP's Color mode is broken PSD file which shows that GIMP's color mode is broken. The image looks different in GIMP than in Photoshop.
I'm making progress on this and I now have something working that looks almost exactly the same as Photoshop's Color blend mode, but I have to work on it some more before I am ready to submit a patch. The old GIMP Color mode converts the image to the HSL color model and then takes Hue and Saturation from the top layer and Lightness from the bottom layer. Unfortunately, Lightness in HSL is not at all the same as Luminance, so the effect is different from what Photoshop does. The current "Color" mode should probably be renamed to "Lightness". (Note that there are already "Hue" and "Saturation" modes with are similar - they also work with HSL).
Correction: The current mode is actually not "Lightness" but "anti-Lightness" (it preserves HS and replaces L). A better name would be "Hue and Saturation" or "Color (HSL)".
Let's make sure this is fixed when using GEGL for the projection. Setting milestone to 2.8.
No unconfirmed milestoned bugs, please.
Note to self: When this is fixed, look into what to do with bug #401754.
Using CIE LCH instead of HSL gives superior results. The result is not identical to PS, but I would argue that GIMP's result is even better. I've pushed this to master now: commit 77e233f4c76cb4b6cf69b9b483a9fec8e27e8f37 Author: Martin Nordholts <martinn@src.gnome.org> Date: Sun Aug 2 11:07:40 2009 +0200 Bug 325564 – Use CIE LCH instead of HSL for layer mode "Color" When GEGL is used for the projection, use CIE LCH instead of HSL for the "Color" layer mode. This give much more accurate and intuitive results. Requires at least 12d5cc4c1bcfb of babl. app/gegl/gimpoperationpointlayermode.c | 67 ++++++++++++++++++-------------- 1 files changed, 38 insertions(+), 29 deletions(-) The implementation is unoptimized, but I plan to rewrite gimpoperationpointlayermode.c and unroll the loop later anyway, so I don't want to spend time now to optimize this. We can close this as FIXED now.
Created attachment 139720 [details] The PS result
Created attachment 139721 [details] For comparision with PS, the GIMP result after the fix
Thanks for your work. However, I don't agree that this issue should be set to FIXED. Suppose that someone has a file made in Photoshop, in which Photoshop's Color mode has been used as the layer blend mode. When you open such a file with GIMP, it will look different. I regard that as an error. A Photoshop file that looks a certain way in Photoshop should look exactly the same in GIMP (or any other program that can display Photoshop files). GIMP should not use its own, different Color mode on Photoshop images. Either the Color mode of GIMP should work the same as in Photoshop, or Photoshop images with Color mode layers should be converted when loading them so that they look the same as in Photoshop.
I agree that an imported .psd file should resemble what the file looks like in PS as much as possible, but GIMP is not a PS clone and does not need to be compatible with PS. I don't know how PS implements its Color layer mode, but I would argue that using LCH is a very good way to do it. If you can argue that another implementation is better, we can consider that, but that we need to work exactly like PS, if only for layer modes, is simply not a valid argument. The design decisions we take should fulfil our product vision, that does not necessarily mean to mimic Photoshop.
*** Bug 625303 has been marked as a duplicate of this bug. ***
Created attachment 167035 [details] [review] Introduction of new non-GEGL LCH layer modes Introduces four new layer modes Hue/Saturation/Color/Lightness in LCH colorspace. Does not change current GEGL modes yet. Still needs some work and cleanup but is usable as is. While this issue is marked FIXED, I'd say it's not. Old behaviour was never broken, just different. Now it's broken. More details in commit message and mailing list.
Nice, thanks. An immediate comment without having looked in detail: * Don't change the name of the legacy enums, that just complicates the patch Reopening as requested.
Review of attachment 167035 [details] [review]: needs-work
Created attachment 167350 [details] [review] Reworked patch for new LCH layer modes new version of the layer mode patch, replaces previous. - Restored original enums - Changed algorithms from floating point to integer Still needs quite a bit work, like not using full 16bit so we -- can shift instead of multiply / divide -- don't need temp 64bit variables. It's now about as fast as the legacy modes. - Lot's of refactoring, trying to adjust more to existing coding style. Cheers.
Created attachment 167876 [details] [review] v3 of layer mode patch -- libgimp functions only here goes the 3rd edition. This patch contains the new libgimp functions only. I'll post the layer mode stuff separately. Details in the commit message.
Created attachment 167877 [details] [review] The layer-mode part (v3) And here go the new layer modes, along with the other v3 patch.
Created attachment 167901 [details] [review] libgimp patch v3 with appropriate updates to generated files included Updates for: app/base/base-enums.c libgimp/gimpenums.h tools/pdbgen/enums.pl were not included in Rupert's patch, which lead to trouble when trying to pull the latest changes from GIT. Anyhow, Rupert's latest patchset is looking pretty good to me, as well as working well. There could be a small issue with the style of variable declaration for the composite functions -- while it is consistent with the rest of app/composite/gimp-composite-generic.c, it is inconsistent with the majority of GIMP code, where guint the; guint variable; guint names; gchar *are; guint16 aligned; This shouldn't stop it from being committed IMO, but it is something that we might want to consider fixing before committing.
Created attachment 167908 [details] [review] Ammended New-L-C-H-layermodes-v3 with autogenerated files Now with autogenerated files in the right patch. (the libgimp patch didn't introduce new enums yet -- and wouldn't be able to stand alone with those three files in it)
sorry, this has become a mess, and my naming of the patches hasn't exactly helped. To clear up the situation, right now you need two patches for the layer modes: (1) "v3 of layer mode patch -- libgimp functions only" Contains only the conversion functions but doesn't do anything to layer modes or enums. (2) "Ammended New-L-C-H-layermodes-v3 with autogenerated files" The actual layer modes. Builds on top of (1). phew...
Created attachment 168347 [details] [review] incremental update to the libgimp v3 patch only an incremental update to the "v3 of layer mode patch -- libgimp functions only" patch. Mostly cleanup, small errors. Details in commit message. Cheers.
Can mark obsolete patches as obsolete please? My immediate reaction is "whoa that's a lot of new code and API". What about having the CIELAB functions in the core, without adding API for it? We won't use the API for 3.0 anyway. Maybe you can use babl instead of reimplementing conversion routines?
That was an incremental patch, so it didn't obsolete anything. Other than David's upload which I don't think I can mark obsolete, none of the uploads are obsolete. Should I only post full patches against master? Should I not have split up the patch (yet)? The whole idea was to be able to concentrate on libgimp first, then once that's settled go about the layer implementation. But it did get a little messy -- sorry for that. The API might have been more complete than necessary (partly to make it easier to plug the whole file as-is into the regression testing I wrote for it). I can take some of it out. The bare minimum would be 4 functions (Lab/LCH forth and back) and 2 types (Lab and LCH). That shouldn't be so bad? The current GEGL/babl implementation of those modes (which seems just as straight forward as mine) is about 50x slower (~2 sec vs 1:37 min for the redraw of a 6MP picture). That is just not usable yet. Also I don't really like the idea of mixing the two routes. I am aware that I'm writing limited lifespan code until GEGL/babl takes over. But that's ok with me.
gimp_lch16_to_lab16 for example is defined in two of your patches, I just want one version of the function to review
Ok that wasn't 100% accurate, one of the patches is David's... but still, the bug report should just have one patch that defines gimp_lch16_to_lab16()
So to summarize, I would like to patches. One adding the functions to libgimp (squash with your first patch), and one adding the new layer modes to GIMP.
Review of attachment 167901 [details] [review]: Rejecting to avoid two versions o essentially the same patch.
Created attachment 168422 [details] [review] libgimp v3.1 (squashed libgimp v3 with incremental patch) Here goes the squashed patch for libgimp. So it's now: libgimp v3.1 and layermodes v3
Some initial comments: * GIMP_LCH_VALUE_MODE should't be called VALUE for the V in HSV, rather LIGHTNESS for the L in LCH. * You don't need v3 -> v3.1 in the commit message, once it hits git master, the history of the patch is irrelevant * Don't add gimp_colorspace_init() to the API, call it for the users of the API when needed instead * In gimp_composite_lch_hue_any_any_any_generic() you do three function calls for each pixel, I haven't done any benchmarking but I wouldn't be surprised if removing function calls from the inner pixel processing loop would make things twice as fast. * Regression tests are good! Why not include them in the patch? Why do you need a plug-in API for the tests? * I don't think we should add things to the GIMP library unless we need it, and for this patch in the core it is not needed. I still think the conversion should happen in the core
* Can you explain the LCH Burn layer mode? What does it do differently from existing Burn?
> * GIMP_LCH_VALUE_MODE should't be called VALUE for the V in HSV, Yes. I stumbled over that a couple of times but decided to leave it so far to make the symmetry to old modes more obvious. But you're right. > * You don't need v3 -> v3.1 in the commit message, ... ok > * Don't add gimp_colorspace_init() to the API, ... ok -- if it goes into core, becomes moot anyway. > * In gimp_composite_lch_hue_any_any_any_generic() you do three > function calls for each pixel, ... I haven't benchmarked either (other than with a wristwatch), but I suspect the biggies to be the remaining floating point atan2 and cos/sin calls. Hue and Saturation (only those two need them) are noticeably slower than Color/Lightness. > wouldn't be surprised if removing function calls from the inner > pixel processing loop ... Unless we pull the conversions in as inlines, that would mean shifting the actual compositing logic away into the color conversion... > * Regression tests are good! Why not include them in the patch? Why do > you need a plug-in API for the tests? I haven't looked into existing tests, but I suspect you would expect some sort of yes/no result. Right now, the test I have gives more of a report. For a sample, see http://leguanease.org/gimp/result-div-no-mul-yes.txt I don't actually need a plug-in API, but access to some of the static vars (scaling factors). But there are other ways to achieve that. If you want to take a look, you can download the current version from http://leguanease.org/gimp/cietest-1.0.tar.gz > * I don't think we should add things to the GIMP library unless we > need it, and for this patch in the core it is not needed. I still > think the conversion should happen in the core Considering that it would solve most of the above problems, you're probably right. Should I put them right into gimp-composite-generic.c? That way, they could easily be inlined if it shows worthwhile. About Burn mode: Bottom line: forget it for now. It's just something I played around with. (sorry, you had asked earlier and I never got back to you about that). The idea was to have a saturation-neutral burn. I had compared the modes with a white layer set to Burn, painting with some 10% black. The difference is immense. But I just noticed when instead of a white layer you take a transparent one, results are only marginally different. Furthermore, strong burns look just gray/black. So while there might be room for a better burn mode, it will take a little more intelligence than just plugging Lab in there. Regards Rupert
I would expect the regression tests to go in to app/tests/test-core.c, so if that works for you, by all means, include what you have there in the next patch update.
I'll take a look at how to integrate the tests. Just took a peek at test-core.c and -- well, it's not immediately obvious what and how to put it in there... I'm thinking maybe a test to check that the results stay in certain bounds. Because every time you tinker with the integer conversions, results change slightly, even though overall quality doesn't necessarily. As to speed and function calls: I put all the conversions into gimp-composite-generic.c and compared inlined to non-inlined. Inlining gives a speed advantage of about 12% (Hue/Sat.) to 15% (Color/Light.) over non-inlining. Object code size increased by ~30kb. - Lightness (LCH) becomes as fast as Value. - Color (LCH) was already slightly faster than Color. - Hue (LCH) and Saturation (LCH) are still slow dogs, about 3x as slow as Color/Lightness. We can omit 1 of 3 function calls by passing data for both input colors and then looping in the conversion. Adds a little complexity to the conversion and saves either a function call or (when inlined) some kb of object code size. [ If this discussion were better taken to the list, please let me know ]
Good news, Hue and Saturation are now 2-3 times faster, by eliminating trig calculations. > * In gimp_composite_lch_hue_any_any_any_generic() you do three > function calls for each pixel, I haven't done any benchmarking but I May brain was stuck, I see what you mean now. But we'll need a buffer for that. Two questions with which someone who knows could save me some research time: 1. Do we need to be thread-safe here? 2. Where do I get the definite maximum number of pixels the gimp_composite_* functions are called with?
Why do you need a buffer? Since I don't think you need a buffer, it should be easy to make he function reentrant and thus thread-safe.
> Why do you need a buffer? Didn't you mean to put the calls to the conversion outside the while(length--) loop in gimp_composite_*() and thus let it convert a bunch of pixels (64?) at a time? That way function calls are reduced to 1/64, while the actual compositing stays in gimp_composite_*(). But the results have to be buffered somewhere.
The color conversion routines that you planned to add to libgimpcolor should not be needed. New code in GIMP is supposed to use babl for pixel format conversions. You might want to put your optimized code into babl. That will also avoid the need to write tests as babl comes with a test framework that doesn't require you to write explicit tests.
Created attachment 168598 [details] [review] New LCH layermodes all-in-one patch Here goes the last iteration of the layer modes patch. The conversions are not in libgimp anymore, but in their own file in app/composite. (I expect you'll want that file to be somewhere else and/or named differently, just let me know) More details in commit message and on the list.
Review of attachment 168598 [details] [review]: We need to take care of usability aspects too, details on the list.
Created attachment 169582 [details] [review] New LCH layermodes, obsolete modes hidden in drop-down New modes now have plain names 'Hue', 'Color' etc., old modes have '(obsolete)' appended. Old modes are shown in dropdown only if a XCF with old modes is active window. Details about that on the list.
On the list you said you had a new patch, right? Could you post it please?
Created attachment 170450 [details] [review] New LCH layermodes, small fixes Sure, I just thought I'd wait a little to see if other small fixes come up. Only changes to previuos patch are: - too-early clamping in lab->rgb fixed - old modes are now called 'legacy' rather than 'obsolete' Rupert
Created attachment 171877 [details] [review] 0001-new-L-a-b-and-L-C-H-layermodes-no-libgimp.patch Rebased against master
We really must release 2.8 now, let's look at this for 2.10 instead.
I have tested this now with the current git version of GIMP. Initial results seem quite promising. (I will do some more testing.) Although I realize that there is probably only comparably few users using GIMP for painting original images, a working color mode is a tremendously important feature for the usual coloring workflow. The current one is not usable because it does not preserve saturation of the color layer. I therefore think this feature should be included in GIMP as soon as possible. If the current (broken) coloring mode must be kept for reasons of backward compatibility, I would very much like to see an alternative color mode that works more like the one from the patch. Maybe a more experienced GIMP developer could assist me with creating an updated patch?
Created attachment 211876 [details] [review] Updated layer modes patch I have modified the patch to work with the current GIMP git version.
Thanks for the effort, but this patch is not going to make it into 2.8, we are in release candidate phase already. For 2.10, all of this will be using GEGL, please have a look at the goat-invasion branch in git. The files changed in your patch are all going to go away, sorry. You are very welcome to come to IRC and discuss how to help with the ongoing GEGL porting.
Jörn, Would you be interested in updating your patch for Git master which now completely relies on GEGL?
Created attachment 296633 [details] [review] Hue/Saturation/Color/Lightness layer modes in CIE Lab/LCH This patch is a first attempt to port Jörn Meier previous work about adding Hue, Saturation, Color and Lightness layermodes in CIE Lab/LCH colors space. - being totally new to colors representations, I followed formulas found here: http://www.brucelindbloom.com/index.html?Equations.html - as RGB to XYZ matrix conversion expects linear RGB values, gimp operations implement a prepare method to force using linear RGBA float babl format, whatether the image precision format is. - being unable to test photoshop psd file, the patch do not modify psd-load / psd-save plugins - no additionnal libgimpcolor/... file(s), should it be ? - not sure that "legacy" is the appropriate term to use for original Hue/Saturation/Color/Value layermodes (I take it from the previous patch) - RGB > XYZ > Lab > LCH > Lab > XYZ > RGB : very poor performances
Thomas Manni, many thanks for your patch! I had looked at the old patch and quickly realized that porting the patch to the new GIMP was not something I could do. The patch applied with no problem. With a saturated blue layer over an image layer, the new Hue blend mode makes the image look orange, whereas the legacy Hue blend mode makes the image look blue, as expected. I'm looking through the code to see if I can figure out why.
Because GIMP is an ICC profile color managed editing application, the XYZ matrices for converting between XYZ and sRGB should be adapted from D65 to D50, using the D65 values from the sRGB and the D50 values from the ICC specs. For consistency with the rest of babl/GEGL/GIMP, use the already D50-adapted sRGB-XYZ matrices in this file: https://git.gnome.org/browse/babl/tree/extensions/CIE.c The new patch duplicates a lot of the babl RGB/XYZ/LAB/LCH code that's in "babl/extensions/CIE.c". That code also closely follows Lindbloom's equations. Is it more efficient to write separate code for GIMP to use?
Created attachment 299174 [details] [review] patch with D50-adapted white point and sRGB/XYZ matrices I modified Manni's patch to use the same D50-adapted (adapted to the ICC D50-adapted illuminant) white point and sRGB/XYZ matrices as the rest of babl/GEGL/GIMP. With the new matrices/white point, blended colors match results from ArgyllCMS xicclu tool. This patch doesn't fix the problem with the LCH Hue blend mode for the blue channel. Currently blending blue in LCH Hue mode makes the image turn orange. So any color with a non-zero blue channel blends incorrectly in the LCH Hue blend mode. I couldn't find the source of the error; LCH color blend mode (which is C+H) with blue works just fine. I also modified the layer blend names to (HSV) instead of (legacy), and added (LCH) to the LCH blend modes. Once the problem with LCH Hue blend mode for the color blue is fixed, I'd like to add JCH blend modes (ciecam02, which is very widely used these days and seems to be better at capturing how people perceive differences in color).
(In reply to Elle Stone from comment #61) > (adapted to the ICC D50-adapted illuminant) white point Sorry, should read "adapted to the ICC D50 profile illuminant".
LCH Saturation blend mode, as well as LCH Hue blend mode, doesn't seem to give correct results if either layer has non-zero blue channel values. LCH color and lightness blend modes use LAB equations rather than LCH equations. But that doesn't explain why the LCH equations work just fine for saturation and hue as long as the blue channel is zero.
In libgimpcolor/gimpcolorspace.c, the following code changes don't seem to make any difference (no guarantee that I even wrote the code correctly): #define DEGREES_PER_RADIAN (180 / 3.14159265358979323846) #define RADIANS_PER_DEGREE (1 / DEGREES_PER_RADIAN) in gimp_lab_to_lch: lch->h = atan2 (lab->B, lab->A) * DEGREES_PER_RADIAN; in gimp_lch_to_lab: lab->A = lch->c * cos (lch->h * RADIANS_PER_DEGREE); lab->B = lch->c * sin (lch->h * RADIANS_PER_DEGREE); Either way, with or without the radians/degrees code, hue and saturation blend correctly, but only if the blue channel is zero for both layers.
(In reply to Elle Stone from comment #64) > In libgimpcolor/gimpcolorspace.c, the following code changes don't seem to > make any difference (no guarantee that I even wrote the code correctly): > > #define DEGREES_PER_RADIAN (180 / 3.14159265358979323846) > #define RADIANS_PER_DEGREE (1 / DEGREES_PER_RADIAN) > in gimp_lab_to_lch: > lch->h = atan2 (lab->B, lab->A) * DEGREES_PER_RADIAN; > in gimp_lch_to_lab: > lab->A = lch->c * cos (lch->h * RADIANS_PER_DEGREE); > lab->B = lch->c * sin (lch->h * RADIANS_PER_DEGREE); > > Either way, with or without the radians/degrees code, hue and saturation > blend correctly, but only if the blue channel is zero for both layers. It turns out that with the above lines of code for radians, the LCH Hue and Sat layer blend modes do work, even with the color blue. Sorry for the confusion!
Created attachment 299242 [details] [review] add code for radians, remove some bits of unused code This patch does the following (after applying the previous patch): 1. Adds code for radians to the LCH Hue blend mode. 2. Removes the code that checks for saturation in the LCH Hue blend mode (it's not needed). 3. Removes some calls to babl in the LCH Saturation blend mode.
(In reply to Elle Stone from comment #66) > 2. Removes the code that checks for saturation in the LCH Hue blend mode > (it's not needed). Hmm, actually it depends on the layer order, so some such code really is needed. I've been editing some images to see how these layer blend modes work in practice (they work really well).
The "if" statement in gimpoperationlchhuemode.c can't ask if layer_lch.c is > 0, because asking if it's greater than 0 still results in completely desaturated colors turning red. Asking if it's greater than 0.1 works, but probably there's a smaller or smallest value that will also work: if (layer_lch.c > 0.1) { out_lch.h = layer_lch.h; } Should the LCH "C" blend mode be labelled as "saturation" or as "chroma"? I think "chroma" might be less confusing.
As mentioned in comment #43 it is possible to avoid the expensive trigonometric functions if you inline the conversion lab->lch and lch->lab: for example for the Hue (LCH) layer mode it is possible to express output Lab (L, a, b) in terms of input (L1, a1, b1) and layer (L2, a2, b2) L = L1 a = c1 * cos (h2) = c1 * c2 * cos (h2) / c2 = c1 * a2 / c2 b = c1 * sin (h2) = c1 * c2 * sin (h2) / c2 = c2 * b2 / c2 where c1 = hypot (a1, b1) or = sqrt (a1 * a1 + b1 * b1) and c2 = hypot (a2, b2) or = sqrt (a2 * a2 + b2 * b2) observing that when c2 == 0 (or < epsilon) the hue is indeterminate, that is the triple (L2, a2=0, b2=0) is mapped by all triples (L2, c2=0, h2=[0,360)) and so it is possible to choose h2 = h1 and output the input unmodified (better than choosing h2 = atan2 (+0, +0) == 0) Another observation is that setting the input format linear these layer modes perform the final blend (with weight 'ratio') always in RGB linear space, differently than all other modes. And I think that the plan here is to add to babl whatever conversion is missing and use babl_process to perform the conversions.
(In reply to Massimo from comment #69) > As mentioned in comment #43 it is possible to avoid the expensive > trigonometric functions if you inline the conversion lab->lch and > lch->lab: I will code up (try to) inlining the conversions today, unless someone else is doing so, in which case please say so! > Another observation is that setting the input format linear > these layer modes perform the final blend (with weight > 'ratio') always in RGB linear space, differently than > all other modes. For radiometrically correct results, the LCH blend modes should use linear gamma RGB for blending as well as calculating, else the blends are subject to gamma errors. But I'm not sure if that's what you are talking about. > > And I think that the plan here is to add to babl whatever > conversion is missing and use babl_process to perform the > conversions. I think it makes sense to get the LCH code working correctly before involving babl for the actual conversions, yes?
(In reply to Elle Stone from comment #70) ... > > > Another observation is that setting the input format linear > > these layer modes perform the final blend (with weight > > 'ratio') always in RGB linear space, differently than > > all other modes. > > For radiometrically correct results, the LCH blend modes should use linear > gamma RGB for blending as well as calculating, else the blends are subject > to gamma errors. But I'm not sure if that's what you are talking about. > I'm saying that in every layer mode operation the 'input' buffer is used in two places: one dependent on the layer mode that, for example here produces the out_tmp array: https://git.gnome.org/browse/gimp/tree/app/operations/gimpoperationhuemode.c#n102 and a second (common to all operations) where this out_tmp color is blended with the input to modulate the effect of the layer mode, based on the opacity and/or the presence of the layer mask etc: https://git.gnome.org/browse/gimp/tree/app/operations/gimpoperationhuemode.c#n135 Always using the format "RGBA float" for this buffer simplifies the conversion to XYZ, useful in its first use, but, in its second use, does not honor the "linear" property of the layer mode for non linear images: https://git.gnome.org/browse/gimp/tree/app/operations/gimpoperationpointlayermode.c#n152 It is an inconsistency. > > > > And I think that the plan here is to add to babl whatever > > conversion is missing and use babl_process to perform the > > conversions. > > I think it makes sense to get the LCH code working correctly before > involving babl for the actual conversions, yes? The problem is that code in libgimpcolor is available to plug-ins and has to be maintained, so it requires a careful design. Besides having the same code in one place ensures that fixing a bug once is fixed everywhere. the babl CIE.c extension did not have the radians/degrees mistake
Created attachment 299465 [details] gimpoperationlchlightnessmode.c not using libgimpcolor files (In reply to Massimo from comment #71) > Always using the format "RGBA float" for this buffer simplifies the > conversion > to XYZ, useful in its first use, but, in its second use, does not > honor the "linear" property of the layer mode for non linear images: > > https://git.gnome.org/browse/gimp/tree/app/operations/ > gimpoperationpointlayermode.c#n152 > The problem is that code in libgimpcolor is available to > plug-ins and has to be maintained, so it requires a careful > design. > > Besides having the same code in one place ensures that fixing > a bug once is fixed everywhere. the babl CIE.c extension did not > have the radians/degrees mistake As a first step towards implementing what you are describing, I removed all the rgb/xyz/lab/lch code from libgimpcolor/colorspace.c and libgimpcolor/colorspace.h, and put the rgb/xyz/lab/lch code inside each of the four app/operations/gimpoperationlch*.c files. I did this so I can work on just one file at a time, starting with the easiest, which is app/operations/gimpoperationlchlightnessmode.c. So the next step is to replace the rgb/xyz/lab/lch code in gimpoperationlchlightnessmode.c (attached) with calls to babl's CIE.c file, and also change the requested format to not specify linear. For this I'll need to go on IRC and ask for help; even after time spent pondering examples from the GEGL and GIMP code base, I'm not sure what to do next.
Created attachment 299489 [details] [review] use babl in lch based layer mode operations yesterday after commenting I played with the idea to use babl for these operations and I'm attaching what I've done so far. I wanted to check that the output is correct, but since in the next days I am busy I attach here a patch that has to be applied after the attachements 299174 and 299242, hoping it is of inspiration if something is wrong.
(In reply to Massimo from comment #73) > Created attachment 299489 [details] [review] [review] > use babl in lch based layer mode operations > > yesterday after commenting I played with the idea to > use babl for these operations and I'm attaching what I've > done so far. > > I wanted to check that the output is correct, but since in > the next days I am busy I attach here a patch that has to > be applied after the attachements 299174 and 299242, hoping > it is of inspiration if something is wrong. I had trouble applying patch 299489, partly from "fatal: corrupt patch at line" errors, which turns out to be an end of file error; and partly because some of the white space errors had already been fixed. So I applied the patch by copying the patch code to the files, and everything compiled and ran without a problem. If I understand the code correctly, patch 299489: 1. Does solve the problem with using linear RGB for input and output, restoring consistency with the rest of GEGL/GIMP (comment 71). 2. Does use the less computationally expensive substitute for the expensive atan2 calculations (comment 69). 3. Doesn't use the code in babl/extensions/CIE.c, but rather puts duplicate code in gimp/app/gegl/gimp-babl.c. So this patch is a step closer to but not intended to be the final code for LCH blend modes, yes? The output from patch 299489 seems to be correct. I'm checking by blending various colors together and comparing GIMP RGB readout with xicclu output. However, based on the RGB readouts, there are small discrepencies between output from patch 299489, in default GIMP, and output from my code that puts all the RGB/XYZ/LAB/LCH code right in the app/operations/gimpoperationslch*.c files. My code generates RGB values that are closer to the RGB values generated by ArgyllCMS xicclu. My hacked version of GIMP disables the babl flips. So the discrepancies could be from babl flip rounding errors? Or from the computationally less expensive substitutes for the atan2 calculations?
(In reply to Elle Stone from comment #74) > > I had trouble applying patch 299489, partly from "fatal: corrupt patch at > line" errors, which turns out to be an end of file error; and partly because > some of the white space errors had already been fixed. So I applied the > patch by copying the patch code to the files, and everything compiled and > ran without a problem. > sorry for that, didn't double check > If I understand the code correctly, patch 299489: > > 1. Does solve the problem with using linear RGB for input and output, > restoring consistency with the rest of GEGL/GIMP (comment 71). No, it doesn't and I have to investigate if this code is used also for painting, in which case the solution could be more difficult. I mean the ..._process functions receive also the operation object from which it would be possible to extract the current format of the buffers, but the ..._process_pixels only receive pointers to float and I'm not sure who's calling these functions. > > 2. Does use the less computationally expensive substitute for the expensive > atan2 calculations (comment 69). > yes > 3. Doesn't use the code in babl/extensions/CIE.c, but rather puts duplicate > code in gimp/app/gegl/gimp-babl.c. So this patch is a step closer to but not > intended to be the final code for LCH blend modes, yes? > The code in babl/extensions/CIE.c implements the reference conversions which only convert from/to "RGBA double", whereas my three conversions convert to/from float and the presence of these routines allows also the use of fast paths for babl gamma flips because babl can find more paths from/to "CIE Lab alpha float", via "RGBA float". The reference code is used anyway to validate that these conversions have errors within the accepted tolerance. If the differences are small they are caused by rounding results to single precision floating points, instead of double.
(In reply to Massimo from comment #75) > (In reply to Elle Stone from comment #74) > No, it doesn't and I have to investigate if this code is used > also for painting, in which case the solution could be more difficult. People will probably want to paint using the LCH blend modes, if that's what you mean. >> My code generates RGB values that are closer to the RGB values generated by >> ArgyllCMS xicclu. My hacked version of GIMP disables the babl flips. So the >> discrepancies could be from babl flip rounding errors? Or from the >> computationally less expensive substitutes for the atan2 calculations? > If the differences are small they are caused by rounding results to single > precision floating points, instead of double. I disabled the babl flips in default babl/GEGL/GIMP, and converted my test file to linear gamma sRGB. With these changes, your new code from patch 299489 produces results that match results from my code to within 0.000001. But with the babl flips enabled as per default babl, there are differences in the sixth and sometimes fifth decimal place. So the discrepencies would seem to be from the babl flips. This matches discrepancies between changing a regular sRGB image to linear precision, vs using LCMS to convert the regular sRGB image to linear gamma sRGB.
(In reply to Elle Stone from comment #76) > (In reply to Massimo from comment #75) > So the discrepencies would > seem to be from the babl flips. This matches discrepancies between changing > a regular sRGB image to linear precision, vs using LCMS to convert the > regular sRGB image to linear gamma sRGB. Hmm, ignore above, the differences are only there when there's an ICC profile conversion involved.
Created attachment 299747 [details] [review] Incorporates Massimo's patch (that wouldn't apply) plus changes lch "saturation" to chroma
Created attachment 299748 [details] [review] use babl in lch based layer mode operations The ..._process_pixels functions are called from 'do_layer_blend' in gimppaintcore-loops.c and I had to duplicate them to be able to know the format of the buffers received. The attached patch, should also fix the inconsistency and behaving like others layer modes. It supersedes my previous one and it is to be applied after the attachements 299174 and 299242
Created attachment 299760 [details] [review] Changes LCH "saturation" to chroma
(In reply to Elle Stone from comment #80) > Created attachment 299760 [details] [review] [review] > Changes LCH "saturation" to chroma Apply after 299174, 299242, and 299748. Chroma is a property of an actual color as located in the LCH and LAB reference color spaces. Saturation is defined relative to a particular RGB working space and changes when the working space is changed.
This bug report has been open since 2006. The LCH blend modes work. They work really well. They make possible a whole realm of editing possibilities hitherto denied to GIMP users. Can this please be pushed to Master?
Created attachment 301942 [details] [review] unique patch for previous work One unique patch containing all previous work. I also remove changes applied to auto generated file tools/pdbgen/enums.pl
Thanks for the merged patch, some comments: - I will push a change that moves the internal modes to the very end, using values 1000, 1001 etc. Please adapt the patch after I pushed this, by changing the order of all places where the new modes appear. - operations/Makefile.am contains spaces, please use tabs and align the \s - indentation of function headers in the new layer mode ops is broken As to the general approach of adding new layer modes, and having proper linear/gamma foobar per mode, let's have a proper discussion and decision at LGM.
Created attachment 303614 [details] [review] Modify the code per request from Mitch Unfortunately the attached patch doesn't actually make the LCH blend modes show up in the layer blend mode dialog. So it's pretty useless, except to maybe keep interest in getting the LCH blend mode code committed to master sooner rather than later.
Elle, - you forget to increment the number of enums in app/widgets/gimpwidgets-constructors.c line 106 (see patch 301942); that's the reason new layer modes do not appear in the menus. - indentation is still broken for some functions in operations (headers and .c files) - I added a separator for more readability in the layer menu, but it's optional...
Great, thanks Elle! Before we proceed, is there a reason the new conversions are not in Babl? I didn't see any comment about this in the bug, so the reason might have been discussed on IRC?
Comment 52 noted that putting the conversions in babl slowed down the calculations quite a bit. It would be nice to get the LCH blend mode code into master and then figure out what would be needed to transfer the relevant portions of the code to babl while still keeping speed of operation comparable.
(In reply to Thomas Manni from comment #86) > Elle, > > - you forget to increment the number of enums in > app/widgets/gimpwidgets-constructors.c line 106 (see patch 301942); that's > the reason new layer modes do not appear in the menus. > > - indentation is still broken for some functions in operations (headers and > .c files) > > - I added a separator for more readability in the layer menu, but it's > optional... Thomas, thanks! Incrementing the number of enums did the trick. I'll submit a replacement patch tomorrow AM (except of course if someone else already has!) with the correcte enums, and also putting the separator back in, and also putting the (HSV) back on the HSV blend modes. I don't know where the indentation is broken, but I'll see if I can find it.
Hmm, but in the current patch, the conversions *are* registered with Babl, they just live in app/gegl/gimp-babl.c, there should not be any speed difference when they are moved to Babl.
Created attachment 303656 [details] [review] updated patch with fixes for enums and such This patch increments the enums, adds in the separator, and puts "(HSV)" after the HSV blend modes. It doesn't fix any indentation issues.
(In reply to Michael Natterer from comment #90) > Hmm, but in the current patch, the conversions *are* registered with Babl, > they just live in app/gegl/gimp-babl.c, there should not be any speed > difference when they are moved to Babl. Transferring the relevant code to babl is beyond my coding skill. If this is done by someone else, and if the LCH blend modes use the already mostly identical code in babl/extensions/CIE.c, then this bug should be fixed to make the CIE.c code RGB to LCH code match Thomas Manni's code: https://bugzilla.gnome.org/show_bug.cgi?id=748187. I misinterpreted Lindbloom to mean "use the rounded results, not the calculations". But really Lindbloom meant just the opposite. It does make a difference. When using the actual calculations rather than the rounded results, the LCH blend mode code produces results that are pretty nearly an exact match to equivalent ArgyllCMS xicclu conversions.
Created attachment 303720 [details] [review] same as previous patch but without gimp_babl.c same content as previous patch, but without the new babl code (since moved to babl, commit f4cc927054d4e3554734456ebc14b2fdc2c895b6)
The LCH blend mode code works really well. Any idea when it might make it into master? Also, the LCH blend mode code needs an LCH color picker. I coded one up for my own use, that needs a lot more work, but it's a first step: https://bugzilla.gnome.org/show_bug.cgi?id=749902
You might also want to have a look at http://registry.gimp.org/node/16814.
Pushing these patches is on the top of my todo, after color management stuff.
Fixed in master, with a slightly cleaned up patch: commit a7b84ded8e14eaf504edf702320096a1a47dd678 Author: Thomas Manni <thomas.manni@free.fr> Date: Wed May 20 15:19:30 2015 +0200 Bug 325564 - Use CIE LCH instead of HSL for layer mode Color Add Hue, Chroma, Color and Lightness layer modes in LCH color space. app/actions/context-commands.c | 6 +- app/actions/layers-commands.c | 6 +- app/core/core-enums.c | 16 +++- app/core/core-enums.h | 12 ++- app/gegl/gimp-gegl-nodes.c | 4 + app/operations/Makefile.am | 8 ++ app/operations/gimp-operations.c | 8 ++ app/operations/gimplayermodefunctions.c | 21 ++++- app/operations/gimplayermodefunctions.h | 3 +- app/operations/gimpoperationlchchromamode.c | 204 +++++++++++++++++++++++++++++++++++++++++ app/operations/gimpoperationlchchromamode.h | 72 +++++++++++++++ app/operations/gimpoperationlchcolormode.c | 193 ++++++++++++++++++++++++++++++++++++++ app/operations/gimpoperationlchcolormode.h | 72 +++++++++++++++ app/operations/gimpoperationlchhuemode.c | 204 +++++++++++++++++++++++++++++++++++++++++ app/operations/gimpoperationlchhuemode.h | 72 +++++++++++++++ app/operations/gimpoperationlchlightnessmode.c | 187 +++++++++++++++++++++++++++++++++++++ app/operations/gimpoperationlchlightnessmode.h | 72 +++++++++++++++ app/paint/gimppaintcore-loops.c | 2 +- app/widgets/gimpwidgets-constructors.c | 12 ++- devel-docs/libgimp/tmpl/gimpenums.sgml | 4 + libgimp/gimpenums.h | 6 +- tools/pdbgen/enums.pl | 10 +- 22 files changed, 1176 insertions(+), 18 deletions(-)
Does this fix help us with bug #401754?
Um, this bug talks about decompose briefly and then only about layer modes. Looks like a duplicate to me.
*** Bug 401754 has been marked as a duplicate of this bug. ***
I misread the other bug, it was a duplicate.
Bug 401754 seems to be about adding HSL (hue-sat-lightness) blend modes to GIMP. The title of the bug report says "LCH" but the contents only refer to HSL and HSV. Quoting 401754: "My idea is add two new modes in layer modes list: they are saturation and lightness based on HSL model." It might be better to change the title of 401754 to reflect the actual contents of the bug report. Whether GIMP will or should add HSL layer blend modes is a separate question. But HSL and HSV are not the same.
Argh, I shouldn't close bugs quickly in between other work. It's indeed not a duplicate :/
*** Bug 638036 has been marked as a duplicate of this bug. ***
Is there a possibility to add an "saturation" blend mode as well in addition to the "chroma" blend mode? The point is, that changing lightness while preserving chromaticity heavily changes saturation. (so my impression: increasing lightness reduces saturation and decreasing lightness increases saturation. My feeling is, that adding lightness requires adding some "color-substance" (chromaticity) in order to preserve the saturation) Maybe this is related to the "LSH color" model I found at Wikipedia. I also think, that darktable can do a similar "chroma scaling" in order to preserve saturation in the "levels" and "tone curve" tools. cheers! Good job so far - you all developers out there with implementing LCH blending modes :-)
(In reply to immanuel from comment #105) > Is there a possibility to add an "saturation" blend mode as well in addition > to the "chroma" blend mode? > The point is, that changing lightness while preserving chromaticity heavily > changes saturation. (so my impression: increasing lightness reduces > saturation and decreasing lightness increases saturation. My feeling is, > that adding lightness requires adding some "color-substance" (chromaticity) > in order to preserve the saturation) I think you are talking about this: http://www.handprint.com/HP/WCL/color3.html#satvsval which is a very different definition of saturation than the naive "saturation" that's calculated using HSV. Allowable chromas (that pertain to real colors) peak around 50% Lightness, but at different Lightnesses for different hues. I think the same is true for Saturation, but I'm not 100% sure as I haven't plotted out saturation for real colors vs Lightness. > Maybe this is related to the "LSH color" model I found at Wikipedia. I couldn't find an "LSH" color model in Wikipedia (doesn't mean it's not there). > I also > think, that darktable can do a similar "chroma scaling" in order to preserve > saturation in the "levels" and "tone curve" tools. I haven't looked at the darktable code or functions. But in any event, your request probably should be a separate bug report. In the meantime, have you tried masks to fade the effect as you reach Lightness values for which a given chroma/hue/color blend seems "too much", for example masking out shadows when LCH color-blending a color with a high L value (I do this to solve the kind of problem I think you are describing).
@Elle: You are right. I'm not talking about "Saturation" unsed in HSV/HSL, but about CIE "Saturation" (which is not definded, as I read) Nevertheless LSH Color model proposed by Eva Lübbe is mentioned here: https://en.wikipedia.org/wiki/Colorfulness Further literature seems to be available only in german. Beside having a definition of saturation according to human perception, the colorspace also claims to solve some asymmetry issues as equal color distances etc. http://deutsches-farbenzentrum.de/wp-content/uploads/2011/10/Habilitation5.24.pdf Page 47 seems to exactly describe our problem here. Here the code snippet from darktable: if(autoscale_ab == 0) ... // in Lab: correct compressed Luminance for saturation: if(L_in > 0.01f) { out[1] = in[1] * out[0] / in[0]; out[2] = in[2] * out[0] / in[0]; } ... http://redmine.darktable.org/projects/darktable/repository/revisions/master/entry/src/iop/tonecurve.c Lines 305 ... 350 It does exactly, what I mean: Adding color in the same ratio, how lightness changes. btw. I was not able to find any layermask / blend mode combination to preserve saturation. Luminance masks don't help, because the ratio of Ligntness is relevant. (I'm not able find a solution with layer blend mode "divide"...)