[Pixman] [PATCH] pixman: Use maximum precision for gradients
Chris Wilson
chris at chris-wilson.co.uk
Mon Dec 3 14:20:46 UTC 2018
Quoting Maarten Lankhorst (2018-12-03 14:00:31)
> The high precision gradient path was falling back to the 8-bit path,
> then expanding it to float. This removed any advantage of the high
> precision path, so make sure our gradients are computed as floats
> by adding a separate path.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> pixman/pixman-conical-gradient.c | 38 +++++++----
> pixman/pixman-gradient-walker.c | 73 ++++++++++++--------
> pixman/pixman-linear-gradient.c | 59 +++++++++++-----
> pixman/pixman-private.h | 5 ++
> pixman/pixman-radial-gradient.c | 111 +++++++++++++++++++++----------
> 5 files changed, 192 insertions(+), 94 deletions(-)
>
> diff --git a/pixman/pixman-conical-gradient.c b/pixman/pixman-conical-gradient.c
> index 8bb46aecdcab..f1d8cb4ffceb 100644
> --- a/pixman/pixman-conical-gradient.c
> +++ b/pixman/pixman-conical-gradient.c
> @@ -51,7 +51,7 @@ coordinates_to_parameter (double x, double y, double angle)
> }
>
> static uint32_t *
> -conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
> +conical_get_scanline (pixman_iter_t *iter, const uint32_t *mask, pixman_bool_t wide)
> {
> pixman_image_t *image = iter->image;
> int x = iter->x;
> @@ -61,7 +61,7 @@ conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
>
> gradient_t *gradient = (gradient_t *)image;
> conical_gradient_t *conical = (conical_gradient_t *)image;
> - uint32_t *end = buffer + width;
> + uint32_t *end = buffer + (wide ? 4 * width : width);
> pixman_gradient_walker_t walker;
> pixman_bool_t affine = TRUE;
> double cx = 1.;
> @@ -109,11 +109,15 @@ conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
> {
> double t = coordinates_to_parameter (rx, ry, conical->angle);
>
> - *buffer = _pixman_gradient_walker_pixel (
> - &walker, (pixman_fixed_48_16_t)pixman_double_to_fixed (t));
> + if (wide)
> + _pixman_gradient_walker_pixel_wide (&walker, (argb_t *)buffer,
> + (pixman_fixed_48_16_t)pixman_double_to_fixed (t));
> + else
> + *buffer = _pixman_gradient_walker_pixel (&walker,
> + (pixman_fixed_48_16_t)pixman_double_to_fixed (t));
> }
>
> - ++buffer;
> + buffer += wide ? 4 : 1;
>
> rx += cx;
> ry += cy;
> @@ -144,11 +148,15 @@ conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
>
> t = coordinates_to_parameter (x, y, conical->angle);
>
> - *buffer = _pixman_gradient_walker_pixel (
> - &walker, (pixman_fixed_48_16_t)pixman_double_to_fixed (t));
> + if (wide)
> + _pixman_gradient_walker_pixel_wide (&walker, (argb_t *)buffer,
> + (pixman_fixed_48_16_t)pixman_double_to_fixed (t));
> + else
> + *buffer = _pixman_gradient_walker_pixel (&walker,
> + (pixman_fixed_48_16_t)pixman_double_to_fixed (t));
> }
>
> - ++buffer;
> + buffer += wide ? 4 : 1;
>
> rx += cx;
> ry += cy;
> @@ -160,15 +168,17 @@ conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
> return iter->buffer;
> }
>
> +
Bonus '\n'
> static uint32_t *
> -conical_get_scanline_wide (pixman_iter_t *iter, const uint32_t *mask)
> +conical_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
> {
> - uint32_t *buffer = conical_get_scanline_narrow (iter, NULL);
> -
> - pixman_expand_to_float (
> - (argb_t *)buffer, buffer, PIXMAN_a8r8g8b8, iter->width);
> + return conical_get_scanline (iter, mask, FALSE);
> +}
>
> - return buffer;
> +static uint32_t *
> +conical_get_scanline_wide (pixman_iter_t *iter, const uint32_t *mask)
> +{
> + return conical_get_scanline (iter, mask, TRUE);
> }
Ok.
> void
> diff --git a/pixman/pixman-gradient-walker.c b/pixman/pixman-gradient-walker.c
> index 822f8e62bae7..f0a82dc8178c 100644
> --- a/pixman/pixman-gradient-walker.c
> +++ b/pixman/pixman-gradient-walker.c
> @@ -122,45 +122,44 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
> left_c = right_c;
> }
>
> - /* The alpha channel is scaled to be in the [0, 255] interval,
> - * and the red/green/blue channels are scaled to be in [0, 1].
> - * This ensures that after premultiplication all channels will
> - * be in the [0, 255] interval.
> - */
> - la = (left_c->alpha * (1.0f/257.0f));
> - lr = (left_c->red * (1.0f/257.0f));
> - lg = (left_c->green * (1.0f/257.0f));
> - lb = (left_c->blue * (1.0f/257.0f));
> -
> - ra = (right_c->alpha * (1.0f/257.0f));
> - rr = (right_c->red * (1.0f/257.0f));
> - rg = (right_c->green * (1.0f/257.0f));
> - rb = (right_c->blue * (1.0f/257.0f));
> -
> + la = left_c->alpha * (1.0f / 65535.f);
> + lr = left_c->red * (1.0f / 65535.f);
> + lg = left_c->green * (1.0f / 65535.f);
> + lb = left_c->blue* (1.0f / 65535.f);
> +
> + ra = right_c->alpha * (1.0f / 65535.f);
> + rr = right_c->red * (1.0f / 65535.f);
> + rg = right_c->green * (1.0f / 65535.f);
> + rb = right_c->blue * (1.0f / 65535.f);
Ok, normalize channels in [0, 65535] range to [0, 1].
> lx = left_x * (1.0f/65536.0f);
> rx = right_x * (1.0f/65536.0f);
> -
> +
> if (FLOAT_IS_ZERO (rx - lx) || left_x == INT32_MIN || right_x == INT32_MAX)
> {
> + float factor = 1.0f / 2.0f;
> +
> walker->a_s = walker->r_s = walker->g_s = walker->b_s = 0.0f;
> - walker->a_b = (la + ra) / 2.0f;
> - walker->r_b = (lr + rr) / 510.0f;
> - walker->g_b = (lg + rg) / 510.0f;
> - walker->b_b = (lb + rb) / 510.0f;
> + walker->a_b = (la + ra) * factor;
> +
> + walker->r_b = (lr + rr) / 2.0f;
> + walker->g_b = (lg + rg) / 2.0f;
> + walker->b_b = (lb + rb) / 2.0f;
All "* factor"?
Newline between alpha and (r,g,b) ? I could rationalize why you might
have one between *_s and the *_b, but not *_s, a_b then \n et al.
> }
> else
> {
> float w_rec = 1.0f / (rx - lx);
>
> walker->a_b = (la * rx - ra * lx) * w_rec;
> - walker->r_b = (lr * rx - rr * lx) * w_rec * (1.0f/255.0f);
> - walker->g_b = (lg * rx - rg * lx) * w_rec * (1.0f/255.0f);
> - walker->b_b = (lb * rx - rb * lx) * w_rec * (1.0f/255.0f);
> -
> walker->a_s = (ra - la) * w_rec;
> - walker->r_s = (rr - lr) * w_rec * (1.0f/255.0f);
> - walker->g_s = (rg - lg) * w_rec * (1.0f/255.0f);
> - walker->b_s = (rb - lb) * w_rec * (1.0f/255.0f);
> +
> + walker->r_b = (lr * rx - rr * lx) * w_rec;
> + walker->g_b = (lg * rx - rg * lx) * w_rec;
> + walker->b_b = (lb * rx - rb * lx) * w_rec;
> +
> + walker->r_s = (rr - lr) * w_rec;
> + walker->g_s = (rg - lg) * w_rec;
> + walker->b_s = (rb - lb) * w_rec;
This too will work better as (a,r,g,b)_s \n (a,r,g,b)_b, I think.
> }
>
> walker->left_x = left_x;
> @@ -183,7 +182,7 @@ _pixman_gradient_walker_pixel (pixman_gradient_walker_t *walker,
>
> y = x * (1.0f / 65536.0f);
>
> - a = walker->a_s * y + walker->a_b;
> + a = (walker->a_s * y + walker->a_b) * 255.f;
> r = a * (walker->r_s * y + walker->r_b);
> g = a * (walker->g_s * y + walker->g_b);
> b = a * (walker->b_s * y + walker->b_b);
Ok.
So one question, I see no changes required for the reftests...
-Chris
More information about the Pixman
mailing list