Re: winex11.drv: Implement FIXME in AlphaBlend

[Roderick Colenbrander-2]

Hi Peter,

I didn't see this patch before because I was away at that time. The
SourceConstantAlpha part looks correct to me. It might make sense to
calculate blendfn.SourceConstantAlpha / 255 before the loop, so that
it isn't recalculated each time.

The other half of your patch is a separate change and it would require
a separate patch. The part looks suspicious though. I don't understand
why you need special locking there.

Roderick

On Mon, Oct 26, 2009 at 4:39 PM, Peter Urbanec <winehq.org@...> wrote:

>
>
> commit df78de34a06e32be9f37a3b34fe68e5b088a8578
> Author: Peter Urbanec <winehq.org@...>
> Date:   Mon Oct 26 21:29:00 2009 +1100
>
>    Implement source alpha plus SourceConstantAlpha combined transparency.
>    Add DIB locking to prevent node icon rendering errors in Fusion composition flow views.
>
> diff --git a/dlls/winex11.drv/xrender.c b/dlls/winex11.drv/xrender.c
> index 90503bf..26dc262 100644
> --- a/dlls/winex11.drv/xrender.c
> +++ b/dlls/winex11.drv/xrender.c
> @@ -1956,9 +1956,6 @@ BOOL CDECL X11DRV_AlphaBlend(X11DRV_PDEVICE *devDst, INT xDst, INT yDst, INT wid
>         return FALSE;
>     }
>
> -    if ((blendfn.AlphaFormat & AC_SRC_ALPHA) && blendfn.SourceConstantAlpha != 0xff)
> -        FIXME("Ignoring SourceConstantAlpha %d for AC_SRC_ALPHA\n", blendfn.SourceConstantAlpha);
> -
>     if(dib.dsBm.bmBitsPixel != 32) {
>         FIXME("not a 32 bpp dibsection\n");
>         return FALSE;
> @@ -1979,11 +1976,35 @@ BOOL CDECL X11DRV_AlphaBlend(X11DRV_PDEVICE *devDst, INT xDst, INT yDst, INT wid
>
>     if (blendfn.AlphaFormat & AC_SRC_ALPHA)
>     {
> -        for(; y >= y2; y--)
> +        if (blendfn.SourceConstantAlpha == 0xff)
>         {
> -            memcpy(dstbits, (char *)dib.dsBm.bmBits + y * dib.dsBm.bmWidthBytes + xSrc * 4,
> +            for(; y >= y2; y--)
> +            {
> +                memcpy(dstbits, (char *)dib.dsBm.bmBits + y * dib.dsBm.bmWidthBytes + xSrc * 4,
>                    widthSrc * 4);
> -            dstbits += (top_down ? -1 : 1) * widthSrc;
> +                dstbits += (top_down ? -1 : 1) * widthSrc;
> +            }
> +        }
> +        else
> +        {
> +            /* Source alpha plus SourceConstantAlpha */
> +            for(; y >= y2; y--)
> +            {
> +                int x;
> +                DWORD *srcbits = (DWORD *)((char *)dib.dsBm.bmBits + y * dib.dsBm.bmWidthBytes) + xSrc;
> +                for (x = 0; x < widthSrc; x++)
> +                {
> +                    DWORD argb = *srcbits++;
> +                    BYTE *s = (BYTE *) &argb;
> +                    s[0] = (s[0] * blendfn.SourceConstantAlpha) / 255;
> +                    s[1] = (s[1] * blendfn.SourceConstantAlpha) / 255;
> +                    s[2] = (s[2] * blendfn.SourceConstantAlpha) / 255;
> +                    s[3] = (s[3] * blendfn.SourceConstantAlpha) / 255;
> +                    *dstbits++ = argb;
> +                }
> +                if (top_down)  /* we traversed the row forward so we should go back by two rows */
> +                    dstbits -= 2 * widthSrc;
> +            }
>         }
>     }
>     else
> @@ -2006,6 +2027,7 @@ BOOL CDECL X11DRV_AlphaBlend(X11DRV_PDEVICE *devDst, INT xDst, INT yDst, INT wid
>
>     }
>
> +    X11DRV_LockDIBSection( devDst, DIB_Status_GdiMod );
>     dst_pict = get_xrender_picture(devDst);
>
>     wine_tsx11_lock();
> @@ -2017,6 +2039,8 @@ BOOL CDECL X11DRV_AlphaBlend(X11DRV_PDEVICE *devDst, INT xDst, INT yDst, INT wid
>     if(!src_format)
>     {
>         WARN("Unable to find a picture format supporting alpha, make sure X is running at 24-bit\n");
> +        wine_tsx11_unlock();
> +        X11DRV_UnlockDIBSection( devDst, TRUE );
>         return FALSE;
>     }
>
> @@ -2055,6 +2079,7 @@ BOOL CDECL X11DRV_AlphaBlend(X11DRV_PDEVICE *devDst, INT xDst, INT yDst, INT wid
>     XDestroyImage(image);
>
>     wine_tsx11_unlock();
> +    X11DRV_UnlockDIBSection( devDst, TRUE );
>     HeapFree(GetProcessHeap(), 0, data);
>     return TRUE;
>  }
>
>
>
>

[Peter Urbanec]

I've taken the following feedback on board and resubmitted the patch.


Andrew Eikum wrote:
> This is less important, but you're not following the code conventions
> of the surrounding code.  The surrounding code does not have spaces
> around function parameters, so yours shouldn't either.

The existing code uses a mixture of styles in the AlphaBlend function
alone. My original patch did not have spaces around function parameters,
however it did have spaces after C language keywords. The resubmitted
patch follows the style of the code immediately before and after the
patch, which does not have spaces after keywords.



Roderick Colenbrander wrote:
> I didn't see this patch before because I was away at that time. The
> SourceConstantAlpha part looks correct to me. It might make sense to
> calculate blendfn.SourceConstantAlpha / 255 before the loop, so that
> it isn't recalculated each time.

I considered this, but because the alpha is calculated using integer
arithmetic on BYTE values, a pre-calculated blend factor would always
degenerate to the value 0. The alternative is to use floating point
arithmetic for the blend factor calculation, but that would most likely
end up being a performance killer. Hopefully the compiler will do a
reasonable optimising job if it is possible to improve the code.

I guess the code could be hand optimised to use of a vectorised
instruction set where available. However, I think that at this point
providing a concise and readable implementation is probably a more
valuable contribution.


> The other half of your patch is a separate change and it would require
> a separate patch. The part looks suspicious though. I don't understand
> why you need special locking there.

Yes, the locking code is not related to the missing AlphaBlend
functionality. It got rolled into the patch because I was fixing some
other issues at the same time. I have a heavily multithreaded
application and without that locking code I end up with areas of the
screen that are not updated correctly. To be honest, I don't have a very
clear picture of how the various locking mechanisms interact, so it is
feasible that the locking code I added does not address the underlying
problem, but only fixes the symptom as a side effect.

I'd be happy to learn more about the interaction between X11, OpenGL and
wine synchronisation mechanisms - any pointers where to look for
explanations?

Best regards,

    Peter Urbanec