529683 - Code cleanup needed in PNG decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

This is a spinoff of bug #517713.

With regard to src/modules/libpr0n/decoders/png/nsPNGDecoder.cpp
a change of this form was recommended to suppress a compiler warning:

>-    profile = ...(whitePoint, primaries, 1/gammaOfFile);
>+    profile = ...(whitePoint, primaries, (float)(1/gammaOfFile));

The Tryserver did not produce that warning for me, but it does generate
a few others:

nsPNGDecoder.cpp: In function
  'void row_callback(png_struct*, png_byte*, png_uint_32, int)':
nsPNGDecoder.cpp:755: warning:
  cast from 'PRUint8*' to 'PRUint32*' increases required alignment of target type
nsPNGDecoder.cpp:785: warning:
  cast from 'png_byte*' to 'PRUint32*' increases required alignment of target type
nsPNGDecoder.cpp:785: warning:
  cast from 'png_byte*' to 'PRUint32*' increases required alignment of target type
nsPNGDecoder.cpp:785: warning:
  cast from 'png_byte*' to 'PRUint32*' increases required alignment of target type
nsPNGDecoder.cpp: In function 'void info_callback(png_struct*, png_info*)':
nsPNGDecoder.cpp:597: warning:
  'intent' may be used uninitialized in this function

Perhaps a little code cleanup is in order here.

Wouldn't changing "1/gammaOfFile" to "1.0/gammaOfFile" be
sufficient to fix the first problem?

Comment 1

[Glenn Randers-Pehrson]

The "intent" warning and the "1/gammaOfFile" problem are trivial to fix.  The alignment warnings are more interesting and suggest a path to performance improvement.  We can request libpng to always return RGBA-32 pixels, and we can pass an aligned target to libpng for it to store the new_row data in.  We could also ask libpng to do the premultiplication, but that feature has not been tested much and might be slower than the thebes macro.

Comment 2

[Glenn Randers-Pehrson]

Attachment: Faster read of RGBA PNG files using aligned row buffer (WIP, v1)

This fixes the possible undefined "intent" and changes 1/gammaOfFile to 1.0/gammaOfFile.  It revises pngrutil.c to put the row buffer in a 16-byte aligned position within "big_row_buf", and aligns interlacebuf on a 4-byte boundary. I still need to track down where decoder->mCMSline is allocated and see if we can align that as well.

Comment 3

[Glenn Randers-Pehrson]

Tryserver builds for the v1 patch are at
http://build.mozilla.org/tryserver-builds/[e-mail]-bug-529683-v1c-Nov20

The unit tests passed.

Comment 4

[Glenn Randers-Pehrson]

Comment on attachment 413766 [details] [diff] [review]
Faster read of RGBA PNG files using aligned row buffer (WIP, v1)

Kicking the aligned-memory part of this back to bug #517713.

Comment 5

[Glenn Randers-Pehrson]

Attachment: v2: code cleanup

- replaces "1/gammaOfFile" with "1.0/gammaOfFile"
 - initializes "intent"
 - wraps some long lines

Comment 6

[Glenn Randers-Pehrson]

v2 tryserver builds are at
http://build.mozilla.org/tryserver-builds/[e-mail]-bug-529683-cleanup-v2-Nov21

Comment 7

[Glenn Randers-Pehrson]

The Tryserver unittests for Linux, WINNT, and OSX with the v2 patch are clean, as of 23 November 0700 EST.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 413861 [details] [diff] [review]
v2: code cleanup

>+++ src/modules/libpr0n/decoders/png/nsPNGDecoder.cpp	2009-11-21 10:55:50.000000000 -0600
>@@ -57,24 +57,28 @@
>-static void PNGAPI frame_info_callback(png_structp png_ptr, png_uint_32 frame_num);
>+static void PNGAPI frame_info_callback(png_structp png_ptr,
>+                           png_uint_32 frame_num);

Will you make sure the png_uint_32 there aligns directly under the png_structp? (It's hard to tell with diff's output sometimes.)

>     // Read data into our header buffer
>-    PRUint32 bytesToRead = PR_MIN(aCount, BYTES_NEEDED_FOR_DIMENSIONS - mHeaderBytesRead);
>+    PRUint32 bytesToRead = PR_MIN(aCount, BYTES_NEEDED_FOR_DIMENSIONS -
>+                           mHeaderBytesRead);

Put mHeaderBytesRead either directly under BYTES_NEEDED... or under aCount, your call as to which.


>-  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>(png_get_progressive_ptr(png_ptr));
>+  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>
>+                          (png_get_progressive_ptr(png_ptr));

I'd rather the whole static_cast go on its own line than it be split up like that.

>   if (interlace_type == PNG_INTERLACE_ADAM7) {
>     if (height < PR_INT32_MAX / (width * channels))
>-      decoder->interlacebuf = (PRUint8 *)nsMemory::Alloc(channels * width * height);
>+      decoder->interlacebuf = (PRUint8 *)nsMemory::Alloc(channels
>+                                                         * width * height);

Put the first * after channels?

>     if (!decoder->interlacebuf) {
>       longjmp(decoder->mPNG->jmpbuf, 5); // NS_ERROR_OUT_OF_MEMORY
>     }
>   }
>   
>   /* Reject any ancillary chunk after IDAT with a bad CRC (bug #397593).
>    * It would be better to show the default frame (if one has already been
>    * successfully decoded) before bailing, but it's simpler to just bail
>@@ -730,17 +746,18 @@ row_callback(png_structp png_ptr, png_by
>    *
>    * where old_row is what was displayed for previous rows.  Note
>    * that the first pass (pass == 0 really) will completely cover
>    * the old row, so the rows do not have to be initialized.  After
>    * the first pass (and only for interlaced images), you will have
>    * to pass the current row, and the function will combine the
>    * old row and the new row.
>    */
>-  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>(png_get_progressive_ptr(png_ptr));
>+  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>
>+                          (png_get_progressive_ptr(png_ptr));

static_cast on its own line?

>-  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>(png_get_progressive_ptr(png_ptr));
>+  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>
>+                          (png_get_progressive_ptr(png_ptr));

And here.

>-  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>(png_get_progressive_ptr(png_ptr));
>+  nsPNGDecoder *decoder = static_cast<nsPNGDecoder*>
>+                          (png_get_progressive_ptr(png_ptr));

And here.

Thanks for this, Glenn!

Comment 9

[Glenn Randers-Pehrson]

This OK?

>-  nsPNGDecoder_*decoder_=_static_cast...);
>+  nsPNGDecoder *decoder =
>+               static_cast...);

or this?

>+  nsPNGDecoder
>+     *decoder = static_cast...);

Comment 10

[Glenn Randers-Pehrson]

Attachment: v3: code cleanup (checked-in)

revised as requested and removed trailing blanks

Successful Tryserver builds are at https://build.mozilla.org/tryserver-builds/[e-mail]-bug-529683-v3-cleanup-Dec03

Comment 11

[Glenn Randers-Pehrson]

Comment on attachment 415999 [details] [diff] [review]
v3: code cleanup (checked-in)

@joe, please transfer your review to the v3 patch and mark v2 obsolete.

Comment 12

[Joe Drew (not getting mail)]

(In reply to comment #9)
> This OK?
> 
> >-  nsPNGDecoder_*decoder_=_static_cast...);
> >+  nsPNGDecoder *decoder =
> >+               static_cast...);
> 
> or this?
> 
> >+  nsPNGDecoder
> >+     *decoder = static_cast...);

Either :) Whichever you prefer.

Comment 13

[Joe Drew (not getting mail)]

This'll have to wait until mozilla-central reopens for general checkins, but it's ready to go.

Comment 14

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/b259db01f88c