Fixes pertaining background color and grayscale conversions #270
Conversation
When processing transparent pixels (with png_set_strip_alpha) while converting to grayscale (with png_set_rgb_to_gray), libpng failed to convert the background color specified to the user to grayscale as well. This caused the documented pattern of handling bKGD chunks to fail for this corner case. Note that in case of conversions in the opposite direction (grayscale to RGB), the background color was already properly converted to RGB. This patch fixes this inconsistency, and adds a test program to the test suite to verify the fix.
When expanding low-bit (4-bit and lower) images to 8 or 16 bits, the background color (as set by png_set_background) only had its red/green/blue fields populated to the correctly expanded values, but the gray values were erroneously left at their unexpanded values. This caused transparency processing (as enabled by png_set_strip_alpha) to use the unexpanded background color when substituting transparent pixels. Rectify this by updating the .gray fields of the background color, and add a test.
| @@ -0,0 +1,246 @@ | |||
| /* pngadhoc.c | |||
There was a problem hiding this comment.
After looking for a while, I couldn't find an existing place in the test suite where such tests would be really appropriate, so I added a new file.
| PNG_INTERLACE_NONE, | ||
| PNG_COMPRESSION_TYPE_DEFAULT, | ||
| PNG_FILTER_TYPE_DEFAULT); | ||
| png_set_bKGD(png_ptr, info_ptr, &write_background); |
There was a problem hiding this comment.
Technically, adding bKGD to the test PNG (here and in the second test) is not necessary, but I wanted to illustrate that the bug happens with the png_set_background usage as suggested by the manual.
| @@ -1202,6 +1202,10 @@ png_init_palette_transformations(png_structrp png_ptr) | |||
| #endif /* READ_EXPAND && READ_BACKGROUND */ | |||
| } | |||
|
|
|||
| #ifdef PNG_READ_RGB_TO_GRAY_SUPPORTED | |||
| static int png_do_rgb_to_gray_1(png_structrp png_ptr, png_color_16p color, png_byte bit_depth); | |||
| #endif | |||
There was a problem hiding this comment.
Not sure what the policy is regarding forward declarations versus shuffling function order around.
| if ((png_ptr->transformations & PNG_RGB_TO_GRAY) == | ||
| PNG_RGB_TO_GRAY_ERR) | ||
| png_error(png_ptr, "nongray background color"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I didn't add code to do the same conversion of png_ptr->trans_color. I couldn't find a situation where doing so would matter, and I don't understand why the grayscale code above is doing it. In any case, the conversion here is lossy, and since its result can't be used to actually perform color-key transparency, it would only have informational value, but even then, there's no way to query it.
| *(dp++) = (png_byte)((color->green >> 8) & 0xff); | ||
| *(dp++) = (png_byte)(color->green & 0xff); | ||
| *(dp++) = (png_byte)((color->blue >> 8) & 0xff); | ||
| *(dp++) = (png_byte)(color->blue & 0xff); |
There was a problem hiding this comment.
This is ugly (encode / swap bytes just so png_do_rgb_to_gray decodes them again), but the alternative is an even uglier refactor.
|
Thank you for your use and contribution to libpng. |
|
Yes; the transforms do not always compose correctly. I identified a number of cases (I'm not sure if this is one, probably not because memory format conversions are not a high priority). The problem is that it is whack-a-mole with the 1.6 (and earlier) implementation. I tried to fix this in 1.7 and believe I got it all right but that's my version of 1.7; this (the transform code) is one place where a complete rewrite is in order (that's how I fixed it.) As a result it's impossible to review a fix like this as just a one file change (to pngrtran.c). The first step is to come up with a test case using pngstest - i.e. a set of arguments to pngstest that demonstrate the problem or, possibly, since pngstest has no support at present for the swap transforms, to implement a new test program. Without a test program accepting such a patch is inadvisable. Maybe in a major release if someone is committed to testing everything. A better patch for libpng 1.6 is to detect the incompatible transforms when they are set and simply png_error out; that is way better than producing a completely valid PNG full of garbage. Clearly since this bug has not been widely reported people who encounter it have already worked round it by doing one or other of the composed transforms themselves. |
|
@CyberShadow do you get a warning from inside png_init_read_transformations, something like: "libpng does not support gamma+background+rgb_to_gray" but any warning should be reported. |
|
No, I do not remember encountering such a warning. Worth pointing out that this pull request is over five years old, so my recall might not be perfect. |
After adding libpng support to my image library, I wrote some test code which generated PNG files with all valid combinations of relevant parameters, and asked libpng to parse each into every compatible format supported by my image library. Some of these combinations produced unexpected results, which seem to be caused by bugs in libpng.
This pull request includes two patches which attempt to rectify these bugs. Please see the individual commits' commit messages for details.
FWIW - the source code for my test suite that identified this bug is here:
https://github.com/CyberShadow/ae/blob/master/utils/graphics/libpng.d