lordpixel, in your code changes for bug # 66814, you have a change to how rowBytes is calculated. Ref: mozilla/gfx/src/mac/nsImageMac.cpp in nsImageMac::CalculateRowBytes() [around line # 656]: What used to be just: rowBytes = ((aWidth * aDepth + 31) / 32) * 4; is now: if (rowBits > 24) rowBytes = ((aWidth * aDepth + 31) / 32) * 4; else rowBytes = ((aWidth * aDepth + 15) / 16) * 2; Running with this new change, favicon masks (such as those in the Personal Toolbar) now appear to have errors. I'll attach a screen shot. Either the changes are doing an incorrect calculation or there must be an error in the code which converts .ICO files. I'll let you decide. :)

Here's the screenshot. [Note that reverting that code change back to the old calculation fixes the problem, although I believe lordpixel mentioned in bug # 66814 that he saw errors elsewhere.]

hrm... well, I could always parameterize it out so the caller can decide the behaviour, as the icon code definately needs the modified calculation. I'll try to take a look soon and see if I can figure out why those icons are breaking... it may well be related to exactly what's in the nsIImage.

CC'ing hyatt (who coded up the .ICO decoder)

Note: Along with .ICO favicons, this bug also affects "moz-icon:" URLs such as the icons you'll get if you load in a file:/// URL on the Mac.

This fixes the problem on my build. Interestingly, if I disable the 2 byte calculation, all of the native icons I have get screwed up. Looks like different parts of the OS toolbox have different expectations about small images. This could be because small icons have their mask included inline with the image data...

One possible source for confusion is that the icon decoder (nsIIconDecoder) uses nsIImage::getLineStride() On Mac OS, the instance variable this uses is ultimately initialized in CreatePixMap using this code: // set the high bit to tell CopyBits that this is a PixMap ioPixMap.rowBytes = rowBytes | 0x8000; As the comment indicates, the mask is used because a native Mac OS PixMap can either be a colour Pixmap or an older black & white datastructure of 1984 vintage called, I think, BitMap. However, we're exposing this value through a getter, and I doubt most of Mozilla expects the high bit to be set! Opinions on whether I should file a bug on this?

Comment on attachment 64612 [details] [diff] [review] Fixes the rowbytes issue by requiring the caller to explicitly ask for the smaller calculation r=sdagley (and btw, I don't think we should bother with BitMaps these days since most Macs we run on can't run anything but color)

> ioPixMap.rowBytes = rowBytes | 0x8000; This is absolutely needed. We pass that pixmap directly into CopyBits calls. GetLineStride() should mask off the top 2 bits.

Danger Will Robinson! Apple has for a long time "suggested" that rowBytes always be divisible by 4. Something in OS X has changed so that the suggestion is now a requirement. We had several crashes in our code (and in the release builds only, to boot) that went away when we enforced a divisible-by-four rowBytes. I don't know the exact circumstances of this Apple bug, and it may be that your changes are safe as currently implemented, but there is potential for hard-to-isolate problems... I've reported this to Apple as Radar #2792873.

Lordpixel, can you get this fixed for 0.9.8? (Note: tree closes tomorrow (Tuesday) at midnight.) Otherwise, Mac builds will look "corrupted" for a lot of users.

Simon - you're right about ioPixMap.rowBytes = rowBytes | 0x8000; It is, in fact masked. I realised this when talking it over with sdagley. Robert - if I can get a super review I will check this in tonight.

Jonathan - with the patch on this bug, row bytes will always be divisible by 4 except when converting to native icons. We currently do not call the icon conversion code anywhere in the tree. I'll do more investigation before we do.

>row bytes will always be divisible by 4 except when converting to native icons Actually, rowBytes will always be divisible by 4 "except when the caller explictly asks for the smaller calculation" (which happens only to happen at the moment when converting icons). The problem is if some well-meaning developer several years from now sees the option to use the alternate calculation without realizing its consequences. We've gotten bit by that before in our own code. I'm not saying that you should do anything differently. But if someone does run into problems in the future, at least they mind turn up my comments here.

Simon seems to have gone home, and more to the point, the tree is burning. I'll attempt to get an a= checkin tomorrow.

Yes. If I can't remove the need for the option with further investigation, I'll make two versions of the method and have the one that allows 2 byte widths be private with very large disclaimers in the source.

*** Bug 120196 has been marked as a duplicate of this bug. ***

Ok, I had a chat with smfr and spent some more time looking at this, and I think I figured out what is going on. 1/ Simon asked me to look at some native mac icons in the debugger to see what rowbytes values they use. Unfortunately this doesn't work as icon data is actually just the raw pixel data - its what gets assigned to PixMap->baseAddr. If you look at the way the icon creation code works, this should become obvious... we create a new PixMap to describe the icon data we want, then use ::CopyBits to copy the data from the nsImageMac into the new PixMap. Then we throw away the PixMap and just return the raw data (which can then be put into an Icon Suite or Icon Family). Making this PixMap by hand is the whole reason we have to calculate rowbytes in the first place :-) When one gets icons from the operating system, similarly one only gets the raw data. There is no accompanying PixMap to look at in the debugger! 2/ I experimented with the parameters passed to the icon creation code to see when the native icons (in this case, generated for the bookmarks menu - this code is not checked in yet) would appear correctly and when they would be messed up. All icons used in the menus are 16x16 with a 1 bit mask. Without my "allow multiples of 2" rowbytes changes their masks are always messed up. If I *force* an 8 bit mask (full alpha channel) they display correctly in the menus with even when rowbytes is a multiple of 4. Therefore this problem is limited to ics# type (16x16 icons with a 1 bit mask). Indeed it is specifically the mask which is corrupted. Larger icons have always been fine in my experience, and this testing I did today also shows small icons with a deeper mask are OK too. 3/ So I looked through the conversion code again. I think I've figured out exactly what is happening. The whole icon architecture has been overhauled several times. Some of the icon data structures are new with Mac OS 9, others have been around since System 6 (pre 1987) and even earlier. As each redesign has occured, the format for the data in the icons has evolved. All of the fancy types (Icon Suite, Icon Family and Icon Refs) attempt to hide the differences between the data formats in a modern API - but we're operating below that level. Specifically, if you look at one bit masked small icons (ics#) and follow my code you'll see that 1 bit masks are a special case. For 8 bit masks, one simply generates the image data, and the mask data (and in the code that's not checked in yet, I go on to inserts both sets of data into a new icon family). For one bit masks, the data structure is less convenient. One must create the icon image data, then a one bit mask, then concatenate the bit data together into one piece (then insert that combined structure into the icon family along with the image data (ie, make an ics8 and an ics#)). This duplication is due to the legacy nature of the old ics# data structure. So my strong suspicion is that since the mask data is meaningless unless it has the image data prepended to it, that's why it has a smaller rowbytes. When we copy the 1 bit mask part, we're really only copying 'half' the datastructure we will eventually assemble. The thing to recall is that, as I said way above, we * never* pass the PixMap we create to do the copy to anything. Its entirely temporary and private, we use if for the call to ::CopyBits, then we discard it and just return the raw data. For this reason - the fact this PixMap is just used to copy some rather odd data temporarily and then immediately discarded, I think we need to allow it to have the special "divisible by two" property and just make sure its safe. We should not allow other code to create such pixmaps... so I'm going to resolve this with a new patch such that the icon creation code calls a private implementation that will give the divisible by 2 pixmap in the right case, but so this functionality is not available in the public API of the nsImageMac class. This will give us icon code which works and ensure no one else can accidentally get the special behaviour, which is probably the wrong thing to do in other cases.

*** Bug 120611 has been marked as a duplicate of this bug. ***

As discussed, this version ensures that all of the 2 byte functionality is contained within nsImageMac. public/protected wrappers now call the private functions and always request the 4 byte behaviour. Seems to be performing flawlessly on my machine. I'd like to get this into 0.9.8 if possible. Therefore I need 2 reviews and an a= by the cutoff!

Comment on attachment 65718 [details] [diff] [review] More sophisticated patch which hides the 2 byte functionality in private functions sr=sfraser

Comment on attachment 65718 [details] [diff] [review] More sophisticated patch which hides the 2 byte functionality in private functions r=sdagley (please add a comment that the align 4 seems to be a requirement for OS X)

a=brendan@mozilla.org for 0.9.8 checkin. /be

Fix checked into trunk. Anyone think this matters enough to get it onto the branch?

Yes. :)

Looks like this has not made it into the branch (I think). Andy, can you check this into the branch please. Thanks.

This patch doesn't apply cleanly to the 0.9.8 branch. I need a mac person to merge and test.

I'll be back around 10:30pm eastern (7:30 pacific...) will try to do something about this then.

Here's a patch against the branch which will go on cleanly. Unfortunately I spotted as issue with the patch checked into the trunk (some of the wrappers were not passing along their results). This patch fixes those issues. Someone needs to make a call about whether reviews are still valid for this patch. Otherwise, it'll need re-reviewing and that'll make it hard to get in on the branch. Once the dust settles, I'll make a patch to fix the additional issue I've spotted on the trunk. Thanks. Aplologies for missing the problem I found tonight.

Comment on attachment 67036 [details] [diff] [review] Patch against branch I applied this patch against GFX on the 0.9.8 branch and indeed it fixes the problem. r=rjc (in case you need it)

Comment on attachment 67036 [details] [diff] [review] Patch against branch sr=sfraser

Checked into the 0.9.8 branch. Please close this bug if that clears everything.

Thanks to everyone who helped to get this into the branch! Here's the promised patch to fix the issue I spotted the other day, on the trunk.

Comment on attachment 67381 [details] [diff] [review] Patch against trunk to solve issue fixed in branch patch sr=sfraser

Looks good to me, just fix the indenting on: ioPixMap, ioBitsHandle, PR_FALSE); Then you will have an r= netdemon

Checked into the trunk