Re: Buglet in gdk-pixbuf

[Owen Taylor <otaylor redhat com>]

degger fhm edu writes:

> Hija,
> 
> I found a problem that looks pretty odd to me but I also
> have trouble understanding what the function is supposed
> to do so please help if you can.
> 
> In 
> static void
> rgb888amsb (GdkImage    *image,
>             guchar      *pixels,
>             int          rowstride,
>             int          x1,
>             int          y1,
>             int          x2,
>             int          y2,
>             GdkColormap *colormap)
> which can be found in gdk/gdkpixbuf-drawable.c:1102
> 
> there's a line (1147)
>  *o++ = (*s << 8) | 0xff; /* untested */
> which gives the compiler warning:
> gdkpixbuf-drawable.c:1147: warning: overflow in implicit constant conversion
> 
> which is obviously correct since both o and s are guint8's. Also what
> value would one expect from left-shifting a guint8 value by 8 and then 
> or-ing with 0xff except than 0xff? 
> Could it be that o and s are supposed to be an guint32 thusly working on
> the full RGBA at once by left-shifting all by one component and setting
> A to 255?

The code here looks majorly screwed up for both big and little endian
cases; it looks like the choice of guint32 and guint8 variables is
backwards for the two cases.

What I assume this code is supposed to do is convert MSB
xRGB data to MSB RGBA data. If we load these as 32 bit quantities,
we have, xRGB and RGBA on a MSB machine, so (*s << 8) | 0xff is
correct. On a small endian machine we have BGRx and ABGR so
(*s >> 8) | 0xff000000 would be right.

Or, we could do it, in either case, as it is done for the little
endian case

#ifdef LITTLE
	  *o++ = s[1];
	  *o++ = s[2];
	  *o++ = s[3];
	  *o++ = 0xff;
	  s += 4;
#endif

But only of o and s are guint8 quantities, not guint32 quantities
as they are in the code!

I would expect the 32 bits at a time formulation to be a fair
bit faster. I haven't checked the code to see if it is actually
true that we are always word-aligned here. If we aren't always
word-aligned, then we need fallbacks to the byte-by-byte case.

Regards,
                                        Owen