Closed
Bug 386585
Opened 18 years ago
Closed 17 years ago
Update libpng to version 1.2.22
Categories
(Core :: Graphics: ImageLib, defect, P3)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla1.9beta2
People
(Reporter: RyanVM, Assigned: glennrp+bmo)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 16 obsolete files)
598.87 KB,
patch
|
tor
:
review+
pavlov
:
superreview+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
Currently, we're using libpng 1.2.16 on the trunk. However, according to the libpng website, libpng 1.2.16 has a NULL-pointer-dereference vulnerability in it which was fixed with libpng 1.2.18. It would probably be a good idea to update accordingly.
Also, while we're currently using version 1.2.16, there are still files in modules/libimg/png which claim that we're using 1.2.12 (such as the readme, changelog, etc). It would probably be a good idea to update those references accordingly at the same time.
Flags: blocking1.9?
Assignee | ||
Comment 1•18 years ago
|
||
I recommend waiting for libpng-1.2.19 which should be out in a few weeks.
Reporter | ||
Comment 2•18 years ago
|
||
OK, I'm going to post a 1.2.18 WIP patch here just because it's almost done, but I'll hold off on review until then.
Reporter | ||
Comment 3•18 years ago
|
||
This patch updates libpng from version 1.2.16 to 1.2.18, along with picking up the few missed updates from 1.2.12 I talked about and removing pngasmrd.h, which is long-obsolete.
This compiles OK and renders PNGs (and APNGs) fine. However, I'll hold off on asking for review until 1.2.19 is released.
Fixing summary for 1.2.19
Flags: blocking1.9? → blocking1.9+
Summary: Update libpng to version 1.2.18 → Update libpng to version 1.2.19
Assignee | ||
Comment 5•18 years ago
|
||
Libpng-1.2.19 will be out in about 2 weeks. The current beta is libpng-1.2.19beta28. This patch updates the trunk to beta28 which is
good for testing. I'll revise the patch when libpng-1.2.19 is out; the
only changes are likely to be some internal dates and version numbers.
I'm particularly interested in speed tests of decoding PNG using 64-bit
platforms, This upgrade should be significantly faster.
Assignee | ||
Comment 6•18 years ago
|
||
Fixes typo in pnggccrd.c; also updates README, etc.
Assignee: nobody → glennrp
Attachment #270584 -
Attachment is obsolete: true
Attachment #272986 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Reporter | ||
Comment 7•18 years ago
|
||
OK, so you're taking this bug then...
You need to update MOZPNG from 10217 to 10219 in configure.in. Also, if you use the following in your diff, pngasmrd.h should show as being removed in the diff view.
+++ /dev/null 1 Jan 1970 00:00:00 -0000
Assignee | ||
Comment 8•18 years ago
|
||
I think 10217 is OK. It doesn't matter much anyhow since all system
libpngs will be rejected because they don't support APNG.
Thanks for the hint about /dev/null. I'll do that in the next upload.
Reporter | ||
Comment 9•18 years ago
|
||
With this patch applied, by build fails with the following error:
imglib2.lib(nsPNGDecoder.obj) : error LNK2019: unresolved external symbol _png_set_progressive_frame_fn referenced in function "void __cdecl info_callback(struct png_struct_def *,struct png_info_struct *)" (?info_callback@@YAXPAUpng_struct_def@@PAUpng_info_struct@@@Z)
xul.dll : fatal error LNK1120: 1 unresolved externals
Assignee | ||
Comment 10•18 years ago
|
||
Restores png_set_progressive_frame_fn() that got lost in a merge conflict.
Uses /dev/null to get rid of pngasmrd.h
Assignee | ||
Updated•18 years ago
|
Attachment #273107 -
Attachment is obsolete: true
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #273298 -
Attachment is obsolete: true
Reporter | ||
Comment 12•18 years ago
|
||
Surely you didn't really mean to remove pngasmrd.h.orig, right? ;-)
Assignee | ||
Comment 13•18 years ago
|
||
Right, I did not mean to do that. Wish me better luck with the next upload.
Assignee | ||
Comment 14•18 years ago
|
||
Improves thread safety of pngvcrd.c under MSVC compiling
Attachment #274710 -
Attachment is obsolete: true
Reporter | ||
Comment 15•17 years ago
|
||
Glenn, your RC2 patch appears to break APNG for me.
http://labs.mozilla.com/2007/08/better-animations-in-firefox-3/
Assignee | ||
Comment 16•17 years ago
|
||
I don't see how the patch would affect APNG. It has mostly to do with revising the assembly code which affects APNG and regular PNG the same way. Just to be sure, please try a build with #define PNG_NO_MMX_CODE at the top of mozpngconf.h.
Reporter | ||
Comment 17•17 years ago
|
||
Still seeing it with MMX disabled too. Here's the error I get if I try to view one of the APNGs on that site:
"The image “http://people.mozilla.com/~dolske/blogimg/macthrob-small.png” cannot be displayed, because it contains errors."
Assignee | ||
Comment 18•17 years ago
|
||
Can you get the actual libpng error report (via PR_LOG or something)?
Assignee | ||
Comment 19•17 years ago
|
||
Maybe APNG is triggering the new error exit at line 1269 of pngrtran.c:
png_error(png_ptr, "Uninitialized row");
Try changing it to a warning:
png_warning(png_ptr, "Uninitialized row");
Reporter | ||
Comment 20•17 years ago
|
||
After changing it to a warning, APNG works fine
Assignee | ||
Comment 21•17 years ago
|
||
Thank you. I'll remove the new error exit from libpng-1.2.19, which will probably be released on August 18.
Assignee | ||
Comment 22•17 years ago
|
||
Ryan, please try this:
1. Restore the png_error call in pngrtran.c
2. At line 1087 of pngset.c, add
png_ptr->flags |= PNG_FLAG_ROW_INIT;
Reporter | ||
Comment 23•17 years ago
|
||
Doesn't work (assuming I did it correctly).
------------------------------------------------------
info_ptr->next_frame_blend_op = blend_op;
info_ptr->valid |= PNG_INFO_fcTL;
+ png_ptr->flags |= PNG_FLAG_ROW_INIT;
return (1);
Assignee | ||
Comment 24•17 years ago
|
||
You did what I had in mind but it evidently wasn't quite right.
Assignee | ||
Comment 25•17 years ago
|
||
Libpng-1.2.19 is released. APNG problem is handled by ifdef'ing out the problematic error test.
Attachment #275344 -
Attachment is obsolete: true
Reporter | ||
Comment 26•17 years ago
|
||
Comment on attachment 277241 [details] [diff] [review]
Update trunk to libpng-1.2.19
Working well. Shall we get it reviewed?
Attachment #277241 -
Flags: superreview?(pavlov)
Attachment #277241 -
Flags: review?(asmith15)
Assignee | ||
Comment 27•17 years ago
|
||
During the previous upgrade there were problems with the Mac-Intel platform.
I would like to hear from someone that this upgrade does not regress that
problem before calling for a review (the fix is simple enough if it
*does* regress). See report of "mac bustage" in bug #334110 comments 29-46.
Assignee | ||
Comment 28•17 years ago
|
||
If you are testing this and do find a regression on Mac-Intel, please
add " || defined(__APPLE__)" to line 467 of pnggccrd.c and let us know if that
fixes the problem.
Comment 29•17 years ago
|
||
Glenn, you may wish to take advantage of the Try Server:
http://wiki.mozilla.org/Build:TryServer
Comment 30•17 years ago
|
||
You need a Mozilla LDAP ccount to build on the try server. But I don't think you need one to download one of the builds. Will someone give this one a try please?
https://build.mozilla.org/tryserver-builds/45-asmith@mozilla.com-firefox-try-mac.dmg
I don't have a mac to test it on.
Assignee | ||
Comment 31•17 years ago
|
||
This patch should fix the Intel-Mac problem. Also, removing pngasmrd.h via an update patch is cumbersome and troublesome so I opened a separate bug #393031 to do that.
Attachment #277241 -
Attachment is obsolete: true
Attachment #277241 -
Flags: superreview?(pavlov)
Attachment #277241 -
Flags: review?(asmith15)
Comment 32•17 years ago
|
||
Are we waiting for 1.2.20 then?
Assignee | ||
Comment 33•17 years ago
|
||
If we don't we will have some unhappy Mac users. I expect to release 1.2.20
fairly quickly, hopefully before August 31. About the TryServer: Is its "mac"
an Intel-mac or a PPC-mac?
Reporter | ||
Comment 34•17 years ago
|
||
Going by previous memory and seemingly verified by the binary size, it appears that the tryserver is doing universal binaries just like the official build boxen do.
Reporter | ||
Updated•17 years ago
|
Summary: Update libpng to version 1.2.19 → Update libpng to version 1.2.20
Assignee | ||
Comment 35•17 years ago
|
||
Attachment #277536 -
Attachment is obsolete: true
Assignee | ||
Comment 36•17 years ago
|
||
I am wondering whether we should eliminate the troublesome MMX assembler code
entirely. It does increase the decoding speed, but for reasonably sized
images (1280x1024) the savings on my Intel Duo platform is only .07 second
for an interlaced image, and none for non-interlaced. For smaller images
the difference is not measurable. Meanwhile, the code
amounts to nearly a third of libpng and is a maintenance nightmare.
If anyone would like to experiment, just build with PNG_NO_ASSEMBLER_CODE
defined and compare results with the rc1 patch alone. Observe the effects on
download size and on decoding time.
Would appreciate thoughts of drivers about the size/speed tradeoff merits.
Glenn
Reporter | ||
Comment 37•17 years ago
|
||
Doesn't Mozilla build with MMX optimizations disabled by default anyway? See bug 263151
Assignee | ||
Comment 38•17 years ago
|
||
They have been on by default at least since checkin of bug #334110.
Reporter | ||
Comment 39•17 years ago
|
||
I just did a build with and without the assembler optimizations on and saw no difference in compiled codesize and a 3.4% difference in performance on a custom image loading test of mine (40MB worth of high resolution PNGs). Whether or not that's enough is up for you to decide. I would certainly say that's something for the next release past 1.2.20, though :)
Also, you might be interested in what mmoy is doing with trying to optimize many of the image decoders (including libpng) using intrinsics so that they can use SSE2. www.vector64.com
Assignee | ||
Comment 40•17 years ago
|
||
What platform? If you were on an Intel MMX machine you should have seen about
8kbytes difference in compiled code size.
Reporter | ||
Comment 41•17 years ago
|
||
XPSP2. libpng gets built into libxul, right?
Assignee | ||
Comment 42•17 years ago
|
||
I don't know where libpng ends up on Windows. One of the DLL files, I guess.
Comment 43•17 years ago
|
||
The performance results vary greatly by image. I find about a 20% performance improvement from the mmx optimizations on a three-year-old K8 laptop. I haven't measured performance differences on newer machines but Intel and AMD have been improving performance on SSE* instructions. I would guess that they're not really
doing much with MMX performance.
I do agree about the maintenance headache on reintegrating code from the PNG folks. It seems like they have platform issues with changes on one platform that can cause problems for other platforms. Having to maintain pngvcrd.c and pnggccrd.c seems problemmatical too. That's the problem tring to maintain the GCC and MSVC formats for similar code.
Assignee | ||
Comment 44•17 years ago
|
||
For testing: compare rc1 and rc2 in terms of filesize and execution speed. The only real difference is the presence or absence of MMX assembler code support.
Reporter | ||
Comment 45•17 years ago
|
||
1.) Your rc2 patch doesn't include the change in line 16 of png.c needed for it to compile. Slight problem ;)
2.) I've noticed these warnings when building for awhile now. Are they worth dealing with?
mozilla/modules/libimg/png/pngget.c(955) : warning C4244: 'return' : conversion from 'png_uint_32' to 'png_byte', possible loss of data
mozilla/modules/libimg/png/pngwutil.c(2207) : warning C4101: 'mins' : unreferenced local variable
3.) Going from RC1 to RC2 led to a 1.5KB codesize reduction and a 3.8% performance hit on my PNG load test, which is thankfully close to my previous results.
Assignee | ||
Comment 46•17 years ago
|
||
1) "rc1" -> "rc2" in png.c (D'Oh!)
2) Yes let's clean up any warnings
3) What platform?
Attachment #278488 -
Attachment is obsolete: true
Reporter | ||
Comment 47•17 years ago
|
||
VC8SP1 using "-O2 -GLS -arch:SSE2"
Assignee | ||
Comment 48•17 years ago
|
||
If those are the only 2 warnings you saw, those will be fixed in the next
patch. The first was actually in the APNG fork, and the other was due to
some recent code removal in the libpng PNG writer.
Assignee | ||
Updated•17 years ago
|
Whiteboard: advice about speed vs complexity/filesize wanted
Assignee | ||
Updated•17 years ago
|
Attachment #278040 -
Attachment description: Update trunk to libpng-1.2.20rc1 → Update trunk to libpng-1.2.20rc1 with MMX support
Assignee | ||
Comment 49•17 years ago
|
||
Attachment #278518 -
Attachment is obsolete: true
Assignee | ||
Comment 50•17 years ago
|
||
Release of libpng-1.2.20 will be delayed for a few days because of some breakage in libpng-1.2.20rc2 (that did not affect the "rc2" patch). I've posted a "preview" patch which internally identifies itself as 1.2.20. It should fix the two warnings mentioned above in comment #45.
Assignee | ||
Updated•17 years ago
|
Attachment #278040 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Whiteboard: advice about speed vs complexity/filesize wanted
Assignee | ||
Comment 51•17 years ago
|
||
Libpng-1.2.20 was released yesterday.
Attachment #279100 -
Attachment is obsolete: true
Attachment #280274 -
Flags: review?(tor)
Assignee | ||
Comment 52•17 years ago
|
||
Comment on attachment 280274 [details] [diff] [review]
Update trunk to libpng-1.2.20
I have received a bug report that libpng-1.2.20 (and 1.2.19) are displaying some
simple transparent images incorrectly. I verified that Firefox exhibits the bug when built with the libpng-1.2.20-update patch. I'm cancelling the request for review until we get to the bottom of this.
Assignee | ||
Updated•17 years ago
|
Attachment #280274 -
Flags: review?(tor)
Assignee | ||
Comment 53•17 years ago
|
||
Fixes bug with decoding 16-bit transparent PNG files.
Attachment #280274 -
Attachment is obsolete: true
Comment 54•17 years ago
|
||
Glenn, you may want to add 3x PNG_CONST at pngpread.c:797
Assignee | ||
Comment 55•17 years ago
|
||
(In reply to comment #54)
> Glenn, you may want to add 3x PNG_CONST at pngpread.c:797
OK. Done in my master copy of libpng; it'll be in the next patch (probably
libpng-1.2.21rc1).
Reporter | ||
Comment 56•17 years ago
|
||
FWIW, 1.2.21beta1 is working fine for me so far on XPSP2.
Summary: Update libpng to version 1.2.20 → Update libpng to version 1.2.21
Assignee | ||
Comment 57•17 years ago
|
||
Unless new bugs appear, the PNG group will probably release libpng-1.2.21rc1 on Sept 25 and libpng-1.2.21 on October 2, 2007. I'll post an rc1 patch and a final patch for review/sr then.
Assignee | ||
Comment 58•17 years ago
|
||
Libpng-1.2.21 has been released.
Attachment #281011 -
Attachment is obsolete: true
Attachment #283630 -
Flags: review?(tor)
Attachment #283630 -
Flags: review?(tor) → review+
Assignee | ||
Comment 59•17 years ago
|
||
Comment on attachment 283630 [details] [diff] [review]
Update trunk to libpng-1.2.21
sr, anyone?
Attachment #283630 -
Flags: superreview?
Reporter | ||
Updated•17 years ago
|
Attachment #283630 -
Flags: superreview? → superreview?(pavlov)
Comment 60•17 years ago
|
||
Comment on attachment 283630 [details] [diff] [review]
Update trunk to libpng-1.2.21
rs=me
Attachment #283630 -
Flags: superreview?(pavlov) → superreview+
Comment 61•17 years ago
|
||
Checking in modules/libimg/png/CHANGES;
/cvsroot/mozilla/modules/libimg/png/CHANGES,v <-- CHANGES
new revision: 3.7; previous revision: 3.6
done
Checking in modules/libimg/png/LICENSE;
/cvsroot/mozilla/modules/libimg/png/LICENSE,v <-- LICENSE
new revision: 1.8; previous revision: 1.7
done
Checking in modules/libimg/png/MOZCHANGES;
/cvsroot/mozilla/modules/libimg/png/MOZCHANGES,v <-- MOZCHANGES
new revision: 3.19; previous revision: 3.18
done
Checking in modules/libimg/png/Makefile.in;
/cvsroot/mozilla/modules/libimg/png/Makefile.in,v <-- Makefile.in
new revision: 1.33; previous revision: 1.32
done
Checking in modules/libimg/png/README;
/cvsroot/mozilla/modules/libimg/png/README,v <-- README
new revision: 3.10; previous revision: 3.9
done
Checking in modules/libimg/png/libpng.txt;
/cvsroot/mozilla/modules/libimg/png/libpng.txt,v <-- libpng.txt
new revision: 3.7; previous revision: 3.6
done
Checking in modules/libimg/png/mozpngconf.h;
/cvsroot/mozilla/modules/libimg/png/mozpngconf.h,v <-- mozpngconf.h
new revision: 3.12; previous revision: 3.11
done
Checking in modules/libimg/png/png.c;
/cvsroot/mozilla/modules/libimg/png/png.c,v <-- png.c
new revision: 3.16; previous revision: 3.15
done
Checking in modules/libimg/png/png.h;
/cvsroot/mozilla/modules/libimg/png/png.h,v <-- png.h
new revision: 3.17; previous revision: 3.16
done
Checking in modules/libimg/png/pngconf.h;
/cvsroot/mozilla/modules/libimg/png/pngconf.h,v <-- pngconf.h
new revision: 3.22; previous revision: 3.21
done
Checking in modules/libimg/png/pngerror.c;
/cvsroot/mozilla/modules/libimg/png/pngerror.c,v <-- pngerror.c
new revision: 3.12; previous revision: 3.11
done
Checking in modules/libimg/png/pnggccrd.c;
/cvsroot/mozilla/modules/libimg/png/pnggccrd.c,v <-- pnggccrd.c
new revision: 3.12; previous revision: 3.11
done
Checking in modules/libimg/png/pngget.c;
/cvsroot/mozilla/modules/libimg/png/pngget.c,v <-- pngget.c
new revision: 3.16; previous revision: 3.15
done
Checking in modules/libimg/png/pngpread.c;
/cvsroot/mozilla/modules/libimg/png/pngpread.c,v <-- pngpread.c
new revision: 3.17; previous revision: 3.16
done
Checking in modules/libimg/png/pngread.c;
/cvsroot/mozilla/modules/libimg/png/pngread.c,v <-- pngread.c
new revision: 3.17; previous revision: 3.16
done
Checking in modules/libimg/png/pngrtran.c;
/cvsroot/mozilla/modules/libimg/png/pngrtran.c,v <-- pngrtran.c
new revision: 3.12; previous revision: 3.11
done
Checking in modules/libimg/png/pngrutil.c;
/cvsroot/mozilla/modules/libimg/png/pngrutil.c,v <-- pngrutil.c
new revision: 3.18; previous revision: 3.17
done
Checking in modules/libimg/png/pngset.c;
/cvsroot/mozilla/modules/libimg/png/pngset.c,v <-- pngset.c
new revision: 3.16; previous revision: 3.15
done
Checking in modules/libimg/png/pngtrans.c;
/cvsroot/mozilla/modules/libimg/png/pngtrans.c,v <-- pngtrans.c
new revision: 3.11; previous revision: 3.10
done
Checking in modules/libimg/png/pngvcrd.c;
/cvsroot/mozilla/modules/libimg/png/pngvcrd.c,v <-- pngvcrd.c
new revision: 3.8; previous revision: 3.7
done
Checking in modules/libimg/png/pngwio.c;
/cvsroot/mozilla/modules/libimg/png/pngwio.c,v <-- pngwio.c
new revision: 3.11; previous revision: 3.10
done
Checking in modules/libimg/png/pngwrite.c;
/cvsroot/mozilla/modules/libimg/png/pngwrite.c,v <-- pngwrite.c
new revision: 3.17; previous revision: 3.16
done
Checking in modules/libimg/png/pngwutil.c;
/cvsroot/mozilla/modules/libimg/png/pngwutil.c,v <-- pngwutil.c
new revision: 3.15; previous revision: 3.14
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Depends on: 399630
This is causing bug 399630 -- bad startup crash.
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 64•17 years ago
|
||
I'll replace this patch with one to upgrade to libpng-1.2.22 as soon as libpng-1.2.22 is released. This should fix problems with decoding the
iCCP chunk.
Attachment #283630 -
Attachment is obsolete: true
Reporter | ||
Updated•17 years ago
|
Status: REOPENED → NEW
Summary: Update libpng to version 1.2.21 → Update libpng to version 1.2.22
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 65•17 years ago
|
||
Libpng-1.2.22 has been released.
Attachment #284763 -
Attachment is obsolete: true
Attachment #284778 -
Flags: review?(tor)
Attachment #284778 -
Flags: review?(tor) → review+
Updated•17 years ago
|
Attachment #284778 -
Flags: superreview?(pavlov)
Comment 66•17 years ago
|
||
Comment on attachment 284778 [details] [diff] [review]
Update trunk to libpng-1.2.22 (checked in)
rs=me -- i assume this one has been tested?
Attachment #284778 -
Flags: superreview?(pavlov) → superreview+
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Target Milestone: mozilla1.9 M9 → ---
Updated•17 years ago
|
Priority: -- → P3
Updated•17 years ago
|
Attachment #284778 -
Flags: approval1.9?
Updated•17 years ago
|
Keywords: checkin-needed
Comment 67•17 years ago
|
||
Comment on attachment 284778 [details] [diff] [review]
Update trunk to libpng-1.2.22 (checked in)
Setting approval1.9+, targeting for M10, and you are clear to land once the tree is open for M10 (i.e., after tree opens, after beta).
Attachment #284778 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Target Milestone: --- → mozilla1.9 M10
Comment 68•17 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Attachment #284778 -
Attachment description: Update trunk to libpng-1.2.22 → Update trunk to libpng-1.2.22 (checked in)
You need to log in
before you can comment on or make changes to this bug.
Description
•