[Xorg-driver-geode] [PATCH 1/5] Correctly calculate the rendering region with the mask picture

[Mart Raudsepp]

Hello,

So some reviewing on the first patch:

On Fri, 2010-07-02 at 15:59 +0800, Huang, FrankR wrote:
> From: Frank Huang <frankr.huang at amd.com>
> 
> *Add a maskflag variable to record if the type is COMP_TYPE_MASK
> *Add the calculation of the rendering region if there is a mask
> picture
> 
> Signed-off-by: Frank Huang <frankr.huang at amd.com>
> ---
>  src/lx_exa.c |   41 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/src/lx_exa.c b/src/lx_exa.c
> index 14980ae..e0def8f 100644
> --- a/src/lx_exa.c
> +++ b/src/lx_exa.c
> @@ -949,6 +949,8 @@ lx_do_composite(PixmapPtr pxDst, int srcX, int srcY, int maskX,
>  
>      unsigned int dstOffset, srcOffset = 0;
>  
> +    int maskflag = 0;
> +

Why do we need a temporary variable for this?

Looks like just checking below (inside the while loop) for
if (exaScratch.type == COMP_TYPE_MASK)
should suffice. Just like is done before entering the loop.

I understand this remained from a now deleted code, so this patch
shouldn't add this unnecessary variable I think.

Of course if this is removed, the commit message line of "Add a
maskflag variable to record if the type is COMP_TYPE_MASK" is redundant
as well.
Also I think the other part would sound better in the log as something
like this:

If the operation is with a shifted position mask, then adjust the
operation region based on the operations mask width and height
parameters (instead of clipping based on source width/height).


>      xPointFixed srcPoint;
>  
>      int opX = dstX;
> @@ -1009,13 +1011,28 @@ lx_do_composite(PixmapPtr pxDst, int srcX, int srcY, int maskX,
>  	srcPoint.y = F(0);
>      }
>  
> +    /* Get the source point offset position */
> +
>      srcOffset = GetSrcOffset(I(srcPoint.x), I(srcPoint.y));
>  
> -    if (exaScratch.srcWidth < opWidth)
> -	opWidth = exaScratch.srcWidth;
> +    /* When mask exists, exaScratch.srcWidth and exaScratch.srcHeight are
> +     * the source width and source height; Otherwise, they are mask width
> +     * and mask height */

Isn't that the other way around? When mask exists, exaScratch.srcWidth
and exaScratch.srcHeight are the mask width and mask height?

Furthermore, I propose that in a later commit we rename these confusing
srcWidth and srcHeight members to something less confusing, that doesn't
make one believe they are always source width/height, but dependent on
the mask existing or not.

> +    /* exaScratch.repeat is the source repeat attribute */
> +    /* If type is COMP_TYPE_MASK, maskX and maskY are not zero, we should
> +     * minus them to do the opeartion in the correct region */

s/minus/subtract/g
s/opeartion/operation/g

Though still a bit confusing wording to me

> -    if (exaScratch.srcHeight < opHeight)
> -	opHeight = exaScratch.srcHeight;
> +    if (exaScratch.type == COMP_TYPE_MASK) {
> +	if ((exaScratch.srcWidth - maskX) < opWidth)
> +	    opWidth = exaScratch.srcWidth - maskX;
> +	if ((exaScratch.srcHeight - maskY) < opHeight)
> +	    opHeight = exaScratch.srcHeight - maskY;

exaScratch.srcWidth and exaScratch.srcHeight are unsigned integers,
whereas maskX and maskY are signed integers.

So if maskX is a bigger number, GCC will be doing unsigned integer math
as the left operand (exaScratch.srcWidth) is unsigned, so if srcWidth is
10 and maskX is 80, it will compare ((unsigned int)10 - (int)80 < 40)
which will be converted by GCC to
((unsigned int)10 - (unsigned int)80 < 40), resulting in:
(4294967226 < 40)

Of course if it did integer math instead, it would end up with opWidth
as a negative number instead, and that would get passed to Cimarron
functions, which take unsigned integers, so the compiler would convert
it to unsigned, resulting in a huge width value being used.

So maybe this logic can be restructured to not issue warnings from a
more verbose compiler:
warning: comparison between signed and unsigned integer expressions

Do you know what should happen if the mask region is completely (or
partially) out of opWidth/opHeight area?

I'm a bit hesitant in us adding this new mishandled case in the code
with this patch. Maybe I'll be happy if a FIXME comment is added, or if
lx_check_composite immediately fallbacks to software if maskX > opWidth
or maskY > opHeight


> +    } else {
> +	if (exaScratch.srcWidth < opWidth)
> +	    opWidth = exaScratch.srcWidth;
> +	if (exaScratch.srcHeight < opHeight)
> +	    opHeight = exaScratch.srcHeight;
> +    }
>  
>      while (1) {
>  
> @@ -1027,6 +1044,7 @@ lx_do_composite(PixmapPtr pxDst, int srcX, int srcY, int maskX,
>  		int direction =
>  		    (opPtr->channel == CIMGP_CHANNEL_A_SOURCE) ? 0 : 1;
>  
> +		maskflag = 1;
>  		if (direction == 1) {
>  		    dstOffset =
>  			GetPixmapOffset(exaScratch.srcPixmap, opX, opY);
> @@ -1067,10 +1085,17 @@ lx_do_composite(PixmapPtr pxDst, int srcX, int srcY, int maskX,
>  		break;
>  	}
>  
> -	opWidth = ((dstX + width) - opX) > exaScratch.srcWidth ?
> -	    exaScratch.srcWidth : (dstX + width) - opX;
> -	opHeight = ((dstY + height) - opY) > exaScratch.srcHeight ?
> -	    exaScratch.srcHeight : (dstY + height) - opY;
> +	if (maskflag == 1) {
> +	    opWidth = ((dstX + width) - opX) > (exaScratch.srcWidth - maskX)
> +		? (exaScratch.srcWidth - maskX) : (dstX + width) - opX;
> +	    opHeight = ((dstY + height) - opY) > (exaScratch.srcHeight - maskY)
> +		? (exaScratch.srcHeight - maskY) : (dstY + height) - opY;
> +	} else {
> +	    opWidth = ((dstX + width) - opX) > exaScratch.srcWidth ?
> +		exaScratch.srcWidth : (dstX + width) - opX;
> +	    opHeight = ((dstY + height) - opY) > exaScratch.srcHeight ?
> +		exaScratch.srcHeight : (dstY + height) - opY;
> +	}
>      }
>  }


Regards,
Mart Raudsepp