>> Can you split the whitespace changes out into a separate diff? It's
>> impossible to spot the real changes in there.
> OK, done. The ignore-whitespace diff still amounts to 111k but that's less than
> half the size of the full diff.
Many thanks.
This is a little ugly (pngerror.c):
warning_number[offset]=*(warning_message+offset+1);
What's wrong with
warning_number[offset] = warning_message[offset+1];
? It could also use a comment; I guess the warning syntax is supposed to
look like "#1234567890 foo bar", but that should be called out.
This is then incomplete:
@@ -281,9 +281,9 @@
break;
}
if((offset > 1) && (offset < 15))
{
- warning_number[offset-1]='\0';
+ warning_number[offset + 1] = '\0';
fprintf(stderr, "libpng warning no. %s: %s\n", warning_number,
warning_message+offset);
}
else
The first part of the conditional should be eliminated; it was there only
to protect against buffer underruns.
It's hard to be certain, but this (pngread.c) looks like it probably should
be const:
+ png_bytep chunk_name = png_ptr->chunk_name;
Ditto the one in png_read_end().
The copyrights here are pointless:
@@ -2209,10 +2211,13 @@
#if defined(PNG_READ_RGB_TO_GRAY_SUPPORTED)
/* reduce RGB files to grayscale, with or without alpha
* using the equation given in Poynton's ColorFAQ at
- * <http://www.inforamp.net/~poynton/>
+ * <http://www.inforamp.net/~poynton/> (THIS LINK IS DEAD June 2008)
* Copyright (c) 1998-01-04 Charles Poynton poynton at inforamp.net
+ * New links:
+ * <http://www.poynton.com/notes/colour_and_gamma/>
+ * Copyright (c) 2006-11-28 Charles Poynton poynton at poynton.com
*
* Y = 0.212671 * R + 0.715160 * G + 0.072169 * B
*
* We approximate this with
They're his sites; he can include the copyrights himself. (The equation
cannot be copyrighted.) All that's needed in libpng is the name and
(optionally) contact info, and that "only" for basic courtesy.
@@ -48,9 +47,17 @@
png_uint_32 PNGAPI
png_get_uint_31(png_structp png_ptr, png_bytep buf)
{
+#ifdef PNG_READ_BIG_ENDIAN_SUPPORTED
png_uint_32 i = png_get_uint_32(buf);
+#else
+ /* Avoid an extra function call by inlining the result. */
+ png_uint_32 i = ((png_uint_32)(*buf) << 24) +
+ ((png_uint_32)(*(buf + 1)) << 16) +
+ ((png_uint_32)(*(buf + 2)) << 8) +
+ (png_uint_32)(*(buf + 3));
+#endif
Don't we already have macros that do precisely this, including casting
buf in case it's not already a png_bytep? I _know_ there's extremely
similar code in zlib.h, and I thought I had added something similar as
part of the alpha-blend macro, though I might be misremembering that.
+png_uint_32 /* PRIVATE */
+png_read_chunk_header(png_structp png_ptr)
+{
+ png_byte buf[8];
+ png_uint_32 length;
+
+ /* read the length and the chunk name */
+ png_read_data(png_ptr, buf, 8);
+ length = png_get_uint_31(png_ptr, buf);
+
+ /* put the chunk name into png_ptr->chunk_name */
+ png_memcpy(png_ptr->chunk_name, buf + 4, 4);
Wouldn't it make sense to do some validation on chunk_name at this point?
If it's garbage (via corruption or whatever), you potentially save a fair
bit of I/O and computation by avoiding reading to the end of the file (and
computing the CRC) due to a ridiculously large chunk length.
You could signal it with UINT32_MAX (or whatever it's called), which is
an invalid chunk length. There are only three callers so far, so the
changes would be pretty minimal.
Does this even compile??
@@ -514,8 +552,9 @@
{
if (png_ptr->flags & PNG_FLAG_CRC_ANCILLARY_NOWARN)
{
png_chunk_error(png_ptr, "CRC error");
+%15+% png_chunk_benign_error(png_ptr, "CRC error");
}
else
{
png_chunk_warning(png_ptr, "CRC error");
OK, I'm a little more than 1/3 of the way through it...
Greg
|