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

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Simon Fraser [no longer active]]

This image displays reversed (attachment coming).

Comment 1

[Simon Fraser [no longer active]]

Attachment: BMP that displays incorrectly

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... could you attach a screenshot how the attachment appears for you? it
displays correctly here (linux)

Comment 3

[Simon Fraser [no longer active]]

Attachment: Screenshot of BMP display in 2003052307

Comment 4

[[e-mail]]

Displays right-way-up for me too, on W2K and Linux.

Comment 5

[Jim Dunn]

yup on mac I see the problem...

Comment 6

[Jim Dunn]

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

Comment 7

[Simon Fraser [no longer active]]

Are we sure we're building the bmp decoders on Mac, and not falling back on
QuickTime?

Comment 8

[Jim Dunn]

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!?!)

Comment 9

[Jim Dunn]

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

Comment 10

[Jim Dunn]

specifically changes between 1.2.1 & 1.3a.  bug 179296 maybe??? looking

Comment 11

[Jim Dunn]

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

Comment 12

[Jim Dunn]

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.

Comment 13

[[e-mail]]

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?

Comment 14

[Jim Dunn]

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

Comment 15

[tor]

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

Fun.  sr=tor

Comment 16

[[e-mail]]

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

Comment 17

[Jim Dunn]

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.

Comment 18

[Christopher Blizzard (:blizzard)]

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

Comment 19

[Paul Wyskoczka]

nominating nsbeta1

Comment 20

[Chris Petersen]

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

Comment 21

[Jim Dunn]

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

[Samir Gehani]

adt: nsbeta1+/adt2

a=adt to land this on the 1.4 branch.  Please add the fixed1.4 keyword once this
lands.

Comment 23

[Jim Dunn]

fixed checked into trunk (6/11) and into the branch (6/12)

Comment 24

[Chris Petersen]

Verified on macho trunk (2003-06-12-09) and branch (2003-06-12-10) builds

Comment 25

[ArronM (:paper)]

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