Need function to convert nsIImage to native Mac Icon format

RESOLVED FIXED

Status

()

Core
ImageLib
--
enhancement
RESOLVED FIXED
18 years ago
17 years ago

People

(Reporter: Andrew Thompson, Assigned: Andrew Thompson)

Tracking

(Blocks: 2 bugs)

Trunk
PowerPC
Mac System 8.5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Please review patch 41379)

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

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

Updated

18 years ago
Blocks: 46177
(Assignee)

Comment 1

18 years ago
Created attachment 23649 [details] [diff] [review]
Rough Rough Rough patch which is supposed to achieve conversion
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?
(Assignee)

Comment 3

18 years ago
'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...

Updated

18 years ago
Blocks: 22456, 36305

Comment 4

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

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

Comment 6

17 years ago
Created attachment 25001 [details] [diff] [review]
Working version, will make nsImageMac->Mac Icon conversion
(Assignee)

Comment 7

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

Comment 8

17 years ago
Created attachment 25021 [details] [diff] [review]
Includes headers, also has cleaned up whitespace and {}. Timeless now owes me a drink ;)
(Assignee)

Comment 9

17 years ago
Created attachment 25417 [details] [diff] [review]
Patch implements Icon and Mask conversion, all sizes all depths
(Assignee)

Comment 10

17 years ago
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.
Status: NEW → ASSIGNED
Keywords: patch, review
Whiteboard: Please review patch 25417

Comment 11

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

Comment 12

17 years ago
Sorry, didn't mean to accept a bug onto you Mike. Meant to take it myself
Assignee: pinkerton → lordpixel
Status: ASSIGNED → NEW
(Assignee)

Comment 13

17 years ago
Created attachment 25505 [details] [diff] [review]
Patch addressing Simon's comments
(Assignee)

Comment 14

17 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: Please review patch 25417 → Please review patch 25505
(Assignee)

Comment 15

17 years ago
Created attachment 25580 [details] [diff] [review]
Includes nsRect.h in nsIImageMac... missed this earlier
(Assignee)

Updated

17 years ago
Whiteboard: Please review patch 25505 → Please review patch 25580
(Assignee)

Updated

17 years ago
Blocks: 71656
(Assignee)

Comment 16

17 years ago
DOINK!

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

Comment 17

17 years ago
Created attachment 41379 [details] [diff] [review]
Icon creation functions now return the correct icon type ('icl8' etc) to decribe the icon made
(Assignee)

Comment 18

17 years ago
<cough> there's been a working patch here since jan 18th...
Whiteboard: Please review patch 25580 → Please review patch 41379

Comment 19

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

Comment 20

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


(Assignee)

Comment 21

17 years ago
Created attachment 41746 [details] [diff] [review]
New version which I hope addresses Simon's comments
(Assignee)

Comment 22

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

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

Updated

17 years ago
Blocks: 80138
(Assignee)

Comment 24

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

Comment 25

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

Updated

17 years ago
Attachment #41379 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #25580 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #25505 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #23649 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #25001 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #25021 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #25417 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #41746 - Attachment is obsolete: true
(Assignee)

Comment 26

17 years ago
Created attachment 54620 [details] [diff] [review]
Revise patch to combat bitrot
(Assignee)

Comment 27

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

Comment 28

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

Grrr. Newlines. Grrr
Attachment #54620 - Attachment is obsolete: true
(Assignee)

Comment 29

17 years ago
Created attachment 54621 [details] [diff] [review]
Anyone know how to prevent codewarrior for saving files which don't end in a newline?
(Assignee)

Comment 30

17 years ago
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 32

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

Comment 33

17 years ago
r/sr=sfraser
(Assignee)

Comment 35

17 years ago
First check in. yipee!

Thanks to simon, pink, saari and asa ;)
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.