#95 Refactoring of color depth code

closed-accepted
None
5
2006-03-01
2005-12-25
No

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.

Discussion

1 2 > >> (Page 1 of 2)
  • Ilya Konstantinov

    • assigned_to: nobody --> astrand
     
  • Ilya Konstantinov

    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."
    :)

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-05

    Screenshot of distorted graphics against Windows 2000 server

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-05

    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.

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-05

    Valgrind log when running against Windows 2000 server

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-05

    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.

     
  • Ilya Konstantinov

    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.

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-05

    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.

     
  • Ilya Konstantinov

    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')

     
  • Ilya Konstantinov

    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)
    ...

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-08

    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.

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-08

    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.

     
  • Ilya Konstantinov

    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.)

     
  • Ilya Konstantinov

    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.

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-24
    • status: open --> closed-accepted
     
  • Peter Åstrand

    Peter Åstrand - 2006-01-24

    Logged In: YES
    user_id=344921

    The new patch seems to work good, and I have applied it.
    Thanks for your contribution!

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-24

    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?

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-24
    • status: closed-accepted --> open-accepted
     
  • Ilya Konstantinov

    Logged In: YES
    user_id=335423

    You mean colors are displayed INcorrectly?

     
  • Peter Åstrand

    Peter Åstrand - 2006-01-24

    Logged In: YES
    user_id=344921

    >You mean colors are displayed INcorrectly?

    Yes.

     
  • Nobody/Anonymous

    Logged In: NO

    Hi Ilya,

    Thanks for the patch. At least the current cvs checkout is
    working on my SGI Octane2 now.

    --
    Jeremy Meng

     
  • Ilya Konstantinov

    Logged In: YES
    user_id=335423

    Sorry it took me some time to get back to this. Turns out
    g_arch_match also implied an LE host, whereas my
    newly-introduced g_compatible_depth didn't imply it.
    So, there - I've renamed it to g_compatible_arch and made it
    depend on the host being LE.

    The value of the optimization g_compatible_arch triggers is
    questionable anyway so I didn't bother solving it in a
    better way (making the code within the optimization add
    BSWAP if running on BE) -- instead we simply use the
    optimization as little as we previously did.

    P.S. I didn't have a BE machine to test it on, so I
    reverse-tested it on my LE machine :)

     
1 2 > >> (Page 1 of 2)

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks