Josh Aas Assignee
	Description • 18 years ago

We should rewrite the Mac OS X icon decoder in Cocoa because the API is uses now is a mess and the icons don't look very good.

	Josh Aas Assignee
	Comment 2 • 18 years ago

For reviewers, the thing to pay attention to in this patch is nsIconChannel::MakeInputStream

The icons look a lot better now because they use proper 8-bit alpha. Once this is done we can turn on icons in the download manager.

	Stefan [:stefanh]
	Comment 3 • 18 years ago

I'm using gcc 3.3 (10.3.9) and it seems that the patch won't work with gcc 3.3:

/Users/Stefan/mozilla-trunk/mozilla/modules/libpr0n/decoders/icon/mac/nsIconChannel.mm: In
   member function `nsresult nsIconChannel::MakeInputStream(nsIInputStream**, 
   int)':
/Users/Stefan/mozilla-trunk/mozilla/modules/libpr0n/decoders/icon/mac/nsIconChannel.mm:260: error: non-lvalue
   in unary `&'
make[3]: *** [nsIconChannel.o] Error 1
make[2]: *** [libs] Error 2
make[1]: *** [libs] Error 2
make: *** [all] Error 2

	Josh Aas Assignee
	Comment 4 • 18 years ago

I knew about that but it is only a warning on gcc4. I'll fix it either on checkin or I'll post another patch after more reviews.

	Josh Aas Assignee
	Comment 5 • 18 years ago

Brings it up to latest trunk, fixes gcc3.3 compile error.

	Stefan [:stefanh]
	Comment 6 • 18 years ago

Hmm, attachment #220080 [details] [diff] [review] seems to break seamonkey's download manager. Before applying the patch I had one .svg file listed in the dm. With the patch applied, opening the dm results in a blank window with a spinning wheel.

	Josh Aas Assignee
	Comment 7 • 18 years ago

Stefan - I just compiled a fresh Seamonkey from the branch with fix v1.1 applied and I was able to download a bunch of stuff, restart the browser and download more stuff with no problems - unless you consider dangerously good looking icons to be a problem. I even tried switching back and forth between trunk and Seamonkey 1.0.1 builds. Can you get a trace from the hang? In the Mac OS X developer tools there is an app called "Spin Control" that is good for getting traces from hangs.

I doubt this patch is to blame. Maybe check to make sure everything is compiling correctly in libpr0n, you might have to do a make clean after applying the patch.

	Robert Kaiser
	Comment 8 • 18 years ago

"dangerously good looking icons" in SeaMonkey? Well, as long as you don't mean those of our default theme ;-)

	Stefan [:stefanh]
	Comment 9 • 18 years ago

(In reply to comment #7)
> I doubt this patch is to blame. Maybe check to make sure everything is
> compiling correctly in libpr0n, you might have to do a make clean after
> applying the patch.

I'll get myself a fresh tree and check if it works with that.

	Mark Mentovai
	Comment 10 • 18 years ago

Comment on attachment 220080 [details] [diff] [review]
fix v1.1

>+    // get the mac creator and file type for this mime object
>+    PRUint32 macType;
>+    mimeInfo->GetMacType(&macType);
>+  
>+    iconImage = [[NSWorkspace sharedWorkspace] iconForFileType:[NSString stringWithCString:(char*)&macType length:4]];

As discussed: check the rv of GetMacType after bug 335840.  If it's successful, use iconForFileType:NSFileTypeForHFSTypeCode(macType).  If unsuccessful and you have a fileExt, use the fileExt.  I'm not sure, but you may also need another fallback to use a generic document icon if you have neither macType or fileExt.

>+  NSBitmapImageRep* bitmapRep = [[NSBitmapImageRep alloc]
>+                                  initWithFocusedViewRect:NSMakeRect(0, 0, desiredImageSize, desiredImageSize)];

Never released, autorelease should be fine.

>+  // We expect a non-planar rep with 32 bits per pixel.
>+  // It should always be that.
>+  if ([bitmapRep isPlanar] ||
>+      [bitmapRep bitsPerPixel] != 32 ||
>+      [bitmapRep samplesPerPixel] != 4 ||
>+      [bitmapRep hasAlpha] != YES)
>+    return NS_ERROR_FAILURE;

I'd like this to print a warning if it returns here.  NS_ENSURE_TRUE?  Or add your own NS_WARNING.

Aren't there cases where the image representation won't be a nice clean 4bpp?  Let's say the icon belongs to an old Classic program.  You might think that's silly, and I would ordinarily agree, except I sometimes wind up with ResEdit mapped to things...

>+  // rgba, pre-multiplied data
>+  unsigned char* bitmapRepData = [bitmapRep bitmapData];
>+  
>+  nsCString iconBuffer;

iconBuffer.SetCapacity(whatever);

where whatever looks like it should be 3 + desiredImageSize * desiredImageSize * 5.

>+  PRUint32 dataCount = (desiredImageSize * desiredImageSize) * 4;
>+  PRUint32 index = 0;
>+  while (index < dataCount) {

There should be a check to ensure that bitmapRep is as big as we think it is.  Assert that dataCount == [bitmapRep bytesPerPlane]?  Maybe add this to the early-return checks above?

>+    if (a == 0) {
>+      r = g = b = 0;

:)

>+    // write data out to our buffer

As discussed - comment why we're writing ARGB and give a hint on where to find the real alpha data.

	Josh Aas Assignee
	Comment 11 • 18 years ago

Addresses comments. In followup patches I will address these two problems:

- check the rv of GetMacType
- handle non-4bpp reps

	Mark Mentovai
	Comment 12 • 18 years ago

Comment on attachment 220168 [details] [diff] [review]
fix v1.2

>+  NS_ENSURE_TRUE(![bitmapRep isPlanar] &&
>+                 [bitmapRep bytesPerPlane] == desiredImageSize * desiredImageSize * 4 &&
>+                 [bitmapRep bitsPerPixel] == 32 &&
>+                 [bitmapRep samplesPerPixel] == 4 &&
>+                 [bitmapRep hasAlpha] == YES,
>+                 NS_ERROR_UNEXPECTED);

produces this warning:

In member function 'nsresult nsIconChannel::MakeInputStream(nsIInputStream**, PRBool)':
warning: comparison between signed and unsigned integer expressions

NSBitmapImageRep's bytesPerPlane is signed.  Add a cast and r+.

	Stefan [:stefanh]
	Comment 13 • 18 years ago

 (In reply to comment #9)
> (In reply to comment #7)
> > I doubt this patch is to blame. Maybe check to make sure everything is
> > compiling correctly in libpr0n, you might have to do a make clean after
> > applying the patch.
> 
> I'll get myself a fresh tree and check if it works with that.
> 

It appears that I crash, not hang - I was just a bit unpatient. I got myself a fresh trunk tree and applied fix 1.1. Then I backed out fix 1.1 (removed the files, checked out the files again and touched them) then I applied the new patch, did a "make clean" and "make" in decoders - I still crash, here's how to repro:

1) Go to http://www.mozilla.org/products/
2) Right-click on the Firefox logo and choose "Save Image As..."
3) Choose a destination, like your desktop
4) Once the download has completed, close the download dialog box (if you have  it configured that way)
5) Open the download manager (Tools --> Download manager)

--> Spinning wheel and, after a couple of minutes, crash.

I'm using gcc3.3 on 10.3.9

	Stefan [:stefanh]
	Comment 14 • 18 years ago

Sorry for all the spam here, just felt that I should attach this crash log... Since I don't know the code I'm not sure I'm making unnecesarily noice here (when looking at comment #12).

	Mark Mentovai
	Comment 15 • 18 years ago

That's this line:

+    iconImage = [[NSWorkspace sharedWorkspace] iconForFileType:[NSString stringWithCString:(char*)&macType length:4]];

That's one of the things I want changed (comment 10).  The code as it's written above is broken, what it means to do (without adding the check on GetMacType) is this:

    iconImage = [[NSWorkspace sharedWorkspace] iconForFileType:NSFileTypeForHFSTypeCode(macType)];

Stefan, could you give that a try and let us know if it still crashes?

	Stefan [:stefanh]
	Comment 16 • 18 years ago

(In reply to comment #15)
> That's this line:
> 
> +    iconImage = [[NSWorkspace sharedWorkspace] iconForFileType:[NSString
> stringWithCString:(char*)&macType length:4]];
> 
> That's one of the things I want changed (comment 10).  The code as it's written
> above is broken, what it means to do (without adding the check on GetMacType)
> is this:
> 
>     iconImage = [[NSWorkspace sharedWorkspace]
> iconForFileType:NSFileTypeForHFSTypeCode(macType)];
> 
> Stefan, could you give that a try and let us know if it still crashes?
>

If I change the above mentioned line I still crash - stack looks the same :-(

	Mark Mentovai
	Comment 17 • 18 years ago

Stefan, I'm unable to reproduce that crash using your steps.  Does it only occur with PNGs or with other file types as well?

Can you break the line up into multiple calls to see which is failing?

NSWorkspace* w = [NSWorkspace sharedWorkspace];
fprintf(stderr, "w = %p\n", w);
iconImage = [w iconForFileType:NSFileTypeForHFSTypeCode(macType)];
fprintf(stderr, "iconImage = %p\n", iconImage);

You can also run with the NSZombieEnabled environment variable set to "YES", this may produce some useful diagnostics on stderr.

	Stefan [:stefanh]
	Comment 18 • 18 years ago

(In reply to comment #17)
> Stefan, I'm unable to reproduce that crash using your steps.  Does it only
> occur with PNGs or with other file types as well?
> 
> Can you break the line up into multiple calls to see which is failing?
> 
> NSWorkspace* w = [NSWorkspace sharedWorkspace];
> fprintf(stderr, "w = %p\n", w);
> iconImage = [w iconForFileType:NSFileTypeForHFSTypeCode(macType)];
> fprintf(stderr, "iconImage = %p\n", iconImage);
> 
> You can also run with the NSZombieEnabled environment variable set to "YES",
> this may produce some useful diagnostics on stderr.
> 
We're talking seamonkey here, right (just in case I've been unclear)?

I don't seem to get any console output from nsIconChannel.mm. Same crash happened with a .txt file in the dm - when I first noticed the crash I had a .svg file in the dm. With biesis help I was able to print out an icon URI from nsIconProtocolHandler.cpp - I also crash if I load a icon in the browser directly (.txt icon). The stack is slightly different (but the first lines are the same).

	Stefan [:stefanh]
	Comment 20 • 18 years ago

> You can also run with the NSZombieEnabled environment variable set to "YES",
> this may produce some useful diagnostics on stderr.
>

Ah, haven't tried this yet, though.

	Mark Mentovai
	Comment 21 • 18 years ago

The point of the previous exercise was not to produce a crash log (it of course would crash) but to see the results of the prints.

Thread 2:
0   libSystem.B.dylib     	0x974b88b8 mach_msg_trap + 0x8
1   libSystem.B.dylib     	0x974b8438 mach_msg + 0x38
2   com.unsanity.ape      	0xc0002afc __ape_internal + 0xce4
3   com.unsanity.ape      	0xc0001910 __ape_agent + 0x40
4   libSystem.B.dylib     	0x974d5990 _pthread_body + 0x28

Oooh.

Would you mind trying again without third-party crust?

	Josh Aas Assignee
	Comment 22 • 18 years ago

Fixes a build warning and changes the way we handle file types.

	Stuart Parmenter
	Comment 23 • 18 years ago

Comment on attachment 220400 [details] [diff] [review]
fix v1.3

you don't really want to use a nsCString here (I see that the old code was -- but that was pretty broken.

I'd use a nsAutoBuffer and write out to that.


With cairo builds, we really want rgba pre-multiplied data.  We'd need a new raw data decoder to support this though.  Probably do that later.

	Josh Aas Assignee
	Comment 24 • 18 years ago

(In reply to comment #23)
> (From update of attachment 220400 [details] [diff] [review] [edit])
> you don't really want to use a nsCString here (I see that the old code was --
> but that was pretty broken.

I don't think anything was broken about it. It worked just fine though its true that the data in question did not actually represent a string.

> I'd use a nsAutoBuffer and write out to that.

A couple problems with that. First, you have to decide its capacity at compile time. We don't know what it is going to be. Secondly, we would have to write the icon data using a messy deference-and-increment scheme. Its much cleaner with the nsCString.

I just don't see what we'd gain by doing that. Sure, the data is not a string but the nsCString code does what we want and nicely compared to nsAutoBuffer. It doesn't even look to me like any other code in Mozilla uses nsAutoBuffer the way you're suggesting.

	Asaf Romano (gone)
	Comment 25 • 18 years ago

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 220400 [details] [diff] [review] [edit] [edit])
> > you don't really want to use a nsCString here (I see that the old code was --
> > but that was pretty broken.
> 
> I don't think anything was broken about it. It worked just fine though its true
> that the data in question did not actually represent a string.
> 
> > I'd use a nsAutoBuffer and write out to that.
> 
> A couple problems with that. First, you have to decide its capacity at compile
> time. We don't know what it is going to be.

see EnsureElemCapacity

	Josh Aas Assignee
	Comment 26 • 18 years ago

This impl uses nsAutoBuffer. I thought ensureElemCapacity just did a check, I didn't see that it actually changes the length of the buffer.

	Josh Aas Assignee
	Comment 27 • 18 years ago

This impl uses a PRUint8*[] buffer. Just an alternative if we don't want to use stack space the way nsAutoBuffer would.

	Mark Mentovai
	Comment 28 • 18 years ago

Comment on attachment 220514 [details] [diff] [review]
fix v1.4

>+  // create our buffer
>+  PRInt32 bufferCapacity = 3 + desiredImageSize * desiredImageSize * 5;
>+  nsAutoBuffer<PRUint8, 1283> iconBuffer; // initial size is for 16x16

What I like about this compared to v1.5 is that it takes care of the delete for you.  I don't like the dangling delete in v1.5, I'm afraid that someone will come along and add an early-return and things will begin leaking.  For that reason, I prefer v1.4 with the nsAutoBuffer.

What I don't like about this is that it always eats up over 1k of stack space that might never be used.  We do a bunch with 32x32 icons which will need to go out and do a malloc anyway.  I'm almost inclined to suggest using 0 for the size so that it doesn't waste any stack space and always mallocs, but since I'm so conflicted, I'm just going to say that I don't really care and you can leave this at 1283 if you want to.  If you do leave the 1283, rewrite it as 3+16*16*5, that's still a constant.

>+  // write header data into buffer
>+  *iconBufferPtr++ = (PRUint8)desiredImageSize;
>+  *iconBufferPtr++ = (PRUint8)desiredImageSize;
>+  *iconBufferPtr++ = (PRUint8)8; // alpha bits per pixel

Don't need to cast 8 to a PRUint8.  (You don't need the two other casts either, but those are OK to leave.)

>+  PRUint32 dataCount = (desiredImageSize * desiredImageSize) * 4;
>+  PRUint32 index = 0;
>+  while (index < dataCount) {
>+    // get data from the bitmap
>+    PRUint8 r = (PRUint8)bitmapRepData[index++];
>+    PRUint8 g = (PRUint8)bitmapRepData[index++];
>+    PRUint8 b = (PRUint8)bitmapRepData[index++];
>+    PRUint8 a = (PRUint8)bitmapRepData[index++];

With all of the casting going on, I think it makes more sense to declare bitmapRepData as PRUint8* and cast what you assign to it: (PRUint8*)[bitmapRep bitmapData];.

>+    // reverse premultiplication
>+    if (a == 0) {
>+      r = g = b = 0;
>+    }
>+    else {
>+      r = ((unsigned int) r) * 255 / a;
>+      g = ((unsigned int) g) * 255 / a;
>+      b = ((unsigned int) b) * 255 / a;

Those casts should be PRUint32.

	Mark Mentovai
	Comment 29 • 18 years ago

Comment on attachment 220515 [details] [diff] [review]
fix v1.5

Cleaner to not need to clean up after yourself...

	Josh Aas Assignee
	Comment 31 • 18 years ago

landed on trunk

	Josh Aas Assignee
	Comment 32 • 18 years ago

Stuart - I'd like to have this for the next Bon Echo alpha, and the code freeze is tonight. Also 336356 on top of this.

	Josh Aas Assignee
	Comment 33 • 18 years ago

landed on MOZILLA_1_8_BRANCH

	Wayne Woods
	Comment 34 • 18 years ago

FYI, the fix for this bug seems to have caused bug 337334. I've cc'd you there, Josh, if you could please take a look.