GNOME Bugzilla – Bug 71597
gdkpixbuf-drawable.c and big endian
Last modified: 2011-02-04 16:11:59 UTC
This is on a sparc. Looks scary. gdkpixbuf-drawable.c: In function `rgb565amsb': gdkpixbuf-drawable.c:668: warning: assignment from incompatible pointer type gdkpixbuf-drawable.c: In function `rgb555msb': gdkpixbuf-drawable.c:830: warning: assignment from incompatible pointer type gdkpixbuf-drawable.c: In function `rgb888amsb': gdkpixbuf-drawable.c:1147: warning: overflow in implicit constant conversion
Also... gdk-pixbuf-xlib-drawable.c: In function `rgb565amsb': gdk-pixbuf-xlib-drawable.c:550: warning: assignment from incompatible pointer type gdk-pixbuf-xlib-drawable.c: In function `rgb555msb': gdk-pixbuf-xlib-drawable.c:673: warning: assignment from incompatible pointer type gdk-pixbuf-xlib-drawable.c: In function `rgb888amsb': gdk-pixbuf-xlib-drawable.c:932: warning: overflow in implicit constant conversion
A patch would be appreciated. The big-endian cases look borked, but don't look hard to fix.
Any good test case known to exercise this code?
Well, unfortunately, only one of the functions will be excercivesd on any particular visual, so it's a little hard to test, at least without writing a special test framework.
Owen: is it OK if I bump the priority on this so that the wipro people will see it in their normal queries and get someone fixing it?
I think we need some other means of tracking important Solaris bugs than using the Priority field; a keyword, tracking bug or whatever. If we use the priority field for this, then it becomes useless for maintainers. (And what if there are urgent, say, OpenBSD bugs, should they also have a high priority?)
Well, we don't use a keyword to indicate important- in every other product in bugzilla that is part of the gnome core, gnome (including wipro and sun) use 'high' priority for that. That's the case whether or not a bug is important for Linux, Solaris, HPhux, or Tru64, or legOS. So... I don't know what to say, except that by the standards used in everything except gtk right now this would be high priority. You can feel free to ignore that standard if you'd like, but I'm asking you not to in this case. [Especially since you don't appear to be using the field for anything else, AFAICT.]
What I'm saying is that if there were 5, say, gnome-core bugs that were causing big problems on my system, then it would be inappropriate (I feel) for me to go and mark them 'Immediate'. We only have one priority field, and that has to do for everyone. I think the maintainer of the module should be the person who gets to use the field. What I want to do, is, if I have 50 bugs open on 2.0.2, and I think there are 15 of them that I'm going to try hard to get fixed before the 2.0.0 release, is mark those bugs as High. If someone wants to keep a list of bugs they think they should fix, they need to do it in some other way, such as a tracking bug. If a bug is particular to a one platform, and especially to a platform that is not commonly used by GTK+ contributors and users, it doesn't belong at "High" prioritity, IMO, whether it's a Solaris-64bit bug or whatever. High priority bugs are the ones that I think block a particular release; Low priority bugs ones that I strongly suspect will be punted. Now, while it would be lovely if this bug was fixed, I really don't want to mark bugs that I don't have any control over as blocking the next release of GTK+, especailly when they apparently don't affect a large user base. (Bugs that are straightforward to fix, as this one is, and that affect a lot of users, generally have patches submitted for them pretty quickly.) And yes, clearly my ideas about the priority field are different than yours since the fact that there were only three priorities in the original b.g.o setup was intentional and closely correlated to how I imagined the field would be used. I just don't feel that it's at all reasonable that the way of getting someone's attention on a bug that someone may have specialized knowledge of is to mark it with a really high priority so that everybody sees it. Generally, when there is a bug that I think someone knows something about or would be a good candidate to fix, I Cc them on the bug and ask them to look at it. If we have a pool of people at Wipro with specialized knowledge/interest in Solaris, then we need a way of directing interest to those particular bugs without flagging those bugs globally. (That just doesn't scale to bugs of particular interest on HP/UX, bugs of particular interest on Red Hat Linux, etc...) Anyways, this is all quite irrelevant to gdkpixbuf-drawable.c and big endian, so we probably should take it to another forum :-)
Yeah, that's fine... it's definitely best for another forum, and we can respectfully disagree on the rest in the meantime :) I'll see what I can work out with the wipro folks in the meantime.
Move open bugs from milestones 2.0.[012] -- > 2.0.3, since 2.0.2 is already out.
*** Bug 104837 has been marked as a duplicate of this bug. ***
Bug 104837 has a patch for rgb888amsb() - but it's not really right. It doesn't address the fact that the types are just all screwed up for this function in both the big and little endian case. Both big and little endian for that function can be done 32bits at a time. Big endian is: (source << 8) | 0xff little endian is: 0xff000000 | (source >> 8)
Bugsquad marked bug 104837 as major/high because of high visibility in gnome-theme-manager for Gnome 2.2. OS should be set to "all" because this is visible on Linux/powerpc. If no one else is working on this, I can try to produce a patch later this week.
Well, ... sigh Assuming that either the big or least endian case is right ... none of them makes sense. The least endian does reordering of 32bit words (0123 -> 123*) which does not make much sense to me - at least it's looking like endianness (but endianness of a long stored in 4 longs (one long for each byte of the original one). The big endian does set everything to 0xff. LE copies 16bytes per pixel, BE copies 1 byte per pixel ... Well given Owen's explanation I assume that BE is broken, too. Because if you switch LITTLE and !LITTE in the variable declarations/initializations it all makes sense again. (4byte/pixel for both). So assuming that both are broken (confounded) I'll attach a patch which makes this routine work like Owen's explanation. Warning: If I'm correct the routine touches 4 times as much memory as needed on Intel. I'm overwhelmed by the implications of this!!! If ... if both architectures use ARGB and BE needs to convert to BGRA, then more shifting is needed and you're better of with bytewise copying. And last but not least the bug reported by me (gnome-background-properties) does not use this function, but is a separate bug.
Created attachment 14088 [details] [review] Implements Owen's description of this functions ...
My bug (see report on 104837) is triggered by the following parameters: (#define d(x) x) masks = 7c00:3e0:1f image depth = 15, bits per pixel = 16 converting using conversion function in bank 2 converting with index 9 Which IMHO looks like the bug is buried more deeply.
Created attachment 14089 [details] [review] This fixes my bug (16bit display, every two pixels swapped)
Created attachment 14090 [details] [review] fixes blueish theme-manager (rgb555amsb) on BE
Well, found another 16bit/8bit confusion in rgb555amsb (see previous patch). The BE/LE amsb functions seem to suffer from being hastily written/patched. :-( This routine also touches too much memory on intel (IMHO). Strange it ever worked for anybody.
It's quite likely that it didn't ever work for anybody... LSB machines are quite likely to have LSB video displays. [ Quick explanation about gdkrgb888amsb - GdkRGB has a somewhat unusual format - it's always ordered RGBA, regardless of byte order. Video hardware and X server memory will, on the other hand, be typically native endian words ordered xRGB or ARGB. (But it might be reversed from native on a remote display situation.) So, the conversion is from bytes ordered to xRGB to RGBA. On a big endian machine, as a word operation, that's a a shift left by 8, but on a little endian machine, it's a shift right by 8. ]
Can someone with cvs access please _apply_ the three patches, I confirmed their correctness on powerpc with 16 and 24 bit depth (sorry my hardware does not support 32 bit). Do you want to have these bugs lasting forever? Side note: Can someone change the OS to all, it's OS independant - only endianness dependant. I'm on Linux/ppc.
Since the target milestone is 2.2.2, I think this bug will last until 2.2.2 ...
*** Bug 109957 has been marked as a duplicate of this bug. ***
Created attachment 17113 [details] [review] More major rewrite of 565, and 555 functions
After looking at the 565 and 555 functions some, I decided that I needed to do some reasonably extensive rewriting; partly to fix some additional problems I saw, but mostly to have code that is actually understandable. The patch may look intimidating, but if you look at the result, you'll see that the 8 565 and 555 functions are all completely trivial now, and there is just one block bit-shifting macros. I'm committing this to CVS and resolving the bug, but I would definitely appreciate testing. Tue Jun 3 17:39:16 2003 Owen Taylor <otaylor@redhat.com> #71597, reported by Morten Welinder * gdk/gdkpixbuf-drawable.c (rgb888amsb): Fix and simplify (Patch from Christian Petig) * gdk/gdkpixbuf-drawable.c (rgb{555,565}{a,}{msb,lsb}): Major rewrite of 555 and 565 conversion routines: - Move all the bit shifting into a small block of macros, eliminating much duplication of complicated arithmetic. - Get rid of 2-pixels at a time code, which was buggy, hard to maintain, caused unaligned accesses, and probably didn't actually perform any better. - Simplify cases where different data types were used for the little and big endian cases, use GUINT16_SWAP_LE_BE() where appropriate.