Feature Request - graceful error returns for reading/writing images

[d3x0r]

I've run into issues where libpng read a bad image and triggered an abort, rather than letting the application gracefully handle it. But then I'm no fan of setjmp/longjmp so maybe it's my refusal to use those that's the issue.

The error path for reading/writing png files isn't exactly that long.... that is - it's not terribly complex to catch if( !somefunction ) return false; and really doesn't anything as esoteric as longjmp to get out of.

pngwrite.c, pngwio.c, pngrio.c pngread.c, pngpread.c have a few functions that can have their return type changed from 'void' to 'int' (or boolean) and return true/false status instead of triggering abort through pngerror.c.

This is basically my applied change for libpng 1.6.40 to add 'PNG_GRACEFUL_ERROR' option which allows errors to go back to the application to handle them gracefully.
d3x0r/SACK@033a9b2
(ignore the CMakeLists.part and pnglibconf.h additions which I end up tracking )

[jbowler]

This is what I want to see for libpng-ng; i.e. a rewrite which uses a full error code scheme, either 'true/false' with separate error codes or segmented error codes (like in Apollo's Aegis, which used 32-bit codes throughout the OS and still remains the best designed OS I've ever used.)

A compatibility layer can be built on this at the API level; APIs call an appropriate set of libpng-ng APIs and translate the error numbers to error codes.

Meanwhile in older versions of libpng (i.e. 1.6 and 1.5) the simplified API has existed to solve this problem (and other problems of general utility) and works

It isn't just setjmp/longjmp. It's also the reliance on callback APIs which have to be blocking (IO and memory). That's why the whole progressive reader code duplication came into existence and that needs fixing in libpng-ng as well. Notice that Mark Adler's zlib implementation is non-blocking and has an API which can be adopted within libpng-ng without itself precluding the use of other LZ77 implementations.

[jbowler]

@d3xor I suggest you launch a discussion on png-mng-implement.

[jbowler]

@d3x0r one of my current points of focus is the extent to which application and library developers find the simplified API inadequate. In view of this you are the perfect person for comment: why isn't the simplified API sufficient for your needs?

[d3x0r]

The code using libpng dates back to the late 90's. the simplified API might be sufficient...
The fixes to the library layer were driven by 'the usage has always worked' and for some reason the library generated abort() out of the blue. I don't think the reader is even paying attention to error codes; either the image worked or it doesn't. I occasionally used the writer code for debugging like font tile sheets built for opengl output; but usually I'm not even doing writes.

[jbowler]

Thanks for the information. png_error can always be produced on 'read' by broken input - the simplified API is the only way at present round that but it doesn't do everything the other reader APIs do. On 'write' abort() is just fine because png_error should only result from internal libpng errors or application errors (including an explicit call to png_error in a callback.)

[jbowler]

@ctruta: [libpng-ng], moving away from non-local goto is a good principle in all cases. Exceptions are for the lazy.

[jbowler]

@ctruta addressed by simplified API in 1.x (cf the comment by @d3x0r to OP), won't be fixed in 1.x, so "resolved" or if you want to create the tag, libpng2 (-2, etc).

[d3x0r]

I did some digging, and implemented the simplified api... should be sufficient, other than there are occasionally images that fail decoding without the following option... (Someone posted about this on the mailing list probably several years ago now, and I just grabbed the fix preemptively)

	png_set_option(png_ptr
	              , PNG_MAXIMUM_INFLATE_WINDOW
	              , PNG_OPTION_ON);

which I don't see set in the simplified API.

What I had to implement before was a bunch of checking for the format and handle coercing libpng to output 32 bit RGBA sort of format (maybe ABGR), but it appears a default background for missing alpha, and other things this does are probably handled in the simplified read API... ?

		png_uint_32 Width, Height;
		int bit_depth, color_type;

		png_get_IHDR (png_ptr, info_ptr, &Width, &Height, &bit_depth, &color_type
		             , NULL, NULL, NULL);

		/* this message is important to someone for some reason or another... */
		//lprintf( "Image is %ld by %ld - %d/%d\n", Width, Height, bit_depth, color_type );

		if (bit_depth > 8)
		// tell libpng to strip 16 bit/color files down to 8 bits/color
			png_set_strip_16 (png_ptr);
		else if (bit_depth < 8)
		// Expand pictures with less than 8bpp to 8bpp
			png_set_packing (png_ptr);

		if( (color_type & PNG_COLOR_MASK_PALETTE ) )
		{
			png_set_palette_to_rgb( png_ptr );
			//Log( "At the moment only 8 bit RGB/Grey images are allowed...\n" );
			//goto no_mem2;
		}

		// gray is 0
		if( color_type == PNG_COLOR_TYPE_GRAY )
			png_set_gray_to_rgb(png_ptr);

		/* flip the RGB pixels to BGR (or RGBA to BGRA) */
		if( !IMGVER(bGLColorMode) )
			if (color_type & PNG_COLOR_MASK_COLOR)
				png_set_bgr(png_ptr);

		if( !(color_type & PNG_COLOR_MASK_ALPHA ) )
		{
			//lprintf( "missing alpha in color, so add color" );
			png_set_filler(png_ptr, 0xFF, PNG_FILLER_AFTER);
		}

[jbowler]

I did some digging, and implemented the simplified api... should be sufficient, other than there are occasionally images that fail decoding without the following option [* * *] PNG_MAXIMUM_INFLATE_WINDOW

Those images were generated by a buggy and very old version of libpng. The program png-fix-itxt which is shipped with libpng 1.6 and may be installed by some operating system distributions will fix the images. Well, if it doesn't I would really like to know. Can you check and file an issue here along with a sample image (this part is essential) if png-fix-itxt does not manage to fix such images?

The images may cause an out-of-bounds memory access on some combinations of old libpng and old zlib. zlib versions which detect the error should be causing libpng to produce meaningful error message. It's something containing the words, "Too far back," if I remember correctly.

If it isn't that I'd really like to know! @ctruta this is the first time this issue has appeared in years...

[jbowler]

What I had to implement before was a bunch of checking for the format and handle coercing libpng to output 32 bit RGBA sort of format (maybe ABGR), but it appears a default background for missing alpha, and other things this does are probably handled in the simplified read API... ?

If you want RGBA (etc) output then the background isn't used at all; libpng (any API) just writes four channels, the colour channels and the alpha. This appears to be what your code is doing except that for RGB input (i.e. no alpha channel) it converts it to RGBA or BGRA, which is slightly odd - traditionally the alpha channel is crammed into the high-order byte so on big-endian systems the format is ABGR. Nevertheless the simplified API supports all four variations.

So I don't see where the "background" comes in. Did you just mean creating an alpha channel? In that case the API does it right - an RGB image is opaque so the alpha channel gets filled with 1.0.

If you want RGB and the input has alpha the API will give an input "A" format; PNG_FORMAT_FLAG_ALPHA will be set. If you just set PNG_FORMAT_RGB then the input will be composited onto the RGB buffer you supply, eliminating the alpha channel. If you want a single background colour (as in the lower level png_set_background) then you need to prefill your buffer with that colour. Normally that's just a bitmap fill but DIY just write a row of RGB pixels then memcpy to the next row. Optimizations should be obvious; it's typically log2(number-of-pixels) memcpy calls.

The documentation of the last paragraph is immediately below the declaration of png_image_finish_read in png.h.