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 _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman