Re: [Pixman] [PATCH 2/2] pixman: Use maximum precision for pixman-bits-image, v2.
Op 09-02-2019 om 13:01 schreef Basile Clement: > On 1/8/19 11:42 AM, Maarten Lankhorst wrote: > >> pixman-bits-image's wide helpers first obtains the 8-bits image, >> then converts it to float. This destroys all the precision that >> the wide path was offering. >> >> Fix this by making get_pixel() take a pointer instead of returning >> a value. Floating point will fill in a argb_t, while the 8-bits path >> will fill a 32-bits ARGB value. This also requires writing a floating >> point bilinear interpolator. With this change pixman can use the full >> floating point precision internally in all paths. >> >> Changes since v1: >> - Make accum and reduce an argument to convolution functions, >> to remove duplication. >> >> Signed-off-by: Maarten Lankhorst > Apologies for the late reply; I have been quite busy with other things > recently. > > The patch looks good to me now! I have not figured out how to visually > test the filters however. Thanks, pushed. Can you rebase the dithering patches so I can take a look? ~Maarten ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 2/2] pixman: Use maximum precision for pixman-bits-image, v2.
On 1/8/19 11:42 AM, Maarten Lankhorst wrote:
> pixman-bits-image's wide helpers first obtains the 8-bits image,
> then converts it to float. This destroys all the precision that
> the wide path was offering.
>
> Fix this by making get_pixel() take a pointer instead of returning
> a value. Floating point will fill in a argb_t, while the 8-bits path
> will fill a 32-bits ARGB value. This also requires writing a floating
> point bilinear interpolator. With this change pixman can use the full
> floating point precision internally in all paths.
>
> Changes since v1:
> - Make accum and reduce an argument to convolution functions,
> to remove duplication.
>
> Signed-off-by: Maarten Lankhorst
Apologies for the late reply; I have been quite busy with other things
recently.
The patch looks good to me now! I have not figured out how to visually
test the filters however.
> ---
> pixman/pixman-bits-image.c | 414 +++--
> pixman/pixman-inlines.h| 25 +++
> 2 files changed, 328 insertions(+), 111 deletions(-)
>
> diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c
> index 9fb91ff5831d..564789e9c685 100644
> --- a/pixman/pixman-bits-image.c
> +++ b/pixman/pixman-bits-image.c
> @@ -36,43 +36,45 @@
> #include "pixman-combine32.h"
> #include "pixman-inlines.h"
>
> -static uint32_t *
> -_pixman_image_get_scanline_generic_float (pixman_iter_t * iter,
> - const uint32_t *mask)
> -{
> -pixman_iter_get_scanline_t fetch_32 = iter->data;
> -uint32_t *buffer = iter->buffer;
> -
> -fetch_32 (iter, NULL);
> +/* Fetch functions */
>
> -pixman_expand_to_float ((argb_t *)buffer, buffer, PIXMAN_a8r8g8b8,
> iter->width);
> +static force_inline void
> +fetch_pixel_no_alpha_32 (bits_image_t *image,
> + int x, int y, pixman_bool_t check_bounds,
> + void *out)
> +{
> +uint32_t *ret = out;
>
> -return iter->buffer;
> +if (check_bounds &&
> + (x < 0 || x >= image->width || y < 0 || y >= image->height))
> + *ret = 0;
> +else
> + *ret = image->fetch_pixel_32 (image, x, y);
> }
>
> -/* Fetch functions */
> -
> -static force_inline uint32_t
> -fetch_pixel_no_alpha (bits_image_t *image,
> - int x, int y, pixman_bool_t check_bounds)
> +static force_inline void
> +fetch_pixel_no_alpha_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))
> -{
> - return 0;
> -}
> -
> -return image->fetch_pixel_32 (image, x, y);
> + ret->a = ret->r = ret->g = ret->b = 0.f;
> +else
> + *ret = image->fetch_pixel_float (image, x, y);
> }
>
> -typedef uint32_t (* get_pixel_t) (bits_image_t *image,
> - int x, int y, pixman_bool_t check_bounds);
> +typedef void (* get_pixel_t) (bits_image_t *image,
> + int x, int y, pixman_bool_t check_bounds, void
> *out);
>
> -static force_inline uint32_t
> +static force_inline void
> bits_image_fetch_pixel_nearest (bits_image_t *image,
> pixman_fixed_t x,
> pixman_fixed_t y,
> - get_pixel_t get_pixel)
> + get_pixel_t get_pixel,
> + void *out)
> {
> int x0 = pixman_fixed_to_int (x - pixman_fixed_e);
> int y0 = pixman_fixed_to_int (y - pixman_fixed_e);
> @@ -82,19 +84,20 @@ bits_image_fetch_pixel_nearest (bits_image_t *image,
> repeat (image->common.repeat, , image->width);
> repeat (image->common.repeat, , image->height);
>
> - return get_pixel (image, x0, y0, FALSE);
> + get_pixel (image, x0, y0, FALSE, out);
> }
> else
> {
> - return get_pixel (image, x0, y0, TRUE);
> + get_pixel (image, x0, y0, TRUE, out);
> }
> }
>
> -static force_inline uint32_t
> -bits_image_fetch_pixel_bilinear (bits_image_t *image,
> - pixman_fixed_t x,
> - pixman_fixed_t y,
> - get_pixel_t get_pixel)
> +static force_inline void
> +bits_image_fetch_pixel_bilinear_32 (bits_image_t *image,
> + pixman_fixed_t x,
> + pixman_fixed_t y,
> + get_pixel_t get_pixel,
> + void *out)
> {
> pixman_repeat_t repeat_mode = image->common.repeat;
> int width = image->width;
> @@ -102,6 +105,7 @@ bits_image_fetch_pixel_bilinear (bits_image_t *image,
> int x1, y1, x2, y2;
> uint32_t tl, tr, bl, br;
> int32_t distx, disty;
> +uint32_t *ret = out;
>
> x1 = x -
Re: [Pixman] [PATCH 2/2] pixman: Use maximum precision for pixman-bits-image
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 (, );
@@ -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, , width);
>> +repeat (repeat_mode, , height);
>> +
>> +get_pixel (image, rx, ry, FALSE, );
>> +}
>> +else
>> +{
>> +get_pixel (image, rx, ry, TRUE, );
>> +}
>> +
>> +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
Re: [Pixman] [PATCH 2/2] pixman: Use maximum precision for pixman-bits-image
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.
- 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, , width);
> + repeat (repeat_mode, , height);
> +
> + get_pixel (image, rx, ry, FALSE, );
> + }
> + else
> + {
> + get_pixel (image, rx, ry, TRUE, );
> + }
> +
> + 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_tget_pixel)
> +static void
> +bits_image_fetch_pixel_separable_convolution_32 (bits_image_t *image,
> + pixman_fixed_t x,
> + pixman_fixed_t y,
> + get_pixel_tget_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, , width);
> repeat (repeat_mode, , height);
>
> -pixel = get_pixel (image, rx, ry, FALSE);
> +get_pixel (image, rx, ry, FALSE, );
> }
> else
> {
> -pixel = get_pixel (image, rx, ry, TRUE);
> +
Re: [Pixman] [PATCH 2/2] pixman: Use maximum precision for pixman-bits-image
Oops, missing commit description..
pixman-bits-image's wide helpers first obtains the 8-bits image,
then converts it to float. This destroys all the precision that
the wide path was offering.
Fix this by making get_pixel() take a pointer instead of returning
a value. Floating point will fill in a argb_t, while the 8-bits path
will fill a 32-bits ARGB value. This also requires writing a floating
point bilinear interpolator. With this change pixman can use the full
floating point precision internally in all paths.
Op 03-01-2019 om 12:18 schreef Maarten Lankhorst:
> Signed-off-by: Maarten Lankhorst
> ---
> pixman/pixman-bits-image.c | 478 ++---
> pixman/pixman-inlines.h| 25 ++
> 2 files changed, 418 insertions(+), 85 deletions(-)
>
> diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c
> index 9fb91ff5831d..cb1c5ce12d46 100644
> --- a/pixman/pixman-bits-image.c
> +++ b/pixman/pixman-bits-image.c
> @@ -36,43 +36,45 @@
> #include "pixman-combine32.h"
> #include "pixman-inlines.h"
>
> -static uint32_t *
> -_pixman_image_get_scanline_generic_float (pixman_iter_t * iter,
> - const uint32_t *mask)
> -{
> -pixman_iter_get_scanline_t fetch_32 = iter->data;
> -uint32_t *buffer = iter->buffer;
> -
> -fetch_32 (iter, NULL);
> +/* Fetch functions */
>
> -pixman_expand_to_float ((argb_t *)buffer, buffer, PIXMAN_a8r8g8b8,
> iter->width);
> +static force_inline void
> +fetch_pixel_no_alpha_32 (bits_image_t *image,
> + int x, int y, pixman_bool_t check_bounds,
> + void *out)
> +{
> +uint32_t *ret = out;
>
> -return iter->buffer;
> +if (check_bounds &&
> + (x < 0 || x >= image->width || y < 0 || y >= image->height))
> + *ret = 0;
> +else
> + *ret = image->fetch_pixel_32 (image, x, y);
> }
>
> -/* Fetch functions */
> -
> -static force_inline uint32_t
> -fetch_pixel_no_alpha (bits_image_t *image,
> - int x, int y, pixman_bool_t check_bounds)
> +static force_inline void
> +fetch_pixel_no_alpha_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))
> -{
> - return 0;
> -}
> -
> -return image->fetch_pixel_32 (image, x, y);
> + ret->a = ret->r = ret->g = ret->b = 0.f;
> +else
> + *ret = image->fetch_pixel_float (image, x, y);
> }
>
> -typedef uint32_t (* get_pixel_t) (bits_image_t *image,
> - int x, int y, pixman_bool_t check_bounds);
> +typedef void (* get_pixel_t) (bits_image_t *image,
> + int x, int y, pixman_bool_t check_bounds, void
> *out);
>
> -static force_inline uint32_t
> +static force_inline void
> bits_image_fetch_pixel_nearest (bits_image_t *image,
> pixman_fixed_t x,
> pixman_fixed_t y,
> - get_pixel_t get_pixel)
> + get_pixel_t get_pixel,
> + void *out)
> {
> int x0 = pixman_fixed_to_int (x - pixman_fixed_e);
> int y0 = pixman_fixed_to_int (y - pixman_fixed_e);
> @@ -82,19 +84,20 @@ bits_image_fetch_pixel_nearest (bits_image_t *image,
> repeat (image->common.repeat, , image->width);
> repeat (image->common.repeat, , image->height);
>
> - return get_pixel (image, x0, y0, FALSE);
> + get_pixel (image, x0, y0, FALSE, out);
> }
> else
> {
> - return get_pixel (image, x0, y0, TRUE);
> + get_pixel (image, x0, y0, TRUE, out);
> }
> }
>
> -static force_inline uint32_t
> -bits_image_fetch_pixel_bilinear (bits_image_t *image,
> - pixman_fixed_t x,
> - pixman_fixed_t y,
> - get_pixel_t get_pixel)
> +static force_inline void
> +bits_image_fetch_pixel_bilinear_32 (bits_image_t *image,
> + pixman_fixed_t x,
> + pixman_fixed_t y,
> + get_pixel_t get_pixel,
> + void *out)
> {
> pixman_repeat_t repeat_mode = image->common.repeat;
> int width = image->width;
> @@ -102,6 +105,7 @@ bits_image_fetch_pixel_bilinear (bits_image_t *image,
> int x1, y1, x2, y2;
> uint32_t tl, tr, bl, br;
> int32_t distx, disty;
> +uint32_t *ret = out;
>
> x1 = x - pixman_fixed_1 / 2;
> y1 = y - pixman_fixed_1 / 2;
> @@ -121,27 +125,77 @@ bits_image_fetch_pixel_bilinear (bits_image_t *image,
> repeat (repeat_mode, , width);
> repeat (repeat_mode, , height);
>
> - tl = get_pixel (image, x1, y1, FALSE);
> - bl =
