Re: [patch] gl: fix gradient color texture generation

On Wed, 25 Jul 2012 22:41:34 +0000, "Henry (Yu) Song - SISA" <[email protected]> wrote:
> commit 143dcad36f057ed2bb4fc2970781a4efc9386602
> Author: Henry Song <[email protected]>
> Date:   Wed Jul 25 15:34:52 2012 -0700
> 
>     gl: (1) increase minimal gradient texture width from  to 256.
>     (2) set gradient color strip center to be a little offset from pixel
>     center
>     
>     (1) gradient texture width of 8 is too small that results in lost of
>     color precision when gradient end/start points are far apart.
>     
>     (2) due to a "bug" in pixman, we should not set p1.x and p2.x to be
>     exact of pixel center - this results in a out-of-bound when pixman
>     loops of over stops.
>     
>     fix test cases: huge-radial, huge-linear.
>     bring test case: linear-gradient-large closer to reference image
> 
> diff --git a/src/cairo-gl-gradient.c b/src/cairo-gl-gradient.c
> index ffb5468..c060ca1 100644
> --- a/src/cairo-gl-gradient.c
> +++ b/src/cairo-gl-gradient.c
> @@ -52,7 +52,12 @@ _cairo_gl_gradient_sample_width (unsigned int                 n_stops,
>      unsigned int n;
>      int width;
>  
> -    width = 8;
> +    /*  minimal 8 sample width is too small for situation where 
> +     * gradient start/end points are far apart.  In GL backend, the 
> +     * gl shader picks gradient color by interpolation.  If the texture
> +     * width is too small, it results in lose of color precision
> +     */
But it should only be picking the minimum width if the total variation
is very small. Can you give an example which produces a subpar result
currently?

Elsewhere I have just used a max_texture_size ramp since the cache hit
rate is usually high enough not to worry about the creation cost.

> +    width = 256;
>      for (n = 1; n < n_stops; n++) {
>  	double dx = stops[n].offset - stops[n-1].offset;
>  	double delta, max;
> @@ -80,6 +85,7 @@ _cairo_gl_gradient_sample_width (unsigned int                 n_stops,
>  	    width = ramp;
>      }
>  
> +    /* increment by 8 is OK */
Maybe rounding to the next pot would be better for more hardware?

>      width = (width + 7) & -8;
>      return MIN (width, 1024);
>  }
> @@ -124,9 +130,17 @@ _cairo_gl_gradient_render (const cairo_gl_context_t    *ctx,
>  	pixman_stops[i].color.alpha = stops[i].color.alpha_short;
>      }
>  
> -    p1.x = _cairo_fixed_16_16_from_double (0.5);
> +    /* due to a "bug" in pixman, we should not use exact center of pixel
> +     * as the p1.x and p2.x.  This is beause pixman generates gradient
> +     * color by interpolate two adjacent stops.  And the step of 
> +     * interpolation is computed from p2.x - p1.x.  If we have set
> +     * at the center of pixel, it results pixman picking stop colors from
> +     * out-of-bound stops array.  see _gradient_walker_reset () in
> +     * pixman-gradient-walker.c
> +     */
> +    p1.x = _cairo_fixed_16_16_from_double (0.6);
How about pixman_fixed_1 - pixman_fixed_1_2 + pixman_fixed_epsilon
which should be (1 << 16) - (1 << 8) + 1?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo