119118 - Issue with calculating rowBytes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Robert John Churchill]

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.  :)

Comment 1

[Robert John Churchill]

Attachment: Screenshot of damaged favicons on the Personal Toolbar

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.]

Comment 2

[Andrew Thompson]

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.

Comment 3

[Robert John Churchill]

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

Comment 4

[Robert John Churchill]

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.

Comment 5

[Andrew Thompson]

Attachment: Fixes the rowbytes issue by requiring the caller to explicitly ask for the smaller calculation

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...

Comment 6

[Andrew Thompson]

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 7

[Steve Dagley]

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)

Comment 8

[Simon Fraser [no longer active]]

> ioPixMap.rowBytes = rowBytes | 0x8000;

This is absolutely needed. We pass that pixmap directly into CopyBits calls.
GetLineStride() should mask off the top 2 bits.

Comment 9

[Jonathan Brecher]

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.

Comment 10

[Robert John Churchill]

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.

Comment 11

[Andrew Thompson]

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.

Comment 12

[Andrew Thompson]

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.

Comment 13

[Jonathan Brecher]

>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.

Comment 14

[Andrew Thompson]

Simon seems to have gone home, and more to the point, the tree is burning.

I'll attempt to get an a= checkin tomorrow.

Comment 15

[Andrew Thompson]

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.

Comment 16

[John Morrison]

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

Comment 17

[Andrew Thompson]

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.

Comment 18

[Terri Preston]

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

Comment 19

[Andrew Thompson]

Attachment: More sophisticated patch which hides the 2 byte functionality in private functions

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 20

[Simon Fraser [no longer active]]

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

sr=sfraser

Comment 21

[Steve Dagley]

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)

Comment 22

[Brendan Eich [:brendan]]

a=[e-mail] for 0.9.8 checkin.

/be

Comment 23

[Andrew Thompson]

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

Comment 24

[Robert John Churchill]

Yes.  :)

Comment 25

[Asa Dotzler [:asa]]

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

Comment 26

[Christopher Blizzard (:blizzard)]

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

Comment 27

[Andrew Thompson]

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

Comment 28

[Andrew Thompson]

Attachment: Patch against branch

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 29

[Robert John Churchill]

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 30

[Simon Fraser [no longer active]]

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

sr=sfraser

Comment 31

[Christopher Blizzard (:blizzard)]

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

Comment 32

[Andrew Thompson]

Attachment: Patch against trunk to solve issue fixed in branch patch

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 33

[Simon Fraser [no longer active]]

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

sr=sfraser

Comment 34

[Brian 'netdragon' Bober]

Looks good to me, just fix the indenting on:

ioPixMap, ioBitsHandle, PR_FALSE);

Then you will have an r= netdemon

Comment 35

[Andrew Thompson]

Checked into the trunk