Patch: winex11drv create_cursor Loading 32bit rgba cursor image

[Pauli]

fix for bug 5486

Patch makes it possible to load and display 32bit rgba cursor. I have
made tested the code with Football Manager 2006. The ealier version of
patch was confimed loading 32bit cursors from Guild Wars.

I would like to hear comments how to improve the code. Is there chance
that this could be included to wine? (without huge modifications)

The code is a hack and not perfect solution. But hack is better than
invisible cursor.

[Pauli]

I looked the code bit more. Then I thought I hate if and removed some of
them. Now I think code is cleaner but it has minor performance hit when
cursor don't have alpha. But create_cursor won't be called every frame.

[Mike Hearn]

Heya Pauli,

A review is enclosed. To submit a patch please send it to
the wine-patches list, where it'll enter Alexandres queue for review.

Firstly I'm kind of surprised this doesn't require the XCursor extension.
Handling the case of a solid alpha channel by converting it to a mask is
fine but you should note with a FIXME cases where it doesn't work properly.
> @@ -422,16 +422,18 @@ static Cursor create_cursor( Display *di
>          }
>          else
>          {
> -
>        int     rbits, gbits, bbits, red, green, blue;
> -
>        int     rbits, gbits, bbits,abits, red, green, blue, alpha;

abits doesn't seem to be used anywhere (apart from the assignment in the
32bit case).
> -
>                    alpha = 255;   // This is vissible?

Yes, 255 is visible/max opacity (note spelling). Use C /* */ style
comments, we require this.
> -
>                        if (ptr->bBitsPerPixel == 32)
> -
>                        {
> -
>                            theChar = theImage[byteIndex++];
> -
>                            alpha = theChar;
> -
>                        }
>                        break;

I'm not sure even why "theChar" exists, apart from in one usage it seems
to be worthless. You could just directly assign it here (with the right
cast). Really this whole function could use a bit of spring cleaning.
> -
> -
>                //bg or fg?
> -
>                // If there is alpha for fg color then we should calculate avarange
> -
>                // alpha and use that to create transperancy for cursor (at least my gnome 
> -
>                // has transperancy with xorg 7)

I'm afraid it's really not clear what you're doing here. Standard X
cursors cannot have an alpha channel, support for this was added with the
XRender/XCursor extensions which you don't seem to be using.

Here you seem to have decided that any value larger than 8 for the alpha
channel is solid and anything less is transparent. This makes no sense. A
much simpler way is to just compare against zero and 255, like this:

if (alpha == 0)
  {
       /* Transparent (note spelling) */
       pAndBits[xorIndex] |= bitShifted;
  }
  else if (alpha < 255)
  {
       static BOOL warned = FALSE;

if (!warned)
       {
           FIXME("non-simple alpha channels are currently unsupported, expect visual glitches!\n");
           warned = TRUE;
       }

/* assume we should display partly transparent pixels anyway */
  }

not this ...
> -
>                if (alpha < threshold>>3)
> -
>                {
> -
>                    // Transperant 
> -
>                    pAndBits[xorIndex] |= bitShifted; 
> -
>                } 

The other thing that confuses me about this code is that on 
first inspection it appears to be backwards. Surely if the
pixel needs to be transparent then that bit of pAndBits 
should be zero, which is what it's initialized to, and it 
should be set if the pixel is opaque?

I expect I'm confused, after all you tested this. But I'd like
somebody to explain it ....
> -
>                // What is going on? 
>
> +/* TRACE("Cursor file pixel,

No dead code please. If you found this helpful that's great but don't submit 
code that is commented out.
> -
>        if (!pixmapBits
> -
>            && !pixmapMask)
>        {
>            XFreePixmap( display, pixmapAll );
>            XFreeGC( display, gc );

This isn't strictly correct, as only the last could have failed.
Something like

if (!pixmapBits || !pixmapMask)
{ 
    if (pixmapBits) XFreePixmap( display, pixmapAll );
    etc  ...
}

would be better.
> @@ -556,6 +592,15 @@ static Cursor create_cursor( Display *di
>              /* Put the image */
>              XCopyArea( display, pixmapBits, pixmapAll, gc,
>                         0, 0, xmax, ymax, 0, ptr->nHeight );
> -
> -
>        if (ptr->bBitsPerPixel == 32) +            { +
> // Keep original transperancy if not rgba cursor +
>         XCopyArea( display, pixmapMask, pixmapAll, gc, +
>        0, 0, xmax, ymax, 0, 0); +            }
> -

The comment here is a bit backwards, it implies it's on a 
non-alpha codepath when the reverse is true. It'd be better 
as

/* Apply the alpha channel */

or something like that.

Final note - you are using a separate mask when we already have one 
in this code. Why not merge your new mask with the existing one?

thanks -mike

[Louis Lenders]

Mike Hearn <mike <at> plan99.net> writes:
> Heya Pauli,
>
> A review is enclosed. To submit a patch please send it to
> the wine-patches list, where it'll enter Alexandres queue for review.
>
> Firstly I'm kind of surprised this doesn't require the XCursor extension.
> Handling the case of a solid alpha channel by converting it to a mask is
> fine but you should note with a FIXME cases where it doesn't work properly.

Any updates on this one? I found quite a bit of applications that suffer from
this bug, so i'd be really happy if some kind of fix went into cvs.  Thanks