From bug 204520: libpng has a lot of functionality that we don't need for simple image display within mozilla. I believe that setting the following options won't lose us any functionality. PNG people, please correct me if I'm wrong. If there are more options we could set I'd love to hear that as well. #define PNG_NO_READ_BACKGROUND #define PNG_NO_READ_UNKNOWN_CHUNKS #define PNG_NO_READ_DITHER #define PNG_NO_READ_SHIFT #define PNG_NO_READ_USER_TRANSFORM #define PNG_NO_READ_iCCP #define PNG_NO_READ_TEXT #define PNG_NO_READ_bKGD #define PNG_NO_READ_pPYs #define PNG_NO_READ_cHRM #define PNG_NO_READ_sBIT #define PNG_NO_READ_sPLT #define PNG_NO_READ_hIST #define PNG_NO_READ_tIME #define PNG_NO_READ_pCAL #define PNG_NO_READ_sCAL These values shrink the text segment of imglib2 by about 26k on linux. Unfortunately the DOM inspector needs png write ability, so we can't shave a further 9k of by setting PNG_NO_WRITE_SUPPORTED.

You don't need to modify libpng at all. You can leave it in pristine condition and instead append the following to png/Makefile and png/Makefile.in: # Remove unused libpng features DEFINES += -DPNG_NO_INFO_IMAGE DEFINES += -DPNG_NO_READ_BACKGROUND DEFINES += -DPNG_NO_READ_DITHER DEFINES += -DPNG_NO_READ_INVERT DEFINES += -DPNG_NO_READ_SHIFT DEFINES += -DPNG_NO_READ_PACK DEFINES += -DPNG_NO_READ_PACKSWAP DEFINES += -DPNG_NO_READ_FILLER DEFINES += -DPNG_NO_READ_SWAP_ALPHA DEFINES += -DPNG_NO_READ_INVERT_ALPHA DEFINES += -DPNG_NO_READ_STRIP_ALPHA DEFINES += -DPNG_NO_READ_RGB_TO_GRAY DEFINES += -DPNG_NO_READ_USER_TRANSFORM DEFINES += -DPNG_NO_READ_bKGD DEFINES += -DPNG_NO_READ_cHRM DEFINES += -DPNG_NO_READ_hIST DEFINES += -DPNG_NO_READ_iCCP DEFINES += -DPNG_NO_READ_pCAL DEFINES += -DPNG_NO_READ_pHYs DEFINES += -DPNG_NO_READ_sBIT DEFINES += -DPNG_NO_READ_sCAL DEFINES += -DPNG_NO_READ_sPLT DEFINES += -DPNG_NO_READ_TEXT DEFINES += -DPNG_NO_READ_tIME DEFINES += -DPNG_NO_READ_oFFS DEFINES += -DPNG_NO_READ_UNKNOWN_CHUNKS DEFINES += -DPNG_NO_READ_USER_CHUNKS DEFINES += -DPNG_NO_USER_MEM DEFINES += -DPNG_NO_READ_EMPTY_PLTE DEFINES += -DPNG_NO_FIXED_POINT_SUPPORTED DEFINES += -DPNG_NO_READ_OPT_PLTE If you prefer you can make it one directive with backslashes as was done in but #204520 but I don't see the advantage. Glenn

There'e a typo in comment 1 and in the attachment: pPYs should be pHYs.

Oops, I missed one (we don't need this because we don't use libpng to handle MNG): DEFINES += -DPNG_NO_MNG_FEATURES

The problem with doing it in the makefile is that there will need to be a copy of them in the libpr0n/decoders/png makefile so that the structure layouts match. I'd prefer to avoid potential maintenance problems of keeping them synced by doing a one line change to the mozilla libpng tree. Thanks for the list of defines - I'll try those out.

OK. You'd just need to remember to patch pngconf.h each time you upgrade libpng. No big deal. I'll take the same approach for MNG reduction.

Using Glenn's defines the savings is now roughly 35k in the linux text segment.

There are also some small savings in the amount of memory malloc'ed for the png structures, small savings in execution time, and maybe more importantly, reduced vulnerability to potential problems with malformed ancillary chunks, when using the embedded libpng.

Re: comment #1 "DOM inspector needs png write ability" I just took a look at inspector's PNG code. It appears to leak because there's no png_destroy_write_struct() to balance the png_create_write-struct(). I'll post a new bug about that. At any rate, it uses very little of libpng's capability so we could squeeze out a few more kbytes.

These will remove libpng features not used by inspector. #define PNG_NO_WRITE_TRANSFORMS #define PNG_NO_WRITE_EMPTY_PLTE #define PNG_NO_WRITE_ANCILLARY_CHUNKS

Thanks. With the latest version of the patch we're up to about 43k savings in the text segment and almost 1k in data (linux). Before: text data bss dec hex filename 213447 5484 16 218947 35743 libimglib2.so After: text data bss dec hex filename 170414 4628 16 175058 2abd2 libimglib2.so

Quick question... are the before and after size info's done on the stripped lib? If now can you post the before & after on the stripped libs as well?

The sizes are from size(1) and don't include the tables that strip removes.

A nitpick about patch #125194: Maybe "#include mozpngconf.h" should be moved down a few lines in pngconf.h, to appear just after "#define PNGCONF_H".

Comment on attachment 125194 [details] [diff] [review] updated patch with write defines (checked in) sr=blizzard

Checked in with #include shuffled into the #define.

tor: we may end up removing PNG write functionality from inspector... see bug 210096. I'll cc you there, so you can follow it up with a PNG_NO_WRITE_SUPPORTED patch if you'd like.

PNG write support is no longer needed by the DOM inspector.

The patch to remove PNG write support in the DOM inspector has been checked in (Bug #210096) so we can proceed with removing write support from libpng, saving several kb.

Checked in.

Here's an opportunity to remove a couple more kilobytes of unused code from libpng, using conditionals already present in libpng-1.2.5: #define PNG_NO_READ_STRIP_ALPHA #define PNG_NO_USER_TRANSFORM_PTR #define PNG_NO_READ_oFFs #define PNG_NO_HANDLE_AS_UNKNOWN #define PNG_NO_CONSOLE_IO #define PNG_NO_ZALLOC_ZERO #define PNG_NO_ERROR_NUMBERS

Reopened because there's an opportunity to remove more unused code. I have also found a way to save about 3 more kb by optionally removing the text of error and warning messages, which mozilla doesn't use. That change is a little more extensive (but fairly simple) and involves changes to many libpng source files. If there's interest (i.e., votes), I'll submit a patch.

Updated patch #128350 to remove sequential read capability from libpng. Mozilla only uses progressive reading. This saves about 5 more kbytes.

Comment on attachment 128475 [details] [diff] [review] Updated patch to remove another 5k of unused libpng code r?

You'd have better luck getting reviews if you requested them from people.

Setting dependency on bug #181936. When the first patch was checked in under this bug, libpng was no longer "pristine" and some members were removed from the png_struct and info_struct, which makes the embedded library binary incompatible with the system library. Therefore, we should either fix bug #181936 with respect to libpng (possibly using the patch that I supplied over there), or we should revert the changes made here and accept the larger library size. CAillon: I thought "r?" *was* a request for review by people in general and implicitly a request for review by tor who is (or was) the bug owner or by pavlov who is the module owner. Tor or Pavlov: r? (r of bug #181936 too?)

glenn, you should use the request tracker to request review from the relevant person. denoting "r?" is insufficient. click 'edit' on your attachment, select "review ?" as you have done, and then type the bugzilla email address of the person you're requesting from. in this case, requesting review from "pavlov" and superreview from "tor@acm" is appropriate. (bugzilla can autocomplete partial entries for you.)

Comment on attachment 128475 [details] [diff] [review] Updated patch to remove another 5k of unused libpng code r? tor; sr? pavlov Also let me know if you want the patch to optionally remove error message text, either as a separate patch or addendum to this one.

pavlov isn't an sr. you need to switch the requests to the order i suggested. request review from pavlov, sr from tor. you should read the sections "code review" and "superreview" at http://www.mozilla.org/hacking/.

Comment on attachment 128475 [details] [diff] [review] Updated patch to remove another 5k of unused libpng code Whatever. I thought Stuart owned libimg and therefore was higher on the pecking order. pav: r? tor: sr?

I'm somewhat hesitant to take the new #ifdefs in libpng proper, as we try to keep this library as close to the official version as possible.

Actually I tend to agree and in fact wouldn't object to reverting the previous patches in this bug (at a cost of 40-50kbytes). See bug #181936 for good reasons to use absolutely pristine 3rd party libraries. However, libpng is already non-pristine and the patch isn't that extensive, with a handful of #ifdefs to skip about 5k of unused code. I intend to put them in libpng version 1.2.6 anyway, so mozilla could just wait for that to happen, and simply put the one-liner in mozpngconf.h to enable the deletions (that one-liner, PNG_NO_SEQUENTIAL_READ_SUPPORTED, could go in now but wouldn't have any effect until libpng is updated). If you like, I can provide the patch that only updates mozpngconf.h (identical to that portion of the current patch) which would produce some footprint savings now and some more later.

Simpler patch that doesn't mess with libpng internals. Removes about 3k of unused code.

Comment on attachment 136338 [details] [diff] [review] Simpler patch to remove some unused libpng code (checked in) I've applied this patch: no problem so far. Could it be considered for reviews ?

Comment on attachment 136338 [details] [diff] [review] Simpler patch to remove some unused libpng code (checked in) Tor: r?

Checked in. Checking in MOZCHANGES; /cvsroot/mozilla/modules/libimg/png/MOZCHANGES,v <-- MOZCHANGES new revision: 3.11; previous revision: 3.10 done Checking in mozpngconf.h; /cvsroot/mozilla/modules/libimg/png/mozpngconf.h,v <-- mozpngconf.h new revision: 3.4; previous revision: 3.3 done