GNOME Bugzilla – Bug 662739
Port to lcms2 for icc V4 profile support
Last modified: 2012-12-15 00:56:23 UTC
Created attachment 199996 [details] [review] lcms2 transition patch Description strings from icc v4 profiles are not read properly and show up as "invalid UTF-8". The cause seems to be that for those profiles, lcms returns a wide char string. I've fixed the problem by using lcms2. It also has the benefit of adding localized profiles support. The main impact of this patch, is that it changes the (name,desc,info) triplet of information with (desc,manufacturer,model,copyright), mapping directly with icc tags we may find in profiles. One particular thing I'm not happy with in this patch, is that I had to duplicate the function that extracts textual information from profiles (lcms_get_profile_info) in the various modules and plug-ins that uses lcms. Obviously, this should go in a file of its own to be shared by all those components.
Created attachment 200082 [details] ICC v4 profile whose description is not read by lcms1
Thanks for this impressive patch. It can't be applied to 2.6 though, would you mind porting it to git master? Also, we are close to the 2.8 release, but it can go into master as soon as 2.8 is out.
OK, I will try to port this to master. There should be a less extreme fix to the description reading problem for 2.6 though.
Laurent, can you look at the current state of master? We applied another lcms2 part in the meantime, but it looks yours does more.
Created attachment 227140 [details] [review] Fix out of gamut color always black Here's a first little patch to fix marking of out of gamut color which always appear black. Lcms2 uses 16bits components so gimp's 8 bits components must be multiplied. This will bug again when gimp switches to variable component bit depth, is there a way to secure that now ?
Thanks. Master already has deeper than 8 bit support, but for CMS purposes we don't know how exactly to handle that yet. So far, the display filters are still 8 bit.
Comment on attachment 227140 [details] [review] Fix out of gamut color always black commit 2d6a880b12214e38731a10fbc4dba28f256a5fa3 Author: Michael Natterer <mitch@gimp.org> Date: Wed Oct 24 18:19:33 2012 +0200 Bug 662739 - Port to lcms2 for icc V4 profile support Apply patch from Laurent Martelli which multiplies the out-of-gammut color components by 256 to match the new 16 bit lcms data type. modules/display-filter-lcms.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Created attachment 228659 [details] [review] Elle's latest patch, cleaned up Elle's patch from ninedegreesbelow.com/temp/gimp-lcms-deprecated.html cleaned up and fixed some warnings. What I don't understand is the code in lcms_drawable_transform(): after cmsDoTransform(), why does the code copy the pixels around manually again, this looks wrong.
The original lcms plug-in did put all the code for indexed RGB images in one function, but for some reason split the regular RGB ICC transform into two functions, one to set up the transform, the second to actually perform the transform. Possibly the hope was that one or the other function could be reused/expanded if/when Gimp was expanded to handle grayscale, Lab, etc? I kept the split into two functions just to follow the original code as closely as possible. I don't see any reason to keep separate functions. I'd be happy to recode as one function.
Created attachment 229787 [details] [review] plug-ins: port lcms to GEGL I took mitch's cleaned up version of elle's patch, and fixed a bunch of stuff. I definitely fixed a few small memory leaks, and simplified some of the code. I think I fixed the undo issues. I might have fixed the issues that elle was seeing with higher bit depths, but I'm not sure because I don't really know much about color management. I don't know what the results should look like. Elle, could you test the patch and see if it does what you expect? (You shouldn't need the modifications in babl any more, if I did this correctly.)
Yes, I will test the patch, and thank you! for looking into the undo issues, etc. I will report back by tomorrow, early afternoon at the latest.
Mike, your patch made a big improvement. Now all the file formats - indexed, 8-bit integer, 16-bit integer, 32-bit floating point - convert properly without needing to modify any babl files. The following two lines in "lcms_image_transform_rgb" should be flipped: const gboolean has_alpha = babl_format_has_alpha (layer_format); const Babl *layer_format = gimp_drawable_get_format (layer_id); I want to study your code - "iter_format" seems to be a big part of what was missing, and you simplified the code, too. But alas "undo" still doesn't work (regardless of whether babl files are modified or not).
Created attachment 229819 [details] [review] Combines function lcms_drawable_transform with lcms_image_transform_rgb This patch moves everything from lcms_drawable_transform to lcms_image_transform_rgb, and reverses two lines in Mike Henning's patch. At this point ICC profile conversions for all the supported image types - indexed (click "visibility" twice to actually see the converted image), 8-bit and 16-bit integer, and 32-bit - all work without modifying any babl files. "Undo" only works (and only if you click around a bit) for indexed images. For file formats other than indexed files, when attempting to "undo" the profile conversion what happens is the "source/from" ICC profile is assigned to the image, but the profile conversion isn't actually undone, so the RGB numbers are for the "destination/to" profile, not the source profile.
I think it's about time this continues in git. Undo is broken because you modify the deawable's pixels directly, with no chance for the undo system to know that something happened. You need to write to the drawable's shadow_buffer() and then call merge_shadow() to apply the changes. commit e27b70aaed61c67c03936aea206c769ccad867ea Author: Elle Stone <l.elle.stone@gmail.com> Date: Sun Nov 25 20:12:42 2012 +0100 Bug 662739 - Port to lcms2 for icc V4 profile support Patch from Elle, with bits from some others, that should finally do the trick. Please everybody test this. plug-ins/common/Makefile.am | 1 + plug-ins/common/lcms.c | 213 ++++++++++++++++++++++++---------------------------- plug-ins/common/plugin-defs.pl | 2 +- 3 files changed, 101 insertions(+), 115 deletions(-)
This should do the trick: commit d7ca2da6cec78b669bc5d8fd87f22e941f4b5f6d Author: Michael Natterer <mitch@gimp.org> Date: Sun Nov 25 20:21:11 2012 +0100 Bug 662739 - Port to lcms2 for icc V4 profile support Fix undo in the lcms plugin. plug-ins/common/lcms.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
Profile conversion works with single and multiple layers, but not with alpha channels (except for indexed images, which does work with alpha channel, and still requires "touching" something, such as changing a layer's visibility). Undo works (with indexed omages still requiring "touching" something, like layer visibitlity). I'm trying to figure out how to add alpha channel support back in.
Created attachment 229947 [details] [review] Hopefully this patch enables profile conversions with alpha channels, nd undo still works
Comment on attachment 199996 [details] [review] lcms2 transition patch There are too many patches in this bug. Marking this one as obsolete, because it got "applied" in some heavily changed form.
Review of attachment 229947 [details] [review]: That looks totally right, but I'm kindof shocked... I just skimmed the LCMS docs and while it does of course support pixel formats with alpha, it will simply always ignore them??? Don't we simply miss some flag that will copy the alpha channel instead of leaving holes in the output?
No, there is no flag. lcms ignores the alpha channel. Mike Henning's patch worked because the RGB channels were transformed by lcms and written back to the same buffer, leaving the alpha channel untouched. But using a src and dest/shadow buffer requires copying the alpha channel values over.
Wouldn't it be faster then to simply memcpy() src to dest before we run the transform and be done? Only if there is an alpha channel of course, and it would work for all pixel formats, without ever having to touch that code again. And is it just me, or are the LCMS docs written in a pretty weird way? They seem to make an effort to sound technical to the point of being incomprehensible ;)
memcpy() src to dest sounds like just the ticket (assuming I understand what memcpy does), and especially as probably more pixel formats will be added to the code, faster execution with less lines of code.
It might be even faster than that to gegl_buffer_copy the entirety of the main buffer to the shadow, and then modify only the shadow buffer. (especially if copy-on-write of GeglBuffers is implemented well)
Fixed the alpha issue. We might indeed want to use gegl_buffer_copy() later. commit 97e07e64767ad49fa6f6758ebf04a49e67c125df Author: Michael Natterer <mitch@gimp.org> Date: Wed Nov 28 00:44:46 2012 +0100 Bug 662739 - Port to lcms2 for icc V4 profile support Need to copy the alpha channel manually, lcms doesn't touch it. Fixes converting layers with alpha. plug-ins/common/lcms.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
In the last patch to the lcms.c plug-in, line 1084, "bpp" should read "layer_bpp".
Argh, never build, then reformat and push :( Thanks will fix.
Hm, did I simply forget to close this bug, or is there anything left to do to close it?
I've still got the port of my 1st patch on 2.6 to 2.8 pending at home. It causes some crash in some situations so I haven't send it yet, but I haven't got time to solve the issue neither. But I believe it only happens for some non ascii text in the profile, so it shouldn't be a show stopper.
I don't think it's a bug any more. There are some things that will improve/add to functionality (I'm planning on working on those items after the holidays), but none of the improvements/enhancements fall in the category of bug fixes, or so it seems to me. Did you take a look at the peculiar way indexed image "undo" works? Is that because of the lcms.c code or a general "undo" issue?
Thanks Elle. I don't really know what you mean, what exactly is peculiar about indexed image undo? Maybe you mean that the lcms plugin randomly crashes when converting indexed images' profiles, maybe causing inconsistencies? If yes, try again: commit 5818b9eac1432268ffa200113e66a3721cb1e41e Author: Michael Natterer <mitch@gimp.org> Date: Fri Dec 14 21:12:36 2012 +0100 plug-ins: fix indexed palette handling in lcms.c gimp_image_get/set_colormap() returns/takes the number of bytes in the colormap, not the number of colors. (cherry picked from commit b47107123a2d043169bf7df35016f52294489507) plug-ins/common/lcms.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
At one point indexed images acted oddly after a profile conversion - you had to do something else to get the conversion to look like it had happened, such as click the visibility icon, and the same for undoing the conversion. I just did git pull -rebase and rebuilt gimp - everything looks perfect. So whatever was causing the indexed images to be a little funny looks to be fixed.
Nice, thanks :)