BMP image displays upside-down ("back-to-front")

VERIFIED FIXED

Status

()

Core
ImageLib
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Simon Fraser, Assigned: Jim Dunn)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
This image displays reversed (attachment coming).
(Reporter)

Comment 1

15 years ago
Created attachment 125010 [details]
BMP that displays incorrectly
hm... could you attach a screenshot how the attachment appears for you? it
displays correctly here (linux)
(Reporter)

Comment 3

15 years ago
Created attachment 125037 [details]
Screenshot of BMP display in 2003052307

Comment 4

15 years ago
Displays right-way-up for me too, on W2K and Linux.
Summary: BMP image displays upside-down and back-to-front → BMP image displays upside-down ("back-to-front")
(Assignee)

Comment 5

15 years ago
yup on mac I see the problem...
(Assignee)

Comment 6

15 years ago
Yeah weird, something is whacked with BMPs on macosX...
several other bmp's just display
"he image "blah" cannot be displayed because it contains errors"
yet works find on win2k  like
http://ic.ucsc.edu/~wxcheng/wewu/images/prot1.bmp
or
http://www.radio.org.lv/Logo/LR_0_cl.bmp

looking...
Status: NEW → ASSIGNED
(Reporter)

Comment 7

15 years ago
Are we sure we're building the bmp decoders on Mac, and not falling back on
QuickTime?
(Assignee)

Comment 8

15 years ago
Yes we are using our code, I can turn on BMPDecoder:5 (in a debug build)
and get prints out... (heck I didn't even know that was an option!?!)

(Assignee)

Comment 9

15 years ago
fyi, this image (and the others I pointed out) looks ok in 1.2.1 but
doesn't in 1.3.  So it was something between those 2 versions - maybe
the rle support ???
(Assignee)

Comment 10

15 years ago
specifically changes between 1.2.1 & 1.3a.  bug 179296 maybe??? looking
(Assignee)

Comment 11

15 years ago
Created attachment 125309 [details] [diff] [review]
Must shift unsigned ints, so that MSB does not get propagated

The fix for bug 179296 (nsBMPDecoder.h) change mBIH.width & mBIH.height 
from PRUint32's to PRInt32's.  However, when you pass an INT thru the
LITTLE_TO_NATIVE32 macro if the MSB is a 1 (negative), when you shift
this you will propagate 1's.  This was messing everything up.
for the LR_0_cl.bmp's, the width turned negative and hence wasn't shown.
Similar mixup for the backward BMP.

So when calling LITTLE_TO_NATIVE32, just cast the entity as unsigned.

Will verify on HP-UX, since Linux & Windows are both little endian and
shouln't run into this, whereas other unixes should.
(Assignee)

Comment 12

15 years ago
Comment on attachment 125309 [details] [diff] [review]
Must shift unsigned ints, so that MSB does not get propagated

NOTE: this bug does appear on other big-endian systems, the patch works there
too.
Attachment #125309 - Flags: superreview?(tor)
Attachment #125309 - Flags: review?(paper)

Comment 13

15 years ago
Comment on attachment 125309 [details] [diff] [review]
Must shift unsigned ints, so that MSB does not get propagated

Do you have to cast back to PRInt32 or will the compiler not complain?
(Assignee)

Comment 14

15 years ago
Created attachment 125321 [details] [diff] [review]
updated patch, move casting to macro - also updated 16bit cast just for kicks

based on Tor's suggestion, I moved the cast to the macro itself.
NOTE: also added cast to 16bit macro just for completeness.

neil, compilers (macosX & hp's aCC) don't complain on the cast back.
Attachment #125309 - Attachment is obsolete: true

Comment 15

15 years ago
Comment on attachment 125321 [details] [diff] [review]
updated patch, move casting to macro - also updated 16bit cast just for kicks

Fun.  sr=tor
Attachment #125321 - Flags: superreview+
Attachment #125321 - Flags: review?(pavlov)

Updated

15 years ago
Attachment #125321 - Flags: review?(pavlov) → review+

Comment 16

15 years ago
Comment on attachment 125321 [details] [diff] [review]
updated patch, move casting to macro - also updated 16bit cast just for kicks

/me wishes that C had JS's >>> operator
(Assignee)

Comment 17

15 years ago
Comment on attachment 125321 [details] [diff] [review]
updated patch, move casting to macro - also updated 16bit cast just for kicks

Lets see if I can get this 
into 1.4 as well.  Since most BMP's are broken on Mac (and big endian
machines).

NOTE: I talked to wanteh and nspr does have this functionality but they are
function calls... so am think we should just stick with this.
Attachment #125321 - Flags: approval1.4?
Comment on attachment 125321 [details] [diff] [review]
updated patch, move casting to macro - also updated 16bit cast just for kicks

a=blizzard for 1.4
Attachment #125321 - Flags: approval1.4? → approval1.4+

Comment 19

15 years ago
nominating nsbeta1
Keywords: nsbeta1
QA Contact: tpreston → petersen

Comment 20

15 years ago
I see this too with the attached BMP. However, I have another BMP used in our
bft tests that doesn't show this problem:

http://mozilla.org/quality/browser/front-end/testcases/imaging/images/Summer.bmp

(Assignee)

Comment 21

15 years ago
Correct, not all BMP's will show it, the width and/or height have to have
the MSB set in the encoded header.  NOTE: these are byte-swapped.
so height = 0x400 - would be encoded as 0004 and is fine
however if height = 0x0190 then it would be encoded as 9001.
So we would byte shift 9001 (which is 1001 0000 0000 0001) right 8 bits.
Since this is *signed* the MSB gets propogated so shift right would
give us 1111 1111 1001 0000 (instead of the *unsigned* value of 
0000 0000 1001 0000)

Comment 22

15 years ago
adt: nsbeta1+/adt2

a=adt to land this on the 1.4 branch.  Please add the fixed1.4 keyword once this
lands.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
(Assignee)

Comment 23

15 years ago
fixed checked into trunk (6/11) and into the branch (6/12)
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.4
Resolution: --- → FIXED

Comment 24

15 years ago
Verified on macho trunk (2003-06-12-09) and branch (2003-06-12-10) builds
Status: RESOLVED → VERIFIED
Keywords: fixed1.4 → verified1.4

Comment 25

15 years ago
Comment on attachment 125309 [details] [diff] [review]
Must shift unsigned ints, so that MSB does not get propagated

removing r/sr requests on obsolete patch
Attachment #125309 - Flags: superreview?(tor)
Attachment #125309 - Flags: review?(paper)
You need to log in before you can comment on or make changes to this bug.