[PATCH 2/2] glamor: Use float matrix for render transformations

[Siarhei Siamashka]

On Sun, 17 Aug 2014 14:16:26 -0700
Keith Packard <keithp at keithp.com> wrote:

> Now that the core X server exposes pixman_f_transform matrices in each
> picture with a transform, use those instead of the fixed point matrices.
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
>  glamor/glamor_render.c | 17 ++++++-----------
>  glamor/glamor_utils.h  | 20 --------------------
>  2 files changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
> index 2386f2e..03a7b63 100644
> --- a/glamor/glamor_render.c
> +++ b/glamor/glamor_render.c
> @@ -790,7 +790,7 @@ combine_pict_format(PictFormatShort * des, const PictFormatShort src,
>  static void
>  glamor_set_normalize_tcoords_generic(glamor_pixmap_private *priv,
>                                       int repeat_type,
> -                                     float *matrix,
> +                                     double *matrix,
>                                       float xscale, float yscale,
>                                       int x1, int y1, int x2, int y2,
>                                       float *texcoords,
> @@ -1164,8 +1164,7 @@ glamor_composite_with_shader(CARD8 op,
>      int source_x_off, source_y_off;
>      int mask_x_off, mask_y_off;
>      PictFormatShort saved_source_format = 0;
> -    float src_matrix[9], mask_matrix[9];
> -    float *psrc_matrix = NULL, *pmask_matrix = NULL;
> +    double *psrc_matrix = NULL, *pmask_matrix = NULL;
>      int nrect_max;
>      Bool ret = FALSE;
>      glamor_composite_shader *shader = NULL, *shader_ca = NULL;
> @@ -1210,10 +1209,8 @@ glamor_composite_with_shader(CARD8 op,
>          glamor_get_drawable_deltas(source->pDrawable,
>                                     source_pixmap, &source_x_off, &source_y_off);
>          pixmap_priv_get_scale(source_pixmap_priv, &src_xscale, &src_yscale);
> -        if (source->transform) {
> -            psrc_matrix = src_matrix;
> -            glamor_picture_get_matrixf(source, psrc_matrix);
> -        }
> +        if (source->transform)
> +            psrc_matrix = &(PictureTransformFloat(source)->m[0][0]);
>      }
>  
>      if (glamor_priv->has_mask_coords) {
> @@ -1221,10 +1218,8 @@ glamor_composite_with_shader(CARD8 op,
>          glamor_get_drawable_deltas(mask->pDrawable, mask_pixmap,
>                                     &mask_x_off, &mask_y_off);
>          pixmap_priv_get_scale(mask_pixmap_priv, &mask_xscale, &mask_yscale);
> -        if (mask->transform) {
> -            pmask_matrix = mask_matrix;
> -            glamor_picture_get_matrixf(mask, pmask_matrix);
> -        }
> +        if (mask->transform)
> +            pmask_matrix = &(PictureTransformFloat(mask)->m[0][0]);
>      }
>  
>      nrect_max = MIN(nrect, GLAMOR_COMPOSITE_VBO_VERT_CNT / 4);
> diff --git a/glamor/glamor_utils.h b/glamor/glamor_utils.h
> index c15d17c..29f221f 100644
> --- a/glamor/glamor_utils.h
> +++ b/glamor/glamor_utils.h
> @@ -91,26 +91,6 @@
>  	}							\
>     } while(0)
>  
> -#define xFixedToFloat(_val_) ((float)xFixedToInt(_val_)			\
> -			      + ((float)xFixedFrac(_val_) / 65536.0))
> -
> -#define glamor_picture_get_matrixf(_picture_, _matrix_)			\
> -  do {									\
> -    int _i_;								\
> -    if ((_picture_)->transform)						\
> -      {									\
> -	for(_i_ = 0; _i_ < 3; _i_++)					\
> -	  {								\
> -	    (_matrix_)[_i_ * 3 + 0] =					\
> -	      xFixedToFloat((_picture_)->transform->matrix[_i_][0]);	\
> -	    (_matrix_)[_i_ * 3 + 1] =					\
> -	      xFixedToFloat((_picture_)->transform->matrix[_i_][1]);	\
> -	    (_matrix_)[_i_ * 3 + 2] = \
> -	      xFixedToFloat((_picture_)->transform->matrix[_i_][2]);	\
> -	  }								\
> -      }									\
> -  }  while(0)
> -
>  #define fmod(x, w)		(x - w * floor((float)x/w))
>  
>  #define fmodulus(x, w, c)	do {c = fmod(x, w);		\

Okay, now I see what's going on. The old glamor code used to convert
the 16.16 fixed point values to 32-bit floats. This is definitely not
great because of http://en.wikipedia.org/wiki/Rounding#Double_rounding
on the "64-bit float" -> "16.16 fixed point" -> "32-bit float"
conversion path. Directly passing 32-bit floats to XRENDER should
somewhat mitigate the accuracy loss, but not totally eliminate it.

The fundamental problem here is that 16.16 fixed point is exactly
borderline accurate. Any 16.16 fixed point value, with the integer part
larger than 256, is going to lose some fractional bits when converted to
32-bit float (which only has 24-bit mantissa). The 32-bit floating
point format is somewhat less accurate than necessary for XRENDER.

And the introduction of the 32-bit floating point format in the
protocol is going to be really harmful. If the new applications start
using it for the sake of glamor, then the existing users of software
rendering drivers (which used to be perfectly fine until now), are
going to be messed up by the "64-bit float" -> "32-bit float" -> "16.16
fixed point" double rounding penalty. Using the 64-bit floating point
format in the protocol is not so bad, because it can store 16.16 fixed
point values without any accuracy loss. However there would be some
performance penalty, caused by the extra conversion. That's the usual
"pick your poison" situation.

Is the accuracy actually good enough with this patch applied and solves
the "off by several pixels" problem? The patch still looks messed up
with the use of a mix of double and float.

-- 
Best regards,
Siarhei Siamashka