Update libpng to version 1.4.7

RESOLVED FIXED in mozilla6

Status

()

Core
ImageLib
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Glenn Randers-Pehrson, Assigned: Glenn Randers-Pehrson)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

7 years ago
Libpng-1.5.0 has been released.  We should update Mozilla's embedded libpng to version 1.5.0, but there are no security or performance issues for Mozilla that I know of, so there's no hurry.
(Assignee)

Updated

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

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
It seems that libpng-1.5.0 may have a bug with regard to decoding interlaced PNG images, so let's wait for 1.5.1.
(Assignee)

Updated

7 years ago
Summary: Update libpng to version 1.5.0 → Update libpng to version 1.5.1
(Assignee)

Comment 2

7 years ago
Changed title because we will be waiting for 1.5.1.
Version: unspecified → Trunk
(Assignee)

Comment 3

7 years ago
The bug was not with interlaced images but with the png_rgb_to_gray
transformation, which we don't use.  But 1.5.1 will be out soon so
we might as well wait anyhow.

Comment 4

7 years ago
Created attachment 509715 [details] [diff] [review]
png_handle_fcTL() fix

Glenn, please add this fix for png_handle_fcTL() too.

It fixed a problem which does *not* affect Firefox, but can cause trouble for people who would install Mozilla's version of libpng as a system library.

Comment 5

6 years ago
Libpng 1.5.2 has been released, can we please get it in to the trunk before Firefox 5.0 ships?

http://libpng.sourceforge.net/

"UPDATE 31 March 2011: The latest released versions are libpng-1.5.2"
(Assignee)

Updated

6 years ago
Summary: Update libpng to version 1.5.1 → Update libpng to version 1.5.2
(Assignee)

Updated

6 years ago
Depends on: 645519
(Assignee)

Comment 6

6 years ago
The reduced libpng that we use doesn't require anything new from libpng-1.5.2.
We do need to apply the patch in bug #644519 though so users can run with a system libpng-1.5.x.

Comment 7

6 years ago
Bug 645519 is fixed now, would it be possible to update to libpng 1.5.2 before the code freeze for Firefox 5.0 on the 12th?
(Assignee)

Comment 8

6 years ago
Probably not.  Applying the diff of system libpng-1.4.3 to the embedded apng-supporting libpng results in .rej files totaling 3793 lines.  So there's
a lot of manual slogging through that to be done and zero payoff, since as I
have said before, libpng-1.5.x doesn't offer anything new for mozilla.

Rushing through a big job like that would just open us up to mistakes and
possible security problems.

On the other hand, upgrading to libpng-1.4.6 would be fairly simple and
that is what I recommend.  I'd need help from someone who is able and
willing to to do triserver tests.  libpng-1.4.6 is scheduled for release
next week but we can test with libpng-1.4.6rc02 now.
(Assignee)

Comment 9

6 years ago
Created attachment 524609 [details] [diff] [review]
v00 Ugrade to libpng-1.4.6
Attachment #509715 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Note the v00 patch internally calls itself libpng-1.4.6rc02.  This is just for testing; a final patch will call itself libpng-1.4.6.
I'll run this through Try for you tonight (EDT) when I get home from work.
(Assignee)

Comment 12

6 years ago
I've asked the PNG group for permission to push 1.4.6 out a little early (it's scheduled for April 14 but is ready to go now).
(Assignee)

Comment 13

6 years ago
(In reply to comment #4)
> Created attachment 509715 [details] [diff] [review]
> png_handle_fcTL() fix
> 
> Glenn, please add this fix for png_handle_fcTL() too.

@max, It's in the v00 patch.
(Assignee)

Comment 14

6 years ago
Created attachment 524780 [details] [diff] [review]
v01 Ugrade to libpng-1.4.6

Libpng-1.4.6 has been released.
Attachment #524609 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
@Ryan, there was one bug in v00's png.h (a stray "};" )
that's fixed in the v01 patch.
Bug 645519 landed on trunk, so the changes to nsPNGDecoder.cpp are already on mozilla-central. Also, apng.diff is not in the Mozilla tree, so I got rid of that as well. Pushed to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8d146b477ab0

Builds will be available at the link below once finished:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-8d146b477ab0
Summary: Update libpng to version 1.5.2 → Update libpng to version 1.4.6
Created attachment 524786 [details] [diff] [review]
libpng 1.4.6 v1 patch

As pushed to Try
Attachment #524780 - Attachment is obsolete: true
Attachment #524786 - Flags: review?(joe)
(Assignee)

Comment 18

6 years ago
Created attachment 524788 [details] [diff] [review]
v02 Upgrade to libpng-1.4.6

Removed stray apng.diff file and the nsPNGDecoder.cpp changes.
Attachment #524786 - Attachment is obsolete: true
Attachment #524786 - Flags: review?(joe)
Glenn, the diff I posted was in ready to apply hg mq format with proper attribution.
(Assignee)

Comment 20

6 years ago
Oops, I thought I was making my own v01 patch obsolete!

I don't see a way to un-obsolete your v1 patch and
make my v02 obsolete.  If you know how, go ahead and do that.
Comment on attachment 524788 [details] [diff] [review]
v02 Upgrade to libpng-1.4.6

You have to click the Edit link on the patch details to mark them obsolete. It changed with one of the recent Bugzilla updates.
Attachment #524788 - Attachment is obsolete: true
Attachment #524786 - Attachment is obsolete: false
Attachment #524786 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Blocks: 648690
(Assignee)

Comment 22

6 years ago
Ok thanks.  I thought that button was just for editing the patch title.
Doesn't build on Windows:
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(711) : error C2143: syntax error : missing ';' before 'type'
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(712) : error C2143: syntax error : missing ';' before 'type'
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(713) : error C2065: 'half_z_window_size' : undeclared identifier
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(713) : warning C4018: '<=' : signed/unsigned mismatch
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(716) : error C2065: 'z_cinfo' : undeclared identifier
I see the failure locally as well. Apparently MSVC doesn't like declaring variables inline?
http://andre.stechert.org/urwhatu/2006/01/error_c2143_syn.html
(Assignee)

Comment 25

6 years ago
Ack.  Fixed that in libpng-1.5.2 last week.
(Assignee)

Comment 26

6 years ago
The fix is being tested in libpng-1.4.7rc01.  Libpng-1.4.7 will be released once I see test results for the various platforms.
Summary: Update libpng to version 1.4.6 → Update libpng to version 1.4.7
Seeing this error now repeatedly:
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(651) : error C2144: syntax error : 'int' should be preceded by ';'
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(651) : error C2208: 'int' : no members defined using this type
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(652) : error C2144: syntax error : 'int' should be preceded by ';'
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(652) : error C2208: 'int' : no members defined using this type

etc...

I'm seeing the same errors locally on a clobber build. This error likely predates the 1.4.7 patch but wasn't hit because of the other build failure. It appears that pngwutil.cpp builds without error now at least.
(Assignee)

Comment 28

6 years ago
What compiler issued these errors?

Not sure what is happening here.  Can you try a build with
PNG_NO_PEDANTIC_WARNINGS defined in mozpngconf.h?  png.h line 651
is the first place PNG_DEPSTRUCT is used, and this will be blank with
PNG_NO_PEDANTIC_WARNINGS defined.

You can put the definition in mozpngconf.h
(Assignee)

Comment 29

6 years ago
This is probably relevant.  The PNG_DEPSTRUCT was blank for
all versions of MSC prior to 1.4.6.  See the libpng CHANGES
file:

version 1.4.6beta07 [March 22, 2011]
  Added attribute definition (warnings) for MSC_VER >= 1300 in pngconf.h
I'm building with MSVC 2010 SP1. I'm doing a local build now with PNG_NO_PEDANTIC_WARNINGS defined in mozpngconf.h.
Created attachment 524880 [details] [diff] [review]
libpng 1.4.7 v1 patch

Now we're in business. My local build succeeded with the mozpngconf.h change. Pushed to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=69ea57309066

Builds when ready:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/ryanvm@gmail.com-69ea57309066

Glenn, I just arbitrarily added the define near the top. Not sure if that's exactly how you'd want it for a final check-in.
Attachment #524786 - Attachment is obsolete: true
Attachment #524880 - Flags: review?(joe)
Attachment #524786 - Flags: review?(joe)
(Assignee)

Comment 32

6 years ago
It doesn't make much difference where you put it.  I'll put it near the
top with a comment in the final version.

Do you know what MSC_VER is defined to on the compiler that emitted the
error messages?  In pngconf.h we turn on the PNG_DEPSTRUCT and other
attributes when MSC_VER >= 1300.
Mozilla uses MSVC 2005 for their official builds (_MSC_VER = 1400). Plan is to upgrade to MSVC 2010 in the future (once they can sort out building jemalloc with it), which is _MSC_VER = 1600.
(Assignee)

Comment 34

6 years ago
Thanks.  I've passed that info along to the PNG developers.
I think we should play it safe and not use the feature for any
MSC version:

     in mozlibpng.h

     #ifdef _MSC_VER
     #  define PNG_NO_PEDANTIC_WARNINGS
     #endif

We don't need the warnings under MSC because we'll still see them while
building our same code with gcc.
(Assignee)

Comment 35

6 years ago
Created attachment 524971 [details] [diff] [review]
v05 Upgrade to libpng-1.4.7
Attachment #524880 - Attachment is obsolete: true
Attachment #524971 - Flags: review?(joe)
Attachment #524880 - Flags: review?(joe)
Created attachment 524972 [details] [diff] [review]
v5 patch update to libpng 1.4.7

Same as the other v05 patch minus the unwanted makefile changes. Functionally the same as the 1.4.7rc1 patch, which passed on Try. Built locally on a clobber build without issue.
Attachment #524971 - Attachment is obsolete: true
Attachment #524972 - Flags: review?(joe)
Attachment #524971 - Flags: review?(joe)
(Assignee)

Comment 37

6 years ago
libpng-1.4.7 has been released.
(Assignee)

Comment 38

6 years ago
(In reply to comment #5 and comment #7)
> Libpng 1.5.2 has been released, can we please get it in to the trunk before
> Firefox 5.0 ships?

Libpng-1.4.7 is out and the patch is ready to go, passed Try, and is
just waiting for review. @Ryan, thanks much for helping with this.

However, if it doesn't make it into 5.0 that's not a problem.  The current
embedded libpng-1.4.3 is adequate, and neither libpng-1.4.6 nor 1.5.2
offers performance or security improvements in the parts that are
not #ifdef'ed out in mozpngconf.h
Attachment #524972 - Flags: review?(joe) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/projects/cedar/rev/7511736d309b
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla6

Comment 40

6 years ago
Glenn, the patch I suggested in comment #4 landed incorrectly.

The patch should have 3 blocks like that, but it only has 2:

-        png_crc_finish(png_ptr, length);
+        png_crc_finish(png_ptr, length-4);
         return;

I also suggested that some existing piece of code that uses info_ptr should be put inside
if (info_ptr != NULL)
{
}

but it also landed a little bit incorrectly. Part of that code ended up both inside and outside. Could you check this, please?
(Assignee)

Comment 41

6 years ago
(In reply to comment #40)
> Glenn, the patch I suggested in comment #4 landed incorrectly.
> ... Could you check this, please?

Yes, it's incorrect but fairly harmless.  The wrong length on
png_crc_finish() will cause a png_error when libpng attempts
to read the next chunk.  The extra code inside the if{} is
harmless except for the increase in the size of the executable.

I'll try to generate a new patch today (against the current trunk
which seems to be still at libpng-1.4.3) but I think it has missed
the Firefox-5.0 cutoff.
(Assignee)

Comment 42

6 years ago
Created attachment 525679 [details] [diff] [review]
v06 Upgrade to libpng-1.4.7

Revised per Max's comment.  Patch is against trunk (assumes v5 has not
been applied).
http://hg.mozilla.org/mozilla-central/rev/7511736d309b
Whiteboard: fixed-in-cedar
(In reply to comment #42)
> Revised per Max's comment.  Patch is against trunk (assumes v5 has not
> been applied).

Can you please attach a patch on top of the current trunk which addresses comment 41?
(Assignee)

Comment 45

6 years ago
@ehsan, hopefully this evening (EST), don't have access to my files and tools right now.
(Assignee)

Comment 46

6 years ago
While making a new patch I will also fix the fact that, when info_ptr is
NULL, max's (and the v06) patch still returns success (sets a mode bit).
It shouldn't set the mode bit in this case.  This error does not affect
Firefox, though.  The fix is easy, just move the mode-setting line inside
the if{}.
Is this patch as landed safe to get on a nightly?  If not, I can just back it out and reland the fixed version...
(Assignee)

Comment 48

6 years ago
Created attachment 525794 [details] [diff] [review]
v07 Fix coding errors in png_handle_fcTL

See comment #40 and comment #46
This patch assumes the v5 patch has already been applied.
Attachment #525679 - Attachment is obsolete: true
(Assignee)

Comment 49

6 years ago
(In reply to comment #47)
> Is this patch as landed safe to get on a nightly?  If not, I can just back it
> out and reland the fixed version...

That's OK.  I made a fresh clone and built the small v07 patch against that.
(Assignee)

Updated

6 years ago
Attachment #525794 - Flags: review?(joe)
v07 builds fine locally. Running through Try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=bf80756497cb
Attachment #525794 - Flags: review?(joe) → review+
(Assignee)

Updated

6 years ago
Blocks: 669863
(Assignee)

Updated

6 years ago
No longer blocks: 669863
(Assignee)

Comment 51

6 years ago
Both the v5 and v07 patches have been checked in to mozilla-central a while ago.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Blocks: 669863
(Assignee)

Comment 52

6 years ago
(In reply to comment #51)
> Both the v5 and v07 patches have been checked in to mozilla-central a while
> ago.

Correction: only the v5 patch was checked in.  Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 53

6 years ago
checkin-needed (of the v07 patch only)
Keywords: checkin-needed

Comment 54

6 years ago
Pushed v07: http://hg.mozilla.org/integration/mozilla-inbound/rev/88329c72d50c
Flags: in-testsuite-
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/88329c72d50c

Please check status of the bug, esp target milestone and such, since it's a bit unclear to me.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.