Reconfigure libpng for smaller imglib2

VERIFIED FIXED

Status

()

Core
ImageLib
VERIFIED FIXED
14 years ago
3 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

({footprint})

Trunk
footprint
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 125131 [details] [diff] [review]
reconfiguration patch

Comment 2

14 years ago
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

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

Comment 4

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

DEFINES         += -DPNG_NO_MNG_FEATURES
(Assignee)

Comment 5

14 years ago
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

14 years ago
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.
(Assignee)

Comment 7

14 years ago
Created attachment 125133 [details] [diff] [review]
updated patch with Glenn's defines

Using Glenn's defines the savings is now roughly 35k in the linux
text segment.
Attachment #125131 - Attachment is obsolete: true

Comment 8

14 years ago
    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

14 years ago
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
(Assignee)

Comment 11

14 years ago
Created attachment 125194 [details] [diff] [review]
updated patch with write defines (checked in)
Attachment #125133 - Attachment is obsolete: true
(Assignee)

Comment 12

14 years ago
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

14 years ago
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?  
(Assignee)

Comment 14

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

Updated

14 years ago
Attachment #125194 - Flags: superreview?(blizzard)
Attachment #125194 - Flags: review?(pavlov)
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".

Updated

14 years ago
Attachment #125194 - Flags: review?(pavlov) → review+
Comment on attachment 125194 [details] [diff] [review]
updated patch with write defines (checked in)

sr=blizzard
Attachment #125194 - Flags: superreview?(blizzard) → superreview+
(Assignee)

Comment 17

14 years ago
Checked in with #include shuffled into the #define.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 18

14 years ago
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.
Created attachment 126352 [details] [diff] [review]
patch to remove libpng write support (checked in)

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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

14 years ago
Attachment #126352 - Flags: superreview+
(Assignee)

Updated

14 years ago
Attachment #126352 - Flags: review?(pavlov)

Updated

14 years ago
Attachment #126352 - Flags: review?(pavlov) → review+
(Assignee)

Comment 21

14 years ago
Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
Created attachment 128350 [details] [diff] [review]
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
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 128475 [details] [diff] [review]
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.
Attachment #128350 - Attachment is obsolete: true
Comment on attachment 128475 [details] [diff] [review]
Updated patch to remove another 5k of unused libpng code

r?
Attachment #128475 - Flags: review?
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?)
Depends on: 181936

Comment 28

14 years ago
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.
Attachment #128475 - Flags: superreview?(pavlov)
Attachment #128475 - Flags: review?(tor)
Attachment #128475 - Flags: review?

Comment 30

14 years ago
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?
Attachment #128475 - Flags: superreview?(tor)
Attachment #128475 - Flags: superreview?(pavlov)
Attachment #128475 - Flags: review?(tor)
Attachment #128475 - Flags: review?(pavlov)
(Assignee)

Comment 32

14 years ago
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.

Updated

14 years ago
Blocks: 171082
Created attachment 136338 [details] [diff] [review]
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.

Updated

14 years ago
Attachment #128475 - Attachment is obsolete: true

Updated

14 years ago
Attachment #128475 - Flags: superreview?(tor)
Attachment #128475 - Flags: review?(pavlov)

Updated

13 years ago
Keywords: footprint

Comment 35

13 years ago
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?
Attachment #136338 - Flags: review?(tor)
(Assignee)

Updated

13 years ago
Attachment #136338 - Flags: superreview+
Attachment #136338 - Flags: review?(tor)
Attachment #136338 - Flags: review+
(Assignee)

Comment 37

13 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago13 years ago
Resolution: --- → FIXED

Updated

12 years ago
Status: RESOLVED → VERIFIED

Updated

10 years ago
Attachment #136338 - Attachment description: Simpler patch to remove some unused libpng code → Simpler patch to remove some unused libpng code (checked in)

Updated

10 years ago
Attachment #126352 - Attachment description: patch to remove libpng write support → patch to remove libpng write support (checked in)

Updated

10 years ago
Attachment #125194 - Attachment description: updated patch with write defines → updated patch with write defines (checked in)
You need to log in before you can comment on or make changes to this bug.