[2/2] pixman: Use maximum precision for pixman-bits-image

Submitted by Maarten Lankhorst on Jan. 7, 2019, 12:22 p.m.

Details

Message ID 29cce759-6d79-9756-d706-09d5e45a11ae@linux.intel.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Maarten Lankhorst Jan. 7, 2019, 12:22 p.m.
Op 04-01-2019 om 18:53 schreef Basile Clement:
> I am far from qualified for doing a proper review on this, but this
> looks reasonable overall.  I only have a few concerns outlined below.
>
> Is there a way to visually test this?  Adding an example in demos/ would
> be great, and would help ensuring there are no scaling issues in the
> conv/sepconv filters (see inline comments below).
>
> I am also a bit concerned by the duplication in the convolution and
> separable_convolution functions.  I understand why it's there, but
> (unlike the bilinear case) those functions are quite large by themselves
> already.  If not too complex, I'd merge them by using type-erased
> `accumulate(int *satot, int *srtot, int *sgtot, int *sbtot, void *pixel,
> pixman_fixed_t params)` and `reduce(int *satot, int *srtot, int *sgtot,
> int *sbtot, void *out)` argument functions.  They should get inlined by
> the compiler anyways.

Managed to make the scale one use the float paths. :)


---------------

Didn't notice anything wrong, so all looks correct with at least simple convolution.

> - Basile
>
> On 1/3/19 12:18 PM, Maarten Lankhorst wrote:
>> +static force_inline void
>> +bits_image_fetch_pixel_convolution_float (bits_image_t   *image,
>> +					  pixman_fixed_t  x,
>> +					  pixman_fixed_t  y,
>> +					  get_pixel_t     get_pixel,
>> +					  void	      *out)
>> +{
>> +    pixman_fixed_t *params = image->common.filter_params;
>> +    int x_off = (params[0] - pixman_fixed_1) >> 1;
>> +    int y_off = (params[1] - pixman_fixed_1) >> 1;
>> +    int32_t cwidth = pixman_fixed_to_int (params[0]);
>> +    int32_t cheight = pixman_fixed_to_int (params[1]);
>> +    int32_t i, j, x1, x2, y1, y2;
>> +    pixman_repeat_t repeat_mode = image->common.repeat;
>> +    int width = image->width;
>> +    int height = image->height;
>> +    int srtot, sgtot, sbtot, satot;
>> +    argb_t *ret = out;
>> +
>> +    params += 2;
>> +
>> +    x1 = pixman_fixed_to_int (x - pixman_fixed_e - x_off);
>> +    y1 = pixman_fixed_to_int (y - pixman_fixed_e - y_off);
>> +    x2 = x1 + cwidth;
>> +    y2 = y1 + cheight;
>> +
>> +    srtot = sgtot = sbtot = satot = 0;
>> +
>> +    for (i = y1; i < y2; ++i)
>> +    {
>> +	for (j = x1; j < x2; ++j)
>> +	{
>> +	    int rx = j;
>> +	    int ry = i;
>> +
>> +	    pixman_fixed_t f = *params;
>> +
>> +	    if (f)
>> +	    {
>> +		argb_t pixel;
>> +
>> +		if (repeat_mode != PIXMAN_REPEAT_NONE)
>> +		{
>> +		    repeat (repeat_mode, &rx, width);
>> +		    repeat (repeat_mode, &ry, height);
>> +
>> +		    get_pixel (image, rx, ry, FALSE, &pixel);
>> +		}
>> +		else
>> +		{
>> +		    get_pixel (image, rx, ry, TRUE, &pixel);
>> +		}
>> +
>> +		satot += pixel.a * f;
>> +		srtot += pixel.r * f;
>> +		sgtot += pixel.g * f;
>> +		sbtot += pixel.b * f;
> I am concerned with those lines (and the corresponding lines in
> separable_convolution).  Unless I am mistaken, `sXtot` and `f` are
> integer variables while `pixel.X` are floating point variables in 0-1,
> so it looks like more truncation than wanted may be occuring here.
>
>> +	    }
>> +
>> +	    params++;
>> +	}
>> +    }
>> +
>> +    ret->a = CLIP (satot / 65536.f, 0.f, 1.f);
>> +    ret->r = CLIP (srtot / 65536.f, 0.f, 1.f);
>> +    ret->g = CLIP (sgtot / 65536.f, 0.f, 1.f);
>> +    ret->b = CLIP (sbtot / 65536.f, 0.f, 1.f);
>>  }
>>  
>> -static uint32_t
>> -bits_image_fetch_pixel_separable_convolution (bits_image_t *image,
>> -                                              pixman_fixed_t x,
>> -                                              pixman_fixed_t y,
>> -                                              get_pixel_t    get_pixel)
>> +static void
>> +bits_image_fetch_pixel_separable_convolution_32 (bits_image_t  *image,
>> +						 pixman_fixed_t x,
>> +						 pixman_fixed_t y,
>> +						 get_pixel_t    get_pixel,
>> +						 void	       *out)
>>  {
>>      pixman_fixed_t *params = image->common.filter_params;
>>      pixman_repeat_t repeat_mode = image->common.repeat;
>> @@ -234,6 +359,7 @@ bits_image_fetch_pixel_separable_convolution (bits_image_t *image,
>>      int32_t x1, x2, y1, y2;
>>      int32_t px, py;
>>      int i, j;
>> +    uint32_t *ret = out;
>>  
>>      /* Round x and y to the middle of the closest phase before continuing. This
>>       * ensures that the convolution matrix is aligned right, since it was
>> @@ -278,11 +404,11 @@ bits_image_fetch_pixel_separable_convolution (bits_image_t *image,
>>                          repeat (repeat_mode, &rx, width);
>>                          repeat (repeat_mode, &ry, height);
>>  
>> -                        pixel = get_pixel (image, rx, ry, FALSE);
>> +                        get_pixel (image, rx, ry, FALSE, &pixel);
>>                      }
>>                      else
>>                      {
>> -                        pixel = get_pixel (image, rx, ry, TRUE);
>> +                        get_pixel (image, rx, ry, TRUE, &pixel);
>>  		    }
>>  
>>                      f = (fy * fx + 0x8000) >> 16;
>> @@ -306,46 +432,153 @@ bits_image_fetch_pixel_separable_convolution (bits_image_t *image,
>>      sgtot = CLIP (sgtot, 0, 0xff);
>>      sbtot = CLIP (sbtot, 0, 0xff);
>>  
>> -    return ((satot << 24) | (srtot << 16) | (sgtot <<  8) | (sbtot));
>> +    *ret = ((satot << 24) | (srtot << 16) | (sgtot <<  8) | (sbtot));
>>  }
>>  
>> -static force_inline uint32_t
>> -bits_image_fetch_pixel_filtered (bits_image_t *image,
>> +static void
>> +bits_image_fetch_pixel_separable_convolution_float (bits_image_t  *image,
>> +						    pixman_fixed_t x,
>> +						    pixman_fixed_t y,
>> +						    get_pixel_t    get_pixel,
>> +						    void	  *out)
>> +{
>> +    pixman_fixed_t *params = image->common.filter_params;
>> +    pixman_repeat_t repeat_mode = image->common.repeat;
>> +    int width = image->width;
>> +    int height = image->height;
>> +    int cwidth = pixman_fixed_to_int (params[0]);
>> +    int cheight = pixman_fixed_to_int (params[1]);
>> +    int x_phase_bits = pixman_fixed_to_int (params[2]);
>> +    int y_phase_bits = pixman_fixed_to_int (params[3]);
>> +    int x_phase_shift = 16 - x_phase_bits;
>> +    int y_phase_shift = 16 - y_phase_bits;
>> +    int x_off = ((cwidth << 16) - pixman_fixed_1) >> 1;
>> +    int y_off = ((cheight << 16) - pixman_fixed_1) >> 1;
>> +    pixman_fixed_t *y_params;
>> +    int srtot, sgtot, sbtot, satot;
>> +    int32_t x1, x2, y1, y2;
>> +    int32_t px, py;
>> +    int i, j;
>> +    argb_t *ret = out;
>> +
>> +    /* Round x and y to the middle of the closest phase before continuing. This
>> +     * ensures that the convolution matrix is aligned right, since it was
>> +     * positioned relative to a particular phase (and not relative to whatever
>> +     * exact fraction we happen to get here).
>> +     */
>> +    x = ((x >> x_phase_shift) << x_phase_shift) + ((1 << x_phase_shift) >> 1);
>> +    y = ((y >> y_phase_shift) << y_phase_shift) + ((1 << y_phase_shift) >> 1);
>> +
>> +    px = (x & 0xffff) >> x_phase_shift;
>> +    py = (y & 0xffff) >> y_phase_shift;
>> +
>> +    y_params = params + 4 + (1 << x_phase_bits) * cwidth + py * cheight;
>> +
>> +    x1 = pixman_fixed_to_int (x - pixman_fixed_e - x_off);
>> +    y1 = pixman_fixed_to_int (y - pixman_fixed_e - y_off);
>> +    x2 = x1 + cwidth;
>> +    y2 = y1 + cheight;
>> +
>> +    srtot = sgtot = sbtot = satot = 0;
>> +
>> +    for (i = y1; i < y2; ++i)
>> +    {
>> +        pixman_fixed_48_16_t fy = *y_params++;
>> +        pixman_fixed_t *x_params = params + 4 + px * cwidth;
>> +
>> +        if (fy)
>> +        {
>> +            for (j = x1; j < x2; ++j)
>> +            {
>> +                pixman_fixed_t fx = *x_params++;
>> +		int rx = j;
>> +		int ry = i;
>> +
>> +                if (fx)
>> +                {
>> +                    pixman_fixed_t f;
>> +                    argb_t pixel;
>> +
>> +                    if (repeat_mode != PIXMAN_REPEAT_NONE)
>> +                    {
>> +                        repeat (repeat_mode, &rx, width);
>> +                        repeat (repeat_mode, &ry, height);
>> +
>> +                        get_pixel (image, rx, ry, FALSE, &pixel);
>> +                    }
>> +                    else
>> +                    {
>> +                        get_pixel (image, rx, ry, TRUE, &pixel);
>> +		    }
>> +
>> +                    f = (fy * fx + 0x8000) >> 16;
>> +
>> +		    satot += pixel.a * f;
>> +		    srtot += pixel.r * f;
>> +		    sgtot += pixel.g * f;
>> +		    sbtot += pixel.b * f;
>> +                }
>> +            }
>> +	}
>> +    }
>> +
>> +    ret->a = CLIP (satot / 65536.f, 0.f, 1.f);
>> +    ret->r = CLIP (srtot / 65536.f, 0.f, 1.f);
>> +    ret->g = CLIP (sgtot / 65536.f, 0.f, 1.f);
>> +    ret->b = CLIP (sbtot / 65536.f, 0.f, 1.f);
>> +}
>> +
>> +static force_inline void
>> +bits_image_fetch_pixel_filtered (bits_image_t  *image,
>> +				 pixman_bool_t  wide,
>>  				 pixman_fixed_t x,
>>  				 pixman_fixed_t y,
>> -				 get_pixel_t    get_pixel)
>> +				 get_pixel_t    get_pixel,
>> +				 void          *out)
>>  {
>>      switch (image->common.filter)
>>      {
>>      case PIXMAN_FILTER_NEAREST:
>>      case PIXMAN_FILTER_FAST:
>> -	return bits_image_fetch_pixel_nearest (image, x, y, get_pixel);
>> +	bits_image_fetch_pixel_nearest (image, x, y, get_pixel, out);
>>  	break;
>>  
>>      case PIXMAN_FILTER_BILINEAR:
>>      case PIXMAN_FILTER_GOOD:
>>      case PIXMAN_FILTER_BEST:
>> -	return bits_image_fetch_pixel_bilinear (image, x, y, get_pixel);
>> +	if (wide)
>> +	    bits_image_fetch_pixel_bilinear_float (image, x, y, get_pixel, out);
>> +	else
>> +	    bits_image_fetch_pixel_bilinear_32 (image, x, y, get_pixel, out);
>>  	break;
>>  
>>      case PIXMAN_FILTER_CONVOLUTION:
>> -	return bits_image_fetch_pixel_convolution (image, x, y, get_pixel);
>> +	if (wide)
>> +	    bits_image_fetch_pixel_convolution_float (image, x, y,
>> +						      get_pixel, out);
>> +	else
>> +	    bits_image_fetch_pixel_convolution_32 (image, x, y,
>> +						   get_pixel, out);
>>  	break;
>>  
>>      case PIXMAN_FILTER_SEPARABLE_CONVOLUTION:
>> -        return bits_image_fetch_pixel_separable_convolution (image, x, y, get_pixel);
>> +	if (wide)
>> +	    bits_image_fetch_pixel_separable_convolution_float (image, x, y,
>> +								get_pixel, out);
>> +	else
>> +	    bits_image_fetch_pixel_separable_convolution_32 (image, x, y,
>> +							     get_pixel, out);
>>          break;
>>  
>>      default:
>>          break;
>>      }
>> -
>> -    return 0;
>>  }
>>  
>>  static uint32_t *
>> -bits_image_fetch_affine_no_alpha (pixman_iter_t *  iter,
>> -				  const uint32_t * mask)
>> +__bits_image_fetch_affine_no_alpha (pixman_iter_t *  iter,
>> +				    pixman_bool_t    wide,
>> +				    const uint32_t * mask)
>>  {
>>      pixman_image_t *image  = iter->image;
>>      int             offset = iter->x;
>> @@ -357,6 +590,8 @@ bits_image_fetch_affine_no_alpha (pixman_iter_t *  iter,
>>      pixman_fixed_t ux, uy;
>>      pixman_vector_t v;
>>      int i;
>> +    get_pixel_t get_pixel =
>> +	wide ? fetch_pixel_no_alpha_float : fetch_pixel_no_alpha_32;
>>  
>>      /* reference point is the center of the pixel */
>>      v.vector[0] = pixman_int_to_fixed (offset) + pixman_fixed_1 / 2;
>> @@ -384,27 +619,45 @@ bits_image_fetch_affine_no_alpha (pixman_iter_t *  iter,
>>      {
>>  	if (!mask || mask[i])
>>  	{
>> -	    buffer[i] = bits_image_fetch_pixel_filtered (
>> -		&image->bits, x, y, fetch_pixel_no_alpha);
>> +	    bits_image_fetch_pixel_filtered (
>> +		&image->bits, wide, x, y, get_pixel, buffer);
>>  	}
>>  
>>  	x += ux;
>>  	y += uy;
>> +	buffer += wide ? 4 : 1;
>>      }
>>  
>> -    return buffer;
>> +    return iter->buffer;
>> +}
>> +
>> +static uint32_t *
>> +bits_image_fetch_affine_no_alpha_32 (pixman_iter_t  *iter,
>> +				     const uint32_t *mask)
>> +{
>> +    return __bits_image_fetch_affine_no_alpha(iter, FALSE, mask);
>> +}
>> +
>> +static uint32_t *
>> +bits_image_fetch_affine_no_alpha_float (pixman_iter_t  *iter,
>> +					const uint32_t *mask)
>> +{
>> +    return __bits_image_fetch_affine_no_alpha(iter, TRUE, mask);
>>  }
>>  
>>  /* General fetcher */
>> -static force_inline uint32_t
>> -fetch_pixel_general (bits_image_t *image, int x, int y, pixman_bool_t check_bounds)
>> +static force_inline void
>> +fetch_pixel_general_32 (bits_image_t *image,
>> +			int x, int y, pixman_bool_t check_bounds,
>> +			void *out)
>>  {
>> -    uint32_t pixel;
>> +    uint32_t pixel, *ret = out;
>>  
>>      if (check_bounds &&
>>  	(x < 0 || x >= image->width || y < 0 || y >= image->height))
>>      {
>> -	return 0;
>> +	*ret = 0;
>> +	return;
>>      }
>>  
>>      pixel = image->fetch_pixel_32 (image, x, y);
>> @@ -433,18 +686,59 @@ fetch_pixel_general (bits_image_t *image, int x, int y, pixman_bool_t check_boun
>>  	pixel |= (pixel_a << 24);
>>      }
>>  
>> -    return pixel;
>> +    *ret = pixel;
>> +}
>> +
>> +static force_inline void
>> +fetch_pixel_general_float (bits_image_t *image,
>> +			int x, int y, pixman_bool_t check_bounds,
>> +			void *out)
>> +{
>> +    argb_t *ret = out;
>> +
>> +    if (check_bounds &&
>> +	(x < 0 || x >= image->width || y < 0 || y >= image->height))
>> +    {
>> +	ret->a = ret->r = ret->g = ret->b = 0;
>> +	return;
>> +    }
>> +
>> +    *ret = image->fetch_pixel_float (image, x, y);
>> +
>> +    if (image->common.alpha_map)
>> +    {
>> +	x -= image->common.alpha_origin_x;
>> +	y -= image->common.alpha_origin_y;
>> +
>> +	if (x < 0 || x >= image->common.alpha_map->width ||
>> +	    y < 0 || y >= image->common.alpha_map->height)
>> +	{
>> +	    ret->a = 0.f;
>> +	}
>> +	else
>> +	{
>> +	    argb_t alpha;
>> +
>> +	    alpha = image->common.alpha_map->fetch_pixel_float (
>> +		    image->common.alpha_map, x, y);
>> +
>> +	    ret->a = alpha.a;
>> +	}
>> +    }
>>  }
>>  
>>  static uint32_t *
>> -bits_image_fetch_general (pixman_iter_t  *iter,
>> -			  const uint32_t *mask)
>> +__bits_image_fetch_general (pixman_iter_t  *iter,
>> +			    pixman_bool_t wide,
>> +			    const uint32_t *mask)
>>  {
>>      pixman_image_t *image  = iter->image;
>>      int             offset = iter->x;
>>      int             line   = iter->y++;
>>      int             width  = iter->width;
>>      uint32_t *      buffer = iter->buffer;
>> +    get_pixel_t     get_pixel =
>> +	wide ? fetch_pixel_general_float : fetch_pixel_general_32;
>>  
>>      pixman_fixed_t x, y, w;
>>      pixman_fixed_t ux, uy, uw;
>> @@ -493,16 +787,31 @@ bits_image_fetch_general (pixman_iter_t  *iter,
>>  		y0 = 0;
>>  	    }
>>  
>> -	    buffer[i] = bits_image_fetch_pixel_filtered (
>> -		&image->bits, x0, y0, fetch_pixel_general);
>> +	    bits_image_fetch_pixel_filtered (
>> +		&image->bits, wide, x0, y0, get_pixel, buffer);
>>  	}
>>  
>>  	x += ux;
>>  	y += uy;
>>  	w += uw;
>> +	buffer += wide ? 4 : 1;
>>      }
>>  
>> -    return buffer;
>> +    return iter->buffer;
>> +}
>> +
>> +static uint32_t *
>> +bits_image_fetch_general_32 (pixman_iter_t  *iter,
>> +			     const uint32_t *mask)
>> +{
>> +    return __bits_image_fetch_general(iter, FALSE, mask);
>> +}
>> +
>> +static uint32_t *
>> +bits_image_fetch_general_float (pixman_iter_t  *iter,
>> +				const uint32_t *mask)
>> +{
>> +    return __bits_image_fetch_general(iter, TRUE, mask);
>>  }
>>  
>>  static void
>> @@ -703,15 +1012,15 @@ static const fetcher_info_t fetcher_info[] =
>>      /* Affine, no alpha */
>>      { PIXMAN_any,
>>        (FAST_PATH_NO_ALPHA_MAP | FAST_PATH_HAS_TRANSFORM | FAST_PATH_AFFINE_TRANSFORM),
>> -      bits_image_fetch_affine_no_alpha,
>> -      _pixman_image_get_scanline_generic_float
>> +      bits_image_fetch_affine_no_alpha_32,
>> +      bits_image_fetch_affine_no_alpha_float,
>>      },
>>  
>>      /* General */
>>      { PIXMAN_any,
>>        0,
>> -      bits_image_fetch_general,
>> -      _pixman_image_get_scanline_generic_float
>> +      bits_image_fetch_general_32,
>> +      bits_image_fetch_general_float,
>>      },
>>  
>>      { PIXMAN_null },
>> @@ -741,7 +1050,6 @@ _pixman_bits_image_src_iter_init (pixman_image_t *image, pixman_iter_t *iter)
>>  	    }
>>  	    else
>>  	    {
>> -		iter->data = info->get_scanline_32;
>>  		iter->get_scanline = info->get_scanline_float;
>>  	    }
>>  	    return;
>> diff --git a/pixman/pixman-inlines.h b/pixman/pixman-inlines.h
>> index 1c8441d6dabe..332e208140a0 100644
>> --- a/pixman/pixman-inlines.h
>> +++ b/pixman/pixman-inlines.h
>> @@ -222,6 +222,31 @@ bilinear_interpolation (uint32_t tl, uint32_t tr,
>>  #endif
>>  #endif // BILINEAR_INTERPOLATION_BITS <= 4
>>  
>> +static force_inline argb_t
>> +bilinear_interpolation_float (argb_t tl, argb_t tr,
>> +			      argb_t bl, argb_t br,
>> +			      float distx, float disty)
>> +{
>> +    float distxy, distxiy, distixy, distixiy;
>> +    argb_t r;
>> +
>> +    distxy = distx * disty;
>> +    distxiy = distx - (1.f - distxy);
>> +    distixy = (1.f - distx) * disty;
>> +    distixiy = (1.f - distx) * (1.f - disty);
>> +
>> +    r.a = tl.a * distixiy + tr.a * distxiy +
>> +          bl.a * distixy  + br.a * distxy;
>> +    r.r = tl.r * distixiy + tr.r * distxiy +
>> +          bl.r * distixy  + br.r * distxy;
>> +    r.g = tl.g * distixiy + tr.g * distxiy +
>> +          bl.g * distixy  + br.g * distxy;
>> +    r.b = tl.b * distixiy + tr.b * distxiy +
>> +          bl.b * distixy  + br.b * distxy;
>> +
>> +    return r;
>> +}
>> +
>>  /*
>>   * For each scanline fetched from source image with PAD repeat:
>>   * - calculate how many pixels need to be padded on the left side

Patch hide | download patch | download mbox

diff --git a/demos/scale.c b/demos/scale.c
index 0995ad08da9d..51ab1d4a2408 100644
--- a/demos/scale.c
+++ b/demos/scale.c
@@ -289,7 +289,7 @@  on_expose (GtkWidget *da, GdkEvent *event, gpointer data)
 
     pixels = calloc (1, area->width * area->height * 4);
     tmp = pixman_image_create_bits (
-        PIXMAN_a8r8g8b8, area->width, area->height, pixels, area->width * 4);
+        PIXMAN_x2r10g10b10, area->width, area->height, pixels, area->width * 4);
 
     if (area->x < app->scaled_width && area->y < app->scaled_height)
     {
@@ -301,7 +301,7 @@  on_expose (GtkWidget *da, GdkEvent *event, gpointer data)
     }
 
     surface = cairo_image_surface_create_for_data (
-        (uint8_t *)pixels, CAIRO_FORMAT_ARGB32,
+        (uint8_t *)pixels, CAIRO_FORMAT_RGB30,
         area->width, area->height, area->width * 4);
 
     cr = gdk_cairo_create (da->window);
@@ -423,8 +423,9 @@  int
 main (int argc, char **argv)
 {
     GtkWidget *window;
-    pixman_image_t *image;
+    pixman_image_t *image, *tmp;
     app_t *app;
+    uint32_t *pixels;
     
     gtk_init (&argc, &argv);
 
@@ -440,7 +441,18 @@  main (int argc, char **argv)
 	return -1;
     }
 
-    app = app_new (image);
+    pixels = calloc (pixman_image_get_height(image), pixman_image_get_stride(image));
+    tmp = pixman_image_create_bits (
+        PIXMAN_x2r10g10b10, pixman_image_get_width(image),
+	pixman_image_get_height(image), pixels, pixman_image_get_stride(image));
+
+    pixman_image_composite (
+            PIXMAN_OP_SRC,
+            image, NULL, tmp,
+            0, 0, 0, 0, 0, 0,
+            pixman_image_get_width(image), pixman_image_get_height(image));
+
+    app = app_new (tmp);
     
     window = get_widget (app, "main");