[PATCH 2/2] fb: Adjust transform or composite coordinates for pixman operations

Jeremy Huddleston jeremyhu at freedesktop.org
Fri Jan 8 12:31:35 PST 2010


I'm trying to disect this commit to see why it's causing problems for Peter Dyballa (his emacs buffers are getting "cut off" ... see his original "xorg-server 1.7.99.2 is faulty" thread).  I'm not too familiar with pixman and this area of the server, so hopefully someone else (Keith?) can respond to this...

I think the problem may lie in the fact that some variables are uninitialized:

eg:

> +    int image_xoff, image_yoff;
> +    pixman_image_t *image = image_from_pict (pPicture, FALSE, &image_xoff, &image_yoff);


which then does:

> image = create_bits_picture (pict, has_clip, xoff, yoff);

which then does:

> fbGetDrawablePixmap (pict->pDrawable, pixmap, *xoff, *yoff);


even though *xoff and *yoff are garbage values.



On Dec 2, 2009, at 16:25, Keith Packard wrote:

> 
> Sorry for replying to myself again -- I meant to describe the effect of
> this patch on source operands.
> 
> What this does is use the entire pixmap as the source operand and then
> adjust either the source transform (if present) or source coordinates to
> fetch pixels from the right part of that pixmap.
> 
> So, this gets the right pixels for source coordinates within the source
> window. What it doesn't do is get the right pixels for anything
> else. None of the repeat modes work, and pixels falling outside of the
> window, but inside the pixmap will get whatever was in the pixmap at
> that location.
> 
> To make the repeat modes work, we'd need to provide a rectangle
> describing the projection of the window into the pixman_image, then
> replace all of the existing pixman_image-based clipping with clipping
> based on the window. This can be done in a backwards compatible
> fashion by setting the clipping to the bounds of the pixman_image by
> default and exposing a new API for source clipping.
> 
> We could also make source clipping work by performing point-in-region
> computations for every source pixel fetch; one suspects that might be a
> bit too expensive for most applications though.
> 



On Dec 2, 2009, at 16:05, Keith Packard wrote:

> Windows (or even pixmaps, in some cases) may not sit at the origin of
> the containing pixmap, so any coordinates relative to the drawable
> must be adjusted. For destinations and untransformed sources, the
> operation coordinates are adjusted. For transformed sources, the
> transform matrix is adjusted.
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> fb/fb.h     |    7 ++++-
> fb/fbpict.c |   71 ++++++++++++++++++++++++++++++++++++++++------------------
> fb/fbtrap.c |    6 +++-
> 3 files changed, 58 insertions(+), 26 deletions(-)
> 
> diff --git a/fb/fb.h b/fb/fb.h
> index ed21f9e..02d6c03 100644
> --- a/fb/fb.h
> +++ b/fb/fb.h
> @@ -2082,8 +2082,11 @@ fbFillRegionSolid (DrawablePtr	pDrawable,
> 		   FbBits	xor);
> 
> extern _X_EXPORT pixman_image_t *
> -image_from_pict (PicturePtr pict,
> -		 Bool       has_clip);
> +image_from_pict (PicturePtr	pict,
> +		 Bool		has_clip,
> +		 int		*xoff,
> +		 int		*yoff);
> +
> extern _X_EXPORT void free_pixman_pict (PicturePtr, pixman_image_t *);
> 
> #endif /* _FB_H_ */
> diff --git a/fb/fbpict.c b/fb/fbpict.c
> index 07a2286..251754b 100644
> --- a/fb/fbpict.c
> +++ b/fb/fbpict.c
> @@ -158,19 +158,24 @@ fbComposite (CARD8      op,
> 	     CARD16     height)
> {
>    pixman_image_t *src, *mask, *dest;
> +    int src_xoff, src_yoff;
> +    int msk_xoff, msk_yoff;
> +    int dst_xoff, dst_yoff;
> 
>    miCompositeSourceValidate (pSrc, xSrc - xDst, ySrc - yDst, width, height);
>    if (pMask)
> 	miCompositeSourceValidate (pMask, xMask - xDst, yMask - yDst, width, height);
> 
> -    src = image_from_pict (pSrc, TRUE);
> -    mask = image_from_pict (pMask, TRUE);
> -    dest = image_from_pict (pDst, TRUE);
> +    src = image_from_pict (pSrc, FALSE, &src_xoff, &src_yoff);
> +    mask = image_from_pict (pMask, FALSE, &msk_xoff, &msk_yoff);
> +    dest = image_from_pict (pDst, TRUE, &dst_xoff, &dst_yoff);
> 
>    if (src && dest && !(pMask && !mask))
>    {
> 	pixman_image_composite (op, src, mask, dest,
> -				xSrc, ySrc, xMask, yMask, xDst, yDst,
> +				xSrc + src_xoff, ySrc + src_yoff,
> +				xMask + msk_xoff, yMask + msk_yoff,
> +				xDst + dst_xoff, yDst + dst_yoff,
> 				width, height);
>    }
> 
> @@ -270,22 +275,22 @@ create_conical_gradient_image (PictGradient *gradient)
> 
> static pixman_image_t *
> create_bits_picture (PicturePtr pict,
> -		     Bool       has_clip)
> +		     Bool       has_clip,
> +		     int	*xoff,
> +		     int	*yoff)
> {
> +    PixmapPtr pixmap;
>    FbBits *bits;
>    FbStride stride;
> -    int bpp, xoff, yoff;
> +    int bpp;
>    pixman_image_t *image;
> 
> -    fbGetDrawable (pict->pDrawable, bits, stride, bpp, xoff, yoff);
> -
> -    bits = (FbBits*)((CARD8*)bits +
> -		     (pict->pDrawable->y + yoff) * stride * sizeof(FbBits) +
> -		     (pict->pDrawable->x + xoff) * (bpp / 8));
> +    fbGetDrawablePixmap (pict->pDrawable, pixmap, *xoff, *yoff);
> +    fbGetPixmapBitsData(pixmap, bits, stride, bpp);
> 
>    image = pixman_image_create_bits (
> 	pict->format,
> -	pict->pDrawable->width, pict->pDrawable->height,
> +	pixmap->drawable.width, pixmap->drawable.height,
> 	(uint32_t *)bits, stride * sizeof (FbStride));
> 
> 
> @@ -311,30 +316,52 @@ create_bits_picture (PicturePtr pict,
> 	if (pict->clientClipType != CT_NONE)
> 	    pixman_image_set_has_client_clip (image, TRUE);
> 
> -	pixman_region_translate (pict->pCompositeClip, - pict->pDrawable->x, - pict->pDrawable->y);
> +	if (*xoff || *yoff)
> +	    pixman_region_translate (pict->pCompositeClip, *xoff, *yoff);
> 
> 	pixman_image_set_clip_region (image, pict->pCompositeClip);
> 
> -	pixman_region_translate (pict->pCompositeClip, pict->pDrawable->x, pict->pDrawable->y);
> +	if (*xoff || *yoff)
> +	    pixman_region_translate (pict->pCompositeClip, -*xoff, -*yoff);
>    }
> 
>    /* Indexed table */
>    if (pict->pFormat->index.devPrivate)
> 	pixman_image_set_indexed (image, pict->pFormat->index.devPrivate);
> 
> +    /* Add in drawable origin to position within the image */
> +    *xoff += pict->pDrawable->x;
> +    *yoff += pict->pDrawable->y;
> +
>    return image;
> }
> 
> static void
> -set_image_properties (pixman_image_t *image, PicturePtr pict)
> +set_image_properties (pixman_image_t *image, PicturePtr pict, Bool has_clip, int *xoff, int *yoff)
> {
>    pixman_repeat_t repeat;
>    pixman_filter_t filter;
> 
>    if (pict->transform)
>    {
> -	pixman_image_set_transform (
> -	    image, (pixman_transform_t *)pict->transform);
> +	/* For source images, adjust the transform to account
> +	 * for the drawable offset within the pixman image,
> +	 * then set the offset to 0 as it will be used
> +	 * to compute positions within the transformed image.
> +	 */
> +	if (!has_clip) {
> +	    struct pixman_transform	adjusted;
> +
> +	    adjusted = *pict->transform;
> +	    pixman_transform_translate(&adjusted,
> +				       NULL,
> +				       pixman_int_to_fixed(*xoff),
> +				       pixman_int_to_fixed(*yoff));
> +	    pixman_image_set_transform (image, &adjusted);
> +	    *xoff = 0;
> +	    *yoff = 0;
> +	} else
> +	    pixman_image_set_transform (image, pict->transform);
>    }
> 
>    switch (pict->repeatType)
> @@ -361,7 +388,8 @@ set_image_properties (pixman_image_t *image, PicturePtr pict)
> 
>    if (pict->alphaMap)
>    {
> -	pixman_image_t *alpha_map = image_from_pict (pict->alphaMap, TRUE);
> +	int alpha_xoff, alpha_yoff;
> +	pixman_image_t *alpha_map = image_from_pict (pict->alphaMap, FALSE, &alpha_xoff, &alpha_yoff);
> 	
> 	pixman_image_set_alpha_map (
> 	    image, alpha_map, pict->alphaOrigin.x, pict->alphaOrigin.y);
> @@ -394,8 +422,7 @@ set_image_properties (pixman_image_t *image, PicturePtr pict)
> }
> 
> pixman_image_t *
> -image_from_pict (PicturePtr pict,
> -		 Bool has_clip)
> +image_from_pict (PicturePtr pict, Bool has_clip, int *xoff, int *yoff)
> {
>    pixman_image_t *image = NULL;
> 
> @@ -404,7 +431,7 @@ image_from_pict (PicturePtr pict,
> 
>    if (pict->pDrawable)
>    {
> -	image = create_bits_picture (pict, has_clip);
> +	image = create_bits_picture (pict, has_clip, xoff, yoff);
>    }
>    else if (pict->pSourcePict)
>    {
> @@ -428,7 +455,7 @@ image_from_pict (PicturePtr pict,
>    }
> 
>    if (image)
> -	set_image_properties (image, pict);
> +	set_image_properties (image, pict, has_clip, xoff, yoff);
> 
>    return image;
> }
> diff --git a/fb/fbtrap.c b/fb/fbtrap.c
> index 830603a..515e2e1 100644
> --- a/fb/fbtrap.c
> +++ b/fb/fbtrap.c
> @@ -40,7 +40,8 @@ fbAddTraps (PicturePtr	pPicture,
> 	    int		ntrap,
> 	    xTrap	*traps)
> {
> -    pixman_image_t *image = image_from_pict (pPicture, FALSE);
> +    int image_xoff, image_yoff;
> +    pixman_image_t *image = image_from_pict (pPicture, FALSE, &image_xoff, &image_yoff);
> 
>    if (!image)
> 	return;
> @@ -56,7 +57,8 @@ fbRasterizeTrapezoid (PicturePtr    pPicture,
> 		      int	    x_off,
> 		      int	    y_off)
> {
> -    pixman_image_t *image = image_from_pict (pPicture, FALSE);
> +    int	mask_xoff, mask_yoff;
> +    pixman_image_t *image = image_from_pict (pPicture, FALSE, &mask_xoff, &mask_yoff);
> 
>    if (!image)
> 	return;
> -- 
> 1.6.5.3
> 
> _______________________________________________
> xorg-devel mailing list
> xorg-devel at lists.x.org
> http://lists.x.org/mailman/listinfo/xorg-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5820 bytes
Desc: not available
Url : http://lists.x.org/archives/xorg-devel/attachments/20100108/7e9954fd/attachment.bin 


More information about the xorg-devel mailing list