Last Comment Bug 648690 - Update libpng to version 1.5.9
: Update libpng to version 1.5.9
Status: RESOLVED FIXED
[gfx.relnote.13]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: mozilla13
Assigned To: Ryan VanderMeulen [:RyanVM]
:
Mentors:
http://libpng.sf.net
Depends on: 624133 669863 733598
Blocks: 734317 745178
  Show dependency treegraph
 
Reported: 2011-04-08 18:04 PDT by Glenn Randers-Pehrson
Modified: 2012-05-24 12:00 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v00 Upgrade bundled libpng to version 1.5.6 (1.41 MB, patch)
2011-11-28 22:11 PST, Glenn Randers-Pehrson
no flags Details | Diff | Splinter Review
v01 Upgrade bundled libpng to version 1.5.8 (1.68 MB, patch)
2012-02-16 01:30 PST, Max Stepin
no flags Details | Diff | Splinter Review
v02 Upgrade bundled libpng to version 1.5.8 (1.69 MB, patch)
2012-02-16 15:56 PST, Ryan VanderMeulen [:RyanVM]
no flags Details | Diff | Splinter Review
v01 Upgrade bundled libpng to version 1.5.9 (1.76 MB, patch)
2012-02-19 06:43 PST, Ryan VanderMeulen [:RyanVM]
no flags Details | Diff | Splinter Review
pngread.patch (2.22 KB, patch)
2012-02-19 09:23 PST, Max Stepin
joe: feedback+
Details | Diff | Splinter Review
v02 Upgrade bundled libpng to version 1.5.9 (upstream changes) (1.76 MB, patch)
2012-02-19 13:18 PST, Ryan VanderMeulen [:RyanVM]
joe: review+
Details | Diff | Splinter Review
v02 Upgrade bundled libpng to version 1.5.9 (Mozilla changes) (1.12 KB, patch)
2012-02-19 13:19 PST, Ryan VanderMeulen [:RyanVM]
no flags Details | Diff | Splinter Review
v03 Upgrade bundled libpng to version 1.5.9 (Mozilla changes) (2.57 MB, patch)
2012-02-25 10:00 PST, Ryan VanderMeulen [:RyanVM]
joe: review+
Details | Diff | Splinter Review
expected-favicon-big64.png.png (868 bytes, image/png)
2012-03-01 13:52 PST, Max Stepin
no flags Details
image1.png (8.22 KB, image/png)
2012-03-01 23:49 PST, Max Stepin
no flags Details
v04 Upgrade bundled libpng to version 1.5.9 (Mozilla changes) (2.58 MB, patch)
2012-03-03 10:57 PST, Ryan VanderMeulen [:RyanVM]
ryanvm: review+
dolske: feedback+
Details | Diff | Splinter Review

Description Glenn Randers-Pehrson 2011-04-08 18:04:23 PDT
Libpng-1.5.2 has been released.  Bug #624133 previously was to update libpng to 1.5.2 but we dropped back to 1.4.6 instead.  In the long term we will want to upgrade to 1.5.x.  This is a large change and it will not be ready in time for Firefox 5.0.  It is not urgent to upgrade to 1.5 because it really offers nothing new in terms of security or performance for Mozilla.
Comment 1 Glenn Randers-Pehrson 2011-06-13 06:38:31 PDT
Libpng-1.5.3 will be released shortly.  It will employ a slightly more accurate function for scaling 16-bit PNG files down to 8 bit.  As a result, some pixel samples will be different by one intensity level from what is generated by earlier versions of libpng.  I assume this will cause a problem with reftests, although the images won't be perceptibly different.  These differences will occur whether we update the bundled libpng, or if a user is using the system libpng and upgrades the system libpng to 1.5.3.  Do you think it would be useful for libpng-1.5.3 to have a function that causes the old algorithm to be used, so the results will be the same no matter which "system" version of libpng is used?
Comment 2 Glenn Randers-Pehrson 2011-07-25 04:33:29 PDT
Libpng-1.5.4 has been released. It fixes four CVEs; however, these are also fixed in libpng-1.4.8 (see bug #669863).  With respect to the question in comment #1, libpng-1.5.4 uses the same (slightly inaccurate) 16-to-8 scaling method by default as earlier versions use.
Comment 3 NVD 2011-09-23 04:07:08 PDT
Now that libpng-1.5.5 has been released, can we update libpng for Firefox 10.0 after the merge on the 27th? It would be nice to be on the current libpng release and not the legacy 1.4.x branch.
Comment 4 Glenn Randers-Pehrson 2011-10-16 21:25:54 PDT
Until now there has been no good reason to move to libpng-1.5.x.  However, libpng-1.5.6, due out fairly soon, is going to offer about a 5-10 percent speedup in decoding interlaced PNGs.  I'm changing the summary to say 1.5.6 instead of 1.5.x.
Comment 5 Glenn Randers-Pehrson 2011-11-28 22:11:23 PST
Created attachment 577495 [details] [diff] [review]
v00 Upgrade bundled libpng to version 1.5.6

Here is a first cut at upgrading libpng to 1.5.6
Someone please see if it passes the tryserver runs.
Comment 6 Ryan VanderMeulen [:RyanVM] 2011-11-29 14:40:00 PST
Builds are failing due to missing pnglibconf.h. Glenn, it appears you didn't hg add them before making the patch. I will update your patch and re-push (and attach it here if it succeeds).
Comment 7 Mozilla RelEng Bot 2011-11-29 14:40:43 PST
Try run for 8262d4257016 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8262d4257016
Results (out of 11 total builds):
    exception: 4
    failure: 7
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-8262d4257016
Comment 8 Ryan VanderMeulen [:RyanVM] 2011-11-29 14:56:42 PST
Glenn, this is still dying even when I hg added the .h files. See the linux build log here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-3b82c676d5e8/try-linux/try-linux-build109.txt.gz
Comment 9 Mozilla RelEng Bot 2011-11-29 15:01:32 PST
Try run for 3b82c676d5e8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3b82c676d5e8
Results (out of 11 total builds):
    exception: 10
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-3b82c676d5e8
Comment 10 Glenn Randers-Pehrson 2011-11-29 16:41:29 PST
Comment on attachment 577495 [details] [diff] [review]
v00 Upgrade bundled libpng to version 1.5.6

Marking the v00 patch obsolete.  I'll have a replacement in a couple of days.
Comment 11 Virtual_ManPL [:Virtual] - (ni? me) 2012-01-14 02:14:04 PST
FYI: libpng-1.5.7 is the last stable

changelog:
http://www.libpng.org/pub/png/src/libpng-1.5.7-README.txt
Comment 12 Max Stepin 2012-02-16 01:30:36 PST
Created attachment 597717 [details] [diff] [review]
v01 Upgrade bundled libpng to version 1.5.8

Proposed patch to upgrade libpng to 1.5.8
Comment 13 Glenn Randers-Pehrson 2012-02-16 09:31:33 PST
Thanks, Max.

Would someone please run this through the tryserver?

I haven't tested but read through the patch and it doesn't
have anything obviously wrong.

libpng-1.5.9 will be out fairly soon, but it'll be an
easy patch on top of this one.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-02-16 09:33:31 PST
I'll send it to Try tonight.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-02-16 09:40:11 PST
Minor nit: please update MOZCHANGES in the final patch. Also, I'm assuming no changes are needed for mozpngconf.h.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-02-16 09:42:25 PST
(In reply to Ryan VanderMeulen from comment #15)
> Minor nit: please update MOZCHANGES in the final patch. Also, I'm assuming
> no changes are needed for mozpngconf.h.

Nevermind on the first nit. Bugzilla's diff wasn't showing the filename for some reason...
Comment 17 Glenn Randers-Pehrson 2012-02-16 09:48:05 PST
(In reply to Ryan VanderMeulen from comment #15)
> Minor nit: please update MOZCHANGES in the final patch. Also, I'm assuming
> no changes are needed for mozpngconf
(In reply to Ryan VanderMeulen from comment #16)
> (In reply to Ryan VanderMeulen from comment #15)
> > Minor nit: please update MOZCHANGES in the final patch. Also, I'm assuming
> > no changes are needed for mozpngconf.h.

The build process should make a libpng.so somewhere in the
build tree.  Run "nm" against that and see if there are any
symbols beginning with "png".  If so, macros to redefine those
to MOZ_something will have to be added to mozpngconf.h

> 
> Nevermind on the first nit. Bugzilla's diff wasn't showing the filename for
> some reason...

Yeah I noticed that this morning too.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-02-16 15:56:37 PST
Created attachment 598041 [details] [diff] [review]
v02 Upgrade bundled libpng to version 1.5.8

Try is currently closed, so I can't push this patch at the moment. If things re-open tonight still, I'll push it.

In the mean time, to make life easier for people like me, please include a proper Mercurial diff as your next patch.
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Also, the attached patch failed to apply cleanly due to bug 727401 (currently security-sensitive). Take a look at pngrutil.c to make sure my un-bitrotting was OK. Below is a link to the changeset in question. Glenn, have you seen this yet for upstreaming?
http://hg.mozilla.org/mozilla-central/rev/592c27677267
Comment 19 Mozilla RelEng Bot 2012-02-16 22:31:28 PST
Try run for 35e6d9a50b4b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=35e6d9a50b4b
Results (out of 25 total builds):
    success: 14
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-35e6d9a50b4b
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-02-17 07:13:34 PST
Looks like the PNG reftests may need some tweaking.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-02-17 12:44:16 PST
(In reply to Ryan VanderMeulen from comment #20)
> Looks like the PNG reftests may need some tweaking.

The failures are all along the lines of off by one. Not sure why the change, though. Glenn, any ideas?
Comment 22 Glenn Randers-Pehrson 2012-02-17 13:03:58 PST
If you are seeing the changes in PNG files being *written*, one thing is that we optimized the zlib datastream in small files.

In the changelog for libpng-1.5.4 is this:
Added PNG_WRITE_OPTIMIZE_CMF_SUPPORTED macro to make the zlib "CMF" byte
    optimization configureable.

To see if this is the problem, try undefining
PNG_WRITE_OPTIMIZE_CMF_SUPPORTED
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-02-17 13:09:22 PST
I don't think any writing is involved. From what I can tell, the PNG reftests basically build a table of 1x1 cells with a certain background color and compare against a static PNG file.
http://mxr.mozilla.org/mozilla-central/source/image/test/reftest/pngsuite-basic-n/
Comment 24 Glenn Randers-Pehrson 2012-02-17 13:31:20 PST
There are improvements in gamma handling, but I don't think mozilla uses libpng's internal
gamma handling.  The rgb-to-gray conversion is more accurate but I don't believe we use that
either.  The reduction from 16-bit to 8-bit is also more accurate, perhaps that's the
culprit
.
Version 1.5.4beta05 [June 16, 2011]
  Renamed png_set_strip_16() to png_set_scale_16() and renamed
    png_set_chop_16() to png_set_strip(16) in an attempt to minimize the
    behavior changes between libpng14 and libpng15.

We use png_set_strip_16() so the behavior should not have changed.  Do you know if the
failures involve 16-bit input files?

There were also changes in the gray-to-rgb conversion; do the failures involve
grayscale PNG inputs?
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-02-17 13:49:15 PST
On Windows, these were the failures. I see that other platforms basically have the same list (though a few differences).

pngsuite-basic-n/basn0g16.png    - 16 bit (64k level) grayscale 
pngsuite-basic-i/basi0g16.png    - 16 bit (64k level) grayscale
pngsuite-basic-i/basi2c16.png    - 3x16 bits rgb color
pngsuite-ancillary/cs3n2c16.png  - color, 13 significant bits (sBIT chunks)
pngsuite-chunkorder/oi1n0g16.png - grayscale mother image with 1 idat-chunk
pngsuite-chunkorder/oi1n2c16.png - color mother image with 1 idat-chunk
pngsuite-chunkorder/oi2n0g16.png - grayscale image with 2 idat-chunks
pngsuite-chunkorder/oi2n2c16.png - color image with 2 idat-chunks
pngsuite-chunkorder/oi4n0g16.png - grayscale image with 4 unequal sized idat-chunks
pngsuite-chunkorder/oi4n2c16.png - color image with 4 unequal sized idat-chunks
pngsuite-chunkorder/oi9n0g16.png - grayscale image with all idat-chunks length one
pngsuite-chunkorder/oi9n2c16.png - color image with all idat-chunks length one
pngsuite-gamma/g03n0g16.png      - grayscale, file-gamma = 0.35
pngsuite-gamma/g04n3p04.png      - paletted, file-gamma = 0.45
pngsuite-gamma/g05n0g16.png      - grayscale, file-gamma = 0.55
pngsuite-gamma/g07n0g16.png      - grayscale, file-gamma = 0.70
pngsuite-gamma/g10n0g16.png      - grayscale, file-gamma = 1.00
pngsuite-gamma/g25n0g16.png      - grayscale, file-gamma = 2.50
pngsuite-palettes/pp0n2c16.png   - six-cube palette-chunk in true-color image
pngsuite-palettes/ps1n2c16.png   - six-cube suggested palette (1 byte) in true-color image
pngsuite-palettes/ps2n2c16.png   - six-cube suggested palette (2 bytes) in true-color image
pngsuite-transparency/wrapper.html?tbbn2c16.png - transparent, blue background chunk
pngsuite-transparency/wrapper.html?tbgn2c16.png - transparent, green background chunk
pngsuite-transparency/wrapper.html?tbwn1g16.png - transparent, white background chunk
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-02-17 13:52:03 PST
Also, pngsuite-basic-n/basn2c16.png - 3x16 bits rgb color
Comment 27 NVD 2012-02-19 04:33:49 PST
http://sourceforge.net/projects/libpng/files/libpng15/1.5.9/

Libpng 1.5.9 is out with the libpng security fix in 10.0.2.

"Fixed CVE-2011-3026 buffer overrun bug. Deal more correctly with the test on iCCP chunk length. Also removed spurious casts that may hide problems on 16-bit systems."
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-02-19 06:43:47 PST
Created attachment 598659 [details] [diff] [review]
v01 Upgrade bundled libpng to version 1.5.9

Here's a 1.5.9 patch. I've pushed it to try as well (though I'm still expecting the reftests to fail). Also, I've added an apng.patch to the diff to make future upgrades hopefully a bit easier.
https://tbpl.mozilla.org/?tree=Try&rev=3c4e595d3688
Comment 29 Max Stepin 2012-02-19 08:48:06 PST
Let me propose two small changes. 

APNG patch always adds the code surrounded by
#ifdef PNG_APNG_SUPPORTED
#endif

Well always except for two places...
One is at the end of png.h where I think we should put

#ifdef PNG_EXPORT_LAST_ORDINAL
#ifdef PNG_APNG_SUPPORTED
  PNG_EXPORT_LAST_ORDINAL(253);
#else
  PNG_EXPORT_LAST_ORDINAL(233);
#endif /* PNG_APNG_SUPPORTED */
#endif

And another is at pngread.c where it more complicated.
But it should also be like

#ifdef PNG_APNG_SUPPORTED
  /* apng code */
#else
  /* libpng code */
#endif

I'm going to make a patch for pngread.c for you to try.
Comment 30 Max Stepin 2012-02-19 09:23:35 PST
Created attachment 598667 [details] [diff] [review]
pngread.patch

Here's the patch for pngread.c - it should apply on top on your latest patch.
Comment 31 Mozilla RelEng Bot 2012-02-19 10:31:14 PST
Try run for 3c4e595d3688 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3c4e595d3688
Results (out of 27 total builds):
    success: 15
    warnings: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-3c4e595d3688
Comment 32 Glenn Randers-Pehrson 2012-02-19 10:38:29 PST
If the warnings are again all involving 16-bit input files, it's most likely due to an improvement in the accuracy of scaling 16-bit images down to 8-bit in libpng15.  If so, we should go ahead and change the call to png_strip_16() to png_scale_16() in image/decoders/nsPNGDecoder.cpp to obtain the most accurate scaling.  For now, if all of the warnings are about 16-bit imputs, don't worry.
Comment 33 Max Stepin 2012-02-19 11:04:32 PST
All of them 16-bit, with one exception:

g04n3p04.png (4-bit)
Comment 34 Ryan VanderMeulen [:RyanVM] 2012-02-19 11:11:09 PST
If the cause of the failure is understood and accepted, I think it would be fine to just adjust the reftests to look for the new values. We should get an ImageLib peer to sign off on that first, though.

Glenn, would you expect the change to nsPNGDecoder.cpp to have any affect on the reftest results?
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-02-19 11:16:31 PST
Comment on attachment 598667 [details] [diff] [review]
pngread.patch

Joe, are you OK with this in principle?
Comment 36 Glenn Randers-Pehrson 2012-02-19 11:32:03 PST
Yes the change to nsPNGDecoder.cpp might also make some off-by-one differences from both the original result and the new results for libpng-1.5.9.  Only for 16-bit input images.  Since there are already changes, this would be a good time to switch to the most accurate method.

The change at line 552 in nsPNGDecoder.cpp would look like this:

  if (bit_depth == 16)
#ifdef PNG_READ_SCALE_16_TO_8_SUPPORTED
    png_set_scale_16(png_ptr);
#else
    png_set_strip_16(png_ptr);
#endif

and make sure that PNG_READ_SCALE_16_TO_8_SUPPORTED is defined
in pnglibconf.h
Comment 37 Max Stepin 2012-02-19 11:37:43 PST
PNG_READ_SCALE_16_TO_8_SUPPORTED is defined

But you should probably investigate that 4-bit image too:

g04n3p04.png

Is it possible that gamma calculations (in fixed/floating point) are slightly different in 1.4 and 1.5 branches?
Comment 38 Joe Drew (not getting mail) 2012-02-19 12:55:45 PST
Comment on attachment 598667 [details] [diff] [review]
pngread.patch

Sure.
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-02-19 13:18:04 PST
Created attachment 598700 [details] [diff] [review]
v02 Upgrade bundled libpng to version 1.5.9 (upstream changes)

Picking up the png.h and pngread.c APNG changes.
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-02-19 13:19:01 PST
Created attachment 598701 [details] [diff] [review]
v02 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)

Companion patch for the nsPNGDecoder.cpp change. Both patches sent to Try.
https://tbpl.mozilla.org/?tree=Try&rev=f26113469378
Comment 41 Ryan VanderMeulen [:RyanVM] 2012-02-19 16:50:58 PST
Still failing the same as before. Also interesting that Linux and Windows have the same 25 failures, while OS X adds 4 more, all 16 bit images:
pngsuite-background/bgai4a16.png
pngsuite-background/bgan6a16.png
pngsuite-background/bggn4a16.png
pngsuite-background/bgyn6a16.png

The other option I was thinking about is that the reftest framework allows for setting fuzz values in the manifest to allow for differences like these. It's usually used to account for subpixel AA differences between platforms, but it would probably work here too.
Comment 42 Mozilla RelEng Bot 2012-02-19 17:16:49 PST
Try run for f26113469378 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f26113469378
Results (out of 25 total builds):
    success: 14
    warnings: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-f26113469378
Comment 43 Max Stepin 2012-02-19 23:16:02 PST
Using a small standalone program I tried to track down when exactly that off-by-one change first appeared.

Turns out it's libpng-1.5.0beta36 

At that beta36 I see a whole lot of new functuons appear in png.c
Some of them:

png_log8bit()
png_log16bit()
png_exp8bit()
png_exp16bit()
png_gamma_8bit_correct()
png_gamma_16bit_correct()
png_gamma_correct()
png_gamma_significant()
png_build_16bit_table()
png_build_16to8_table()
png_build_8bit_table()
Comment 44 Glenn Randers-Pehrson 2012-02-20 06:28:30 PST
Yes, gamma is involved in the 16-bit scaling in libpng15.  You can defeat that (temporarily, just to see if that is the cause of the differences) by commenting out or deleting the first of the two instances of png_set_gamma() in nsPNGDecoder.cpp

Also, only one of PNG_READ_SCALE_16_TO_8 or PNG_READ_STRIP_16_TO_8 should be enabled.  If we want to use the more accurate scaling, don't enable PNG_READ_STRIP_16_TO_8.  If we decide we want to use the faster, simpler, leaner current method, enable PNG_READ_STRIP_16_TO_8 and also disable PNG_READ_GAMMA in mozpngconf.h
Comment 45 Max Stepin 2012-02-20 09:35:38 PST
(In reply to Glenn Randers-Pehrson from comment #44)
> Yes, gamma is involved in the 16-bit scaling in libpng15.  You can defeat
> that (temporarily, just to see if that is the cause of the differences) by
> commenting out or deleting the first of the two instances of png_set_gamma()
> in nsPNGDecoder.cpp

I tried that, and yes, if gamma is not set, there are no differences.

But we need gamma, I think.

> Also, only one of PNG_READ_SCALE_16_TO_8 or PNG_READ_STRIP_16_TO_8 should be
> enabled.

OK, but there is no harm in defining both for now, I think. We can take care of minor things like that sometimes later.

The only remaining problem is to figure out how to fix reftests... 
We need someone who knows that stuff.
Comment 46 Glenn Randers-Pehrson 2012-02-20 11:38:15 PST
OK, that's fine.  We don't need to worry about the reftest "failures" now that we are sure of the cause.  Note that we still have gamma data from the gAMA chunk; it's just that we don't process it internally but leave it to the CMS.
Comment 47 Glenn Randers-Pehrson 2012-02-20 11:41:00 PST
The only "harm" of defining both SCALE and STRIP versions is that we will never use the STRIP code (png_do_chop) so it's a few hundred bytes of bloat.  We used to worry about even minor amounts of bloat.
Comment 48 Ryan VanderMeulen [:RyanVM] 2012-02-20 13:29:02 PST
It sounds like the best approach for the reftests then is to change the 25 tests that fail across all platforms (though one thing I haven't confirmed is that in fact they fail exactly the same on all) and to set the remaining 4 with a fuzz value of 1. It's either than or I set them all to a fuzz of 1.

What sayeth the ImageLib peers CCed here?
Comment 49 Max Stepin 2012-02-21 19:33:43 PST
The remaining 4 tests in pngsuite-background directory were always problematic because they involve 2 steps: first is image decoding, second is blending on the background. 

That second part works differently on different platforms. And I think hardware acceleration could change the results on the same platform.

So I agree on updating the tests (using img2html.html converter) and using fuzz 1 for pngsuite-background.

I also argee with Glenn on removing this from mozpngconf.h
#define PNG_READ_STRIP_16_TO_8_SUPPORTED
Comment 50 Ryan VanderMeulen [:RyanVM] 2012-02-23 17:23:18 PST
The pngsuite-background case gets even better. It's currently marked as a known-fail for all platforms *except* OS X. And in fact, looking at the logs, it does indeed fail on the other platforms. So at least we're consistent across all platforms now!

I'm going to set it as fuzzy on all platforms for now and see what happens.
Comment 51 Mozilla RelEng Bot 2012-02-24 00:00:30 PST
Try run for c5d799db0b48 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c5d799db0b48
Results (out of 25 total builds):
    success: 13
    warnings: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-c5d799db0b48
Comment 52 Mozilla RelEng Bot 2012-02-25 06:03:43 PST
Try run for 6edbdf98eee2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6edbdf98eee2
Results (out of 5 total builds):
    exception: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-6edbdf98eee2
Comment 53 Mozilla RelEng Bot 2012-02-25 09:31:06 PST
Try run for 918161311ce2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=918161311ce2
Results (out of 25 total builds):
    success: 23
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-918161311ce2
Comment 54 Ryan VanderMeulen [:RyanVM] 2012-02-25 09:39:35 PST
Victory! \o/
Comment 55 Ryan VanderMeulen [:RyanVM] 2012-02-25 10:00:39 PST
Created attachment 600687 [details] [diff] [review]
v03 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)

To summarize everything up to this point, this patch does the following:

1.) Increments the minimum libpng system library version in configure.in to 1.5.9
2.) Changes nsPNGDecoder.cpp to use the more accurate png_set_scale_16 instead of png_set_strip_16.
3.) Changes many of the PNG reftests to reflect subtle off by one changes caused by changes in libpng 1.5.x. See comment 46 and some of the preceeding comments for more information.
4.) Changes the pngsuite-background tests from fails-if(!cocoaWidget) to failing only on Android (as is the case with most of the rest of the PNG test suite) and fuzzy-if on OSX with one allowable point of variation. So in effect, the test coverage is actually improved now compared to where it used to be as Linux and Windows both pass now where they used to fail.

At this point, it is completely green on Try.
Comment 56 Ryan VanderMeulen [:RyanVM] 2012-02-25 10:06:02 PST
Forgot to mention, it also removes the PNG_READ_STRIP_16_TO_8_SUPPORTED define from mozpngconf.h per comment 49.
Comment 57 Max Stepin 2012-02-25 10:10:39 PST
All right, sounds great!
Comment 58 Max Stepin 2012-02-25 11:01:27 PST
You should probably mark the previous patch as obsolete.
Comment 59 Ryan VanderMeulen [:RyanVM] 2012-02-25 11:02:24 PST
It's two separate patches. One's the upstream changes and one's the rest.
Comment 60 Joe Drew (not getting mail) 2012-02-27 11:09:28 PST
Comment on attachment 598700 [details] [diff] [review]
v02 Upgrade bundled libpng to version 1.5.9 (upstream changes)

rs=joe
Comment 61 Joe Drew (not getting mail) 2012-02-27 11:10:25 PST
Comment on attachment 600687 [details] [diff] [review]
v03 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)

Review of attachment 600687 [details] [diff] [review]:
-----------------------------------------------------------------

i approve this message
Comment 63 Ryan VanderMeulen [:RyanVM] 2012-02-27 17:44:45 PST
So, little did I know, but there are other PNG tests that aren't reftests! Backed out due to xpcshell test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/52b5e922f56e

TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_apng.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_apng.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending
Checking apng1A...

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_apng.js | data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAADCAIAAADZSiLoAAAACGFjVEwAAAADAAAAAM7tusAAAAAaZmNUTAAAAAAAAAADAAAAAwAAAAAAAAAAAfQD6AAAdRYgGAAAABBJREFUeJxj+M/AAEEMWFgAj44I+JFjxLkAAAAaZmNUTAAAAAEAAAADAAAAAwAAAAAAAAAAAfQD6AAA7mXKzAAAABNmZEFUAAAAAnicY2D4zwBFWFgAhpcI+AwIYZYAAAAaZmNUTAAAAAMAAAADAAAAAwAAAAAAAAAAAfQD6AAAA/MZJQAAABNmZEFUAAAABHicY2Bg+A9DmCwAfaAI+INZ9ZsAAAAASUVORK5CYII= == data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAADCAIAAADZSiLoAAAACGFjVEwAAAADAAAAAM7tusAAAAAaZmNUTAAAAAAAAAADAAAAAwAAAAAAAAAAAfQD6AAAdRYgGAAAABBJREFUCJlj+M/AAEEMWFgAj44I+H2CySsAAAAaZmNUTAAAAAEAAAADAAAAAwAAAAAAAAAAAfQD6AAA7mXKzAAAABNmZEFUAAAAAgiZY2D4zwBFWFgAhpcI+I731VcAAAAaZmNUTAAAAAMAAAADAAAAAwAAAAAAAAAAAfQD6AAAA/MZJQAAABNmZEFUAAAABAiZY2Bg+A9DmCwAfaAI+AGmQVoAAAAASUVORK5CYII= - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _do_check_eq :: line 556
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_check_eq :: line 577
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_apng.js :: run_test_for :: line 386
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_apng.js :: run_test :: line 365
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _execute_test :: line 326
JS frame :: -e :: <TOP_LEVEL> :: line 1

TEST-INFO | (xpcshell/head.js) | exiting test
<<<<<<<
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_png.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_png.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending
Checking png1A...

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_png.js | data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAADCAIAAADZSiLoAAAAEUlEQVR4nGP4z8AAQTAamQkAhpcI+Clh3zIAAAAASUVORK5CYII= == data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAADCAIAAADZSiLoAAAAEUlEQVQImWP4z8AAQTAamQkAhpcI+DeMzFcAAAAASUVORK5CYII= - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _do_check_eq :: line 556
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_check_eq :: line 577
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_png.js :: run_test_for :: line 118
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_encoder_png.js :: run_test :: line 103
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _execute_test :: line 326
JS frame :: -e :: <TOP_LEVEL> :: line 1

TEST-INFO | (xpcshell/head.js) | exiting test
<<<<<<<
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_imgtools.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/image/test/unit/test_imgtools.js | test failed (with xpcshell return code: 3), see following log:
>>>>>>>

And test_imgtools.js doesn't give any debugging info. Yay.
Comment 64 Ryan VanderMeulen [:RyanVM] 2012-02-27 17:46:41 PST
Tests live here if anyone wants to look at them:
http://mxr.mozilla.org/mozilla-central/source/image/test/unit/
Comment 65 Max Stepin 2012-02-27 20:49:36 PST
Identical, except for first 2 bytes of zstream... interesting.
Comment 66 Max Stepin 2012-02-27 23:32:15 PST
OK, some optimization code for zstream (that changes its first 2 bytes) was running unconditionally before, but now it's conditional and we need this #define to activate it:

#define PNG_WRITE_OPTIMIZE_CMF_SUPPORTED

Lets just add this line into mozpngconf.h
Comment 67 Glenn Randers-Pehrson 2012-02-28 05:11:54 PST
This option is normally "on" in the system libpng builds but since we're using mozpngconf.h to control the feature support in our embedded libpng, we would need to turn it on explicitly.  The optimization does not change the image, but it changes a flag in the zlib datastream that allows a subsequent decoder of the PNG to allocate less memory.  However, the optimization does increase the size of the compiled libpng somewhat (320 bytes on my platform), so if we are still concerned about footprint we could leave it off.
Comment 68 Ryan VanderMeulen [:RyanVM] 2012-02-28 05:57:22 PST
What kind of memory usage improvements are we talking about here? My hunch is that 320 bytes of footprint increase is probably worth it in exchange for less memory usage, especially if it's code that was previously being used unconditionally before. Anyway, I'll add that define to mozpngconf.h and push it to Try tonight to confirm that theory.
Comment 69 Glenn Randers-Pehrson 2012-02-28 06:12:42 PST
The memory improvement is in code that *reads* the file written by mozilla.  zlib sets its "sliding window" to a power of two, from 256 to 32k.  This optimization allows the reader to use a smaller window than without the optimization.  So the savings could be no more than 32k but will usually be none or some power of 2 less than 32k.  I forget whether multiple copies of the sliding window are allocated but I don't think so.
Comment 70 Ryan VanderMeulen [:RyanVM] 2012-02-28 19:47:17 PST
Try results will be posting here later, but the long story short is that comment #66 does fix the test_encoder_png.js and test_encoder_apng.js failures.
https://tbpl.mozilla.org/?tree=Try&rev=e89439c6ac3c

It doesn't help with the test_imgtools.js failure, though. Justin Dolske has a patch in bug 731093 that will provide better debugging info. I'm going to do another push to Try with it applied. My first impression from looking into them thus far is that it's an issue of comparing an image to a reference png and differences are present. So maybe more of the same like the reftests.
https://tbpl.mozilla.org/?tree=Try&rev=509109e947e7
Comment 71 Mozilla RelEng Bot 2012-02-28 21:31:10 PST
Try run for e89439c6ac3c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e89439c6ac3c
Results (out of 288 total builds):
    exception: 4
    success: 233
    warnings: 47
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-e89439c6ac3c
Comment 72 Mozilla RelEng Bot 2012-02-28 23:02:11 PST
Try run for 509109e947e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=509109e947e7
Results (out of 2 total builds):
    success: 1
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-509109e947e7
Comment 73 Max Stepin 2012-02-28 23:43:52 PST
favicon-big64.png
has a gAMA chunk, so after loading it and scaling to 16x16 and saving as
expected-favicon-big64.png.png 
we might get slightly different result, just as we did with reftests.
Comment 74 Ryan VanderMeulen [:RyanVM] 2012-02-29 19:22:54 PST
I tried scaling the image with canvas and saving it. Likewise for the jpegs that were failing. No dice. Still working on it!
https://tbpl.mozilla.org/?tree=Try&rev=44d3caede65f
Comment 75 Max Stepin 2012-03-01 13:52:03 PST
Created attachment 602091 [details]
expected-favicon-big64.png.png

Try this updated file.
Comment 76 Ryan VanderMeulen [:RyanVM] 2012-03-01 18:01:37 PST
Yes, that one worked. How did you generate it? I did my attempt with a canvas element and that clearly didn't work...
Comment 77 Max Stepin 2012-03-01 20:07:25 PST
A bit of hacking... I had to create the actual favicon and then extract the 16x16 version from places.sqlite 

What are the other failures? Are they similar?
Comment 78 Max Stepin 2012-03-01 23:14:39 PST
I guess test_imgtools.js might be failing because image1.png in that directory contains gAMA chunk, with the same consequences. I think instead of changing two JPG results we should just change image1.png itself, by removing that gAMA chunk (but applying gamma information to the pixels first).
Comment 79 Max Stepin 2012-03-01 23:49:05 PST
Created attachment 602266 [details]
image1.png

Try this.
Comment 80 Mozilla RelEng Bot 2012-03-02 00:02:50 PST
Try run for e39082a0322b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e39082a0322b
Results (out of 279 total builds):
    success: 227
    warnings: 50
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ryanvm@gmail.com-e39082a0322b
Comment 81 Ryan VanderMeulen [:RyanVM] 2012-03-02 04:50:37 PST
To be clear, comment 80 was a push prior to comment 79.
Comment 82 Ryan VanderMeulen [:RyanVM] 2012-03-02 19:21:37 PST
Looks good! Running a full Try run again to confirm that we're in the clear.

Justin, see comment 78. Since you wrote the test, do you have any issue with us changing the reference PNG as described?
Comment 83 Ryan VanderMeulen [:RyanVM] 2012-03-03 10:57:16 PST
Created attachment 602637 [details] [diff] [review]
v04 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)

It passes a full Try run now.
https://tbpl.mozilla.org/?tree=Try&rev=2e2e6aea2946

Joe, we adjusted a few more images to pass the xpcshell tests similar to the reftest adjustments made earlier. I'm asking for feedback from Justin since we modified the starting image a bit for test_imgtools.js.
Comment 84 Justin Dolske [:Dolske] 2012-03-03 11:04:57 PST
Comment on attachment 602637 [details] [diff] [review]
v04 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)

Didn't look in detail, but seems fine. The tests were generally just checking for whatever output was generated at the time, such that we'd catch unexpected changes. Modifying them for expected changes is cool.
Comment 85 Ryan VanderMeulen [:RyanVM] 2012-03-03 19:17:16 PST
Comment on attachment 602637 [details] [diff] [review]
v04 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)

Got an rs from joe on IRC.
Comment 87 Max Stepin 2012-03-03 22:45:17 PST
Looks great! Thanks everybody!

Note You need to log in before you can comment on or make changes to this bug.