[Pixman] [PATCH 2/2] pixman: Use maximum precision for pixman-bits-image
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Mon Jan 7 12:22:43 UTC 2019
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. :)
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");
---------------
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
More information about the Pixman
mailing list