[Pixman] [PATCH 1/2] Implement floating point gradient computation

[Adam Jackson]

On Thu, 2019-01-03 at 12:18 +0100, Maarten Lankhorst wrote:
> From: Basile Clement <basile-pixman at clement.pm>
> 
> This patch modifies the gradient walker to be able to generate floating
> point values directly in addition to a8r8g8b8 32 bit values.  This is
> then used by the various gradient implementations to render in floating
> point when asked to do so, instead of rendering to a8r8g8b8 and then
> expanding to floating point as they were doing previously.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

This series seems to be a pretty severe performance regression.
test/radial-perf-test on my machine goes from ~0.003 to ~0.006, which
is not great. At -O2 gcc doesn't seem to be smart enough to inline
radial_get_scanline (etc) into its callers or to specialize it for the
two possible pixel widths, but it does at -O3 and it doesn't fix the
performance. Likewise LTO doesn't help any.

What _does_ help is pasting in the old implementation of
_pixman_gradient_walker_pixel as the body of
pixman_gradient_walker_pixel_32. Apparently the out-of-line call to
pixman_contract_from_float is pretty punishing.

Cosmetic comments inline below...

> diff --git a/pixman/pixman-gradient-walker.c b/pixman/pixman-gradient-walker.c
> index 822f8e62bae7..9f11cf450e25 100644
> --- a/pixman/pixman-gradient-walker.c
> +++ b/pixman/pixman-gradient-walker.c
> @@ -122,10 +122,10 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
>  	    left_c = right_c;
>      }
>  
> -    /* The alpha channel is scaled to be in the [0, 255] interval,
> +    /* The alpha channel is scaled to be in the [0, 1] 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.
> +     * be in the [0, 1] interval.
>       */

This comment reads a bit funny now.

> @@ -103,10 +106,10 @@ linear_get_scanline_narrow (pixman_iter_t  *iter,
>      pixman_fixed_48_16_t dx, dy;
>      gradient_t *gradient = (gradient_t *)image;
>      linear_gradient_t *linear = (linear_gradient_t *)image;
> -    uint32_t *end = buffer + width;
> +    uint32_t *end = buffer + width * (Bpp / 4);
>      pixman_gradient_walker_t walker;
>  
> -    _pixman_gradient_walker_init (&walker, gradient, image->common.repeat);
> +    _pixman_gradient_walker_init (&walker,gradient, image->common.repeat);

Whitespace error.

> diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
> index 73a54146ddb1..0a07467c6593 100644
> --- a/pixman/pixman-private.h
> +++ b/pixman/pixman-private.h
> @@ -340,18 +340,18 @@ _pixman_image_validate (pixman_image_t *image);
>   */
>  typedef struct
>  {
> -    float		    a_s, a_b;
> -    float		    r_s, r_b;
> -    float		    g_s, g_b;
> -    float		    b_s, b_b;
> -    pixman_fixed_48_16_t    left_x;
> -    pixman_fixed_48_16_t    right_x;
> -
> -    pixman_gradient_stop_t *stops;
> -    int                     num_stops;
> -    pixman_repeat_t	    repeat;
> -
> -    pixman_bool_t           need_reset;
> +    float		     a_s, a_b;
> +    float		     r_s, r_b;
> +    float		     g_s, g_b;
> +    float		     b_s, b_b;
> +    pixman_fixed_48_16_t     left_x;
> +    pixman_fixed_48_16_t     right_x;
> +
> +    pixman_gradient_stop_t  *stops;
> +    int                      num_stops;
> +    pixman_repeat_t	     repeat;
> +
> +    pixman_bool_t            need_reset;
>  } pixman_gradient_walker_t;

This appears to just be adding another space between the type and slot
names? Seems unnecessary.

- ajax