Closed Bug 386585 Opened 13 years ago Closed 12 years ago

Update libpng to version 1.2.22

Categories

(Core :: ImageLib, defect, P3, major)

defect

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?
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.
Attached patch Update libpng 1.2.16 to 1.2.18 (obsolete) — Splinter Review
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.
Assignee: nobody → glennrp
Attachment #270584 - Attachment is obsolete: true
Attachment #272986 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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
Attached patch Update trunk to libpng-1.2.19rc1 (obsolete) — Splinter Review
Attachment #273298 - 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.
Attached patch Update trunk to libpng-1.2.19rc2 (obsolete) — Splinter Review
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.
Attached patch Update trunk to libpng-1.2.19 (obsolete) — Splinter Review
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?
Attachment #277241 - Flags: superreview?(pavlov)
Attachment #277241 - Flags: review?(asmith15)
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://build.mozilla.org/tryserver-builds/45-asmith@mozilla.com-firefox-try-mac.dmg

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.
Attachment #277241 - Attachment is obsolete: true
Attachment #277241 - Flags: superreview?(pavlov)
Attachment #277241 - Flags: review?(asmith15)
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
Attachment #277536 - Attachment is obsolete: true
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
Attachment #278518 - Attachment is obsolete: true
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
Attached patch Update trunk to libpng-1.2.20 (obsolete) — Splinter Review
Libpng-1.2.20 was released yesterday.
Attachment #279100 - Attachment is obsolete: true
Attachment #280274 - Flags: review?(tor)
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.
Attachment #280274 - Flags: review?(tor)
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.
Attached patch Update trunk to libpng-1.2.21 (obsolete) — Splinter Review
Libpng-1.2.21 has been released.
Attachment #281011 - Attachment is obsolete: true
Attachment #283630 - Flags: review?(tor)
Attachment #283630 - Flags: review?(tor) → review+
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: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Depends on: 399542
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Update trunk to libpng-1.2.22rc1 (obsolete) — Splinter Review
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
Status: NEW → ASSIGNED
Libpng-1.2.22 has been released.
Attachment #284763 - Attachment is obsolete: true
Attachment #284778 - Flags: review?(tor)
Attachment #284778 - Flags: review?(tor) → review+
Attachment #284778 - Flags: superreview?(pavlov)
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+
Target Milestone: mozilla1.9 M9 → ---
Priority: -- → P3
Attachment #284778 - Flags: approval1.9?
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+
Target Milestone: --- → mozilla1.9 M10
No longer depends on: 399630
checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attachment #284778 - Attachment description: Update trunk to libpng-1.2.22 → Update trunk to libpng-1.2.22 (checked in)
Depends on: 403481
No longer depends on: 403481
Blocks: 495609
You need to log in before you can comment on or make changes to this bug.