Code cleanup needed in PNG decoder

RESOLVED FIXED in mozilla1.9.3a1

Status

()

--
minor
RESOLVED FIXED
9 years ago
9 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)

(Assignee)

Description

9 years ago
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)

Updated

9 years ago
Assignee: nobody → glennrp+bmo
(Assignee)

Comment 1

9 years ago
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.
(Assignee)

Comment 2

9 years ago
Created attachment 413766 [details] [diff] [review]
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.
(Assignee)

Comment 3

9 years ago
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.
(Assignee)

Updated

9 years ago
Severity: minor → normal
Keywords: perf
(Assignee)

Comment 4

9 years ago
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
(Assignee)

Comment 5

9 years ago
Created attachment 413861 [details] [diff] [review]
v2: code cleanup

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

Updated

9 years ago
Severity: normal → minor
Keywords: perf
(Assignee)

Updated

9 years ago
Attachment #413861 - Flags: review?(joe)
(Assignee)

Comment 7

9 years ago
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+
(Assignee)

Comment 9

9 years ago
This OK?

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

or this?

>+  nsPNGDecoder
>+     *decoder = static_cast...);
Status: NEW → ASSIGNED
(Assignee)

Comment 10

9 years ago
Created attachment 415999 [details] [diff] [review]
v3: code cleanup (checked-in)

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
(Assignee)

Comment 11

9 years ago
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
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

9 years ago
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.