Closed
Bug 529683
Opened 15 years ago
Closed 15 years ago
Code cleanup needed in PNG decoder
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: glennrp+bmo, Assigned: glennrp+bmo)
Details
Attachments
(1 file, 2 obsolete files)
21.24 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
Assignee: nobody → glennrp+bmo
Assignee | ||
Comment 1•15 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•15 years ago
|
||
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•15 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 | ||
Comment 4•15 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•15 years ago
|
||
- replaces "1/gammaOfFile" with "1.0/gammaOfFile" - initializes "intent" - wraps some long lines
Assignee | ||
Comment 6•15 years ago
|
||
v2 tryserver builds are at http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-bug-529683-cleanup-v2-Nov21
Assignee | ||
Updated•15 years ago
|
Attachment #413861 -
Flags: review?(joe)
Assignee | ||
Comment 7•15 years ago
|
||
The Tryserver unittests for Linux, WINNT, and OSX with the v2 patch are clean, as of 23 November 0700 EST.
Comment 8•15 years ago
|
||
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•15 years ago
|
||
This OK? >- nsPNGDecoder_*decoder_=_static_cast...); >+ nsPNGDecoder *decoder = >+ static_cast...); or this? >+ nsPNGDecoder >+ *decoder = static_cast...);
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•15 years ago
|
||
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•15 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)
Comment 12•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #415999 -
Flags: review?(joe) → review+
Updated•15 years ago
|
Attachment #413861 -
Attachment is obsolete: true
Comment 13•15 years ago
|
||
This'll have to wait until mozilla-central reopens for general checkins, but it's ready to go.
Keywords: checkin-needed
Comment 14•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b259db01f88c
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Assignee | ||
Updated•15 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.
Description
•