From 1775bdeb2450a04034c42c9503a5748cc8f98a14 Mon Sep 17 00:00:00 2001 From: John Bowler Date: Tue, 24 Nov 2015 22:24:18 -0800 Subject: [PATCH] Better filter checking Improve pngvalid coverage of filter combinations, remove the (new in 1.7) code which disabled previous-row filters on the first row of an image in some cases. Signed-off-by: John Bowler --- contrib/libtests/pngvalid.c | 60 ++++++++++++++++++------ pngwutil.c | 93 ------------------------------------- 2 files changed, 46 insertions(+), 107 deletions(-) diff --git a/contrib/libtests/pngvalid.c b/contrib/libtests/pngvalid.c index 4d8da5f01..94d37a27a 100644 --- a/contrib/libtests/pngvalid.c +++ b/contrib/libtests/pngvalid.c @@ -276,7 +276,8 @@ make_four_random_bytes(png_uint_32* seed, png_bytep bytes) make_random_bytes(seed, bytes, 4); } -#if defined PNG_READ_SUPPORTED || defined PNG_WRITE_tRNS_SUPPORTED +#if defined PNG_READ_SUPPORTED || defined PNG_WRITE_tRNS_SUPPORTED ||\ + defined PNG_WRITE_FILTER_SUPPORTED static void randomize(void *pv, size_t size) { @@ -285,7 +286,7 @@ randomize(void *pv, size_t size) } #define RANDOMIZE(this) randomize(&(this), sizeof (this)) -#endif /* READ || WRITE_tRNS */ +#endif /* READ || WRITE_tRNS || WRITE_FILTER */ #ifdef PNG_READ_TRANSFORMS_SUPPORTED static unsigned int @@ -309,8 +310,8 @@ random_choice(void) return x & 1; } -#endif -#endif /* PNG_READ_SUPPORTED */ +#endif /* READ_RGB_TO_GRAY || READ_FILLER */ +#endif /* READ_TRANSFORMS */ /* A numeric ID based on PNG file characteristics. The 'do_interlace' field * simply records whether pngvalid did the interlace itself or whether it @@ -3649,6 +3650,31 @@ deinterlace_row(png_bytep buffer, png_const_bytep row, * layout details. See make_size_images below for a way to make images * that test odd sizes along with the libpng interlace handling. */ +#ifdef PNG_WRITE_FILTER_SUPPORTED +static void +choose_random_filter(png_structp pp, int start) +{ + /* Choose filters randomly except that on the very first row ensure that + * there is at least one previous row filter. + */ + int filters; + + RANDOMIZE(filters); + filters &= PNG_ALL_FILTERS; + + /* There may be no filters; skip the setting. */ + if (filters != 0) + { + if (start && filters < PNG_FILTER_UP) + filters |= PNG_FILTER_UP; + + png_set_filter(pp, 0/*method*/, filters); + } +} +#else /* !WRITE_FILTER */ +# define choose_random_filter(pp, start) ((void)0) +#endif /* !WRITE_FILTER */ + static void make_transform_image(png_store* const ps, png_byte const colour_type, png_byte const bit_depth, unsigned int palette_number, @@ -3767,6 +3793,7 @@ make_transform_image(png_store* const ps, png_byte const colour_type, } # endif /* do_own_interlace */ + choose_random_filter(pp, pass == 0 && y == 0); png_write_row(pp, buffer); } } @@ -3943,9 +3970,6 @@ make_size_image(png_store* const ps, png_byte const colour_type, int npasses = npasses_from_interlace_type(pp, interlace_type); png_uint_32 y; int pass; -# ifdef PNG_WRITE_FILTER_SUPPORTED - int nfilter = PNG_FILTER_VALUE_LAST; -# endif png_byte image[16][SIZE_ROWMAX]; /* To help consistent error detection make the parts of this buffer @@ -4008,15 +4032,23 @@ make_size_image(png_store* const ps, png_byte const colour_type, * does accept a filter number (per the spec) as well as a bit * mask. * - * The apparent wackiness of decrementing nfilter rather than - * incrementing is so that Paeth gets used in all images bigger - * than 1 row - it's the tricky one. + * The code now uses filters at random, except that on the first + * row of an image it ensures that a previous row filter is in + * the set so that libpng allocates the row buffer. */ - png_set_filter(pp, 0/*method*/, - nfilter >= PNG_FILTER_VALUE_LAST ? PNG_ALL_FILTERS : nfilter); + { + int filters; + + RANDOMIZE(filters); + filters %= PNG_FILTER_VALUE_LAST; + if (filters < 0) filters = -filters; + filters = 8 << filters; - if (nfilter-- == 0) - nfilter = PNG_FILTER_VALUE_LAST-1; + if (pass == 0 && y == 0 && filters < PNG_FILTER_UP) + filters |= PNG_FILTER_UP; + + png_set_filter(pp, 0/*method*/, filters); + } # endif png_write_row(pp, row); diff --git a/pngwutil.c b/pngwutil.c index e46a9b908..9d7c64fa9 100644 --- a/pngwutil.c +++ b/pngwutil.c @@ -2264,44 +2264,6 @@ png_write_filter_row(png_structrp png_ptr, png_bytep prev_pixels, } } } - - /* The filters are pre-calculated in png_set_filter, however if the - * image is interlaced some passes may still be too narrow or short to - * allow certain filters. In any case the first row of the pass - * doesn't need to consider PAETH or UP (AVG is still different). - */ - if (first_row_in_pass) - { - if ((filters_to_try & PNG_FILTER_UP) != 0U) - { - filters_to_try &= PNG_BIC_MASK(PNG_FILTER_UP); - filters_to_try |= PNG_FILTER_NONE; - } - - if ((filters_to_try & PNG_FILTER_PAETH) != 0U) - { - filters_to_try &= PNG_BIC_MASK(PNG_FILTER_PAETH); - filters_to_try |= PNG_FILTER_SUB/*equialent to PAETH here*/; - } - - /* If this leaves the AVG filter it will be used on the first row - * this is handled in the filter implementation by setting prev_row - * to NULL below. - */ - } - - /* Check for a narrow image; the blocking will never return just one - * pixel at the start unless the pass is only one pixel wide, this test - * needs to happen after the one above on PAETH: - */ - if (width == 1U) - { - if ((filters_to_try & PNG_FILTER_SUB) != 0U) - { - filters_to_try &= PNG_BIC_MASK(PNG_FILTER_SUB); - filters_to_try |= PNG_FILTER_NONE; - } - } } /* start of row */ else if (prev_row != NULL) @@ -2392,61 +2354,6 @@ png_set_filter(png_structrp png_ptr, int method, int filtersIn) return; } - /* New in 1.7.0: adjust the mask according to the image characteristics. - * This used to happen on every row, doing it here means that these checks - * happen only once every png_set_filter call, or once per image. - */ - if (filters != PNG_FILTER_NONE) - { - /* Test to see if there are enough rows to allow previous-row filters to - * work. Note that the AVG filter is still significant because it uses - * half the value of the previous pixel as the predictor, but it is - * ignored in this case. - */ - if (png_ptr->height <= (png_ptr->interlaced == PNG_INTERLACE_NONE ? 1U : - (png_ptr->width == 1U ? 3U : 2U))) - { - /* Replace 'up' by the equivalent 'none': */ - if ((filters & (PNG_FILTER_UP)) != 0) - { - filters &= PNG_BIC_MASK(PNG_FILTER_UP); - filters |= PNG_FILTER_NONE; - } - - /* Replace 'paeth' by the equivalent 'sub': */ - if ((filters & PNG_FILTER_PAETH) != 0) - { - filters &= PNG_BIC_MASK(PNG_FILTER_PAETH); - filters |= PNG_FILTER_SUB; - } - - /* Remove 'avg' unless it is the only filter in which case 'none' is - * used. (This chooses compression speed of very short images over a - * probably pointless compression option for a one line image; short - * images are common, the sub-case which benefits from AVG is not. - */ - if ((filters & PNG_FILTER_AVG) != 0) - { - filters &= PNG_BIC_MASK(PNG_FILTER_AVG); - if (filters == 0U) - filters |= PNG_FILTER_NONE; - } - } - - /* Also check for SUB on narrow images; it's equivalent to NONE on the - * first pixel. - */ - if (png_ptr->width <= (png_ptr->interlaced == PNG_INTERLACE_NONE ? 1U : - (png_ptr->height == 1U ? 3U : 1U))) - { - if ((filters & PNG_FILTER_SUB) != 0) - { - filters &= PNG_BIC_MASK(PNG_FILTER_SUB); - filters |= PNG_FILTER_NONE; - } - } - } - debug(filters != 0U && (filters & PNG_BIC_MASK(PNG_ALL_FILTERS)) == 0U); png_ptr->filter_mask = png_check_bits(png_ptr, filters, 8);