57327 - nsImageMac should use PixMaps, not GWorlds, to reduce memory use

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Simon Fraser [no longer active]]

Spinning off from bug 20743...

We're currently using GWorlds for all image bits, which involves significant 
overhead (4-6K per GWorld), and lots (29) of small handles that go into the 
application heap, even when allocating image bits in temp mem.

I think we need to go back to hand-rolling our own PixMaps here, as the code used 
to do (back up to an older version to get the code back).

Comment 1

[Simon Fraser [no longer active]]

Target for mozilla 0.9

Comment 2

[Simon Fraser [no longer active]]

Attachment: Changes to nsImage to use PixMaps, not GWorlds

Comment 3

[Simon Fraser [no longer active]]

cc some strong mac hackers

Comment 4

[Simon Fraser [no longer active]]

Removing donttest keyword. Actually, this does need testing. Things to test:
1. Loading of large images
2. Stress-test image loading under low memory conditions.
3. Printing of images (including transparent GIFs and scaled images).

Comment 5

[Simon Fraser [no longer active]]

Attachment: New patch

Comment 6

[Simon Fraser [no longer active]]

The new patch fixes the following:
1. dtor now correctly disposes the two handles
2. Mask off the top 2 bits of the rowBytes instead of just the first
3. when printing, use the correct destination pixmap.

Comment 7

[Patrick C. Beard]

Is it better to pass the address of a PixMapHandle or a dereferenced PixMap?

The old code passed a pointer to a PixMap, rather than the address of a

PixMapHandle. Clearly, for compatibility with pre-color Quickdraw, one could

always safely treat a CGrafPort as a GrafPort, and you could pass

&port->portBits, which would end up being the address of a PixMapHandle.

Anyway, you are using:



::CopyBits((BitMap*)&mImagePixmap, (BitMap*)*destPixels, &srcRect, &dstRect, 

srcCopy, nsnull);



I would rather see all of the (BitMap*) parameters be dereferenced PixMapHandles,

which of course should probably be locked. Other than this nit, sr=beard.

Comment 8

[Simon Fraser [no longer active]]

CopyBits determines what is being passed in by examining the top two bits of the 
rowBytes field (as described in <http://developer.apple.com/technotes/qd/
qd_15.html>). The top bit is used to distinguish between a bitmap and a pixmap, 
and we always set this in CreatePixMap(). The next bit is used by CopyBits to 
tell if the pixMap is part of a CGrafPtr or not. This bit is set for the pixMap 
of a CGrafPtr, in which case CopyBits knows that it's being passed a 
PixMapHandle, and will dereference it. We are *not* setting this bit. So, since 
we only set the top bit, it is safe to pass the address of a PixMap structure to 
CopyBits.

Comment 9

[Simon Fraser [no longer active]]

Fix checked in. To verify this bug, please test the following:
1. Image rendering with various screen depths.
2. Drawing and printing of transparent GIFs
3. Drawing and printing of PNGs with alpha
4. Stress-test image display under low memory conditions

Comment 10

[Chris Petersen]

Tested in 16 bit and 32 bit depths (1152 X 870 / 1280 X 1024). Opened multiple
JPG's ,PNG's and GIF's at various different sizes. Printed out several
transparent GIF's and alpha channel PNG files. Verified fixed in the Mac
2001010408 build.