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

[walter harms]

Am 18.08.2014 18:31, schrieb 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.
> 

So it would be better to use the 16.16 format for transfer and use double
for internal calculations ?

re,
 wh