409381 - gif images (with transparency) display corrupted

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[philippe (part-time)]

Attachment: Screen shot from bonsai page

Gif images display with bad colour shifts

based on hourly builds:
20071221_0155 build: OK
20071221_0240 build: Bad

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1198230900&maxdate=1198233599

bug 406580

Comment 1

[Reed Loden [:reed]]

Looks fine on Linux.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2007122106 Minefield/3.0b3pre

Comment 2

[philippe (part-time)]

Possibly only PowerPC Mac is affected (I can't test Intel Mac)

Comment 3

[Steve Snyder]

(In reply to comment #2)
> Possibly only PowerPC Mac is affected (I can't test Intel Mac)

Yes, this is certainly an endian issue, caused by the byteswap here:

    http://mxr.mozilla.org/mozilla/source/gfx/thebes/public/gfxColor.h#72

The PPC undoutedly does not need to be swapped prior to OR'ing in the 0xFF alpha value.

Comment 4

[[e-mail]]

Attachment: Color shift on JPEG image screenshot

JPEG images have the same color problem. The attached screenshot is the image at http://www.ju-ju.com/wp/wp-content/themes/mallow/images/banner.jpg

Comment 5

[[e-mail]]

BTW. The above screenshot was taken on a PPC Mac:

Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122104 Minefield/3.0b3pre

Comment 6

[Steve Snyder]

Attn PPC users: Will something like this do?

#if !defined(__ppc__)
#  define GFX_0XFF_PPIXEL_FROM_BPTR(pbptr) \
     (GFX_BYTESWAP32(*((PRUint32 *)(pbptr))) >> 8) | (0xFF << 24)
#else
#  define GFX_0XFF_PPIXEL_FROM_BPTR(pbptr) \
     (*((PRUint32 *)(pbptr)) >> 8) | (0xFF << 24)
#endif /* __ppc__ */

In the case of the PPC (and what about MIPS?) we would read RGBX bytes, where X
is garbage, the first byte of the next pixel.  The valid (non-garbage) 3 bytes
would be shifted down and the 0xFF alpha written to the high byte of the 32-bit
pixel.

Is this enough to fix it, or the alpha actually in the low byte of the 32-bit
pixel?

Comment 7

[Steve Snyder]

(In reply to comment #4)
> Created an attachment (id=294218) [details]
> Color shift on JPEG image screenshot

Yes, and PNG images will show the same problem.

Comment 9

[philippe (part-time)]

(In reply to comment #6)
> Attn PPC users: Will something like this do?
> 
> #if !defined(__ppc__)
> #  define GFX_0XFF_PPIXEL_FROM_BPTR(pbptr) \
>      (GFX_BYTESWAP32(*((PRUint32 *)(pbptr))) >> 8) | (0xFF << 24)
> #else
> #  define GFX_0XFF_PPIXEL_FROM_BPTR(pbptr) \
>      (*((PRUint32 *)(pbptr)) >> 8) | (0xFF << 24)
> #endif /* __ppc__ */
> 

Yeah !  That fixes it here.

Comment 10

[philippe (part-time)]

Attachment: possible patch ?

Code provided by Steve in comment 6 as a patch.

That works fine on my side.
I don't know if this is the correct way to handle this problem, however.

Comment 12

[Steve Snyder]

Attachment: Fix for PPC, with explanatory comment

Keep 32-bit access, but don't swap bytes for PowerPC.

Really needs an endian test, not a CPU arch test, but this will do.

Comment 14

[Colin Barrett [:cbarrett]]

Comment on attachment 294371 [details] [diff] [review]
Fix for PPC, with explanatory comment

I've used the IS_BIG_ENDIAN macro before to do endian-ness before, see: http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsDragService.mm#214

Using _ppc_ is wrong.

Comment 15

[Steve Snyder]

Attachment: Big-endian test, not PPC-specific

Same logic as previous patch, but tests for the definition of IS_BIG_ENDIAN.  More accurately reflects the need for byte-swapping.

I can't test this on my x86 systems, so I'm not requesting a review until I get some feedback from users of big-endian systems.

Comment 16

[philippe (part-time)]

(In reply to comment #15)
> Created an attachment (id=294398) [details]
> Big-endian test, not PPC-specific
> 
> Same logic as previous patch, but tests for the definition of IS_BIG_ENDIAN. 
> More accurately reflects the need for byte-swapping.

Works just fine here.

Comment 17

[Mike Connor [:mconnor]]

As entertaining as this is for about five minutes, we need this fixed ASAP.

Comment 19

[Ray Marotta]

Does anyone know when a possible fix for this bug will come out?
I have not been able to update the code since 12/20....
Using Mac OS X 10.5.1 on PPC Mac

- R.Marotta

Comment 20

[Chris Lawson (gone)]

(In reply to comment #19)
> Does anyone know when a possible fix for this bug will come out?

As soon as attachment 294398 [details] [diff] [review] has review and superreview and someone checks it in.

While you're waiting for that to happen, please familiarise yourself with

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Thanks.

Comment 21

[Justin Dolske [:Dolske]]

Checked in.

Checking in gfx/thebes/public/gfxColor.h;
/cvsroot/mozilla/gfx/thebes/public/gfxColor.h,v  <--  gfxColor.h
new revision: 1.17; previous revision: 1.16

Comment 22

[philippe (part-time)]

verified
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007122415 Minefield/3.0b3pre