208607 - Reconfigure libpng for smaller imglib2

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[tor]

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.

Comment 1

[tor]

Attachment: reconfiguration patch

Comment 2

[Glenn Randers-Pehrson]

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

Comment 3

[Glenn Randers-Pehrson]

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

Comment 4

[Glenn Randers-Pehrson]

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

DEFINES         += -DPNG_NO_MNG_FEATURES

Comment 5

[tor]

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.

Comment 6

[Glenn Randers-Pehrson]

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.

Comment 7

[tor]

Attachment: updated patch with Glenn's defines

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

Comment 8

[Glenn Randers-Pehrson]

    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.

Comment 9

[Glenn Randers-Pehrson]

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.

Comment 10

[Glenn Randers-Pehrson]

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

Comment 11

[tor]

Attachment: updated patch with write defines (checked in)

Comment 12

[tor]

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

Comment 13

[Jim Dunn]

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?  

Comment 14

[tor]

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

Comment 15

[Glenn Randers-Pehrson]

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 16

[Christopher Blizzard (:blizzard)]

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

sr=blizzard

Comment 17

[tor]

Checked in with #include shuffled into the #define.

Comment 18

[[e-mail]]

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.

Comment 19

[Glenn Randers-Pehrson]

Attachment: patch to remove libpng write support (checked in)

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

Comment 20

[Glenn Randers-Pehrson]

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.

Comment 21

[tor]

Checked in.

Comment 22

[Glenn Randers-Pehrson]

Attachment: Patch to remove more unused code from libpng

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

Comment 23

[Glenn Randers-Pehrson]

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.

Comment 24

[Glenn Randers-Pehrson]

Attachment: Updated patch to remove another 5k of unused libpng code

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

Comment 25

[Glenn Randers-Pehrson]

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

r?

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 27

[Glenn Randers-Pehrson]

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?)

Comment 28

[[e-mail]]

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 29

[Glenn Randers-Pehrson]

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.

Comment 30

[[e-mail]]

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 31

[Glenn Randers-Pehrson]

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?

Comment 32

[tor]

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.

Comment 33

[Glenn Randers-Pehrson]

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.

Comment 34

[Glenn Randers-Pehrson]

Attachment: Simpler patch to remove some unused libpng code (checked in)

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

Comment 35

[Bernard Alleysson]

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 36

[Glenn Randers-Pehrson]

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

Tor: r?

Comment 37

[tor]

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