After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 71597 - gdkpixbuf-drawable.c and big endian
gdkpixbuf-drawable.c and big endian
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: X11
1.3.x
Other All
: High normal
: ---
Assigned To: gtk-bugs
gtk-bugs
AES[DUPSINK]
: 104837 109957 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2002-02-15 15:14 UTC by Morten Welinder
Modified: 2011-02-04 16:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implements Owen's description of this functions ... (1.09 KB, patch)
2003-02-04 08:27 UTC, Christof Petig
none Details | Review
This fixes my bug (16bit display, every two pixels swapped) (1.14 KB, patch)
2003-02-04 09:25 UTC, Christof Petig
none Details | Review
fixes blueish theme-manager (rgb555amsb) on BE (599 bytes, patch)
2003-02-04 09:45 UTC, Christof Petig
none Details | Review
More major rewrite of 565, and 555 functions (21.04 KB, patch)
2003-06-03 22:05 UTC, Owen Taylor
none Details | Review

Description Morten Welinder 2002-02-15 15:14:10 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
Comment 1 Morten Welinder 2002-02-15 15:19:18 UTC
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

Comment 2 Owen Taylor 2002-02-15 15:24:53 UTC
A patch would be appreciated. The big-endian cases
look borked, but don't look hard to fix.
Comment 3 Morten Welinder 2002-02-15 15:37:37 UTC
Any good test case known to exercise this code?
Comment 4 Owen Taylor 2002-02-19 12:34:55 UTC
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.
Comment 5 Luis Villa 2002-03-20 22:22:11 UTC
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?
Comment 6 Owen Taylor 2002-03-25 23:36:43 UTC
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?)
Comment 7 Luis Villa 2002-03-29 21:19:25 UTC
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.]
Comment 8 Owen Taylor 2002-03-29 22:15:56 UTC
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 :-)
Comment 9 Luis Villa 2002-03-29 22:24:46 UTC
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.
Comment 10 Matthias Clasen 2002-04-05 13:33:04 UTC
Move open bugs from milestones 2.0.[012] -- > 2.0.3, since 2.0.2 is already out.
Comment 11 Owen Taylor 2003-02-03 02:40:36 UTC
*** Bug 104837 has been marked as a duplicate of this bug. ***
Comment 12 Owen Taylor 2003-02-03 03:53:50 UTC
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) 
Comment 13 Matt Brubeck 2003-02-03 17:36:06 UTC
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.
Comment 14 Christof Petig 2003-02-04 08:24:22 UTC
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.
Comment 15 Christof Petig 2003-02-04 08:27:49 UTC
Created attachment 14088 [details] [review]
Implements Owen's description of this functions ...
Comment 16 Christof Petig 2003-02-04 08:36:13 UTC
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.
Comment 17 Christof Petig 2003-02-04 09:25:53 UTC
Created attachment 14089 [details] [review]
This fixes my bug (16bit display, every two pixels swapped)
Comment 18 Christof Petig 2003-02-04 09:45:00 UTC
Created attachment 14090 [details] [review]
fixes blueish theme-manager (rgb555amsb) on BE
Comment 19 Christof Petig 2003-02-04 09:49:55 UTC
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.
Comment 20 Owen Taylor 2003-02-04 17:11:03 UTC
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. ]
Comment 21 Christof Petig 2003-03-05 11:36:21 UTC
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.
Comment 22 Matthias Clasen 2003-03-05 11:43:58 UTC
Since the target milestone is 2.2.2, I think this bug will last until 2.2.2 ...
Comment 23 Andrew Sobala 2003-04-05 10:09:33 UTC
*** Bug 109957 has been marked as a duplicate of this bug. ***
Comment 24 Owen Taylor 2003-06-03 22:05:35 UTC
Created attachment 17113 [details] [review]
More major rewrite of 565, and 555 functions
Comment 25 Owen Taylor 2003-06-03 22:09:38 UTC
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.