FW: [Xorg-driver-geode] [PATCH 1/5] Correctly calculatethe rendering region with the mask picture
Huang, FrankR
FrankR.Huang at amd.com
Sun Jul 4 23:53:38 PDT 2010
-----Original Message-----
From: Huang, FrankR
Sent: 2010年7月5日 14:53
To: 'Mart Raudsepp'
Cc: xorg-driver-geode at lists.x.org
Subject: RE: [Xorg-driver-geode] [PATCH 1/5] Correctly calculatethe rendering region with the mask picture
Mart,
Good review. I must say thanks for your every careful thought at each place. That can give me much feedback for my current modification and future improvement based on the patch I release. I must admit that these patch are not accurate at some places. That is definite because you can imagine there is no one line code touch these special conditions before this patch.
Please see my reply below. CC to xorg-devel at lists.x.org for other guys' advice.
By the way, I appreciate your preciseness when reviewing.
Thanks,
Frank
-----Original Message-----
From: xorg-driver-geode-bounces+frankr.huang=amd.com at lists.x.org [mailto:xorg-driver-geode-bounces+frankr.huang=amd.com at lists.x.org] On Behalf Of Mart Raudsepp
Sent: 2010年7月5日 13:14
To: xorg-driver-geode at lists.x.org
Subject: Re: [Xorg-driver-geode] [PATCH 1/5] Correctly calculatethe rendering region with the mask picture
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).
[Frank] Just as you have seen, the variable maskflag is no use in this patch. The condition can be judged by "if (exaScratch.type == COMP_TYPE_MASK).This variable is used in the patch 3. I release a new patch 3 in the noon today. As I explained in IRC, when the OP is PictOpSrc, the region outside of the rendering region should be black which you can get with my application. So there is a trick here to do the black rendering to the region. I simply change the type from COMP_TYPE_MASK to COMP_TYPE_ONEPASS, and op from PictOpSrc to PictOpClear. That is my creation:). So in the next while loop, the exaScratch.type has been changed, but this must be a variable to record this is still a unfinished COM_TYPE_MASK pass. Otherwise, the while loop will be breaked and the black region is not complete.
If you can not understand what I mean, simply do an experiment of replacing it with "if (exaScratch.type == COMP_TYPE_MASK" and see the black region.
I agree with you that with this patch, I delete the maskflag and use "if (exaScratch.type == COMP_TYPE_MASK)" instead. And in patch 3, I add it.
> 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.
[Frank] Good advice. You can get the answer from the lx_prepare_composite() when these two parameters are assigned. These two parameters mislead me at one time same as you. So we can use maskWidth and MaskHeight in the future to record the Mask's Width and Mask's Height if Mask is not zero.
> + /* 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
[Frank] Poor English:)
Accepted.
> - 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
[Frank] Good finding. Actually I met this problem with these parameters' type. I have no idea why the exaScratch.srcWidth and exaScratch.srcHeight are unsigned int! This will introduce much bug here. If you pay attention to my patch 5, I use "if exaScratch.srcWidth < src" instead of "if exaScratch.srcWidth - src < 0". So I totally be confused by this.
I can answer the question when the mask region is out of opWidth/opHeight(width/height) because Janothon has given my answer how to handle this. Even though I havn't add the code, I know how to handle it but not sure. So still I don't add any code yet. See the mail in 6/29/2010 from Janothon. Maybe adding the FIXME is a good choice for future patch.
> + } 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
Xorg-driver-geode at lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-geode
More information about the xorg-devel
mailing list