Chalk crashes when loading a PNG it saved #20

This is building chalk with libpng 1.6 - no problem when built with libpng 1.4.

Open chalk and create an image ..
The layer 1 data shown is:

Colorspace: RGB (8-bit integer/channel)
Profile: sRGB built-in - (lcms internal)

.. and save or export as a PNG.
A console message is shown:

libpng warning: profile matches sRGB but writing iCCP instead

Load that image and chalk crashes.

Load that image into Gimp and this message is shown:

The image 'image.png' has an embedded colour profile
sRGB built-in
Convert the image to the RGB working space (sRGB built-in)?

Select Convert, and chalk will open the PNG.
Select Keep, and chalk will crash when trying to open the PNG.

Please, can you attach backtrace?

Backtrace attached

TDE_crash_handler_backtrace.txt

34 KiB

There is a very suspicious comment "// XXX: Hardcoded for icc type -- is that correct for us?" at kis_png_converter.cpp:245. That definitely points to the reason.

And even more, the code contains several parts like

#if PNG_LIBPNG_VER_MAJOR > 1 || ( PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 5 )

which will do different things for 1.6 and 1.4 I suppose.

I tried a build with libpng-1.5.30, and that loads a saved png ok, so it looks as though the problem was introduced with libpng-1.6.

For libpng 1.6.37, pngwrite.c has this snippet ..

#ifdef PNG_COLORSPACE_SUPPORTED
      /* Write only one of sRGB or an ICC profile.  If a profile was supplied
       * and it matches one of the known sRGB ones issue a warning.
       */
#  ifdef PNG_WRITE_iCCP_SUPPORTED
         if ((info_ptr->colorspace.flags & PNG_COLORSPACE_INVALID) == 0 &&
             (info_ptr->valid & PNG_INFO_iCCP) != 0)
         {
#    ifdef PNG_WRITE_sRGB_SUPPORTED
               if ((info_ptr->valid & PNG_INFO_sRGB) != 0)
                  png_app_warning(png_ptr,
                      "profile matches sRGB but writing iCCP instead");
#     endif

            png_write_iCCP(png_ptr, info_ptr->iccp_name,
                info_ptr->iccp_profile);
         }
#     ifdef PNG_WRITE_sRGB_SUPPORTED
         else
#     endif
#  endif

As it's an iccp profile which seems to be causing the problem, I looked at what could be redefined ..
PNG_WRITE_iCCP_SUPPORTED is set in libpng [pnglibconf.h], so I didn't think anything could be done about that as it's a system file.
The next option is PNG_INFO_iCCP, which according to the libpng-manual can be set invalid for an application.

The png_free_data() function will turn off the "valid" flag for anything
it frees.  If you need to turn the flag off for a chunk that was freed by
your application instead of by libpng, you can use

    png_set_invalid(png_ptr, info_ptr, mask);

    mask - identifies the chunks to be made invalid,
           containing the bitwise OR of one or
           more of
             ..
             PNG_INFO_iCCP, ..

So I added png_set_invalid() to kis_png_converter.cpp

--- filters/chalk/png/kis_png_converter.cpp
+++ filters/chalk/png/kis_png_converter.cpp
@@ -202,4 +202,5 @@
     // read all PNG info up to image data
     png_read_info(png_ptr, info_ptr);
+    png_set_invalid(png_ptr, info_ptr, PNG_INFO_iCCP);
 
     // Read information about the png

I still get the libpng warning message, and the prompt from the Gimp, so I don't understand why this works, but the saved image does then load without chalk crashing.

This probably isn't the best solution, but hopefully it might trigger an idea from someone who has a better understanding of what needs to be done here.

I tried a build with libpng-1.5.30, and that loads a saved png ok, so it looks as though the problem was introduced with libpng-1.6. For libpng 1.6.37, pngwrite.c has this snippet .. ``` #ifdef PNG_COLORSPACE_SUPPORTED /* Write only one of sRGB or an ICC profile. If a profile was supplied * and it matches one of the known sRGB ones issue a warning. */ # ifdef PNG_WRITE_iCCP_SUPPORTED if ((info_ptr->colorspace.flags & PNG_COLORSPACE_INVALID) == 0 && (info_ptr->valid & PNG_INFO_iCCP) != 0) { # ifdef PNG_WRITE_sRGB_SUPPORTED if ((info_ptr->valid & PNG_INFO_sRGB) != 0) png_app_warning(png_ptr, "profile matches sRGB but writing iCCP instead"); # endif png_write_iCCP(png_ptr, info_ptr->iccp_name, info_ptr->iccp_profile); } # ifdef PNG_WRITE_sRGB_SUPPORTED else # endif # endif ``` As it's an iccp profile which seems to be causing the problem, I looked at what could be redefined .. PNG_WRITE_iCCP_SUPPORTED is set in libpng [pnglibconf.h], so I didn't think anything could be done about that as it's a system file. The next option is PNG_INFO_iCCP, which according to the libpng-manual can be set invalid for an application. ``` The png_free_data() function will turn off the "valid" flag for anything it frees. If you need to turn the flag off for a chunk that was freed by your application instead of by libpng, you can use png_set_invalid(png_ptr, info_ptr, mask); mask - identifies the chunks to be made invalid, containing the bitwise OR of one or more of .. PNG_INFO_iCCP, .. ``` So I added png_set_invalid() to kis_png_converter.cpp ```diff --- filters/chalk/png/kis_png_converter.cpp +++ filters/chalk/png/kis_png_converter.cpp @@ -202,4 +202,5 @@ // read all PNG info up to image data png_read_info(png_ptr, info_ptr); + png_set_invalid(png_ptr, info_ptr, PNG_INFO_iCCP); // Read information about the png ``` I still get the libpng warning message, and the prompt from the Gimp, so I don't understand why this works, but the saved image does then load without chalk crashing. This probably isn't the best solution, but hopefully it might trigger an idea from someone who has a better understanding of what needs to be done here.

Hi @Ray-V,
I may be blatantly wrong, I only had a quick high level look but IMO I don't think the problem is the lib itself, it's in the way we save and reload the image.
I have not spent much time looking into this, but basically what we are doing (correct me if I am wrong) is that we are saving an image with sRGB and when we reload it the code uses iCCP (hard coded in the code apparently).

Recoding for sRGB instead of iCCP is not something I can do, so I'll use the patch, modified for libpng 1.6 which is causing the crash, until a better fix is available.

--- filters/chalk/png/kis_png_converter.cpp
+++ filters/chalk/png/kis_png_converter.cpp
@@ -202,4 +202,7 @@
     // read all PNG info up to image data
     png_read_info(png_ptr, info_ptr);
+#if PNG_LIBPNG_VER >= 10600
+    png_set_invalid(png_ptr, info_ptr, PNG_INFO_iCCP);
+#endif
 
     // Read information about the png

I've tried the saved/exported PNGs in a number of image viewers, and they display ok. The messages I've seen are only apparent if chalk is run from a console, or the image is loaded into the Gimp, so I don't think that will be an issue.

From what I can gather, there is now no 'standard' sRGB profile, so what would be the default sRGB profile?
These make interesting reading, and if the latter is still relevant would there be any benefit in setting a default sRGB profile?
https://ninedegreesbelow.com/photography/srgb-history.html
https://ninedegreesbelow.com/photography/srgb-bad-monitor-profile.html

If it is necessary to convert any image to the RGB working space, there are a number of profiles included with chalk [for example share/apps/chalk/profiles/srgb_color_space_profile.icm] which can be installed with Imagemagick's 'convert', or GraphicsMagick's 'gm convert'.
This will at least allow the user to choose their preferred sRGB profile.

Chalk no longer crashes after applying PR #34. The PR has been merged and backported.

@Ray-V: can you test before I close the issue?

Side note question: in Chalk we create an RGB image, but it gets saved as iCCP. Is that ok or should we also consider saving a RGB image as RGB?

That's ok - it works for me.

Great,thanks for the feedback @Ray-V.