[Pixman] [PATCH v10 06/15] pixman-filter: reduce amount of malloc/free/memcpy to generate filter
Oded Gabbay
oded.gabbay at gmail.com
Thu Feb 4 01:51:42 PST 2016
On Tue, Feb 2, 2016 at 8:28 AM, <spitzak at gmail.com> wrote:
> From: Bill Spitzak <spitzak at gmail.com>
>
> Rearranged so that the entire block of memory for the filter pair
> is allocated first, and then filled in. Previous version allocated
> and freed two temporary buffers for each filter and did an extra
> memcpy.
>
> v8: small refactor to remove the filter_width function
>
> v10: Restored filter_width function but with arguments changed to
> match later patches
>
> Signed-off-by: Bill Spitzak <spitzak at gmail.com>
>
> Reviewed-by: Oded Gabbay <oded.gabbay at gmail.com>
When you change and then re-change the patch, you definitely can't
keep this tag :)
Only when you resend it without *any* changes or when you fix
according to a specific comment you can keep it.
> ---
> pixman/pixman-filter.c | 59 ++++++++++++++++++++++----------------------------
> 1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
> index acfb110..6c077b6 100644
> --- a/pixman/pixman-filter.c
> +++ b/pixman/pixman-filter.c
> @@ -217,23 +217,17 @@ integral (pixman_kernel_t reconstruct, double x1,
> }
> }
>
> -static pixman_fixed_t *
> -create_1d_filter (int *width,
> +static void
> +create_1d_filter (int width,
> pixman_kernel_t reconstruct,
> pixman_kernel_t sample,
> double size,
> - int n_phases)
> + int n_phases,
> + pixman_fixed_t *p)
> {
> - pixman_fixed_t *params, *p;
> double step;
> int i;
>
> - *width = ceil (size * filters[sample].width + filters[reconstruct].width);
> -
> - p = params = malloc (*width * n_phases * sizeof (pixman_fixed_t));
> - if (!params)
> - return NULL;
> -
> step = 1.0 / n_phases;
>
> for (i = 0; i < n_phases; ++i)
> @@ -248,8 +242,8 @@ create_1d_filter (int *width,
> * and sample positions.
> */
>
> - x1 = ceil (frac - *width / 2.0 - 0.5);
> - x2 = x1 + *width;
> + x1 = ceil (frac - width / 2.0 - 0.5);
> + x2 = x1 + width;
>
> total = 0;
> for (x = x1; x < x2; ++x)
> @@ -277,7 +271,7 @@ create_1d_filter (int *width,
> }
>
> /* Normalize */
> - p -= *width;
> + p -= width;
> total = 1 / total;
> new_total = 0;
> for (x = x1; x < x2; ++x)
> @@ -289,10 +283,8 @@ create_1d_filter (int *width,
> }
>
> if (new_total != pixman_fixed_1)
> - *(p - *width / 2) += (pixman_fixed_1 - new_total);
> + *(p - width / 2) += (pixman_fixed_1 - new_total);
> }
> -
> - return params;
> }
>
> #ifdef PIXMAN_GNUPLOT
> @@ -336,6 +328,14 @@ gnuplot_filter(int width, int samples, const pixman_fixed_t* p)
> }
> #endif
>
> +/* Besides calculating the width, this can modify the scale and subsample_bits */
Well, it doesn't modify them, so I think we should match the comment
to the function, no ?
And if you intend to change it in the future, then specifically write
it like that, i.e. "in the future, this function can be re-written to
also modify the size & subsample bits"
> +static int
> +filter_width(pixman_kernel_t reconstruct, pixman_kernel_t sample,
> + double* size, int* subsample_bits)
1. Why do we need *subsample_bits if we don't use it in the function ?
Please remove it.
2. Why pass size as pointer ? We don't change it, so why not by value ?
And if in the future you do want to change them, then do it in the
future patches, not here. It is very confusing.
> +{
> + return ceil (filters[reconstruct].width + *size * filters[sample].width);
> +}
> +
> /* Create the parameter list for a SEPARABLE_CONVOLUTION filter
> * with the given kernels and size parameters
> */
> @@ -352,42 +352,35 @@ pixman_filter_create_separable_convolution (int *n_values,
> {
> double sx = fabs (pixman_fixed_to_double (size_x));
> double sy = fabs (pixman_fixed_to_double (size_y));
> - pixman_fixed_t *horz = NULL, *vert = NULL, *params = NULL;
> + pixman_fixed_t *params;
> int subsample_x, subsample_y;
> int width, height;
>
> + width = filter_width (reconstruct_x, sample_x, &sx, &subsample_bits_x);
> subsample_x = (1 << subsample_bits_x);
> - subsample_y = (1 << subsample_bits_y);
>
> - horz = create_1d_filter (&width, reconstruct_x, sample_x, sx, subsample_x);
> - vert = create_1d_filter (&height, reconstruct_y, sample_y, sy, subsample_y);
> + height = filter_width (reconstruct_y, sample_y, &sy, &subsample_bits_y);
> + subsample_y = (1 << subsample_bits_y);
>
> - if (!horz || !vert)
> - goto out;
> -
> *n_values = 4 + width * subsample_x + height * subsample_y;
> -
> +
???
> params = malloc (*n_values * sizeof (pixman_fixed_t));
> if (!params)
> - goto out;
> + return NULL;
>
> params[0] = pixman_int_to_fixed (width);
> params[1] = pixman_int_to_fixed (height);
> params[2] = pixman_int_to_fixed (subsample_bits_x);
> params[3] = pixman_int_to_fixed (subsample_bits_y);
>
> - memcpy (params + 4, horz,
> - width * subsample_x * sizeof (pixman_fixed_t));
> - memcpy (params + 4 + width * subsample_x, vert,
> - height * subsample_y * sizeof (pixman_fixed_t));
> + create_1d_filter (width, reconstruct_x, sample_x, sx, subsample_x,
> + params + 4);
> + create_1d_filter (height, reconstruct_y, sample_y, sy, subsample_y,
> + params + 4 + width * subsample_x);
>
> #ifdef PIXMAN_GNUPLOT
> gnuplot_filter(width, subsample_x, params+4);
> #endif
>
> -out:
> - free (horz);
> - free (vert);
> -
> return params;
> }
> --
> 1.9.1
>
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman
More information about the Pixman
mailing list