After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 478528 - Layer and Image previews are not color managed
Layer and Image previews are not color managed
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: User Interface
2.4.x
Other All
: High normal
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
: 727038 760761 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-09-20 09:23 UTC by Dennis Gnad
Modified: 2016-04-15 15:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
use cmsFLAGS_COPY_ALPHA (4.29 KB, patch)
2015-09-22 16:54 UTC, Massimo
none Details | Review

Description Dennis Gnad 2007-09-20 09:23:20 UTC
Please describe the problem:
When opening an image with color managment the little preview in the layer overview does not use color managment!

Steps to reproduce:
1. Open http://www.fc-prints.de/fileadmin/daten/fcPrints_Testbild_100ppi.jpg
2. Choose to KEEP the profile
3. Look at the image in big and the thumbnail in the Layer Overview


Actual results:
The image in the layer overview does show a non color managed image which is different to the one in big.

Expected results:
Both the Image in normal size and in the layers dialog have the same colors.

Does this happen every time?
Yes

Other information:
Comment 1 Dennis Gnad 2007-09-20 09:31:36 UTC
I am very sorry, it is much more better visible here (and I intended to post this first):

http://www.fc-prints.de/fileadmin/daten/farbkreis.jpg
Comment 2 Sven Neumann 2007-09-20 11:35:31 UTC
Yes, and we aren't going to change that, at least not in the near future.
Comment 3 Sven Neumann 2007-09-20 11:38:55 UTC
You are supposed to convert to sRGB. Working in other color spaces is strongly discouraged until all parts of GIMP have been made aware of color management. This is not likely going to happen anytime soon. See also bug #467930.
Comment 4 kenkyee 2009-05-13 16:12:41 UTC
Assuming the layer overview preview image uses the same preview widget that the rest of gimp users, this is probably a bug in the preview widget (same as bug 556608).
Comment 5 Michael Natterer 2014-04-11 15:59:40 UTC
*** Bug 727038 has been marked as a duplicate of this bug. ***
Comment 6 Michael Natterer 2015-09-03 00:29:33 UTC
Fixed in master. Does not fix previews in plug-ins, which is tracked
in bug 556608.

commit 09fed0a0e5553e1febe41fe358b97860c45eb9d8
Author: Michael Natterer <mitch@gimp.org>
Date:   Thu Sep 3 02:25:51 2015 +0200

    Bug 478528 - Layer and Image previews are not color managed
    
    Implement color management in GimpViewRenderer: if the viewable is a
    GimpColorManaged (true for images and layers), keep around a
    GimpColorTransform and convert the preview image to display colors.

 app/widgets/gimpviewrenderer.c | 128 +++++++++++++++++++++++++++++++++++------
 app/widgets/gimpviewrenderer.h |   4 ++
 2 files changed, 116 insertions(+), 16 deletions(-)
Comment 7 Massimo 2015-09-06 06:51:17 UTC
(In reply to Michael Natterer from comment #6)
> Fixed in master. Does not fix previews in plug-ins, which is tracked
> in bug 556608.
> 
> commit 09fed0a0e5553e1febe41fe358b97860c45eb9d8
> Author: Michael Natterer <mitch@gimp.org>
> Date:   Thu Sep 3 02:25:51 2015 +0200
> 
>     Bug 478528 - Layer and Image previews are not color managed
>     
>     Implement color management in GimpViewRenderer: if the viewable is a
>     GimpColorManaged (true for images and layers), keep around a
>     GimpColorTransform and convert the preview image to display colors.
> 
>  app/widgets/gimpviewrenderer.c | 128
> +++++++++++++++++++++++++++++++++++------
>  app/widgets/gimpviewrenderer.h |   4 ++
>  2 files changed, 116 insertions(+), 16 deletions(-)

Since this commit layer previews in the preview dialog are
skewed.

The problem is here:

https://git.gnome.org/browse/gimp/tree/libgimpcolor/gimpcolorprofile.c#n963

The babl_format "cairo-RGB24" has babl_model "R'G'B'", that
means that its 'else if' branch is never executed and this
function return babl and lcms formats incompatible (cairo-RGB24
has a padding byte).

A solution could be to move the "cairo-RGB24" 'else if' before
the "R'G'B'" 'else if'.
Comment 8 Michael Natterer 2015-09-06 12:00:16 UTC
Argh of course. I looked at this code like 20 times to track down the bug,
but only tested alpha layers, which have a premultiplied Babl model, and
are not affected by the bug... Will fix, thanks!
Comment 9 Michael Natterer 2015-09-06 12:16:01 UTC
Fixed in master:

commit f9da5cc6ccaa8fa2ad4ec277837312ecba7101cb
Author: Michael Natterer <mitch@gimp.org>
Date:   Sun Sep 6 14:12:58 2015 +0200

    Bug 478528 - Layer and Image previews are not color managed
    
    Fix gimp_color_profile_get_format() to return the right format for
    "cairo-RGB24": simply check for the more special cairo formats first,
    then check for the general RGB models. Found by Massimo.

 libgimpcolor/gimpcolorprofile.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
Comment 10 Massimo 2015-09-20 08:36:06 UTC
The layer previews for images in linear precision are
wrong, here:

https://git.gnome.org/browse/gimp/tree/app/widgets/gimpviewrenderer.c#n1265

the configured image profile is used, for example
'GIMP built-in Linear RGB', but the data that is passed
to lcms has been converted to "R'G'B'/A u8" because
'gimp_drawable_get_preview_format' always returns a gamma
corrected format:

https://git.gnome.org/browse/gimp/tree/app/core/gimpdrawable-preview.c#n63

similarly for the projection previews:

https://git.gnome.org/browse/gimp/tree/app/core/gimpimage-preview.c#n115


Another source of errors generating previews (as seen
with Brasseur's dalay lama jpegs) is GEGL computing the 
pyramid in the buffer color space, but that's another 
issue probably already tracked in a GEGL bug report.
Comment 11 Massimo 2015-09-20 12:07:56 UTC
Layer previews in the layers dialog do not update
correctly after adding an alpha channel and erasing
an elliptical selection.

steps to reproduce:

A new opaque image (R'G'B' u8)           <Ctrl>-n <Enter>
Layer->Transparency->Add alpha Channel   <Alt>-l a h
Select Elliptical tool                   E
Select an ellipse and press <Del>

the preview is not updated, whereas gimp-2.8 shows the
checkerboard in the selected area.

BTW lcms from master adds a cmsFLAGS_COPY_ALPHA flag
so that a transform copies also the alpha channel probably
simplifying GIMP code in few places
Comment 12 Michael Natterer 2015-09-20 19:01:36 UTC
Indeed, I forgot about the preview_format() function.

About wrong updates when adding/removing alpha, that won't be fixed by
cmsFLAGS_COPY_ALPHA because we realy need a new transform here, the
pixel format has changed.
Comment 13 Michael Natterer 2015-09-20 19:24:31 UTC
I just pushed this for fixing color picking when there is a profile:

commit 9550fbff3cdfbd6ee037f880e0ec711be5b49275
Author: Michael Natterer <mitch@gimp.org>
Date:   Sun Sep 20 21:17:54 2015 +0200

    Bug 748749 - picked colors don't match image colors...
    
    ...when a color profile is active
    
    This commit adds more (still unused) infrastructure to fix this bug:
    
    Ee now keep around color transforms from layer pixels to "R'G'B'A
    double" (which is GimpRGB's format) and back. Also add utility
    function gimp_image_color_profile_pixel_to_srgb() which converts a
    picked pixel to GimpRGB, using the cached color transform.

 app/core/gimpimage-color-profile.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 app/core/gimpimage-color-profile.h |   9 ++++-
 app/core/gimpimage-private.h       |   5 +++
 app/core/gimpimage.c               |   5 +--
 4 files changed, 118 insertions(+), 7 deletions(-)


Which makes me thing that maybe we should simply convert image pixels
to sRGB before handing them out as previews, and in GimpViewRenderer
only convert from sRGB to the monitor profile. this would involve
two conversions per peview though, and I don't really like that.

Any comments?
Comment 14 Massimo 2015-09-21 04:51:42 UTC
(In reply to Michael Natterer from comment #12)
> Indeed, I forgot about the preview_format() function.
> 
> About wrong updates when adding/removing alpha, that won't be fixed by
> cmsFLAGS_COPY_ALPHA because we realy need a new transform here, the
> pixel format has changed.

(In reply to Michael Natterer from comment #13)
> I just pushed this for fixing color picking when there is a profile:
>
... 
> 
> Which makes me thing that maybe we should simply convert image pixels
> to sRGB before handing them out as previews, and in GimpViewRenderer
> only convert from sRGB to the monitor profile. this would involve
> two conversions per peview though, and I don't really like that.
> 
> Any comments?

My comment on the cmsFLAGS_COPY_ALPHA was based on the observation
that there are already many conversions per view because of the 
alpha copy done in GIMP:

https://git.gnome.org/browse/gimp/tree/app/gegl/gimp-gegl-loops.c#n753

when the destination buffer is a wrapper around a cairo surface
with alpha channel

gegl_buffer_copy performs one conversion and gegl_buffer_iter_next
with READ_WRITE two.

I tried to use cmsFLAGS_COPY_ALPHA, but it appeared to do nothing
because the renderer format is not updated promptly
Comment 15 Michael Natterer 2015-09-21 18:24:06 UTC
This fixes preview rendering when alpha changes, now you can try
the cmsFLAGS_COPY_ALPHA which will save us copies once we can use
the new lcms. The same separate alpha copying code is in all places
that do an actual lcms transform, which really sucks.

commit 35367359136ef51fb6db8120ca0d0341ae6c32b2
Author: Michael Natterer <mitch@gimp.org>
Date:   Mon Sep 21 20:20:02 2015 +0200

    Bug 478528 - Layer and Image previews are not color managed
    
    Fix color managed layer previews when adding/removing alpha:
    
    Implement GimpDrawable::alpha_changed() in GimpLayer and emit
    GimpColorManaged::profile_changed() so all cached color transforms are
    nuked.

 app/core/gimplayer.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)
Comment 16 Michael Natterer 2015-09-21 18:31:14 UTC
That was too quick, the commit is 616c57b8cf94283e85fccaa5a7847c2cb251e3b8.
Comment 17 Massimo 2015-09-22 16:54:26 UTC
Created attachment 311890 [details] [review]
use cmsFLAGS_COPY_ALPHA

(In reply to Michael Natterer from comment #15)
> This fixes preview rendering when alpha changes, now you can try
> the cmsFLAGS_COPY_ALPHA which will save us copies once we can use
> the new lcms. The same separate alpha copying code is in all places
> that do an actual lcms transform, which really sucks.
> 

Thanks, attached the patch showing that GIMP code is simplified
when lcms 2.8 cmsFLAGS_COPY_ALPHA will be available.

It seems to work, but benchmarking the projection
(File->Debug->Benchmark Projection) it shows slight
improvement only for images in half linear mode where
gimp_display_shell_profile_convert_buffer executes
a gamma conversion to just copy the alpha channel and
falls back to a reference babl_fish.
Comment 18 Michael Natterer 2015-09-22 18:23:25 UTC
Yeah I don't expect huge improvements, but that separate copying is
just too ugly, I look forward to depend on the new lcms :)
Comment 19 Michael Natterer 2016-01-18 00:47:29 UTC
*** Bug 760761 has been marked as a duplicate of this bug. ***
Comment 20 Michael Natterer 2016-03-27 17:11:51 UTC
This is a real bug now...
Comment 21 Michael Natterer 2016-04-15 15:57:10 UTC
Fixed again in master:

commit 9e8499bb48c8ceda5864e0b877ffb7a43a22d605
Author: Michael Natterer <mitch@gimp.org>
Date:   Fri Apr 15 16:52:25 2016 +0100

    Bug 478528 - Layer and Image previews are not color managed
    
    Change gimp_viewable_get_[new]_preview() to return buffers of the
    image's and layers' colorspace, but in u8. This way layer and image
    previews can transform them correctly to the display profile.
    
    Note: this makes plug-ins receive thumbnail buffers in that
    pixel format too.
    
    Also change gimp_viewable_get_[new]_pixbuf() to always return sRGB
    buffers that can reasonably be put to screen directly, or put into DND
    buffers. This is at least more correct now.

 app/core/gimpdrawable-preview.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 app/core/gimpdrawable-preview.h |  14 +++++-
 app/core/gimpdrawable.c         |   1 +
 app/core/gimpimage-preview.c    |  83 +++++++++++++++++++++++++++++++-
 app/core/gimpimage-preview.h    |   4 ++
 app/core/gimpimage.c            |   1 +
 6 files changed, 244 insertions(+), 8 deletions(-)