Re: winex11.drv: Implement FIXME in AlphaBlend

[Roderick Colenbrander]

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  wrote:
> commit df78de34a06e32be9f37a3b34fe68e5b088a8578
> Author: Peter Urbanec 
> 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;
>  }

winex11.drv: Implement FIXME in AlphaBlend

[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