Custom ICC not read from PNG

[Beep6581]

5.3-357-ga31f522
libpng 1.6.34
cairo-1.14.10
cairomm-1.12.0

http://rawtherapee.com/shared/test_images/redblue.png
http://rawtherapee.com/shared/test_images/redblue.icc

RawTherapee does not display the colors in redblue.png correctly.
A warning is thrown in the console:

libpng warning: iCCP: Not recognizing known sRGB profile that has been edited

If you open the PNG and set the ICC manually, the colors are fine.

I found that libpng-1.6.* is more stringent in ICC checks than previous versions. The warning is probably being treated as a fatal error by RT and so the profile is not used.

Ref: https://discuss.pixls.us/t/favorite-test-images/6199/7?u=morgan_hardwood

[agriggio]

I'll take a look, thanks for the analysis!

[Beep6581]

http://www.libpng.org/pub/png/libpng-manual.txt

Error detection in some chunks has improved; in particular the iCCP chunk
reader now does pretty complete validation of the basic format. Some bad
profiles that were previously accepted are now accepted with a warning or
rejected, depending upon the png_set_benign_errors() setting, in particular
the very old broken Microsoft/HP 3144-byte sRGB profile. Starting with
libpng-1.6.11, recognizing and checking sRGB profiles can be avoided by
means of

#if defined(PNG_SKIP_sRGB_CHECK_PROFILE) && \
    defined(PNG_SET_OPTION_SUPPORTED)
   png_set_option(png_ptr, PNG_SKIP_sRGB_CHECK_PROFILE,
       PNG_OPTION_ON);
#endif

It's not a good idea to do this if you are using the "simplified API",
which needs to be able to recognize sRGB profiles conveyed via the iCCP
chunk.

The PNG spec requirement that only grayscale profiles may appear in images
with color type 0 or 4 and that even if the image only contains gray pixels,
only RGB profiles may appear in images with color type 2, 3, or 6, is now
enforced. The sRGB chunk is allowed to appear in images with any color type
and is interpreted by libpng to convey a one-tracer-curve gray profile or a
three-tracer-curve RGB profile as appropriate.

[agriggio]

so what's the bottom line? even if the profile is not strictly valid, I'd say we should try and use it anyway. do you agree?

[Beep6581]

I agree, RT should still try to load it.

By the way, this sort of problem, that the embedded ICC profile was found but not used, or that it was used but is known to be broken, is something that the user should be informed about, see #4261.

[Beep6581]

I couldn't find where PNG files get their profile from, maybe they don't?

ImageIO::loadJPEGFromMemory and ImageIO::loadJPEG have this:

    if (hasprofile) {
        embProfile = cmsOpenProfileFromMem (loadedProfileData, loadedProfileLength);
    } else {
        embProfile = nullptr;
    }

ImageIO::loadTIFF has this:

    if (TIFFGetField(in, TIFFTAG_ICCPROFILE, &loadedProfileLength, &profdata)) {
        embProfile = cmsOpenProfileFromMem (profdata, loadedProfileLength);
        loadedProfileData = new char [loadedProfileLength];
        memcpy (loadedProfileData, profdata, loadedProfileLength);
    } else {
        embProfile = nullptr;
    }

ImageIO::loadPNG has only this:

    embProfile = nullptr;

[agriggio]

This works for me, but I'm on libpng v1.2.57. @Beep6581 can you please test?

diff --git a/rtengine/imageio.cc b/rtengine/imageio.cc
--- a/rtengine/imageio.cc
+++ b/rtengine/imageio.cc
@@ -356,6 +356,18 @@
         png_set_strip_alpha(png);
     }
 
+    // reading the embedded ICC profile if any
+    if (png_get_valid(png, info, PNG_INFO_iCCP)) {
+        png_charp name;
+        int compression_type;
+        png_charp profdata;
+        png_uint_32 proflen;
+        png_get_iCCP(png, info, &name, &compression_type, &profdata, &proflen);
+        embProfile = cmsOpenProfileFromMem(profdata, proflen);
+        loadedProfileData = new char[proflen];
+        memcpy(loadedProfileData, profdata, proflen);
+    }
+
     //setting gamma
     double gamma;
 

[Beep6581]

@agriggio compilation fails using GCC-5.4.0 with:

[  5%] Building CXX object rtengine/CMakeFiles/rtengine.dir/imageio.cc.o
/home/morgan/programs/code-rawtherapee/rtengine/imageio.cc: In member function ‘int rtengine::ImageIO::loadPNG(Glib::ustring)’:
/home/morgan/programs/code-rawtherapee/rtengine/imageio.cc:365:78: error: invalid conversion from ‘char**’ to ‘png_bytepp {aka unsigned char**}’ [-fpermissive]
         png_get_iCCP(png, info, &name, &compression_type, &profdata, &proflen);
                                                                              ^
In file included from /usr/include/libpng16/png.h:377:0,
                 from /home/morgan/programs/code-rawtherapee/rtengine/imageio.cc:20:
/usr/include/libpng16/png.h:2151:30: note:   initializing argument 5 of ‘png_uint_32 png_get_iCCP(png_const_structrp, png_inforp, png_charpp, int*, png_bytepp, png_uint_32*)’
 PNG_EXPORT(158, png_uint_32, png_get_iCCP, (png_const_structrp png_ptr,
                              ^
/usr/include/libpng16/pngconf.h:287:70: note: in definition of macro ‘PNG_FUNCTION’
 #  define PNG_FUNCTION(type, name, args, attributes) attributes type name args
                                                                      ^
/usr/include/libpng16/pngconf.h:311:4: note: in expansion of macro ‘PNG_EXPORTA’
    PNG_EXPORTA(ordinal, type, name, args, PNG_EMPTY)
    ^
/usr/include/libpng16/png.h:2151:1: note: in expansion of macro ‘PNG_EXPORT’
 PNG_EXPORT(158, png_uint_32, png_get_iCCP, (png_const_structrp png_ptr,
 ^

[agriggio]

@Beep6581 libpng version?

[Beep6581]

Still libpng-1.6.34 ;]

[agriggio]

@Beep6581 try this please:

diff --git a/rtengine/imageio.cc b/rtengine/imageio.cc
--- a/rtengine/imageio.cc
+++ b/rtengine/imageio.cc
@@ -308,6 +308,11 @@
         return IMIO_HEADERERROR;
     }
 
+    // silence the warning about "invalid" sRGB profiles -- see #4260
+#if defined(PNG_SKIP_sRGB_CHECK_PROFILE) && defined(PNG_SET_OPTION_SUPPORTED)
+    png_set_option(png, PNG_SKIP_sRGB_CHECK_PROFILE, PNG_OPTION_ON);
+#endif
+
     png_infop info = png_create_info_struct (png);
     png_infop end_info = png_create_info_struct (png);
 
@@ -360,7 +365,11 @@
     if (png_get_valid(png, info, PNG_INFO_iCCP)) {
         png_charp name;
         int compression_type;
+#if PNG_LIBPNG_VER_MAJOR > 1 || (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR > 4)
+        png_bytep profdata;
+#else
         png_charp profdata;
+#endif
         png_uint_32 proflen;
         png_get_iCCP(png, info, &name, &compression_type, &profdata, &proflen);
         embProfile = cmsOpenProfileFromMem(profdata, proflen);

[Beep6581]

@agriggio do I apply this on top of the previous patch?

error: patch failed: rtengine/imageio.cc:360
error: rtengine/imageio.cc: patch does not apply

[Beep6581]

One applied on top of the other, and RT compiled!
Testing.

[Beep6581]

Works!

[agriggio]

great! If no objections, I'll push later today then