GNOME Bugzilla – Bug 604610
[patch] Add color management support to gdk_pixbuf_save
Last modified: 2012-11-15 21:42:26 UTC
Created attachment 149752 [details] [review] patch for comments The attached patch adds color management support for gdk_pixbuf_save. This allows applications to embed color data in the PNG image, which means that it can display perfectly even when on a color-managed workstation. The patch also adds gamma and white, red, green and blue chromatics information, as some simpler image editors (ones not using a CMM) only support the simple XYZ based colorspace transforms. I've deliberately kept this simple, as otherwise we would need to depend on a CMM in gdk (lcms for example), something which I'm not overly keen on doing. If we did depend on lcms, we could derive the color-title, color-gamma, color-white, color-red, color-green and color-blue parameters ourselves and not rely on the application to parse the ICC data and pass this to gdk. At the moment gdk is essentially a passthough to libpng, with next-to-no logic about what the values actually mean. I've tested this patch by encoding an image with my monitor ICC profile, and then displaying it in eog. eog honours the icc profile, and the image is suitably corrected. If you want me to include a demo application to showcase this, just ask, I've got some hacky code I've been using for testing that I can clean up. Anyway, patch is attached, I would love some feedback before I spend any more time on the reading bits. Thanks.
Also, for what it's worth, I think putting the CMM code (e.g. lcms) into gdk long term is a good idea, so that applications can just do simple transforms using a GdkImage. Otherwise, little libraries (or egg code) will spring up that allow people to use a CMM just to adapt a GdkImage to the display profile, or to change the rendering intent.
Review of attachment 149752 [details] [review]: If we support saving icc profiles, we probably also have to make embedded profiles available for loaded pngs, to make roundtrips possible. ::: gdk-pixbuf/gdk-pixbuf-io.c @@ +1909,3 @@ + * is only supported on some applications. The other options are easy to obtain + * if the appplication is willing to depend on a CMS, such as lcms. + * All parameters need to be listed here and the expected format needs to be spelled out, e.g. "color-white is expected to contain a pair of floating-point numbers, separated by a comma". Also, saying the other parameters are optional is somewhat misleading, since color-profile itself is optional too. Also, if you specify one of color-white, -red, -green or -blue, don't you have to specify all ? @@ +1920,3 @@ + * "color-blue", "0.745,0.145f", + * NULL); + * </programlisting></informalexample> The example would be better if it showed how to do the base64 encoding of a 'raw' icc profile. Otherwise people _will_ overlook this. Maybe even show how to derive the other information using lcms api. ::: gdk-pixbuf/io-png.c @@ +799,3 @@ + + /* get second number, ignoring seporator */ + value = endptr + 1; I am fairly sure that people will trip over whitespace here. So maybe be liberal about that. But you should check for the separator being a comma. Otherwise this code is probably going to accept things like 123a567 @@ +883,3 @@ + _("Titles for color profiles must have at least 1 and at most 79 characters.")); + success = FALSE; + goto cleanup; Here is what the png spec has to say about the title Profile names shall contain only printable Latin-1 characters and spaces (only character codes 32-126 and 161-255 decimal are allowed). Leading, trailing, and consecutive spaces are not permitted. There needs to be some more checking here to keep us from creating non-spec-conforming pngs... @@ +900,3 @@ + } + } else if (strcmp (*kiter, "color-gamma") == 0) { + color_gamma = atof (*viter); Danger, locale dependency ! Should use g_ascii_strtod here. @@ +1124,3 @@ + +#if defined(PNG_cHRM_SUPPORTED) + if (color_white_x > 0.0f) { Here, you implicitly assume that color-white being provided implies that the other colors are set as well. Better check... @@ +1138,3 @@ + /* fallback to a default value if this is not provided */ + if (color_title == NULL) + color_title = g_strdup ("display ICC profile"); "display" is slightly misleading here, I think. It is just the profile that was provided, there is no guarantee that it is related to any display characteristics. How about just "ICC profile".
Created attachment 149803 [details] [review] new patch Thanks for the review. In the new patch I've added the calls to gdk_pixbuf_set_option, and removed the gAMA and cHRM functionality completely. The latter needs more work to be useful, and I really want to get the profile support in before I start doing the more complicated bits. I want to also add writing profiles to jpeg too before I add the other entries, so we can choose better formats (more flexible) for the floating point data (e.g. using XYZ rather than Yxy) before the ABI is locked in stone. I've also added a simple test in demos, which illustrates the reading and writing of profiles. When I've got the ack on this patch I'll start looking at jpeg, and then the more exotic formats like cHRM. Thanks.
Review of attachment 149803 [details] [review]: Looks good in general; much better without the extra options. I wonder if we can get rid of the color-title option too ? Is there much benefit to it ? For apps that actually use the profile, the 'desc' tag inside the profile is just as good, no ? And looking at the ICC profile spec, embedding profiles in jpeg or tiff images does not use a separate title. ::: gdk-pixbuf/io-png.c @@ +349,3 @@ + } +#endif + Unfortunately, the pixbuf loader code sucks and has another place where gdk_pixbuf_set_option needs to be called. See png_info_callback further down. @@ +909,3 @@ + goto cleanup; + } + color_title = g_strdup (*viter); Here color_title gets allocated... @@ +1083,2 @@ + g_free (color_profile); + ...should it get freed here ?
(In reply to comment #4) > I wonder if we can get rid of the color-title option too ? Is there much > benefit to it ? Looking at various png decoding apps, nothing seems to use it. > And looking at the ICC profile spec, > embedding profiles in jpeg or tiff images does not use a separate title. Right, makes sense. There's a nice unicode encoded description in the encoded profile, like you say. > Unfortunately, the pixbuf loader code sucks and has another place where > gdk_pixbuf_set_option needs to be called. See png_info_callback further down. Ahh, missed that. New patch coming up.
Created attachment 149906 [details] [review] new patch, hopefully good to commit
Actually, thinking about it, we should probably call the "color-profile" name "icc-profile" as that's what it is. It's defined by the ICC spec.
I've also added the icc-profile decoding and encoding into io-tiff.c and io-jpeg.c although I would rather submit these as separate patches for review. I've tested embedding (and retrieving) a CMYK 2Mb icc profile as a stress test on all image formats, and it seems to work flawlessly. When the PNG patch is reviewed I'll submit the tiff patch (trivial) and the jpeg patch (not at all trivial, needs careful review and fuzzing re-testing). Thanks, Richard.
Review of attachment 149906 [details] [review]: Looks good to commit if you remove the unused function and do the color-profile -> icc-profile renaming ::: gdk-pixbuf/io-png.c @@ +819,3 @@ } +static gboolean This function is unused now and should be removed.
Review of attachment 149906 [details] [review]: Thanks, changes made.
Created attachment 150006 [details] [review] icc-profile for tiff formats This is pretty trivial, please review.
Created attachment 150007 [details] [review] icc-profile for jpeg formats This one is much more involved, as I had to rewrite part of the exif decoder to be able to get the profile data. The data is chunked in an odd way, so the ICC spec (version 4) is recommended reading if you want to review it. Thanks.
Created attachment 150008 [details] [review] documentation update Trivial, depends on the first two patches.
Created attachment 150009 [details] [review] trivial, adds example code for the tiff and jpeg demo program
Review of attachment 150006 [details] [review]: Looks fine to me. Would be nice to make the test program verify tiff load/save as well. I also note that the docs for gdk_pixbuf_save don't mention that tiff can be saved; might be nice to fix that in passing.
Comment on attachment 150006 [details] [review] icc-profile for tiff formats I've added this, along with the doc and demo parts of the 4th patch. Thanks for the review.
Created attachment 150161 [details] [review] new jpeg patch This is a new patch for jpeg icc-profile support, resynced against git master, and with the test and doc additions rolled into a single patch. Please review. Thanks.
Review of attachment 150161 [details] [review]: ::: gdk-pixbuf/io-jpeg.c @@ +1273,3 @@ + } + goto cleanup; + retval = FALSE; A little comment, use G_GSIZE_FORMAT conversion specifier for printing values of type gsize
The following fix has been pushed: 0179dfd Add icc-profile option to gdk-pixbuf for the JPEG image format
Created attachment 223343 [details] [review] Add icc-profile option to gdk-pixbuf for the JPEG image format This makes it possible to provide a color profile when saving jpeg files and to read color profile information when loading it.
Unfortunately it seems like the 0179dfd commit broke EXIF parsing in JPEGs. Please see https://bugs.launchpad.net/ubuntu/+source/gdk-pixbuf/+bug/1077186
Looks like upstream gdk-pixbuf developers do not do proper branching for reasons I don't know, and I agree that "The commit is rather invasive, adds a new feature, and seriously, I question how this got into a "bug fix release"!" https://bugs.launchpad.net/ubuntu/+source/gdk-pixbuf/+bug/1077186/comments/10 is the interesting downstream comment.
I don't know what kind of "contract" you have with the Ubuntu developers, but, although they can be pretty anal about getting even simple bug fixes in after a release, they hand-wave in all Gnome micro releases without - apparently - no or little review. Maybe they will have to reconsider this practice. https://wiki.ubuntu.com/StableReleaseUpdates/MicroReleaseExceptions Anyway, it looks from my code analysis that this regression touches JPEG photos in big-endian format. If this bug fix release was thoroughly tested, but with little-endian JPEGs, that can excuse things. However, I guess you should add big-endian photos to your regression tests. The fix here is simple, reading out the orientation tag value was wrongly changed from de_get16() to de_get32() in the above commit. I will put together a patch including comments and references and post it in a new bug report (unless you want it here or some mailing list instead).
Patch attached to bug #688427.