gif images (with transparency) display corrupted

VERIFIED FIXED in mozilla1.9beta3

Status

()

Core
Graphics
P1
major
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: philippe (part-time), Assigned: Steve Snyder)

Tracking

({regression})

Trunk
mozilla1.9beta3
PowerPC
Mac OS X
regression
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 294200 [details]
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
Flags: blocking1.9?
Looks fine on Linux.

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

Comment 2

11 years ago
Possibly only PowerPC Mac is affected (I can't test Intel Mac)
(Assignee)

Comment 3

11 years ago
(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

11 years ago
Created attachment 294218 [details]
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

11 years ago
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
(Assignee)

Comment 6

11 years ago
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?
(Assignee)

Comment 7

11 years ago
(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.
(Reporter)

Updated

11 years ago
Duplicate of this bug: 409483
(Reporter)

Comment 9

11 years ago
(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.
(Reporter)

Comment 10

11 years ago
Created attachment 294355 [details] [diff] [review]
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.
Attachment #294355 - Flags: review?(swsnyder)

Updated

11 years ago
Duplicate of this bug: 409529
(Assignee)

Comment 12

11 years ago
Created attachment 294371 [details] [diff] [review]
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.
Assignee: nobody → swsnyder
Attachment #294355 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #294371 - Flags: review+
Attachment #294355 - Flags: review?(swsnyder)
Attachment #294371 - Flags: superreview?(pavlov)
Attachment #294371 - Flags: review?(pavlov)
Attachment #294371 - Flags: review+
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.
(Assignee)

Comment 15

11 years ago
Created attachment 294398 [details] [diff] [review]
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.
Attachment #294398 - Flags: superreview?(pavlov)
Attachment #294398 - Flags: review?(pavlov)
Attachment #294371 - Attachment is obsolete: true
Attachment #294371 - Flags: superreview?(pavlov)
Attachment #294371 - Flags: review?(pavlov)
(Reporter)

Comment 16

11 years ago
(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.
As entertaining as this is for about five minutes, we need this fixed ASAP.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1

Updated

11 years ago
Duplicate of this bug: 409591

Comment 19

11 years ago
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

11 years ago
(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.
Attachment #294398 - Flags: superreview?(pavlov)
Attachment #294398 - Flags: superreview+
Attachment #294398 - Flags: review?(pavlov)
Attachment #294398 - Flags: review+

Updated

11 years ago
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
(Reporter)

Comment 22

11 years ago
verified
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007122415 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.