Hello, So some reviewing on the first patch:
On Fri, 2010-07-02 at 15:59 +0800, Huang, FrankR wrote: > From: Frank Huang <[email protected]> > > *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 <[email protected]> > --- > 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 _______________________________________________ Xorg-driver-geode mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-driver-geode
