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