From: SourceForge.net <no...@so...> - 2006-01-24 14:35:38
|
Patches item #1390148, was opened at 2005-12-25 19:18 Message generated for change (Comment added) made by astrand You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=381349&aid=1390148&group_id=24366 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: Accepted Priority: 5 Submitted By: Ilya Konstantinov (ikonst) Assigned to: Peter Åstrand (astrand) Summary: Refactoring of color depth code Initial Comment: This patch: 1. Refactors the code which chooses the best visual. The code was changed to ensure a visual which allows bitwise copying of bitmaps from Windows to X pixmaps to be used in preference to all others. Otherwise, it makes sure to chose the best visual, as long as it's not a problematic >24bpp visual. If a TrueColor visual is not available, it picks an 8-plane 256-colormap-entries PseudoColor visual, whcih is the only pseudocolor (colormap'd) visual we support. All in all, we support 8, 15, 16 and 24 color depths. In all other cases, we'll avoid running at all, presenting an intelligible error message (instead of being unable to draw and just displaying a blank window). 2. Rephrases error messages to be more precise. 3. Makes a clear distinction between the terms "depth" and "bpp": depth is the meaningful bits, bpp is the actual bits per pixel, including alignment. The patch makes sure the correct terms are used throughout variable names and messages. 4. Eliminates some compilation warnings along the way :) The patch was tested on: - 24-plane R8G8B8 visual with 32bpp pixmaps - 16-plane R5G6B5 visual with 16bpp pixmaps - 15-plane R5G5B5 visual with 16bpp pixmaps - 8-plane 256-colormap-size visual Whenever available, I made sure to test the optimized (no-bitmap-translation) code path. ---------------------------------------------------------------------- >Comment By: Peter Åstrand (astrand) Date: 2006-01-24 15:35 Message: Logged In: YES user_id=344921 >You mean colors are displayed INcorrectly? Yes. ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-24 15:11 Message: Logged In: YES user_id=335423 You mean colors are displayed INcorrectly? ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-24 14:44 Message: Logged In: YES user_id=344921 I've just discovered that with this patch, colors are now displayed correctly when running rdesktop on Solaris, using this configuration: RDP depth: 16, display depth: 24, display bpp: 32, X server BE: 0, host BE: 1 Can you take a look? ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-24 13:45 Message: Logged In: YES user_id=344921 The new patch seems to work good, and I have applied it. Thanks for your contribution! ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-10 21:27 Message: Logged In: YES user_id=335423 The new patch (rdesktop-visuals-take2.patch): 1. Fixes the issue described by Peter. 2. Separates the issue of bitmap format optimization into two things: 1. RDP depth and X11 Visual depth match in format so we can do direct bitmap copies 2. RDP depth is not the same but the X11 Visual depth has the same color masks as RDP has at that depth. This brings in some optimization into the bitmap translation. ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-08 20:06 Message: Logged In: YES user_id=335423 Just tried exactly that: cvs update -C apply your patch build ./rdesktop win2kserver -a 16 WARNING: Remote desktop does not support colour depth 16; falling back to 8 and no display problems. It's just as usual. The valgrind log is clean as well. Checked both on my x86 and x86-64 machines so far, and all seems fair and square. Are you sure you didn't have some experimental code in your run? What video driver? (I tried NVidia's non-free one and Radeon's free one.) ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-08 18:50 Message: Logged In: YES user_id=344921 Re display corruption: The problem only happens when falling back from depth 16 to 8: $ ./rdesktop brie -a 16 WARNING: Remote desktop does not support colour depth 16; falling back to 8 The server is Windows 2000. ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-08 18:41 Message: Logged In: YES user_id=344921 >What's the endianess of your client machine? What's the >endianess of your X server? LSBFirst on both. ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-07 17:45 Message: Logged In: YES user_id=335423 I'm also unable to reproduce the same error (8bpp Win2K on 16bpp display) with Valgrind's memcheck. The closest I've got seems to be an error within Xlib itself: ==14696== Syscall param writev(vector[...]) points to uninitialised byte(s) ==14696== at 0x1BBB81C3: writev (in /lib/tls/libc-2.3.5.so) ==14696== by 0x1BA76616: _X11TransSocketWritev (Xtranssock.c:2147) ==14696== by 0x1BA768C6: _X11TransWritev (Xtrans.c:907) ==14696== by 0x1BA5AFFC: _XSend (XlibInt.c:1411) ==14696== by 0x1BA4EA6B: PutSubImage (PutImage.c:838) ... ==14696== Address 0x1BF6436A is 834 bytes inside a block of size 201476 alloc'd ==14696== at 0x1B90459D: malloc (vg_replace_malloc.c:130) ==14696== by 0x1BA5C077: _XAllocScratch (XlibInt.c:2923) ==14696== by 0x1BA4EDE2: PutSubImage (PutImage.c:811) ==14696== by 0x1BA4F1D2: XPutImage (PutImage.c:1024) ==14696== by 0x8054278: ui_desktop_restore (xwin.c:2697) ... ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-07 17:34 Message: Logged In: YES user_id=335423 Re display corruption of 8bpp RDP on 16bpp display, I'm unable to reproduce it. What's the endianess of your client machine? What's the endianess of your X server? (Can be checked with xdpyinfo | grep 'image byte order') ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-05 23:36 Message: Logged In: YES user_id=344921 What I mean is that we should perhaps change the default value for g_server_bpp (or g_server_depth as it's called in patch) to a higher value than 8. Currently, users are regularly doing "rdesktop -a 24", because very seldom you actually want depth 8 even if the server supports more. ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-05 14:18 Message: Logged In: YES user_id=335423 Peter, thanks for the response. I'll apply your fixed patch to my code, then proceed to check out your 8bit-on-16bit issue and will Valgrind the code. As to depth 8, how come? We first try TrueColor visuals, finding one with the best characteristics (but nothing >24, since that spells truble). Then, if that fails, we take a PseudoColor 8 depth visual. ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-05 13:38 Message: Logged In: YES user_id=344921 The attached patch is slightly modified: * All the visual select stuff has been lifted out to a separate function. ui_init became very long... * Indent fixes, by running indent all. I'm also wondering: rdesktop is currently using depth 8 as default. Does this really make sense? I suggest that we change this to 16, 24 or 32, if the fallback mechanism works correctly. ---------------------------------------------------------------------- Comment By: Peter Åstrand (astrand) Date: 2006-01-05 13:34 Message: Logged In: YES user_id=344921 With this patch, the graphic is distorted when running on a 16-bit display against a server with 8bpp. Also, valgrind reports errors in this case. The attached screenshot shows the problem. ---------------------------------------------------------------------- Comment By: Ilya Konstantinov (ikonst) Date: 2006-01-03 17:03 Message: Logged In: YES user_id=335423 Peter, to quote you: "If you are able to come up with a patch that solves real problems and that makes, I think we should apply it." :) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=381349&aid=1390148&group_id=24366 |