Hi Everybody,
My name is Tres and I have been invited to join the E/Eterm development
list by MeJ after sending him an email regarding the last patch to
Eterm. The patch is designed to remove that horrible blue acid trip
effect that has plagued some of us. I have left the original email in
tact so that the members of the group can see the whole context of the
discussion. I propose two different solutions to the problem below.
On Mon, 2005-04-25 at 14:26 -0400, Michael Jennings wrote:
> On Saturday, 23 April 2005, at 22:11:32 (-0600),
> Tres Melton wrote:
>
> > I have a couple of questions regarding the last patch
> > (pixmap-colmod.patch). Something about it just seems WRONG!
> >
> > # ifdef WORDS_BIGENDIAN
> > - r = (ptr[x + 1] * rm) >> 8;
> > - g = (ptr[x + 2] * gm) >> 8;
> > - b = (ptr[x + 3] * bm) >> 8;
> > - ptr[x + 1] = r;
> > - ptr[x + 2] = g;
> > - ptr[x + 3] = b;
> > + r = (ptr[x + 6] * rm) >> 8;
> > + g = (ptr[x + 5] * gm) >> 8;
> > + b = (ptr[x + 4] * bm) >> 8;
> > + ptr[x + 6] = r;
> > + ptr[x + 5] = g;
> > + ptr[x + 4] = b;
> > # else
> >
> > This applies to the shade_ximage_32 function inside pixmap.c. By
> > definition we are working with an array of 32 bit pixels (4 bytes).
> > Why then does the patch access 4 or more bytes away from the primary
> > pointer? I have an AMD64 and used to suffer from a horrible blue
> > tinting in my windows; the colmod patch fixed that. The problem is
> > that, the way that I read the patch, it is going to miss the first
> > pixel of the window and overshoot the array by 1 (4 bytes) while
> > dealing with the last pixel.
>
> At first glance, it looks like you're right. But you agree it solves
> the problem, so I have to ask...why does it work? :)
This appears to work because it is an off by one error (one pixel = 4
bytes). It will fail to adjust the top left pixel of the image map and
will go one pixel past the end of the image array at the bottom right.
This is a horrible patch and needs to be reverted immediately. It
should have no effect on my machine (an AMD64) because it is little
endian but it does. It also inverts the order (to little endian) thus
defeating the big endian concept.
> > The next problem that I have is that it applies inside an #ifdef for big
> > endian words. AMD64 is little endian. Source:
> >
> > http://66.102.7.104/search?q=cache:vaJ5wlK10aEJ:www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/26568.pdf+site:amd.com+endian+AMD64&hl=en
> > and
> > http://66.102.7.104/search?q=cache:Zd_0sRa4kGYJ:www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/32035.pdf+site:amd.com+endian+AMD64&hl=en
This is where the problem is.
> > If I use the code snippet from the little endian stuff right below that
> > then that horrible blue acid trip goes away too. I think that the
> > proper fix is to have ./configure correctly determine that AMD64 is
> > little endian and to back out the pixmap-colmod.patch.
>
> If it's detecting AMD64 as big-endian improperly, that sounds like a
> bug in autoconf. Are you aware of a fix for this?
It is properly detecting AMD64 as little endian. What it is doing is
including /usr/include/libast/sysdefs.h and getting hosed in there. If
you look at a piece of sysdefs.h you will find:
#ifndef WORDS_BIGENDIAN
# define WORDS_BIGENDIAN 0
#endif
but in the Eterm code it does things like #ifdef WORDS_BIGENDIAN. Well
WORDS_BIGENDIAN gets defined to a false value but we don't check for
that. We assume that if it is defined then it is true. This is the
heart of the problem. My question is this: Should we remove the code
chunk above from libast/sysdefs.h or should we alter the code in Eterm
to read something like:
#if WORDS_BIGENDIAN
---do something
#endif
Either solution will work but we must be consistent. What decision will
cause the least amount of aggravation? Does something in libast require
that macro to be defined? If nothing depends on it being defined then
we should remove it; if something does require it to be defined then we
will have to alter the code in Eterm.
> > The following code snippet will save three integers on the stack and
> > more importantly allow the compiler to better optimize the instructions
> > as it starts with red and finishes with it before moving on to green.
> > This allows the red values to stay in registers while being manipulated.
> >
> > ptr[x + 2] = (unsigned char) ((ptr[x + 2] * rm) >> 8);
> > ptr[x + 1] = (unsigned char) ((ptr[x + 1] * gm) >> 8);
> > ptr[x + 0] = (unsigned char) ((ptr[x + 0] * bm) >> 8);
>
> Can you send me a patch for this please? You can incorporate your
> corrected AMD64 fix as well if you'd like.
Sure, I can send a patch for that but I would like to straighten out the
endian-ness knot first as the two seem to be somewhat entangled. If you
prefer I will send a patch now but It will most likely have to change
once the endian-ness gets straightened out. Let me know what you want
MeJ.
> > I am the same Tres that submitted the patch to fix the window manager
> > hints for AMD64 through [EMAIL PROTECTED] (SpanKY) last week and have
> > spent quite a bit of time going through the source. I would like to
> > port the MMX extensions to 64 bits (I've already started). I've been on
> > sourceforge.net and everything there seems pretty dated so I went to
> > Eterm's home page and got your email address there. Where are things
> > like this discussed and can I join in. I'm a C/C++/Asm programmer that
> > is a little bit rusty but trying to get back in the game.
>
> We discuss these types of things on the E development list. You can
> subscribe here:
>
> http://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
> The sources you'll want to use are in CVS, info here:
>
> https://sourceforge.net/cvs/?group_id=2
>
> Thanks,
> Michael
>
> --
> Michael Jennings (a.k.a. KainX) http://www.kainx.org/ <[EMAIL PROTECTED]>
> n + 1, Inc., http://www.nplus1.net/ Author, Eterm (www.eterm.org)
> -----------------------------------------------------------------------
> "If you can scrounge up another brain cell, you might captivate us
> further...but I doubt it. You couldn't get a clue during
> clue-mating season in a field full of horny clues if you smeared
> your body with clue musk and did the clue mating dance." -- OS2Bot
Another issue that needs to get resolved in the ./configure script is
that it enables MMX on my AMD64. That's fine, I have MMX, but the code
in mmx_cmod.S is not compatible with a 64 bit chip. The proper thing to
do at the moment is to disable MMX on AMD64. Eterm won't compile
without doing that via "./configure --disable-mmx". That should be
automatic. There is also a HAVE_MMX macro that enables/disables certain
things. I propose adding a HAVE_MMX_64 macro for using the MMX
extensions on AMD64 (and EM64T).
I'm working on other things for Eterm right now including an MMX port to
AMD64. I have been in touch with, Willem-Jan Monsuwe <[EMAIL PROTECTED]>,
the original author of the code and it sounds like he did it just for
fun when MMX first came out and is no longer interested in it. I sent a
preliminary mmx_64_cmod.S file and hope he looks it over. If I don't
hear back from him or he doesn't want to get involved I'll try and
finish the port myself. It has been a while since I've coded in Asm so
it might take some time for me to complete the port.
I have a few other patches that I would like to submit once we untangle
the knot that this endian-ness has caused so could someone please tell
me the best way to handle it: remove the #define from libast or rewrite
the Eterm code?
Thanks all.
--
Tres
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
