Code cleanup needed in PNG decoder

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
--
minor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: glennrp+bmo, Assigned: glennrp+bmo)

Tracking

Trunk
mozilla1.9.3a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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?
Assignee: nobody → glennrp+bmo
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.
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.
Tryserver builds for the v1 patch are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-bug-529683-v1c-Nov20

The unit tests passed.
Severity: minor → normal
Keywords: perf
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.
Attachment #413766 - Attachment is obsolete: true
Posted patch v2: code cleanup (obsolete) — Splinter Review
- replaces "1/gammaOfFile" with "1.0/gammaOfFile"
 - initializes "intent"
 - wraps some long lines
Severity: normal → minor
Keywords: perf
Attachment #413861 - Flags: review?(joe)
The Tryserver unittests for Linux, WINNT, and OSX with the v2 patch are clean, as of 23 November 0700 EST.
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!
Attachment #413861 - Flags: review?(joe) → review+
This OK?

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

or this?

>+  nsPNGDecoder
>+     *decoder = static_cast...);
Status: NEW → ASSIGNED
revised as requested and removed trailing blanks

Successful Tryserver builds are at https://build.mozilla.org/tryserver-builds/glennrp@gmail.com-bug-529683-v3-cleanup-Dec03
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.
Attachment #415999 - Flags: review?(joe)
(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.
Attachment #415999 - Flags: review?(joe) → review+
Attachment #413861 - Attachment is obsolete: true
This'll have to wait until mozilla-central reopens for general checkins, but it's ready to go.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b259db01f88c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #415999 - Attachment description: v3: code cleanup → v3: code cleanup (checked-in)
You need to log in before you can comment on or make changes to this bug.