66814 - Need function to convert nsIImage to native Mac Icon format

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Andrew Thompson]

Currently icons in chrome are in XP Formats (e.g. JPEG, PNG, GIF). There are 
occassions where we need to be able to convert these to Mac native icon formats 
(cicn, icon, ic18 etc) for use in the UI. (e.g. proxy icons, bookmarks menu)

I've been doing some work trying to beat some sample Apple code which does this 
into shape... I have it mostly integrated, but am finding it very difficult to 
see how to test it.

Also the usual caveats re: where I've inserted it apply. Its currently a patch to 
nsImageMac, but its as likely we'd want to seperate it into a friend class or 
something similar.

This is very very rough. Stuff is commented out and all sorts of nastiness, error 
reporting isn't finished. etc. etc.

Im posting it because I'm going to be away for a few weeks and Pinkerton's last 
status report mentioned he was going to be looking at stuff similar to this soon.

This patch contains code derived from sample Apple code.

Comment 1

[Andrew Thompson]

Attachment: Rough Rough Rough patch which is supposed to achieve conversion

Comment 2

[Mike Pinkerton (not reading bugmail)]

if we are going to put this on nsImageMac, it should be on the nsIImageMac
interface I just created last week. Seems perfect for this type of thing.

cc'ing sfraser who would be interested in this as well, and saari as he's
working on imagelib stuff. Who owns this area of code? It's not me, though I'll
take the bug if no one wants it.

why is the out param of MakeIcon a char**? Why isn't it a Handle?

Comment 3

[Andrew Thompson]

'Cause Handle is a Mac type, (hence also nsrect not Rect) and I wasn't sure what 
the NSPR equivalent is.

Guess it doesn't matter so much/at all for this code...

Comment 4

[saari (gone)]

Yeah, just put it on nsIImageMac interface like Pink said and I'm fine with it.
I don't really own this, but I don't have a better suggestion than Pinkerton or
Simon.

Comment 5

[Simon Fraser [no longer active]]

Good ideas here. Should be fairly easy to go from nsIImage -> GWorld -> PixMap -> 
Icon. (How's that for a worthless comment).

Comment 6

[Andrew Thompson]

Attachment: Working version, will make nsImageMac->Mac Icon conversion

Comment 7

[Andrew Thompson]

OK, this version works pretty good. I've done some testing at 16x16 and 32x32
sizes for 1 4 and 8 bits icons. Could use some 32 bit testing I guess.

This version is OK, what it doesn't do is icon masks. We could check this in now
if people don't care about masks. Review before the amount of code gets out of
hand would be nice too.

This does fix a related bug in ConvertToPICT - well I feel its a bug, I need to
talk to pinkerton about it. Curently ConvertToPict doesn't set up 
mImagePixmap.baseAddr befoe creating the PICT. I fixed this to work like
nsImageMac::Draw() and it works properly for me, without this fix ConvertToPICT
is extremely flaky. Mike, what do you say? Good call or not?

Oh, also I just realised this patch doesn't include nsImageMac.h or
nsIImageMac.h. Um, not going to spam people with that tonight. I'll fix once I
get some feedback.

Comment 8

[Andrew Thompson]

Attachment: Includes headers, also has cleaned up whitespace and {}. Timeless now owes me a drink ;)

Comment 9

[Andrew Thompson]

Attachment: Patch implements Icon and Mask conversion, all sizes all depths

Comment 10

[Andrew Thompson]

Some comments on the changes to existing functions.

The change to ::Init is to remove what we believe was the unintended hiding of 
the instance variable mAlphaDepth by a local variable. This is smfr's code, I 
spoke to him and he figures its unintentional Both my ConvertToIcon and Pink's 
ConvertToPict use mAlphaDepth.

The change to ::CalculateRowBytes makes it return the right value for images of 
less than 24 bytes. This covers the small 1 bit masks for the 16x16 icons that my 
mask creation code needs to create. I left the behaviour for larger images 
unchaged, though the documentation referenced in this function's header comment 
does say that this value isn't even really required to be even on modern 
processors, much less a multiple of 4. I can see that 4 could be more efficient.

Comment 11

[Simon Fraser [no longer active]]

Comments on the patch:

-      ioPixMap.pmTable = aColorTable ? aColorTable : GetCTable(32 + 1);                
// default to black & white colortable
+      // default to black & white colortable
+      ioPixMap.pmTable = aColorTable ? aColorTable : GetCTable(32 + 1);                

Try to avoid gratuitous source reformatting, and moving comments around.

+NS_IMETHODIMP
+nsImageMac::ConvertToIcon(  const nsRect& aSrcRegion, 
+                            const PRInt16 aIconDepth, 
+                            const PRInt16 aIconSize,
+                            Handle* aOutIcon) {

In this method, null out the return value at the top, so you don't
have to do it on each error condition.

+        HNoPurge(dstHandle);

Why? No-one has made the handle purgable, and you allocated it, so
you know this.


+            //1 bit masks are tricker, we must create an '#' resource 
+            //which inclues both a1 1-bit icon and a mask (icm#, ics#, ICN# or 
ich#)
+            Handle iconHandle, maskHandle;

You need to init both these to nsnull, so you don't try to dispose a
bad handle in the error condition.

+            //make 1 bit icon and mask as above
+            Handle iconHandle, maskHandle;

Same.

+        HNoPurge(dstHandle);

Again, unnecessary.


+    destPixmap = (PixMapHandle)NewHandle(sizeof(PixMap));    
+    err = CreatePixMap( aDestRegion.right - aDestRegion.left,
+                        aDestRegion.bottom - aDestRegion.top,
+                        aDestDepth, 
+                        destColorTable, 
+                        **destPixmap, 
+                        *aDestData);   

Here you need to lock the destPixmap handle, since CreatePixMap may move
memory. Or why not just have a stack-based PixMap?

+nsresult 
+nsImageMac::ConcatBitsHandles( Handle aSrcData1, 
+                               Handle aSrcData2,
+                               Handle *aDstData)

Can't you just use HandToHand() and HandAndHand() in here? Or are you
using AllocateBitsHandle() to get the benefits of spilling into
temp mem if necessary?

+        StHandleLocker dstLocker(*aDstData);                    
+        ::BlockMove(*aSrcData1, (Ptr)((long)**aDstData), src1Size);
+        ::BlockMove(*aSrcData2, (Ptr)(((long)**aDstData) + src1Size), src2Size);
+        err = MemError();

You should use BlockMoveData, not BlockMove, since the latter flushes the
processors caches.

Note that you don't have to lock handles before calling BlockMove, since
BlockMove is guaranteed not to move memory. You could also write this more
simply as:

        ::BlockMoveData(*aSrcData1, **aDstData, src1Size);
        ::BlockMoveData(*aSrcData2, **aDstData + src1Size, src2Size);

Having a the destData be a local Handle would also make the code
more readable:

nsresult 
nsImageMac::ConcatBitsHandles( Handle aSrcData1, 
                               Handle aSrcData2,
                               Handle *aDstData)
{
    PRInt32 src1Size = ::GetHandleSize(aSrcData1);
    PRInt32 src2Size = ::GetHandleSize(aSrcData2);

    *aDstData = nsnull;
    
    Handle    destHandle = nil;
    
    OSErr   err = AllocateBitsHandle(src1Size + src2Size, &destHandle);
    if (err != noErr) return NS_ERROR_OUT_OF_MEMORY;
    
    ::BlockMoveData(*aSrcData1, *destHandle, src1Size);
    ::BlockMoveData(*aSrcData2, *destHandle + src1Size, src2Size);

    *aDstData = destHandle;
    return noErr;
        
} // ConcatBitsHandles

Comment 12

[Andrew Thompson]

Sorry, didn't mean to accept a bug onto you Mike. Meant to take it myself

Comment 13

[Andrew Thompson]

Attachment: Patch addressing Simon's comments

Comment 14

[Andrew Thompson]

Patch should address all of Simon's comments. A lot of the stuff like setting no 
purge is copied from other example code. Ignorance is the most dangerous thing of 
all in coding. Thanks for a most effective review.

As for the HandToHand thing - yeah, I did write this method to get the overspill 
into temp memory. Strikes me as doing no harm.... and thinking about it now, a 
128x128 32 bit Mac OS X icon is 65K, so its possible we could hit the app heap 
limit.

Comment 15

[Andrew Thompson]

Attachment: Includes nsRect.h in nsIImageMac... missed this earlier

Comment 16

[Andrew Thompson]

DOINK!

This patch has been sat here since February... any chance of an r= ?

Comment 17

[Andrew Thompson]

Attachment: Icon creation functions now return the correct icon type ('icl8' etc) to decribe the icon made

Comment 18

[Andrew Thompson]

<cough> there's been a working patch here since jan 18th...

Comment 19

[Simon Fraser [no longer active]]

Comments on the patch in attachment 41379 [details] [diff] [review]:

 #include <quickdraw.h>
+#include "nsRect.h"

Please move your include up, so it's adjacent to the nsISupports include. Please 
also fix the case in quickdraw.h -> QuickDraw.h

Why is mozilla/gfx/src/mac/nsIImageMac.h in the patch twice?

+    // if bits per row is 24 or less, needs 3 bytes or less

Can you point me to documentation to justify this change?

+    *aOutIcon = nsnull;
+    *aOutIconType = nsnull;

Maybe check for null params first -- NS_ENSURE_ARG_POINTER

+    OSType iconType = MakeIconType(aIconSize, aIconDepth, false);
+    if (iconType == nsnull) {

nsnull should only be used for pointer comparison. (iconType == 0) is fine.


+    result = CopyPixMap(    srcRect, iconRect, aIconDepth, 
+                            PR_FALSE, &dstHandle); 
+                           
+    if(NS_SUCCEEDED(result)) *aOutIcon = dstHandle;
+    return result;        

Why not just

  return CopyPixMap(....aOutIcon);   ?

+    Handle          dstHandle;

Probably a good idea to init this to nsnull, since it isn't totally obvious that 
the code below will do the right thing. (Same goes for previous function.)

+    *aOutMask = nsnull;
+    *aOutIconType = nsnull;

Check args again?

In nsImageMac::CopyPixMap, maybe check for null aDestData arg.

+        if (!mMaskBitsHandle) {
+            aDestData = nsnull;
+            return NS_ERROR_INVALID_ARG;

Should be * aDestData = nsnull.

+        if (!mImageBitsHandle) {
+            aDestData = nsnull;
+            return NS_ERROR_INVALID_ARG;

ditto

+    if(err) {
+        aDestData = nsnull;
+        return NS_ERROR_FAILURE;

ditto

+    if(err) {
+        aDestData = nsnull;
+        return NS_ERROR_FAILURE;
+    } else {
+        // lock and set up bits handles

No need for the 'else'. You've just returned. This also fixes the awkward issue 
that the last line of this method is not a return.

+        //Clean up PixMap - sufficient?
+        if (destPixmap.pmTable) ::DisposeCTable(destPixmap.pmTable);
+        return (err == noErr) ? NS_OK : NS_ERROR_FAILURE;    

So if you return an error here, shouldn't you DisposeHandle(*aDestData) ?

In general, I dislike the way returned handles are managed. I much prefer to keep 
the handle to be returned in a local variable until the very end. Then you only 
fill in the return value (which you set to nsnull initially) when you know you 
will return success.

ConcatBitsHandles()

This function could be done with a HandToHand() followed by a HandAndHand().


+    err = AllocateBitsHandle(size, aMask);
+    if (err) {
+        return NS_ERROR_OUT_OF_MEMORY;
+    } else {
+        StHandleLocker dstLocker(*aMask);
+        ::memset(**aMask, 0xFF, size);
+        return NS_OK;
+    }

Ugh. Try

   err = AllocateBitsHandle(size, aMask);
   if (err != noErr)
       return NS_ERROR_OUT_OF_MEMORY;

   StHandleLocker dstLocker(*aMask);
   ::memset(**aMask, 0xFF, size);
   return NS_OK;

Also, the locking isn't strictly necessary; you can be pretty sure that memset 
doesn't move memory. See comments above about returned handle management.

Comment 20

[Andrew Thompson]

Cool! I'll get on the rest of the comments later, but on this subject

+    // if bits per row is 24 or less, needs 3 bytes or less

Can you point me to documentation to justify this change?


The code in question is:

-  PRInt32   rowBytes = ((aWidth * aDepth + 31) / 32) * 4;
+    PRInt32 rowBits = aWidth * aDepth;
+    PRInt32 rowBytes;
+    // if bits per row is 24 or less, needs 3 bytes or less
+    if (rowBits > 24)   
+        rowBytes = ((aWidth * aDepth + 31) / 32) * 4;
+    else
+        rowBytes = ((aWidth * aDepth + 15) / 16) * 2;


Well, firstly, this works and the old code produces garbage... why do I know that?

The case where this commonly occurs is 16x16 icon masks (which are 1 bit,
naturally). Simon, you quote: http://developer.apple.com/technotes/qd/qd_15.html
as saying it has to be a muliple of 4, but I'm not sure it really does...

For small icon masks we get:

(((16 * 1) + 31) / 32 ) * 4 = 4 bytes wide by the old method

(((16 * 1) + 15) / 16) * 2 = 2 bytes wide by the new one

It seems for small images Mac OS makes the saving and only uses the 2 bytes
actually required. This is what I discovered from experimentation - without this
adjustment for very small pixmaps the data gets mangled, with it it comes out
fine. The closest thing I can find to documentation comes from the above
mentioned QD15"

"Even or odd rowBytes?

Since the dawn of Macintosh, it has been said that rowBytes should be even
because each row of a pixMap must contain an integral number of words. Actually,
rowBytes has to be even because QuickDraw accesses bitmap data using word or
long operands, and these generate address errors when it references an odd
address on the 68000, which would happen if rowBytes is odd. The 68020 and later
handle odd addresses fine, and so rowBytes can be odd. But, it is still
recommended that rowBytes be even, because misaligned accesses incur a
performance penalty."

You'll note it says even or odd, not multiples of 4. Further it says "QuickDraw
accesses bitmap data using word *or* long operands" so I'm guessing for very
small pixmaps it'll use 16 bit word operations and 32 bit long operations for
large ones. I picked 24 because its the bondary case: 25 1 bit pixels require at
least 4 bytes to store and so the previous formula works fine.

Comment 21

[Andrew Thompson]

Attachment: New version which I hope addresses Simon's comments

Comment 22

[Andrew Thompson]

The other comments:

>ConcatBitsHandles()
>This function could be done with a HandToHand() followed by a HandAndHand().

Yeah, but I want the fallback to temp memory behaviour. If this code is 
allocating a 128x128 32bit/8bit mask aqua icon that's getting to be 80 kilobytes 
its trying to get from the app heap (then again, I guess its moot on Mac OS X)

>Also, the locking isn't strictly necessary; you can be pretty sure that memset 
>doesn't move memory. 

I just realised I forgot to take this out (or indeed decide to leave it in). 
Guess it doesn't matter though...

Comment 23

[Steve Dagley]

It's been pointed out to me that we've once again not gotten lordpixel's patch
into Mozilla.  Knowing sfraser is buried in the Mac NSPR guts right now I've
cc:'ed scc in hopes he can sr= this and maybe saari and/or beard can give it an
r= (being as they're the most Mac graphic saavy folks I know) and we could get
this into 0.9.5

Comment 24

[Andrew Thompson]

That would be nice :) Anyone looking for any further input from me?

Comment 25

[saari (gone)]

code looks okay at first glance. Definitely make sure this hasn't bit rotted and
do a Carbon build to make sure we don't break anything with opaque structures, etc.

Comment 26

[Andrew Thompson]

Attachment: Revise patch to combat bitrot

Comment 27

[Andrew Thompson]

Saari - this patch compiles under Carbon w/ CodeWarrior 7 (should work in 5 too).

Have fought back bit rot :)

Will test ASAP.

The only structs it uses directly are Pixmaps and Rects, which remain regular
structs in Carbon, as far as I can tell.

Comment 28

[Andrew Thompson]

Comment on attachment 54620 [details] [diff] [review]
Revise patch to combat bitrot

Grrr. Newlines. Grrr

Comment 29

[Andrew Thompson]

Attachment: Anyone know how to prevent codewarrior for saving files which don't end in a newline?

Comment 30

[Andrew Thompson]

OK, tested by porting menu icon code to Mac OS X. Seems to work fine, at least 
for the cases that exercises, which are fairly typical.

Comment 31

[Andrew Thompson]

Eye candy: http://pixel.recoil.org/BookMarksMenuX.gif

Comment 32

[Asa Dotzler [:asa]]

saaari, pink, smfr, beard, can we  get a review on this so it  doesn't rot  again? 

Comment 33

[Simon Fraser [no longer active]]

r/sr=sfraser

Comment 34

[Mike Pinkerton (not reading bugmail)]

r=pink

Comment 35

[Andrew Thompson]

First check in. yipee!

Thanks to simon, pink, saari and asa ;)