Closed Bug 386585 Opened 15 years ago Closed 14 years ago
Update libpng to version 1
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.
I recommend waiting for libpng-1.2.19 which should be out in a few weeks.
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.
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
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.
Fixes typo in pnggccrd.c; also updates README, etc.
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
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.
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
Restores png_set_progressive_frame_fn() that got lost in a merge conflict. Uses /dev/null to get rid of pngasmrd.h
Attachment #273107 - Attachment is obsolete: true
Surely you didn't really mean to remove pngasmrd.h.orig, right? ;-)
Right, I did not mean to do that. Wish me better luck with the next upload.
Improves thread safety of pngvcrd.c under MSVC compiling
Attachment #274710 - Attachment is obsolete: true
Glenn, your RC2 patch appears to break APNG for me. http://labs.mozilla.com/2007/08/better-animations-in-firefox-3/
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.
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."
Can you get the actual libpng error report (via PR_LOG or something)?
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");
After changing it to a warning, APNG works fine
Thank you. I'll remove the new error exit from libpng-1.2.19, which will probably be released on August 18.
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;
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);
You did what I had in mind but it evidently wasn't quite right.
Libpng-1.2.19 is released. APNG problem is handled by ifdef'ing out the problematic error test.
Attachment #275344 - Attachment is obsolete: true
Comment on attachment 277241 [details] [diff] [review] Update trunk to libpng-1.2.19 Working well. Shall we get it reviewed?
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.
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.
Glenn, you may wish to take advantage of the Try Server: http://wiki.mozilla.org/Build:TryServer
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://email@example.com I don't have a mac to test it on.
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.
Are we waiting for 1.2.20 then?
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?
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.
Summary: Update libpng to version 1.2.19 → Update libpng to version 1.2.20
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
Doesn't Mozilla build with MMX optimizations disabled by default anyway? See bug 263151
They have been on by default at least since checkin of bug #334110.
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
What platform? If you were on an Intel MMX machine you should have seen about 8kbytes difference in compiled code size.
XPSP2. libpng gets built into libxul, right?
I don't know where libpng ends up on Windows. One of the DLL files, I guess.
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.
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.
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.
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
VC8SP1 using "-O2 -GLS -arch:SSE2"
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.
Whiteboard: advice about speed vs complexity/filesize wanted
Attachment #278040 - Attachment description: Update trunk to libpng-1.2.20rc1 → Update trunk to libpng-1.2.20rc1 with MMX support
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.
Attachment #278040 - Attachment is obsolete: true
Whiteboard: advice about speed vs complexity/filesize wanted
Libpng-1.2.20 was released yesterday.
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.
Fixes bug with decoding 16-bit transparent PNG files.
Attachment #280274 - Attachment is obsolete: true
Glenn, you may want to add 3x PNG_CONST at pngpread.c:797
(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).
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
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.
Libpng-1.2.21 has been released.
Comment on attachment 283630 [details] [diff] [review] Update trunk to libpng-1.2.21 sr, anyone?
Attachment #283630 - Flags: superreview?
Attachment #283630 - Flags: superreview? → superreview?(pavlov)
Comment on attachment 283630 [details] [diff] [review] Update trunk to libpng-1.2.21 rs=me
Attachment #283630 - Flags: superreview?(pavlov) → superreview+
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: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Depends on: 399630
This is causing bug 399630 -- bad startup crash.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → NEW
Summary: Update libpng to version 1.2.21 → Update libpng to version 1.2.22
Libpng-1.2.22 has been released.
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+
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+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
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.