diff --git a/ANNOUNCE b/ANNOUNCE index 82ceab39c..83fb03a63 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -70,6 +70,14 @@ Version 1.5.7beta03 [November 16, 2011] Added run-time detection of NEON support. Added contrib/libtests; includes simplified API test and timing test and a color conversion utility for rapid checking of failed 'pngstest' results. + Multiple transform bug fixes plus a work-round for double gamma correction. + libpng does not support more than one transform that requires linear data + at once - if this is tried typically the results is double gamma + correction. Since the simplified APIs can need rgb to gray combined with + a compose operation it is necessary to do one of these outside the main + libpng transform code. This check-in also contains fixes to various bugs + in the simplified APIs themselves and to some bugs in compose and rgb to + gray (on palette) itself. Send comments/corrections/commendations to png-mng-implement at lists.sf.net: (subscription required; visit diff --git a/CHANGES b/CHANGES index a8eaa45fd..9b5e7a03b 100644 --- a/CHANGES +++ b/CHANGES @@ -3715,6 +3715,14 @@ Version 1.5.7beta03 [November 16, 2011] Added run-time detection of NEON support. Added contrib/libtests; includes simplified API test and timing test and a color conversion utility for rapid checking of failed 'pngstest' results. + Multiple transform bug fixes plus a work-round for double gamma correction. + libpng does not support more than one transform that requires linear data + at once - if this is tried typically the results is double gamma + correction. Since the simplified APIs can need rgb to gray combined with + a compose operation it is necessary to do one of these outside the main + libpng transform code. This check-in also contains fixes to various bugs + in the simplified APIs themselves and to some bugs in compose and rgb to + gray (on palette) itself. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/pngread.c b/pngread.c index e2df70092..01dc75291 100644 --- a/pngread.c +++ b/pngread.c @@ -1316,7 +1316,9 @@ png_read_png(png_structp png_ptr, png_infop info_ptr, * be made to work with the progressive one. */ /* Do all the *safe* initialization - 'safe' means that png_error won't be - * called, so setting up the jmp_buf is not required. + * called, so setting up the jmp_buf is not required. This means that anything + * called from here must *not* call png_malloc - it has to call png_malloc_warn + * instead so that control is returned safely back to this routine. */ static int png_image_read_init(png_imagep image) @@ -1620,7 +1622,7 @@ png_image_read_composite(png_voidp argument) startx = PNG_PASS_START_COL(pass); stepx = PNG_PASS_COL_OFFSET(pass); y = PNG_PASS_START_ROW(pass); - stepy = PNG_PASS_COL_OFFSET(pass); + stepy = PNG_PASS_ROW_OFFSET(pass); } else @@ -1629,20 +1631,22 @@ png_image_read_composite(png_voidp argument) startx = 0; stepx = stepy = 1; } + + /* The following are invariants across all the rows: */ + startx *= channels; + stepx *= channels; for (; ylocal_row; - png_bytep outrow = row; - png_uint_32 x; + png_bytep outrow = row + startx; + png_const_bytep end_row = row + width * channels; /* Read the row, which is packed: */ png_read_row(png_ptr, inrow, NULL); /* Now do the composition on each pixel in this row. */ - for (x = startx; ximage; + png_structp png_ptr = image->opaque->png_ptr; + png_infop info_ptr = image->opaque->info_ptr; + png_byte interlace_type = png_ptr->interlaced; + png_uint_32 height = image->height; + png_uint_32 width = image->width; + int pass, passes; + + /* Double check the convoluted logic below. We expect to get here with + * libpng doing rgb to gray and gamma correction but background processing + * left to the png_image_read_background function. The rows libpng produce + * might be 8 or 16-bit but should always have two channels; gray plus alpha. + */ + if ((png_ptr->transformations & PNG_RGB_TO_GRAY) == 0) + png_error(png_ptr, "lost rgb to gray"); + + if ((png_ptr->transformations & PNG_COMPOSE) != 0) + png_error(png_ptr, "unexpected compose"); + + /* The palette code zaps PNG_GAMMA in place... */ + if ((png_ptr->color_type & PNG_COLOR_MASK_PALETTE) == 0 && + (png_ptr->transformations & PNG_GAMMA) == 0) + png_error(png_ptr, "lost gamma correction"); + + if (png_get_channels(png_ptr, info_ptr) != 2) + png_error(png_ptr, "lost/gained channels"); + + switch (interlace_type) + { + case PNG_INTERLACE_NONE: + passes = 1; + break; + + case PNG_INTERLACE_ADAM7: + passes = PNG_INTERLACE_ADAM7_PASSES; + break; + + default: + png_error(png_ptr, "unknown interlace type"); + } + + switch (png_get_bit_depth(png_ptr, info_ptr)) + { + default: + png_error(png_ptr, "unexpected bit depth"); + break; + + case 8: + /* 8-bit sRGB gray values with an alpha channel; the alpha channel is + * to be removed by composing on a backgroundi: either the row if + * display->background is NULL or display->background.green if not. + * Unlike the code above ALPHA_OPTIMIZED has *not* been done. + */ + for (pass = 0; pass < passes; ++pass) + { + png_bytep row = display->first_row; + unsigned int startx, stepx, stepy; + png_uint_32 y; + + if (interlace_type == PNG_INTERLACE_ADAM7) + { + /* The row may be empty for a short image: */ + if (PNG_PASS_COLS(width, pass) == 0) + continue; + + startx = PNG_PASS_START_COL(pass); + stepx = PNG_PASS_COL_OFFSET(pass); + y = PNG_PASS_START_ROW(pass); + stepy = PNG_PASS_ROW_OFFSET(pass); + } + + else + { + y = 0; + startx = 0; + stepx = stepy = 1; + } + + if (display->background == NULL) + { + for (; ylocal_row; + png_bytep outrow = row + startx; + png_const_bytep end_row = row + width; + + /* Read the row, which is packed: */ + png_read_row(png_ptr, inrow, NULL); + + /* Now do the composition on each pixel in this row. */ + for (; outrow < end_row; outrow += stepx) + { + png_byte alpha = inrow[1]; + + if (alpha > 0) /* else no change to the output */ + { + png_uint_32 component = inrow[0]; + + if (alpha < 255) /* else just use component */ + { + /* Since PNG_OPTIMIZED_ALPHA was not set it is + * necessary to invert the sRGB transfer + * function and multiply the alpha out. + */ + component = png_sRGB_table[component] * alpha; + component += png_sRGB_table[outrow[0]] * + (255-alpha); + component = PNG_sRGB_FROM_LINEAR(component); + } + + outrow[0] = (png_byte)component; + } + + inrow += 2; /* gray and alpha channel */ + } + + row += display->row_bytes; + } + } + + else /* constant background value */ + { + png_byte background8 = display->background->green; + png_uint_16 background = png_sRGB_table[background8]; + + for (; ylocal_row; + png_bytep outrow = row + startx; + png_const_bytep end_row = row + width; + + /* Read the row, which is packed: */ + png_read_row(png_ptr, inrow, NULL); + + /* Now do the composition on each pixel in this row. */ + for (; outrow < end_row; outrow += stepx) + { + png_byte alpha = inrow[1]; + + if (alpha > 0) /* else use background */ + { + png_uint_32 component = inrow[0]; + + if (alpha < 255) /* else just use component */ + { + component = png_sRGB_table[component] * alpha; + component += background * (255-alpha); + component = PNG_sRGB_FROM_LINEAR(component); + } + + outrow[0] = (png_byte)component; + } + + else + outrow[0] = background8; + + inrow += 2; /* gray and alpha channel */ + } + + row += display->row_bytes; + } + } + } + break; + + case 16: + /* 16-bit linear with pre-multiplied alpha; the pre-multiplication must + * still be done and, maybe, the alpha channel removed. This code also + * handles the alpha-first option. + */ + { + unsigned int outchannels = png_get_channels(png_ptr, info_ptr); + int preserve_alpha = (image->format & PNG_FORMAT_FLAG_ALPHA) != 0; + int swap_alpha = 0; + + if (preserve_alpha && (image->format & PNG_FORMAT_FLAG_AFIRST)) + swap_alpha = 1; + + for (pass = 0; pass < passes; ++pass) + { + png_uint_16p row = (png_uint_16p)display->first_row; + unsigned int startx, stepx, stepy; /* all in pixels */ + png_uint_32 y; + + if (interlace_type == PNG_INTERLACE_ADAM7) + { + /* The row may be empty for a short image: */ + if (PNG_PASS_COLS(width, pass) == 0) + continue; + + startx = PNG_PASS_START_COL(pass); + stepx = PNG_PASS_COL_OFFSET(pass); + y = PNG_PASS_START_ROW(pass); + stepy = PNG_PASS_ROW_OFFSET(pass); + } + + else + { + y = 0; + startx = 0; + stepx = stepy = 1; + } + + startx *= outchannels; + stepx *= outchannels; + + for (; ylocal_row, NULL); + inrow = (png_uint_16p)display->local_row; + + /* Now do the pre-multiplication on each pixel in this row. + */ + for (; outrow < end_row; outrow += stepx) + { + png_uint_32 component = inrow[0]; + png_uint_16 alpha = inrow[1]; + + if (alpha > 0) /* else 0 */ + { + if (alpha < 65535) /* else just use component */ + { + component *= alpha; + component += 32767; + component /= 65535; + } + } + + else + component = 0; + + outrow[swap_alpha] = (png_uint_16)component; + if (outchannels > 1) + outrow[1 ^ swap_alpha] = alpha; + + inrow += 2; /* components and alpha channel */ + } + + row += display->row_bytes; + } + } + } + break; + } + + return 1; +} + /* The guts of png_image_finish_read as a png_safe_execute callback. */ static int png_image_read_end(png_voidp argument) @@ -1698,6 +1973,7 @@ png_image_read_end(png_voidp argument) png_uint_32 format = image->format; int linear = (format & PNG_FORMAT_FLAG_LINEAR) != 0; int do_local_compose = 0; + int do_local_background = 0; /* to avoid double gamma correction bug */ int passes = 0; /* Add transforms to ensure the correct output format is produced then check @@ -1713,6 +1989,38 @@ png_image_read_end(png_voidp argument) png_fixed_point output_gamma; int mode; /* alpha mode */ + /* Do this first so that we have a record if rgb to gray is happening. */ + if (change & PNG_FORMAT_FLAG_COLOR) + { + /* gray<->color transformation required. */ + if (format & PNG_FORMAT_FLAG_COLOR) + png_set_gray_to_rgb(png_ptr); + + else + { + /* libpng can't do both rgb to gray and + * background/pre-multiplication if there is also significant gamma + * correction, because both operations require linear colors and + * the code only supports one transform doing the gamma correction. + * Handle this by doing the pre-multiplication or background + * operation in this code, if necessary. + * + * TODO: fix this by rewriting pngrtran.c (!) + * + * For the moment (given that fixing this in pngrtran.c is an + * enormous change) 'do_local_background' is used to indicate that + * the problem exists. + */ + if (base_format & PNG_FORMAT_FLAG_ALPHA) + do_local_background = 1/*maybe*/; + + png_set_rgb_to_gray_fixed(png_ptr, PNG_ERROR_ACTION_NONE, + PNG_RGB_TO_GRAY_DEFAULT, PNG_RGB_TO_GRAY_DEFAULT); + } + + change &= ~PNG_FORMAT_FLAG_COLOR; + } + /* Set the gamma appropriately, linear for 16-bit input, sRGB otherwise. */ { @@ -1723,25 +2031,58 @@ png_image_read_end(png_voidp argument) else input_gamma_default = PNG_DEFAULT_sRGB; - if (linear) - { + /* Call png_set_alpha_mode to set the default for the input gamma; the + * output gamma is set by a second call below. + */ + png_set_alpha_mode_fixed(png_ptr, PNG_ALPHA_PNG, input_gamma_default); + } + + if (linear) + { + /* If there *is* an alpha channel in the input it must be multiplied + * out; use PNG_ALPHA_STANDARD, otherwise just use PNG_ALPHA_PNG. + */ + if (base_format & PNG_FORMAT_FLAG_ALPHA) mode = PNG_ALPHA_STANDARD; /* associated alpha */ - output_gamma = PNG_GAMMA_LINEAR; - } else - { mode = PNG_ALPHA_PNG; - output_gamma = PNG_DEFAULT_sRGB; + + output_gamma = PNG_GAMMA_LINEAR; + } + + else + { + mode = PNG_ALPHA_PNG; + output_gamma = PNG_DEFAULT_sRGB; + } + + /* If 'do_local_background' is set check for the presence of gamma + * correction; this is part of the work-round for the libpng bug + * described above. + * + * TODO: fix libpng and remove this. + */ + if (do_local_background) + { + png_fixed_point gtest; + + /* This is 'png_gamma_threshold' from pngrtran.c; the test used for + * gamma correction, the screen gamma hasn't been set on png_struct + * yet; it's set below. png_struct::gamma, however, is set to the + * final value. + */ + if (png_muldiv(>est, output_gamma, png_ptr->gamma, PNG_FP_1) && + !png_gamma_significant(gtest)) + do_local_background = 0; + + else if (mode == PNG_ALPHA_STANDARD) + { + do_local_background = 2/*required*/; + mode = PNG_ALPHA_PNG; /* prevent libpng doing it */ } - /* If the input default and output gamma do not match, because sRGB is - * being changed to linear, set the input default now via a dummy call - * to png_set_alpha_mode_fixed. - */ - if (input_gamma_default != output_gamma) - png_set_alpha_mode_fixed(png_ptr, PNG_ALPHA_PNG, - input_gamma_default); + /* else leave as 1 for the checks below */ } /* If the bit-depth changes then handle that here. */ @@ -1765,8 +2106,16 @@ png_image_read_end(png_voidp argument) */ if (base_format & PNG_FORMAT_FLAG_ALPHA) { + /* If RGB->gray is happening the alpha channel must be left and the + * operation completed locally. + * + * TODO: fix libpng and remove this. + */ + if (do_local_background) + do_local_background = 2/*required*/; + /* 16-bit output: just remove the channel */ - if (linear) /* compose on black (well, pre-multiply) */ + else if (linear) /* compose on black (well, pre-multiply) */ png_set_strip_alpha(png_ptr); /* 8-bit output: do an appropriate compose */ @@ -1843,19 +2192,6 @@ png_image_read_end(png_voidp argument) */ png_set_alpha_mode_fixed(png_ptr, mode, output_gamma); - if (change & PNG_FORMAT_FLAG_COLOR) - { - /* gray<->color transformation required. */ - if (format & PNG_FORMAT_FLAG_COLOR) - png_set_gray_to_rgb(png_ptr); - - else - png_set_rgb_to_gray_fixed(png_ptr, PNG_ERROR_ACTION_NONE, - PNG_RGB_TO_GRAY_DEFAULT, PNG_RGB_TO_GRAY_DEFAULT); - - change &= ~PNG_FORMAT_FLAG_COLOR; - } - # ifdef PNG_FORMAT_BGR_SUPPORTED if (change & PNG_FORMAT_FLAG_BGR) { @@ -1881,7 +2217,13 @@ png_image_read_end(png_voidp argument) * code to remove. */ if (format & PNG_FORMAT_FLAG_ALPHA) - png_set_swap_alpha(png_ptr); + { + /* Disable this if doing a local background, + * TODO: remove this when local background is no longer required. + */ + if (do_local_background != 2) + png_set_swap_alpha(png_ptr); + } else format &= ~PNG_FORMAT_FLAG_AFIRST; @@ -1950,8 +2292,10 @@ png_image_read_end(png_voidp argument) /* Update the 'info' structure and make sure the result is as required; first * make sure to turn on the interlace handling if it will be required * (because it can't be turned on *after* the call to png_read_update_info!) + * + * TODO: remove the do_local_background fixup below. */ - if (!do_local_compose) + if (!do_local_compose && do_local_background != 2) passes = png_set_interlace_handling(png_ptr); png_read_update_info(png_ptr, info_ptr); @@ -1964,9 +2308,14 @@ png_image_read_end(png_voidp argument) if (info_ptr->color_type & PNG_COLOR_MASK_ALPHA) { - /* This channel may be removed below. */ + /* do_local_compose removes this channel below. */ if (!do_local_compose) - info_format |= PNG_FORMAT_FLAG_ALPHA; + { + /* do_local_background does the same if required. */ + if (do_local_background != 2 || + (format & PNG_FORMAT_FLAG_ALPHA) != 0) + info_format |= PNG_FORMAT_FLAG_ALPHA; + } } else if (do_local_compose) /* internal error */ @@ -2027,6 +2376,19 @@ png_image_read_end(png_voidp argument) return result; } + else if (do_local_background == 2) + { + int result; + png_bytep row = png_malloc(png_ptr, png_get_rowbytes(png_ptr, info_ptr)); + + display->local_row = row; + result = png_safe_execute(image, png_image_read_background, display); + display->local_row = NULL; + png_free(png_ptr, row); + + return result; + } + else { png_alloc_size_t row_bytes = display->row_bytes; diff --git a/pngrtran.c b/pngrtran.c index 72cd5ebec..084c4e514 100644 --- a/pngrtran.c +++ b/pngrtran.c @@ -1465,6 +1465,28 @@ png_init_read_transformations(png_structp png_ptr) } #endif /* PNG_READ_BACKGROUND_SUPPORTED && PNG_READ_EXPAND_16_SUPPORTED */ +#if defined(PNG_READ_BACKGROUND_SUPPORTED) && \ + (defined(PNG_READ_SCALE_16_TO_8_SUPPORTED) || \ + defined(PNG_READ_STRIP_16_TO_8_SUPPORTED)) + if ((png_ptr->transformations & (PNG_16_TO_8|PNG_SCALE_16_TO_8)) && + (png_ptr->transformations & PNG_COMPOSE) && + !(png_ptr->transformations & PNG_BACKGROUND_EXPAND) && + png_ptr->bit_depth == 16) + { + /* On the other hand, if a 16-bit file is to be reduced to 8-bits per + * component this will also happen after PNG_COMPOSE and so the background + * color must be pre-expanded here. + * + * TODO: fix this too. + */ + png_ptr->background.red = (png_uint_16)(png_ptr->background.red * 257); + png_ptr->background.green = + (png_uint_16)(png_ptr->background.green * 257); + png_ptr->background.blue = (png_uint_16)(png_ptr->background.blue * 257); + png_ptr->background.gray = (png_uint_16)(png_ptr->background.gray * 257); + } +#endif + /* NOTE: below 'PNG_READ_ALPHA_MODE_SUPPORTED' is presumed to also enable the * background support (see the comments in scripts/pnglibconf.dfa), this * allows pre-multiplication of the alpha channel to be implemented as @@ -1512,6 +1534,16 @@ png_init_read_transformations(png_structp png_ptr) #ifdef PNG_READ_BACKGROUND_SUPPORTED if (png_ptr->transformations & PNG_COMPOSE) { + /* Issue a warning about this combination: because RGB_TO_GRAY is + * optimized to do the gamma transform if present yet do_background has + * to do the same thing if both options are set a + * double-gamma-correction happens. This is true in all versions of + * libpng to date. + */ + if (png_ptr->transformations & PNG_RGB_TO_GRAY) + png_warning(png_ptr, + "libpng does not support gamma+background+rgb_to_gray"); + if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE) { /* We don't get to here unless there is a tRNS chunk with non-opaque @@ -1726,7 +1758,13 @@ png_init_read_transformations(png_structp png_ptr) else /* Transformation does not include PNG_BACKGROUND */ #endif /* PNG_READ_BACKGROUND_SUPPORTED */ - if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE) + if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE +#ifdef PNG_READ_RGB_TO_GRAY_SUPPORTED + /* RGB_TO_GRAY needs to have non-gamma-corrected values! */ + && ((png_ptr->transformations & PNG_EXPAND) == 0 || + (png_ptr->transformations & PNG_RGB_TO_GRAY) == 0) +#endif + ) { png_colorp palette = png_ptr->palette; int num_palette = png_ptr->num_palette; @@ -2165,12 +2203,22 @@ png_do_read_transformations(png_structp png_ptr, png_row_infop row_info) #ifdef PNG_READ_GAMMA_SUPPORTED if ((png_ptr->transformations & PNG_GAMMA) && +#ifdef PNG_READ_RGB_TO_GRAY_SUPPORTED + /* Because RGB_TO_GRAY does the gamma transform. */ + !(png_ptr->transformations & PNG_RGB_TO_GRAY) && +#endif #if (defined PNG_READ_BACKGROUND_SUPPORTED) ||\ (defined PNG_READ_ALPHA_MODE_SUPPORTED) + /* Because PNG_COMPOSE does the gamma transform if there is something to + * do (if there is an alpha channel or transparency.) + */ !((png_ptr->transformations & PNG_COMPOSE) && ((png_ptr->num_trans != 0) || (png_ptr->color_type & PNG_COLOR_MASK_ALPHA))) && #endif + /* Because png_init_read_transformations transforms the palette, unless + * RGB_TO_GRAY will do the transform. + */ (png_ptr->color_type != PNG_COLOR_TYPE_PALETTE)) png_do_gamma(row_info, png_ptr->row_buf + 1, png_ptr); #endif diff --git a/pngwrite.c b/pngwrite.c index df91d9be2..fad6cc670 100644 --- a/pngwrite.c +++ b/pngwrite.c @@ -1878,7 +1878,7 @@ png_write_image_8bit(png_voidp argument) /* Need 32 bit accuracy in the sRGB tables */ png_uint_32 component = *in_ptr++; - /* The following gives 65535 for an alpha of 0, which is fine, + /* The following gives 1.0 for an alpha of 0, which is fine, * otherwise if 0/0 is represented as some other value there is * more likely to be a discontinuity which will probably damage * compression when moving from a fully transparent area to a @@ -1891,11 +1891,17 @@ png_write_image_8bit(png_voidp argument) /* component 0 && alpha < 65535) + else if (component > 0) { - component *= reciprocal; - component += 64; /* round to nearest */ - component >>= 7; + if (alpha < 65535) + { + component *= reciprocal; + component += 64; /* round to nearest */ + component >>= 7; + } + + else + component *= 255; /* Convert the component to sRGB. */ *out_ptr++ = (png_byte)PNG_sRGB_FROM_LINEAR(component);