81361 - mBytesPerPixel set incorrectly in nsImageMac.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Image: Painting, defect)

Description

[Andrew Thompson]

The mBytesPerPixel member is never set in the Mac version of nsIImage.

This is easy to fix in ::Init. Will attach patch.

Comment 1

[Andrew Thompson]

Attachment: Fixes so this variable is set

Comment 2

[Simon Fraser [no longer active]]

Do we every use mBytesPerPixel? Also, can you do it in 1 line?

Comment 3

[Andrew Thompson]

Its returned by the accessor function GetBytesPix() in nsIImage and I want to use 
it as a part of fixing the dependent bug 46177. The issue is that the Mac code 
never sets it to anything useful, though we do know the value during ::Init().


In one line?

mBytesPerPixel = (aDepth < 8) ? 1 : aDepth / 8;

I'll make a patch out of that if you like. I guess is is better. It was late :)

Comment 4

[Stuart Parmenter]

over to saari

Comment 5

[saari (gone)]

We never used this, although I tried in libpr0n and got burnt by it. I just
worked around it as we always have before.

Comment 6

[Andrew Thompson]

hmmm, well, I intend to use it in Mac only code, and this patch sets it, IMO, 
correctly where it was never set before. So unless ::Init() is passed garbage 
(which I suspect its not as it would have caused other problems?) I should be 
fine on Mac OS.

Sufficient to convince you to r= ? If not, what do I need to do to make progress?

Comment 7

[Andrew Thompson]

Hrm, this one liner accicentally got checked in in another patch the other night,  
though obviously, that patch was reviewed. I know. My bad... would you like me to 
make a patch to back it out or are people willing to r= in the current state?

Comment 8

[Steve Dagley]

Comment on attachment 34925 [details] [diff] [review]
Fixes so this variable is set

r=sdagley (since we now have code that needs this to be set)

Comment 9

[Andrew Thompson]

I've been thinking about this, and shouldn't this be:

mBytesPerPixel = (aDepth % 8) == 0 ? aDepth/8 : (aDepth / 8) + 1;

If so, I'll make a new patch.

Comment 10

[ArronM (:paper)]

I ran into the checked-in code recently and had problems with it.  I expected
GetBytesPix() to return 4 instead of 3 for Macs for 24 bit images, since the mac
imglib stores 24-bit images in 4 pixels.

However, it returned 3.  So I decided not to use GetBytesPix, and hardcoded a 
#if defined(XP_MAC) || defined(XP_MACOSX)
line like everyone else seems to do.

I suggest changing it to 4 for 24 bit images.

There's only 1 object that actually uses GetBytesPix
(/gfx/src/ps/nsPostScriptObj.cpp), and it only checks to see if it's 1.  So,
changing it to 4 will have no ramifications on existing Mozilla code. 

Comment 11

[Stuart Parmenter]

Moving bugs to new Image: GFX component

Comment 12

[ArronM (:paper)]

Attachment: Fix mBytesPerPixel on Mac

mBytesPerPixel should be based on mImagePixmap.pixelSize, which stores the
number of bits per pixel, and is set by the nsImageMac::CreatePixMap call
(which called CreatPixMapInternal, which, as you can see by
http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsImageMac.cpp#602 , it's
set pixelSize to 32 for 24 bit-depth images)

Comment 13

[Andrew Thompson]

r=lordpixel

Seems eminently reasonable to me.

Comment 14

[tor]

Comment on attachment 108419 [details] [diff] [review]
Fix mBytesPerPixel on Mac

sr=tor

Comment 15

[ArronM (:paper)]

checked in earlier today