New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Images are rendered incorrectly on wide gamut monitors #189

Closed
haasn opened this Issue Dec 1, 2014 · 36 comments

Comments

Projects
None yet
5 participants
@haasn

haasn commented Dec 1, 2014

sxiv does not perform any adaptation of images to different display gamuts, which has a very strong impact on wide gamut monitors.

Is there a possibility to integrate some form of at least simple gamut mapping in sxiv, or perhaps a fork that enables this functionality?

Something as easy as using a user-specified 3x3 matrix would be better than nothing.

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 1, 2014

Owner

I have no practical experience with wide gamut monitors. Does one really apply color modifications in every application or is it usually enough to load a color profile into the display server? I would merge code adding/changing color modifications into sxiv, however I will not write it myself.

Owner

muennich commented Dec 1, 2014

I have no practical experience with wide gamut monitors. Does one really apply color modifications in every application or is it usually enough to load a color profile into the display server? I would merge code adding/changing color modifications into sxiv, however I will not write it myself.

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 1, 2014

X11 has no concept of server side color management. The application is in charge of sending a correct image to the display, which is usually client side because it depends on the specifics of the content, e.g. a web browser displaying two images on the same page.

A completely correct (and simple) implementation would be to use an optional third party library like the portable, lightweight LittleCMS to transform the entire image to the display colorspace (which can be queried from X11 in theory, although there are multiple different standards in use - simpler to just require a path to the display profile as an argument) before displaying it.

An implementation would encompass the following lines:

  1. Load the display profile
  2. Load the image profile (if tagged) or fall back to sRGB
  3. Combine the two to form a transformation
  4. Transform the image buffer using it

All of this is trivial to implement using LittleCMS, which I have experience working with. I'd be willing to contribute the code if you (or someone else) is willing to review/fix/integrate it.

On December 1, 2014 9:22:09 AM GMT+01:00, "Bert Münnich" notifications@github.com wrote:

I have no practical experience with wide gamut monitors. Does one
really apply color modifications in every application or is it usually
enough to load a color profile into the display server? I would merge
code adding/changing color modifications into sxiv, however I will not
write it myself.


Reply to this email directly or view it on GitHub:
#189 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

haasn commented Dec 1, 2014

X11 has no concept of server side color management. The application is in charge of sending a correct image to the display, which is usually client side because it depends on the specifics of the content, e.g. a web browser displaying two images on the same page.

A completely correct (and simple) implementation would be to use an optional third party library like the portable, lightweight LittleCMS to transform the entire image to the display colorspace (which can be queried from X11 in theory, although there are multiple different standards in use - simpler to just require a path to the display profile as an argument) before displaying it.

An implementation would encompass the following lines:

  1. Load the display profile
  2. Load the image profile (if tagged) or fall back to sRGB
  3. Combine the two to form a transformation
  4. Transform the image buffer using it

All of this is trivial to implement using LittleCMS, which I have experience working with. I'd be willing to contribute the code if you (or someone else) is willing to review/fix/integrate it.

On December 1, 2014 9:22:09 AM GMT+01:00, "Bert Münnich" notifications@github.com wrote:

I have no practical experience with wide gamut monitors. Does one
really apply color modifications in every application or is it usually
enough to load a color profile into the display server? I would merge
code adding/changing color modifications into sxiv, however I will not
write it myself.


Reply to this email directly or view it on GitHub:
#189 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 1, 2014

Owner

On 01.12.2014 12:27, Niklas Haas wrote:

An implementation would encompass the following lines:

  1. Load the display profile
  2. Load the image profile (if tagged) or fall back to sRGB

From a file?

All of this is trivial to implement using LittleCMS, which I have experience working with. I'd be willing to contribute the code if you (or someone else) is willing to review/fix/integrate it.

If it's really that trivial, reviewing/integrating should be trivial too
and no fixes should be needed.

I'm not sure about adding another dependency, so almost all new code
should go into its own new file to keep the number of ifdefs to a minimum.

Owner

muennich commented Dec 1, 2014

On 01.12.2014 12:27, Niklas Haas wrote:

An implementation would encompass the following lines:

  1. Load the display profile
  2. Load the image profile (if tagged) or fall back to sRGB

From a file?

All of this is trivial to implement using LittleCMS, which I have experience working with. I'd be willing to contribute the code if you (or someone else) is willing to review/fix/integrate it.

If it's really that trivial, reviewing/integrating should be trivial too
and no fixes should be needed.

I'm not sure about adding another dependency, so almost all new code
should go into its own new file to keep the number of ifdefs to a minimum.

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 1, 2014

Display profile from a file is the easiest solution. Image profiles are embedded in the files, and the sRGB profile is constructed in memory (lCMS has a function for that).

On December 1, 2014 12:42:37 PM GMT+01:00, "Bert Münnich" notifications@github.com wrote:

On 01.12.2014 12:27, Niklas Haas wrote:

An implementation would encompass the following lines:

  1. Load the display profile
  2. Load the image profile (if tagged) or fall back to sRGB

From a file?

All of this is trivial to implement using LittleCMS, which I have
experience working with. I'd be willing to contribute the code if you
(or someone else) is willing to review/fix/integrate it.

If it's really that trivial, reviewing/integrating should be trivial
too
and no fixes should be needed.

I'm not sure about adding another dependency, so almost all new code
should go into its own new file to keep the number of ifdefs to a
minimum.


Reply to this email directly or view it on GitHub:
#189 (comment)

haasn commented Dec 1, 2014

Display profile from a file is the easiest solution. Image profiles are embedded in the files, and the sRGB profile is constructed in memory (lCMS has a function for that).

On December 1, 2014 12:42:37 PM GMT+01:00, "Bert Münnich" notifications@github.com wrote:

On 01.12.2014 12:27, Niklas Haas wrote:

An implementation would encompass the following lines:

  1. Load the display profile
  2. Load the image profile (if tagged) or fall back to sRGB

From a file?

All of this is trivial to implement using LittleCMS, which I have
experience working with. I'd be willing to contribute the code if you
(or someone else) is willing to review/fix/integrate it.

If it's really that trivial, reviewing/integrating should be trivial
too
and no fixes should be needed.

I'm not sure about adding another dependency, so almost all new code
should go into its own new file to keep the number of ifdefs to a
minimum.


Reply to this email directly or view it on GitHub:
#189 (comment)

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 2, 2014

I've looked at this a bit, seems like imlib2 doesn't really give us access to image tags/metadata.

Would need to patch imlib2 first. It's listed as legacy/imlib2 on the Enlightenment git. Do you know if it's maintained at all?

haasn commented Dec 2, 2014

I've looked at this a bit, seems like imlib2 doesn't really give us access to image tags/metadata.

Would need to patch imlib2 first. It's listed as legacy/imlib2 on the Enlightenment git. Do you know if it's maintained at all?

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 2, 2014

Owner

On 02.12.2014 09:38, Niklas Haas wrote:

I've looked at this a bit, seems like imlib2 doesn't really give us access to image tags/metadata.

Imlib2 abstracts everything away except pixels.

Would need to patch imlib2 first. Do you know if it's maintained at all?

Seems to be unmaintained for quite some time; didn't get a reply to my
mails a couple of years ago.

Can libexif be of any help?

Owner

muennich commented Dec 2, 2014

On 02.12.2014 09:38, Niklas Haas wrote:

I've looked at this a bit, seems like imlib2 doesn't really give us access to image tags/metadata.

Imlib2 abstracts everything away except pixels.

Would need to patch imlib2 first. Do you know if it's maintained at all?

Seems to be unmaintained for quite some time; didn't get a reply to my
mails a couple of years ago.

Can libexif be of any help?

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 2, 2014

Yes, seems like libexif exports the embedded profile as part of the EXIF specification. That's useful.

I see you load the image in libexif and immediately destroy it again in exif_auto_orientate. Would it be fine moving those few lines to img_load?

haasn commented Dec 2, 2014

Yes, seems like libexif exports the embedded profile as part of the EXIF specification. That's useful.

I see you load the image in libexif and immediately destroy it again in exif_auto_orientate. Would it be fine moving those few lines to img_load?

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 2, 2014

Owner

On 02.12.2014 09:58, Niklas Haas wrote:

I see you load the image in libexif and immediately destroy it again in exif_auto_orientate. Would it be fine moving those few lines to img_load?

I'd rather load the image multiple times via libexif instead of
spreading more #ifdefs throughout the code.
Also, moving it into img_load might break the code handling thumbnail
creation, which also calls exif_auto_orientate.

Owner

muennich commented Dec 2, 2014

On 02.12.2014 09:58, Niklas Haas wrote:

I see you load the image in libexif and immediately destroy it again in exif_auto_orientate. Would it be fine moving those few lines to img_load?

I'd rather load the image multiple times via libexif instead of
spreading more #ifdefs throughout the code.
Also, moving it into img_load might break the code handling thumbnail
creation, which also calls exif_auto_orientate.

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 2, 2014

For some reason, libexif doesn't even seem to be loading the file.

exif_data_new_from_file(file->path) returns NULL for all the files I've tested it on. Do you know what's going on there?

haasn commented Dec 2, 2014

For some reason, libexif doesn't even seem to be loading the file.

exif_data_new_from_file(file->path) returns NULL for all the files I've tested it on. Do you know what's going on there?

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 2, 2014

Owner

On 02.12.2014 10:51, Niklas Haas wrote:

For some reason, libexif doesn't even seem to be loading the file.

exif_data_new_from_file(file->path) returns NULL for all the files I've tested it on. Do you know what's going on there?

Do you refer to the original call in exif_auto_orientate, or is this
part of code you've already added? In case of the latter, please paste
your code, if you want help.

I've never had issues using libexif.

Owner

muennich commented Dec 2, 2014

On 02.12.2014 10:51, Niklas Haas wrote:

For some reason, libexif doesn't even seem to be loading the file.

exif_data_new_from_file(file->path) returns NULL for all the files I've tested it on. Do you know what's going on there?

Do you refer to the original call in exif_auto_orientate, or is this
part of code you've already added? In case of the latter, please paste
your code, if you want help.

I've never had issues using libexif.

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 2, 2014

It's part of the original code in place. I can reproduce it by making just the following change to exif_auto_orientate:

    if ((ed = exif_data_new_from_file(file->path)) == NULL) {
                printf("oops! %s\n", file->path);
        return;
        }

From testing on a few more files, it seems like the call fails if there isn't any EXIF data attached, and it seems like attached color profiles don't count as EXIF data, at least not the ones I've tested. Maybe there's a separate EXIF flag for color profiles that's independent from this, which is unused in my test files.

Note that, for example, the tool “exiftool” displays information about my files fine, including the embedded profile.

haasn commented Dec 2, 2014

It's part of the original code in place. I can reproduce it by making just the following change to exif_auto_orientate:

    if ((ed = exif_data_new_from_file(file->path)) == NULL) {
                printf("oops! %s\n", file->path);
        return;
        }

From testing on a few more files, it seems like the call fails if there isn't any EXIF data attached, and it seems like attached color profiles don't count as EXIF data, at least not the ones I've tested. Maybe there's a separate EXIF flag for color profiles that's independent from this, which is unused in my test files.

Note that, for example, the tool “exiftool” displays information about my files fine, including the embedded profile.

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 2, 2014

Owner

On 02.12.2014 11:18, Niklas Haas wrote:

From testing on a few more files, it seems like the call fails if there isn't any EXIF data attached, and it seems like attached color profiles don't count as EXIF data, at least not the ones I've tested. Maybe there's a separate EXIF flag for color profiles that's independent from this, which is unused in my test files.

Are you sure libexif gives you access to the ICC color profile at all?

Owner

muennich commented Dec 2, 2014

On 02.12.2014 11:18, Niklas Haas wrote:

From testing on a few more files, it seems like the call fails if there isn't any EXIF data attached, and it seems like attached color profiles don't count as EXIF data, at least not the ones I've tested. Maybe there's a separate EXIF flag for color profiles that's independent from this, which is unused in my test files.

Are you sure libexif gives you access to the ICC color profile at all?

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 2, 2014

It defines a tag EXIF_TAG_INTER_COLOR_PROFILE, which is a reference to Exif.Image.InterColorProfile (see http://www.exiv2.org/tags.html). I guess this tag is not being used in my test images, though.

I wonder if this kind of EXIF-based image tagging is a common occurrence for real world tagged images. I'll have to do some later research, or figure out another way of easily getting profile data without invoking more dependencies.

I never anticipated getting the image tags would be this involved; usually media libraries like ffmpeg expose all of this information.

haasn commented Dec 2, 2014

It defines a tag EXIF_TAG_INTER_COLOR_PROFILE, which is a reference to Exif.Image.InterColorProfile (see http://www.exiv2.org/tags.html). I guess this tag is not being used in my test images, though.

I wonder if this kind of EXIF-based image tagging is a common occurrence for real world tagged images. I'll have to do some later research, or figure out another way of easily getting profile data without invoking more dependencies.

I never anticipated getting the image tags would be this involved; usually media libraries like ffmpeg expose all of this information.

haasn added a commit to haasn/sxiv that referenced this issue Dec 2, 2014

Add color management support via LittleCMS
At the moment this just assumes everything is sRGB, which works for the
vast majority of files in existence, but ideally we'd want to respect
tagged profiles in the file headers.

libexif and imlib2 don't seem to read them, though.

This partially addresses muennich#189
@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 2, 2014

I believe the correct thing to do here would be to contribute extra code to read ICC profiles from tagged files as an optional LittleCMS plugin, perhaps using ffmpeg or some other heavier dependency. It would stay outside of sxiv itself that way.

As for sxiv itself, that means the code could be kept mostly as-is, but one thing I'm uneasy about is the fact that the device profile is loaded (and then freed) in img_apply_cms, which for large device profiles could dominate the performance of opening the image itself.

It should be more reasonable in theory to open the device profile on program launch and keep the cmsHPROFILE around until sxiv exits, but that would involve scattering a few more #ifdefs throughout the code. Are you too uncomfortable with that?

haasn commented Dec 2, 2014

I believe the correct thing to do here would be to contribute extra code to read ICC profiles from tagged files as an optional LittleCMS plugin, perhaps using ffmpeg or some other heavier dependency. It would stay outside of sxiv itself that way.

As for sxiv itself, that means the code could be kept mostly as-is, but one thing I'm uneasy about is the fact that the device profile is loaded (and then freed) in img_apply_cms, which for large device profiles could dominate the performance of opening the image itself.

It should be more reasonable in theory to open the device profile on program launch and keep the cmsHPROFILE around until sxiv exits, but that would involve scattering a few more #ifdefs throughout the code. Are you too uncomfortable with that?

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 7, 2014

Heh, looks like ImageMagick would be a simple way to get the tagged profile in a cross-format way.

I also realized that the proper place to put the display profile configuration should be in config.h.

haasn commented Dec 7, 2014

Heh, looks like ImageMagick would be a simple way to get the tagged profile in a cross-format way.

I also realized that the proper place to put the display profile configuration should be in config.h.

haasn added a commit to haasn/sxiv that referenced this issue Dec 7, 2014

Add color management support via LittleCMS
At the moment this just assumes everything is sRGB, which works for the
vast majority of files in existence, but ideally we'd want to respect
tagged profiles in the file headers.

libexif and imlib2 don't seem to read them, though.

This partially addresses muennich#189
@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 7, 2014

Owner

I will not merge a commit with a FIXME comment in it and I will not accept depending on ImageMagick.

If I am going to merge this in the end, then I will disable it by default, but I will also try to come up with some automatic way to include the appropriate CFLAGS and LIBS flags based on a non-empty DISPLAY_PROFILE setting in config.h.

Owner

muennich commented Dec 7, 2014

I will not merge a commit with a FIXME comment in it and I will not accept depending on ImageMagick.

If I am going to merge this in the end, then I will disable it by default, but I will also try to come up with some automatic way to include the appropriate CFLAGS and LIBS flags based on a non-empty DISPLAY_PROFILE setting in config.h.

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 7, 2014

Would you accept depending on libjpeg? It seems like that's the only common format with embedded profiles that people really care about at all, and most graphics professionals for who it really matters will absolutely not be using sxiv.

I've done an analysis of the files I have and I only have a handful of tagged jpeg files and photos, and that's pretty much it.

Come to think of it, manually parsing the JPEG header is also an option instead of depending on libjpeg, but I haven't look at it at all. If it's more complicated than, say, 5 lines of code, it's probably not worth it.

Edit: I've looked at it a bit, reading it manually is a nightmare, reading it with libjpeg is doable. There's official LittleCMS example code for this, which we could just scavenge (preferably into a separate file): https://github.com/mm2/Little-CMS/blob/master/utils/jpgicc/iccjpeg.c#L107

(This is exactly what chromium does for handling ICC profiles, incidentally - it includes a verbatim copy of that C file!)

I can't really figure out what license it's under, it's either public domain, MIT (LittleCMS's license) or under the IJG license (since somebody from the IJG authored it), both of which are extremely permissive. Simple matter of copying the license info into the file. Chromium seems to have decided it's under the IJG license, so that's what I'd go with.

haasn commented Dec 7, 2014

Would you accept depending on libjpeg? It seems like that's the only common format with embedded profiles that people really care about at all, and most graphics professionals for who it really matters will absolutely not be using sxiv.

I've done an analysis of the files I have and I only have a handful of tagged jpeg files and photos, and that's pretty much it.

Come to think of it, manually parsing the JPEG header is also an option instead of depending on libjpeg, but I haven't look at it at all. If it's more complicated than, say, 5 lines of code, it's probably not worth it.

Edit: I've looked at it a bit, reading it manually is a nightmare, reading it with libjpeg is doable. There's official LittleCMS example code for this, which we could just scavenge (preferably into a separate file): https://github.com/mm2/Little-CMS/blob/master/utils/jpgicc/iccjpeg.c#L107

(This is exactly what chromium does for handling ICC profiles, incidentally - it includes a verbatim copy of that C file!)

I can't really figure out what license it's under, it's either public domain, MIT (LittleCMS's license) or under the IJG license (since somebody from the IJG authored it), both of which are extremely permissive. Simple matter of copying the license info into the file. Chromium seems to have decided it's under the IJG license, so that's what I'd go with.

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 8, 2014

Owner

Conditionally depending on libjpeg would be fine.
We have to be certain about what license iccjpeg.c is under before including that file. We then have to also include a new section in the README.md for the copyright notice of that file.
Also, please extend the comment for DISPLAY_PROFILE in config.def.h, so that it at least gives an example path of where to typically find such a file.

Owner

muennich commented Dec 8, 2014

Conditionally depending on libjpeg would be fine.
We have to be certain about what license iccjpeg.c is under before including that file. We then have to also include a new section in the README.md for the copyright notice of that file.
Also, please extend the comment for DISPLAY_PROFILE in config.def.h, so that it at least gives an example path of where to typically find such a file.

haasn added a commit to haasn/sxiv that referenced this issue Dec 9, 2014

Add support for embedded EXIF and PNG ICC profiles
This continues to partially address muennich#189
@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 9, 2014

I've asked about the license upstream over at mm2/Little-CMS#37, and will wait for further updates before including JPEG support.

In the meantime, I've written code to handle files with EXIF metadata (libexif), as well as custom support for PNG files (since the format seemed simple enough, and this way I can be 100% sure that I support all edge cases properly), which turns out to require zlib (for DEFLATE support; but if you'd prefer not having the dependency there is a small, portable single-source-file drop in replacement available in the public domain called miniz).

I've extensively tested the PNG stuff on every combination of chunks to make sure the CMS bits are all working correctly, but there might still be some portability issues. I have little experience writing portable C code.

This commit moves the bulk of the effort over to a new file colorspace.c. Let me know what you think of the changes, please.

haasn commented Dec 9, 2014

I've asked about the license upstream over at mm2/Little-CMS#37, and will wait for further updates before including JPEG support.

In the meantime, I've written code to handle files with EXIF metadata (libexif), as well as custom support for PNG files (since the format seemed simple enough, and this way I can be 100% sure that I support all edge cases properly), which turns out to require zlib (for DEFLATE support; but if you'd prefer not having the dependency there is a small, portable single-source-file drop in replacement available in the public domain called miniz).

I've extensively tested the PNG stuff on every combination of chunks to make sure the CMS bits are all working correctly, but there might still be some portability issues. I have little experience writing portable C code.

This commit moves the bulk of the effort over to a new file colorspace.c. Let me know what you think of the changes, please.

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Dec 9, 2014

Owner

The amount of code and dependencies needed for this clearly exceeded the level of a trivial patchset.
As I am not able to fully test this, I would ask you to keep your fork alive and up to date for some time, so that I can merge this in the future, once it became clear that there are enough people using it.

Also, although there are no written coding style guidelines for sxiv, I would still ask you to try to adhere to the ones that are discernible from the rest of the code.

Owner

muennich commented Dec 9, 2014

The amount of code and dependencies needed for this clearly exceeded the level of a trivial patchset.
As I am not able to fully test this, I would ask you to keep your fork alive and up to date for some time, so that I can merge this in the future, once it became clear that there are enough people using it.

Also, although there are no written coding style guidelines for sxiv, I would still ask you to try to adhere to the ones that are discernible from the rest of the code.

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 9, 2014

I can do that, since I doubt there will be a single person alive who is interested in my patches other than myself. The “trivial” bit I was referring to is really just the stuff inside img_apply_cms, which is just calling a few LittleCMS functions.

As for coding styles, is there something specific that's bothering you? I can try to look at more code and adjust a bit in the future, if/when you plan on merging.

Is it okay with you to keep this issue alive for future feedback/comments/critique or would it be better to close it?

haasn commented Dec 9, 2014

I can do that, since I doubt there will be a single person alive who is interested in my patches other than myself. The “trivial” bit I was referring to is really just the stuff inside img_apply_cms, which is just calling a few LittleCMS functions.

As for coding styles, is there something specific that's bothering you? I can try to look at more code and adjust a bit in the future, if/when you plan on merging.

Is it okay with you to keep this issue alive for future feedback/comments/critique or would it be better to close it?

haasn added a commit to haasn/sxiv that referenced this issue Dec 9, 2014

Add support for embedded EXIF and PNG ICC profiles
This continues to partially address muennich#189
@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 10, 2014

I've contacted members of the IJG as well as Tom Lane about the license of iccjpeg.c, and it seems like file was originally intended to be included in libjpeg (back in 1997) Tom left the IJG before it got merged and nobody remembered it since.

It's back on the IJG's watchlist with low priority, and will be merged into libjpeg in the future. The IJG license is totally appropriate for it. I will include the file in my fork until it can eventually be removed.

haasn commented Dec 10, 2014

I've contacted members of the IJG as well as Tom Lane about the license of iccjpeg.c, and it seems like file was originally intended to be included in libjpeg (back in 1997) Tom left the IJG before it got merged and nobody remembered it since.

It's back on the IJG's watchlist with low priority, and will be merged into libjpeg in the future. The IJG license is totally appropriate for it. I will include the file in my fork until it can eventually be removed.

haasn added a commit to haasn/sxiv that referenced this issue Dec 10, 2014

Add support for embedded JPEG ICC profiles
This adds all missing functionality w.r.t color management and therefore
resolves muennich#189

I've also made the inclusion of colorspace. and iccjpeg.c conditional.
@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Dec 10, 2014

This patch implements the last of the missing functionality. If you want to easily see the result of this, you can use this rather pessimal tagged JPEG image as an example:

Browser color test image

I've also created a series of PNG test images, but I'm not sure how easy it is to reproduce those results without using a wide gamut monitor profile.

A very simple way to do this would be to set DISPLAY_PROFILE to something absurd like ProPhotoRGB.icc (probably available in /usr/share/color or on the Internet(tm)), which should, if everything's working correctly, make everything look extremely washed out. An exception would be these two test images, one of which is manually tagged with ProPhoto RGB's colorspace/gamma values using the cHRM and gAMA fields, and the other which simply includes an embedded ProPhotoRGB ICC profile:

Tagged cHRM+gAMA
Tagged iCCP

haasn commented Dec 10, 2014

This patch implements the last of the missing functionality. If you want to easily see the result of this, you can use this rather pessimal tagged JPEG image as an example:

Browser color test image

I've also created a series of PNG test images, but I'm not sure how easy it is to reproduce those results without using a wide gamut monitor profile.

A very simple way to do this would be to set DISPLAY_PROFILE to something absurd like ProPhotoRGB.icc (probably available in /usr/share/color or on the Internet(tm)), which should, if everything's working correctly, make everything look extremely washed out. An exception would be these two test images, one of which is manually tagged with ProPhoto RGB's colorspace/gamma values using the cHRM and gAMA fields, and the other which simply includes an embedded ProPhotoRGB ICC profile:

Tagged cHRM+gAMA
Tagged iCCP

@muennich

This comment has been minimized.

Show comment
Hide comment
@muennich

muennich Mar 28, 2015

Owner

I'm closing this. Might be subject of a future pull request.

Owner

muennich commented Mar 28, 2015

I'm closing this. Might be subject of a future pull request.

@muennich muennich closed this Mar 28, 2015

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Mar 28, 2015

Alright. For what it's worth, I'm very happy with the current implementation, although it's a bit of a hit on performance. I've added a flag to disable it manually, since sometimes I do want to use sxiv as a “simplistic just-show-the-colors” debugging mechanism. :)

haasn commented Mar 28, 2015

Alright. For what it's worth, I'm very happy with the current implementation, although it's a bit of a hit on performance. I've added a flag to disable it manually, since sometimes I do want to use sxiv as a “simplistic just-show-the-colors” debugging mechanism. :)

@SammysHP

This comment has been minimized.

Show comment
Hide comment
@SammysHP

SammysHP Apr 10, 2015

What's the status of this? I really like sxiv, but at the same time I need color management support.

As far as I can see these are the relevant commits:

0d68346 Add color management support via LittleCMS
b964c90 Add support for embedded EXIF and PNG ICC profiles
33d4209 Add support for embedded JPEG ICC profiles
b64c2b4 Add option to disable color management

Of course, this is no small modification and code style is not irrelevant. Nevertheless it would be a huge improvement for sxiv as there are only very few lightweight image viewers with color management support.

I'll create an Arch PKGBUILD for your fork, @haasn, and will test it. Would be great if you could keep your fork up-to-date and and if we could see this improvement upstream in the future.

SammysHP commented Apr 10, 2015

What's the status of this? I really like sxiv, but at the same time I need color management support.

As far as I can see these are the relevant commits:

0d68346 Add color management support via LittleCMS
b964c90 Add support for embedded EXIF and PNG ICC profiles
33d4209 Add support for embedded JPEG ICC profiles
b64c2b4 Add option to disable color management

Of course, this is no small modification and code style is not irrelevant. Nevertheless it would be a huge improvement for sxiv as there are only very few lightweight image viewers with color management support.

I'll create an Arch PKGBUILD for your fork, @haasn, and will test it. Would be great if you could keep your fork up-to-date and and if we could see this improvement upstream in the future.

@SammysHP

This comment has been minimized.

Show comment
Hide comment
@SammysHP

SammysHP Apr 10, 2015

I tested it and – what should I say? – nothing happened. The issue is this check:

https://github.com/haasn/sxiv/blob/master/image.c#L59

    if (*DISPLAY_PROFILE == '\0')
        return;

DISPLAY_PROFILE is empty of course. I cannot compile sxiv again every time I update my profile (it gets a new name). Other programs are able to get the profile from the system. Never looked into detail how this works, but I think it's not that easy (or will require some more dependencies). E.g. which monitor is used? Was sxiv moved to another monitor during runtime? …

Anyway, here's my PKGBUILD: https://gist.github.com/SammysHP/c4b2c989895670079693

SammysHP commented Apr 10, 2015

I tested it and – what should I say? – nothing happened. The issue is this check:

https://github.com/haasn/sxiv/blob/master/image.c#L59

    if (*DISPLAY_PROFILE == '\0')
        return;

DISPLAY_PROFILE is empty of course. I cannot compile sxiv again every time I update my profile (it gets a new name). Other programs are able to get the profile from the system. Never looked into detail how this works, but I think it's not that easy (or will require some more dependencies). E.g. which monitor is used? Was sxiv moved to another monitor during runtime? …

Anyway, here's my PKGBUILD: https://gist.github.com/SammysHP/c4b2c989895670079693

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Apr 10, 2015

Would be great if you could keep your fork up-to-date and and if we could see this improvement upstream in the future.

I am keeping it up to date.

Other programs are able to get the profile from the system. Never looked into detail how this works, but I think it's not that easy (or will require some more dependencies).

There are a few ways, the easiest of which seems to be using the _ICC_PROFILE atom from the root window. (For multiple monitors, the other ones are named _ICC_PROFILE_1 and so forth, iirc).

haasn commented Apr 10, 2015

Would be great if you could keep your fork up-to-date and and if we could see this improvement upstream in the future.

I am keeping it up to date.

Other programs are able to get the profile from the system. Never looked into detail how this works, but I think it's not that easy (or will require some more dependencies).

There are a few ways, the easiest of which seems to be using the _ICC_PROFILE atom from the root window. (For multiple monitors, the other ones are named _ICC_PROFILE_1 and so forth, iirc).

SammysHP added a commit to SammysHP/sxiv that referenced this issue Jul 3, 2016

Add support for embedded JPEG ICC profiles
This adds all missing functionality w.r.t color management and therefore
resolves muennich#189

I've also made the inclusion of colorspace. and iccjpeg.c conditional.
@cipriancraciun

This comment has been minimized.

Show comment
Hide comment
@cipriancraciun

cipriancraciun Aug 14, 2016

I'm curios what is the status of color management in sxiv?

After a quick look at this issue, and the 1.3.2 release changelog, it seems it's not yet available in upstream.

(@haasn It does seem that your fork ( https://github.com/haasn/sxiv ) contains some patches, but it's mixed with merges, and it's a little bit behind the current master.)

Are there any patches that cleanly apply over the current 1.3.2 release? (In the interim I'll try @haasn patches over the current release.)

Thanks,
Ciprian.

cipriancraciun commented Aug 14, 2016

I'm curios what is the status of color management in sxiv?

After a quick look at this issue, and the 1.3.2 release changelog, it seems it's not yet available in upstream.

(@haasn It does seem that your fork ( https://github.com/haasn/sxiv ) contains some patches, but it's mixed with merges, and it's a little bit behind the current master.)

Are there any patches that cleanly apply over the current 1.3.2 release? (In the interim I'll try @haasn patches over the current release.)

Thanks,
Ciprian.

cipriancraciun added a commit to cipriancraciun/sxiv that referenced this issue Aug 14, 2016

Add support for embedded JPEG ICC profiles
This adds all missing functionality w.r.t color management and therefore
resolves muennich#189

I've also made the inclusion of colorspace. and iccjpeg.c conditional.
@cipriancraciun

This comment has been minimized.

Show comment
Hide comment
@cipriancraciun

cipriancraciun Aug 14, 2016

OK, so I've cherry-picked @haasn patches over the current 1.3.2, and it seems they are working correctly (at least for JPEG and only I've after "light" testing).

The resulting branch is available in my fork, for those interested.

I have also added the -P switch to allow selecting the display profile per invocation. (Instead of only allowing it to be hard-coded in config.def.h under DISPLAY_PROFILE.)

As it stands now, I think it's quite enough for using sxiv in a color-managed environment. However it lacks the following two features:

  • choosing the desired rendering intent (currently it's hard-coded to relative colorimetric); (this would be useful especially on "poor" monitors, and it can easily be added as a switch;)
  • applying the color-management to thumbnails; (this is less useful than the above and much harder to achieve, especially since thumbnails are cached;)

@muennich Are you interested in eventually merging this feature in upstream? From the previous comments, I understand there are still some "issues" with this feature, but if one would to "perfect" it, would you be open in including it? (To be clear, I did not develop the color-management code, I've only cherry-picked @haasn commits, made a minor fixup, and added the command line switch, but I might find some time to address potential issues.)

Thanks both @haasn and @muennich
Ciprian.

cipriancraciun commented Aug 14, 2016

OK, so I've cherry-picked @haasn patches over the current 1.3.2, and it seems they are working correctly (at least for JPEG and only I've after "light" testing).

The resulting branch is available in my fork, for those interested.

I have also added the -P switch to allow selecting the display profile per invocation. (Instead of only allowing it to be hard-coded in config.def.h under DISPLAY_PROFILE.)

As it stands now, I think it's quite enough for using sxiv in a color-managed environment. However it lacks the following two features:

  • choosing the desired rendering intent (currently it's hard-coded to relative colorimetric); (this would be useful especially on "poor" monitors, and it can easily be added as a switch;)
  • applying the color-management to thumbnails; (this is less useful than the above and much harder to achieve, especially since thumbnails are cached;)

@muennich Are you interested in eventually merging this feature in upstream? From the previous comments, I understand there are still some "issues" with this feature, but if one would to "perfect" it, would you be open in including it? (To be clear, I did not develop the color-management code, I've only cherry-picked @haasn commits, made a minor fixup, and added the command line switch, but I might find some time to address potential issues.)

Thanks both @haasn and @muennich
Ciprian.

@SammysHP

This comment has been minimized.

Show comment
Hide comment
@SammysHP

SammysHP Aug 14, 2016

OK, so I've cherry-picked @haasn patches

I already did this: https://github.com/SammysHP/sxiv/tree/colormanagement

I have also added the -P switch

I programmed some code to read the profile from the X-atom. This is not perfect yet because it does not respect the active screen and does not cache the profile.

With the lcms implementation I noticed a huge drop in performance. So this would be something that must be improved, but maybe an improvement is not possible.

I also talked to @haasn some weeks ago and he said that he is not interested in development for sxiv anymore.

SammysHP commented Aug 14, 2016

OK, so I've cherry-picked @haasn patches

I already did this: https://github.com/SammysHP/sxiv/tree/colormanagement

I have also added the -P switch

I programmed some code to read the profile from the X-atom. This is not perfect yet because it does not respect the active screen and does not cache the profile.

With the lcms implementation I noticed a huge drop in performance. So this would be something that must be improved, but maybe an improvement is not possible.

I also talked to @haasn some weeks ago and he said that he is not interested in development for sxiv anymore.

@cipriancraciun

This comment has been minimized.

Show comment
Hide comment
@cipriancraciun

cipriancraciun Aug 14, 2016

I programmed some code to read the profile from the X-atom. This is not perfect yet because it does not respect the active screen and does not cache the profile.

For exactly this reason, of not being aware of which screen sxiv displays on, I would rather prefer explicitly giving the profile as an argument. (Furthermore I think in most cases sxiv will be integrated in various scripts, thus it's not a big deal of explicitly stating it as an argument.)

(As a side-note, even Firefox wants you to give the profile explicitly as a file via the gfx.color_management.display_profile configuration under about:config.)

With the lcms implementation I noticed a huge drop in performance. So this would be something that must be improved, but maybe an improvement is not possible.

I think the main reason for this performance drop, especially for large files, is that I think the same file is parsed multiple times (once to load its data, once to try to load its profile via libexif, and a third time in case libexif fails by failing back to libjpeg).

I don't think there is something that can be done at this moment, not without rewriting major parts of sxiv, which relies on imglib (from what I gather), which doesn't expose the meta-data, or the raw bytes of the file.

One solution is to cache the profile data per path (similar to the thumbnails), etc.

P.S.: Perhaps a (partial) rewrite of sxiv with focus on the specific use-case of photographic workflow would be a better idea in the long term. (I have some ideas...)

cipriancraciun commented Aug 14, 2016

I programmed some code to read the profile from the X-atom. This is not perfect yet because it does not respect the active screen and does not cache the profile.

For exactly this reason, of not being aware of which screen sxiv displays on, I would rather prefer explicitly giving the profile as an argument. (Furthermore I think in most cases sxiv will be integrated in various scripts, thus it's not a big deal of explicitly stating it as an argument.)

(As a side-note, even Firefox wants you to give the profile explicitly as a file via the gfx.color_management.display_profile configuration under about:config.)

With the lcms implementation I noticed a huge drop in performance. So this would be something that must be improved, but maybe an improvement is not possible.

I think the main reason for this performance drop, especially for large files, is that I think the same file is parsed multiple times (once to load its data, once to try to load its profile via libexif, and a third time in case libexif fails by failing back to libjpeg).

I don't think there is something that can be done at this moment, not without rewriting major parts of sxiv, which relies on imglib (from what I gather), which doesn't expose the meta-data, or the raw bytes of the file.

One solution is to cache the profile data per path (similar to the thumbnails), etc.

P.S.: Perhaps a (partial) rewrite of sxiv with focus on the specific use-case of photographic workflow would be a better idea in the long term. (I have some ideas...)

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Aug 14, 2016

@cipriancraciun As you mentioned, CMS in sxiv is more of an afterthought than anything, due to the way it has to parse images multiple times rather than extracting the profile to begin with. The problem as usual is imglib - which is where color management should go (and sxiv would only carry code for obtaining the profile and passing it to imglib). But seeing as imglib is deprecated and nobody seems to be interested in improving it, this feels like a dead end.

Personally I would like to kindly suggest any interest in implementing image-based color management should go into the widely more popular libraries ffmpeg/libavcodec instead - code in there to extract the embedded image profile or colorimetry information from image files would allow them to work in ffmpeg-based image viewers (such as mpv, which I personallly use these days), of which I think there are more than imglib-based image viewers.

(For those interested in the sxiv interface I think it would be a more sustainable project to rebase sxiv on top of ffmpeg instead of imglib - since the former is highly active and the latter is dead. The former also e.g. supports high-bit-depth images and displays, which is part of the reason I had to personally stop using sxiv, apart from simply supporting more formats in general)

haasn commented Aug 14, 2016

@cipriancraciun As you mentioned, CMS in sxiv is more of an afterthought than anything, due to the way it has to parse images multiple times rather than extracting the profile to begin with. The problem as usual is imglib - which is where color management should go (and sxiv would only carry code for obtaining the profile and passing it to imglib). But seeing as imglib is deprecated and nobody seems to be interested in improving it, this feels like a dead end.

Personally I would like to kindly suggest any interest in implementing image-based color management should go into the widely more popular libraries ffmpeg/libavcodec instead - code in there to extract the embedded image profile or colorimetry information from image files would allow them to work in ffmpeg-based image viewers (such as mpv, which I personallly use these days), of which I think there are more than imglib-based image viewers.

(For those interested in the sxiv interface I think it would be a more sustainable project to rebase sxiv on top of ffmpeg instead of imglib - since the former is highly active and the latter is dead. The former also e.g. supports high-bit-depth images and displays, which is part of the reason I had to personally stop using sxiv, apart from simply supporting more formats in general)

@cipriancraciun

This comment has been minimized.

Show comment
Hide comment
@cipriancraciun

cipriancraciun Aug 15, 2016

@haasn I understand that imglib is "dead" and perhaps another image library would be useful. But why do you suggest ffmpeg? What features does it have for image handling that other libraries lack? (From what I gather its main purpose is supporting video, thus although it also supports images, it might seem an overkill...)

(The only other image library I worked with, and which seemed extremely easy to use, was OpenCV.)

cipriancraciun commented Aug 15, 2016

@haasn I understand that imglib is "dead" and perhaps another image library would be useful. But why do you suggest ffmpeg? What features does it have for image handling that other libraries lack? (From what I gather its main purpose is supporting video, thus although it also supports images, it might seem an overkill...)

(The only other image library I worked with, and which seemed extremely easy to use, was OpenCV.)

@haasn

This comment has been minimized.

Show comment
Hide comment
@haasn

haasn Aug 15, 2016

@cipriancraciun I suggest ffmpeg because my preferred image viewer uses it :p

haasn commented Aug 15, 2016

@cipriancraciun I suggest ffmpeg because my preferred image viewer uses it :p

@drrossum

This comment has been minimized.

Show comment
Hide comment
@drrossum

drrossum Jun 8, 2017

@cipriancraciun, @haasn This extension works great and is exactly what I was looking for. This is a must for viewing photos on a wide-gamut screen.

drrossum commented Jun 8, 2017

@cipriancraciun, @haasn This extension works great and is exactly what I was looking for. This is a must for viewing photos on a wide-gamut screen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment