glamor: Fix temp picture coordinates in glamor_composite_clipped_region

To understand this patch, let's start at the protocol interface where
the relationship between the coordinate spaces is documented:

        static Bool
        _glamor_composite(CARD8 op,
                          PicturePtr source,
                          PicturePtr mask,
                          PicturePtr dest,
                          INT16 x_source,
                          INT16 y_source,
                          INT16 x_mask,
                          INT16 y_mask,
                          INT16 x_dest, INT16 y_dest,
                          CARD16 width, CARD16 height, Bool fallback)

The coordinates are passed to this function directly off the wire and
are all relative to their respective drawables. For Windows, this means
that they are relative to the upper left corner of the window, in
whatever pixmap that window is getting drawn to.

_glamor_composite calls miComputeCompositeRegion to construct a clipped
region to actually render to. In reality, miComputeCompositeRegion clips
only to the destination these days; source clip region based clipping
would have to respect the transform, which isn't really possible. The
returned region is relative to the screen in which dest lives; offset by
dest->drawable.x and dest->drawable.y.

What is important to realize here is that, because of clipping, the
composite region may not have the same position within the destination
drawable as x_dest, y_dest. The protocol coordinates now exist solely to
'pin' the three objects together.

        extents->x1,y1		Screen origin of clipped operation
        width,height            Extents of the clipped operation
        x_dest,y_dest		Unclipped destination-relative operation coordinate
        x_source,y_source	Unclipped source-relative operation coordinate
        x_mask,y_mask		Unclipped mask-relative operation coordinate

One thing we want to know is what the offset is from the original
operation origin to the clipped origin

        Destination drawable relative coordinates of the clipped operation:

               x_dest_clipped = extents->x1 - dest->drawable.x
               y_dest_clipped = extents->y1 - dest->drawable.y

        Offset from the original operation origin:

                x_off_clipped = x_dest_clipped - x_dest
                y_off_clipped = y_dest_clipped - y_dest

        Source drawable relative coordinates of the clipped operation:

                x_source_clipped = x_source + x_off_clipped;
                y_source_clipped = y_source + y_off_clipped;

        Mask drawable relative coordinates of the clipped operation:

                x_mask_clipped = x_source + x_off_clipped;
                y_mask_clipped = y_source + y_off_clipped;

This is where the original code fails -- it doesn't subtract the
destination drawable location when computing the distance that the
operation has been moved by clipping. Here's what it does when
constructing a temporary source picture:

        temp_src =
            glamor_convert_gradient_picture(screen, source,
                                            extent->x1 + x_source - x_dest,
                                            extent->y1 + y_source - y_dest,
                                            width, height);
        ...
        x_temp_src = -extent->x1 + x_dest;
        y_temp_src = -extent->y1 + y_dest;

glamor_convert_gradient_picture needs source drawable relative
coordinates, but that is not what it's getting; it's getting
screen-relative coordinates for the destination, adjusted by the
distance between the provided source and destination operation
coordinates. We want x_source_clipped and y_source_clipped:

        x_source_clipped = x_source + x_off_clipped
                         = x_source + x_dest_clipped - x_dest
                         = x_source + extents->x1 - dest->drawable.x - x_dest

x_temp_src/y_temp_src are supposed to be the coordinates of the original
operation translated to the temporary picture:

        x_temp_src = x_source - x_source_clipped;
        y_temp_src = y_source - y_source_clipped;

Note that x_source_clipped/y_source_clipped will never be less than
x_source/y_source because all we're doing is clipping. This means that
x_temp_src/y_temp_src will always be non-positive; the original source
coordinate can never be strictly *inside* the temporary image or we
could have made the temporary image smaller.

        x_temp_src = x_source - x_source_clipped
                   = x_source - (x_source + x_off_clipped)
                   = -x_off_clipped
                   = x_dest - x_dest_clipped
                   = x_dest - (extents->x1 - dest->drawable.x)

Again, this is off by the destination origin within the screen
coordinate space.

The code should look like:

        temp_src =
            glamor_convert_gradient_picture(screen, source,
                                            extent->x1 + x_source - x_dest - dest->pDrawable->x,
                                            extent->y1 + y_source - y_dest - dest->pDrawable->y,
                                            width, height);

        x_temp_src = -extent->x1 + x_dest + dest->pDrawable->x;
        y_temp_src = -extent->y1 + y_dest + dest->pDrawable->y;

Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Markus Wick <markus@selfnet.de>

1 files changed, 8 insertions, 8 deletions

diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
index 14ab738eb..e5d5d2cb1 100644
--- a/glamor/glamor_render.c
+++ b/glamor/glamor_render.c
@@ -1450,8 +1450,8 @@ glamor_composite_clipped_region(CARD8 op,
                     || source_pixmap->drawable.height != height)))) {
         temp_src =
             glamor_convert_gradient_picture(screen, source,
-                                            extent->x1 + x_source - x_dest,
-                                            extent->y1 + y_source - y_dest,
+                                            extent->x1 + x_source - x_dest - dest->pDrawable->x,
+                                            extent->y1 + y_source - y_dest - dest->pDrawable->y,
                                             width, height);
         if (!temp_src) {
             temp_src = source;
@@ -1459,8 +1459,8 @@ glamor_composite_clipped_region(CARD8 op,
         }
         temp_src_priv =
             glamor_get_pixmap_private((PixmapPtr) (temp_src->pDrawable));
-        x_temp_src = -extent->x1 + x_dest;
-        y_temp_src = -extent->y1 + y_dest;
+        x_temp_src = -extent->x1 + x_dest + dest->pDrawable->x;
+        y_temp_src = -extent->y1 + y_dest + dest->pDrawable->y;
     }
 
     if (mask
@@ -1474,8 +1474,8 @@ glamor_composite_clipped_region(CARD8 op,
          * to do reduce one convertion. */
         temp_mask =
             glamor_convert_gradient_picture(screen, mask,
-                                            extent->x1 + x_mask - x_dest,
-                                            extent->y1 + y_mask - y_dest,
+                                            extent->x1 + x_mask - x_dest - dest->pDrawable->x,
+                                            extent->y1 + y_mask - y_dest - dest->pDrawable->y,
                                             width, height);
         if (!temp_mask) {
             temp_mask = mask;
@@ -1483,8 +1483,8 @@ glamor_composite_clipped_region(CARD8 op,
         }
         temp_mask_priv =
             glamor_get_pixmap_private((PixmapPtr) (temp_mask->pDrawable));
-        x_temp_mask = -extent->x1 + x_dest;
-        y_temp_mask = -extent->y1 + y_dest;
+        x_temp_mask = -extent->x1 + x_dest + dest->pDrawable->x;
+        y_temp_mask = -extent->y1 + y_dest + dest->pDrawable->y;
     }
     /* Do two-pass PictOpOver componentAlpha, until we enable
      * dual source color blending.