Closed
Bug 335723
Opened 19 years ago
Closed 19 years ago
rewrite Mac icon decoder
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1alpha2
People
(Reporter: jaas, Assigned: jaas)
References
Details
(Keywords: fixed1.8.1)
Attachments
(3 files, 6 obsolete files)
29.33 KB,
text/plain
|
Details | |
33.44 KB,
text/plain
|
Details | |
37.50 KB,
patch
|
mark
:
review+
pavlov
:
superreview+
pavlov
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Attachment #220045 -
Flags: review?(vladimir)
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.
Attachment #220045 -
Flags: review?(vladimir) → review?(pavlov)
Attachment #220045 -
Flags: review?(pavlov) → review?(mark)
Comment 3•19 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
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.
Brings it up to latest trunk, fixes gcc3.3 compile error.
Attachment #220045 -
Attachment is obsolete: true
Attachment #220080 -
Flags: review?(mark)
Attachment #220045 -
Flags: review?(mark)
Comment 6•19 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.
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.
Comment 8•19 years ago
|
||
"dangerously good looking icons" in SeaMonkey? Well, as long as you don't mean those of our default theme ;-)
Comment 9•19 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.
Comment 10•19 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.
Attachment #220080 -
Flags: review?(mark) → review-
Assignee | ||
Comment 11•19 years ago
|
||
Addresses comments. In followup patches I will address these two problems:
- check the rv of GetMacType
- handle non-4bpp reps
Attachment #220168 -
Flags: review?(mark)
Comment 12•19 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+.
Attachment #220168 -
Flags: review?(mark) → review+
Attachment #220168 -
Flags: superreview?(pavlov)
Comment 13•19 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
Comment 14•19 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).
Comment 15•19 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?
Comment 16•19 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 :-(
Comment 17•19 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.
Comment 18•19 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).
Comment 19•19 years ago
|
||
Comment 20•19 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.
Comment 21•19 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?
Assignee | ||
Comment 22•19 years ago
|
||
Fixes a build warning and changes the way we handle file types.
Attachment #220080 -
Attachment is obsolete: true
Attachment #220168 -
Attachment is obsolete: true
Attachment #220400 -
Flags: review?(mark)
Attachment #220168 -
Flags: superreview?(pavlov)
Updated•19 years ago
|
Attachment #220400 -
Flags: review?(mark) → review+
Attachment #220400 -
Flags: superreview?(pavlov)
Comment 23•19 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.
Attachment #220400 -
Flags: superreview?(pavlov) → superreview-
Assignee | ||
Comment 24•19 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.
Comment 25•19 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
Assignee | ||
Comment 26•19 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.
Attachment #220400 -
Attachment is obsolete: true
Attachment #220514 -
Flags: review?(mark)
Assignee | ||
Comment 27•19 years ago
|
||
This impl uses a PRUint8*[] buffer. Just an alternative if we don't want to use stack space the way nsAutoBuffer would.
Attachment #220515 -
Flags: review?(mark)
Comment 28•19 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.
Attachment #220514 -
Flags: review?(mark) → review+
Comment 29•19 years ago
|
||
Comment on attachment 220515 [details] [diff] [review]
fix v1.5
Cleaner to not need to clean up after yourself...
Attachment #220515 -
Flags: review?(mark)
Assignee | ||
Comment 30•19 years ago
|
||
Attachment #220514 -
Attachment is obsolete: true
Attachment #220515 -
Attachment is obsolete: true
Attachment #220524 -
Flags: superreview?(pavlov)
Attachment #220524 -
Flags: review?(mark)
Updated•19 years ago
|
Attachment #220524 -
Flags: review?(mark) → review+
Updated•19 years ago
|
Attachment #220524 -
Flags: superreview?(pavlov) → superreview+
Attachment #220524 -
Flags: approval-branch-1.8.1?(pavlov)
Assignee | ||
Comment 31•19 years ago
|
||
landed on trunk
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 32•19 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.
Updated•19 years ago
|
Attachment #220524 -
Flags: approval-branch-1.8.1?(pavlov) → approval-branch-1.8.1+
Assignee | ||
Comment 33•19 years ago
|
||
landed on MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Updated•19 years ago
|
Target Milestone: --- → mozilla1.8.1alpha2
Comment 34•19 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•