[cairo] OVER / SOURCE optimization for cairo_paint

[Behdad Esfahbod]

Hi Antoine,

Good stuff.  Comments below.

On Sat, 2008-02-16 at 18:57 -0500, Antoine Azar wrote:

> >Here, you're introducing a very long 'if' block for the 
> >optimization. I think this would be much readable as a separate 
> >function. Maybe something 
> >like  cairo_gstate_paint_opaque_with_source or so. Fortunately, the 
> >current function is accepting only a single parameter, so it's not 
> >painful to pass a lot of arguments down to a
> >new function, (nested functions would be really nice---and support 
> >for them in gcc is rather tempting).
> 
> I don't know the code well enough yet, but it seems many 
> optimizations have been implemented here and there at various levels. 
> If it's possible at all, it would be good to move these optimizations 
> as early in the code path as possible, and engineer our functions 
> based on that. That was kind of the idea I mentionned to have a layer 
> between the user and the gstate functions to convert between "what 
> the user asked for" and "what we really want to do internally". Or is 
> that already the role of the functions in cairo.c or in gstate?

cairo.c should really be a wrapper around gstate.  And gstate should
really be a wrapper around surface and scaled-font.  This particular
optimization for example is more suitable to go in the surface layer,
for example because we use cairo_surface_paint() internally when we
don't have and don't need any gstate.

That said, there's this comment in cairo-surface.c that seems wrong:

    /* These operators aren't interpreted the same way by the backends;
     * they are implemented in terms of other operators in cairo-gstate.c
     */
    assert (op != CAIRO_OPERATOR_SOURCE && op != CAIRO_OPERATOR_CLEAR);

as gstate doesn't do anything specific about those operators.  That
happens in other places, not gstate.


> Looking forward to your feedback!
> Thanks,
> Antoine
> 
> diff --git a/src/cairo-gstate.c b/src/cairo-gstate.c
> index 0b775ca..9c764ec 100644
> --- a/src/cairo-gstate.c
> +++ b/src/cairo-gstate.c
> @@ -879,6 +879,16 @@ _cairo_gstate_paint (cairo_gstate_t *gstate)
>   {
>       cairo_status_t status;
>       cairo_pattern_union_t pattern;
> +    cairo_operator_t op = gstate->op;
> +
> +    //optimization related declarations
> +    cairo_rectangle_int_t extents;
> +    const cairo_pattern_union_t *pattern_union;
> +    cairo_fixed_t x_fixed, y_fixed;
> +    cairo_fixed_t dx_fixed, dy_fixed;
> +    cairo_rectangle_t surface_rect;
> +    cairo_path_fixed_t *path;
> +    cairo_matrix_t matrix_invert;

Most of these can be moved into the inner blocks they are used.  Inside the if for example.

> +                    surface_rect.x = floor(surface_rect.x);
> +                    surface_rect.y = floor(surface_rect.y);
> +                    surface_rect.width = ceil(surface_rect.width);
> +                    surface_rect.height = ceil(surface_rect.height);

Why is the floor/ceil needed here in the first place?

> +                    if (surface_rect.width <= 0 || surface_rect.height <= 0)
> +                       return CAIRO_STATUS_SUCCESS;
> +
> +                    cairo_matrix_transform_point 
> (&gstate->target->device_transform, &surface_rect.x, &surface_rect.y);
> +                    cairo_matrix_transform_point 
> (&gstate->target->device_transform, &surface_rect.width, &surface_rect.height);
> +
> +                    //FIXME: why doesn't user_to_backend implement 
> the floor and ceil as done above (and why do we need them in the 
> first place anyways?)?
...
> +                    status = _cairo_path_fixed_move_to (path, 
> x_fixed, y_fixed);
> +                    status |= _cairo_path_fixed_rel_line_to (path, 
> dx_fixed, 0);
> +                    status |= _cairo_path_fixed_rel_line_to (path, 
> 0, dy_fixed);
> +                    status |= _cairo_path_fixed_rel_line_to (path, 
> -dx_fixed, 0);
> +                    status |= _cairo_path_fixed_rel_line_to (path, 
> 0, -dy_fixed);
> +                    status |= _cairo_path_fixed_close_path (path);

The last line_to is not necessary.  It's included in close_path().


> +                    if (status) {
> +                        printf("ERROR\n");
> +                       return status; //instead continue without optimization?
> +                    }

No, no need to continue without the optimization.  If something went
wrong, something like out-of-memory should have happened.


-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759