-----Original Message-----
From: Huang, FrankR
Sent: 2010年7月5日 14:53
To: 'Mart Raudsepp'
Cc: [email protected]
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 [email protected] for other guys'
advice.
By the way, I appreciate your preciseness when reviewing.
Thanks,
Frank
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf
Of Mart Raudsepp
Sent: 2010年7月5日 13:14
To: [email protected]
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 <[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).
[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
[email protected]
http://lists.x.org/mailman/listinfo/xorg-driver-geode
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
