Closed Bug 81361 Opened 24 years ago Closed 22 years ago

mBytesPerPixel set incorrectly in nsImageMac.cpp

Categories

(Core Graveyard :: Image: Painting, defect)

PowerPC
All
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.3beta

People

(Reporter: lordpixel, Assigned: saari)

References

Details

Attachments

(1 file, 1 obsolete file)

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

This is easy to fix in ::Init. Will attach patch.
Attached patch Fixes so this variable is set (obsolete) — Splinter Review
Blocks: 64177
Severity: normal → trivial
Keywords: patch, review
Blocks: 46177
No longer blocks: 64177
Do we every use mBytesPerPixel? Also, can you do it in 1 line?
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 :)
over to saari
Assignee: pavlov → saari
We never used this, although I tried in libpr0n and got burnt by it. I just
worked around it as we always have before.
Target Milestone: --- → mozilla1.0
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?
Target Milestone: mozilla1.0 → mozilla1.0.1
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?
Status: NEW → ASSIGNED
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)
Attachment #34925 - Flags: review+
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.
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. 
Moving bugs to new Image: GFX component
Component: ImageLib → Image: GFX
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)
Summary: mBytesPerPixel is never set in nsImageMac.cpp → mBytesPerPixel set incorrectly in nsImageMac.cpp
Target Milestone: mozilla1.0.1 → mozilla1.3beta
Attachment #34925 - Attachment is obsolete: true
r=lordpixel

Seems eminently reasonable to me.
Comment on attachment 108419 [details] [diff] [review]
Fix mBytesPerPixel on Mac

sr=tor
Attachment #108419 - Flags: superreview+
checked in earlier today
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: