Closed Bug 648690 Opened 14 years ago Closed 13 years ago

Update libpng to version 1.5.9

Categories

(Core :: Graphics: ImageLib, enhancement)

Product:
Core
Component:
Graphics: ImageLib
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: glennrp+bmo, Assigned: RyanVM)

References

(
URL
)

Details

(Whiteboard: [gfx.relnote.13])

Attachments

(4 files, 7 obsolete files)

v02 Upgrade bundled libpng to version 1.5.9 (upstream changes)
1.76 MB, patch
joe
: review+
Details | Diff | Splinter Review
expected-favicon-big64.png.png
868 bytes, image/png
Details
image1.png
8.22 KB, image/png
Details
v04 Upgrade bundled libpng to version 1.5.9 (Mozilla changes)
2.58 MB, patch
RyanVM
: review+
Dolske
: feedback+
Details | Diff | Splinter Review
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.
Assignee: nobody → glennrp+bmo
Status: NEW → ASSIGNED
URL: http://libpng.sf.net
Depends on: 624133
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?
Depends on: 669863
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.
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.
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.
Summary: Update libpng to version 1.5.x → Update libpng to version 1.5.6
Version: unspecified → Trunk
Here is a first cut at upgrading libpng to 1.5.6 Someone please see if it passes the tryserver runs.
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).
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
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
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 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.
Attachment #577495 - Attachment is obsolete: true
Proposed patch to upgrade libpng to 1.5.8
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.
I'll send it to Try tonight.
Summary: Update libpng to version 1.5.6 → Update libpng to version 1.5.8
Minor nit: please update MOZCHANGES in the final patch. Also, I'm assuming no changes are needed for mozpngconf.h.
(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...
(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.
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
Attachment #597717 - Attachment is obsolete: true
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
Looks like the PNG reftests may need some tweaking.
(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?
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
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/
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?
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
Also, pngsuite-basic-n/basn2c16.png - 3x16 bits rgb color
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."
Summary: Update libpng to version 1.5.8 → Update 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
Attachment #598041 - Attachment is obsolete: true
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.
Attached patch pngread.patch (obsolete) — — Splinter Review
Here's the patch for pngread.c - it should apply on top on your latest patch.
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
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.
All of them 16-bit, with one exception: g04n3p04.png (4-bit)
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 on attachment 598667 [details] [diff] [review] pngread.patch Joe, are you OK with this in principle?
Attachment #598667 - Flags: feedback?(joe)
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
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 on attachment 598667 [details] [diff] [review] pngread.patch Sure.
Attachment #598667 - Flags: feedback?(joe) → feedback+
Picking up the png.h and pngread.c APNG changes.
Attachment #598659 - Attachment is obsolete: true
Attachment #598667 - Attachment is obsolete: true
Companion patch for the nsPNGDecoder.cpp change. Both patches sent to Try. https://tbpl.mozilla.org/?tree=Try&rev=f26113469378
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.
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
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()
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
(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.
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.
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.
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?
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
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.
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
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
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
Victory! \o/
Attachment #598700 - Flags: review?(joe)
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.
Attachment #598701 - Attachment is obsolete: true
Attachment #600687 - Flags: review?(joe)
Forgot to mention, it also removes the PNG_READ_STRIP_16_TO_8_SUPPORTED define from mozpngconf.h per comment 49.
All right, sounds great!
You should probably mark the previous patch as obsolete.
It's two separate patches. One's the upstream changes and one's the rest.
Attachment #598700 - Attachment description: v02 Upgrade bundled libpng to version 1.5.9 → v02 Upgrade bundled libpng to version 1.5.9 (upstream changes)
Comment on attachment 598700 [details] [diff] [review] v02 Upgrade bundled libpng to version 1.5.9 (upstream changes) rs=joe
Attachment #598700 - Flags: review?(joe) → review+
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
Attachment #600687 - Flags: review?(joe) → review+
Assignee: glennrp+bmo → ryanvm
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.
Tests live here if anyone wants to look at them: http://mxr.mozilla.org/mozilla-central/source/image/test/unit/
Identical, except for first 2 bytes of zstream... interesting.
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
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.
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.
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.
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
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
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
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.
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
Try this updated file.
Yes, that one worked. How did you generate it? I did my attempt with a canvas element and that clearly didn't work...
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?
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).
Attached image image1.png —
Try this.
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
To be clear, comment 80 was a push prior to comment 79.
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?
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.
Attachment #600687 - Attachment is obsolete: true
Attachment #602637 - Flags: review?(joe)
Attachment #602637 - Flags: feedback?(dolske)
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.
Attachment #602637 - Flags: feedback?(dolske) → feedback+
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.
Attachment #602637 - Flags: review?(joe) → review+
Looks great! Thanks everybody!
https://hg.mozilla.org/mozilla-central/rev/17b1c6ad13bd https://hg.mozilla.org/mozilla-central/rev/a59316d3580c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 733598
Blocks: 734317
Blocks: 745178
Whiteboard: [gfx.relnote.13]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: