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 <jbowler@acm.org>

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 /* READ_RGB_TO_GRAY || READ_FILLER */
-#endif /* PNG_READ_SUPPORTED */
+#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
+                * The code now uses filters at random, except that on the first
-                * incrementing is so that Paeth gets used in all images bigger
+                * row of an image it ensures that a previous row filter is in
-                * than 1 row - it's the tricky one.
+                * 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;
                   
-               if (nfilter-- == 0)
+                  RANDOMIZE(filters);
-                  nfilter = PNG_FILTER_VALUE_LAST-1;
+                  filters %= PNG_FILTER_VALUE_LAST;
+                  if (filters < 0) filters = -filters;
+                  filters = 8 << filters;
+
+                  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);

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);