[cairo] OVER / SOURCE optimization for cairo_paint

Antoine Azar cairo at antoineazar.com
Sat Feb 16 15:57:30 PST 2008


Hi Carl,

>Could you please attach a patch instead? That way we can more easily 
>review just what the changes are, and we can much more easily apply 
>the patch, and run your code.

I had actually purposely not attached a patch because the code is in 
"draft form". I'm more hoping for feedback on the logic for computing 
the source extents and such. Which also explains the comment format 
and the personal notes in the code :-) But I'm attaching the patch 
here (both inlined and as attachment). You can mostly ignore style 
issues for now, I'll clean it up when I'll submit a real patch.

>That's a comment that doesn't really add anything, so please remove 
>that. Also we don't use the C++-style one-line comments in cairo[*]. 
>So please just use the /*...*/ style instead.

Out of curiosity, why not? If I'm not mistaken, most C compilers 
accept them anyways? I find it much easier while developing to 
comment out blocks of code when the valid comments only use // and 
not /* */ (and I'm not a big fan of #if 0)

>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?

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;

      if (gstate->source->status)
         return gstate->source->status;
@@ -891,8 +901,98 @@ _cairo_gstate_paint (cairo_gstate_t *gstate)
      if (status)
         return status;

+    //Optimize a very common case of calling paint with an OVER 
operator for opaque surfaces.
+    //Replace it with a more efficient SOURCE operator, and 
constrain the operation to the source's extents.
+    if ( _cairo_pattern_is_opaque(&pattern.base) && (op == 
CAIRO_OPERATOR_OVER || op == CAIRO_OPERATOR_SOURCE))
+    {
+        pattern_union = (cairo_pattern_union_t *) &pattern;
+        switch (pattern_union->base.type)
+        {
+            case CAIRO_PATTERN_TYPE_SOLID:
+            case CAIRO_PATTERN_TYPE_LINEAR:
+            case CAIRO_PATTERN_TYPE_RADIAL:
+               op = CAIRO_OPERATOR_SOURCE;
+                break;
+            case CAIRO_PATTERN_TYPE_SURFACE:
+                //in all cases set the operator to source
+                op = CAIRO_OPERATOR_SOURCE;
+
+                if (pattern.surface.base.extend != CAIRO_EXTEND_NONE)
+                {
+                    //We'll need to fill the whole destination 
anyways, so go on with paint
+                    break;
+                }
+                else
+                {
+                    //create a path around the source's extents and 
call fill with that
+                    path = _cairo_path_fixed_create();
+
+                    //extents of the source
+                    status = 
_cairo_surface_get_extents(pattern.surface.surface, &extents);
+                    if (status) {
+                        printf("ERROR\n");
+                       return status; //instead continue without optimization?
+                    }
+
+                    //multiply by the source's inverse matrix and by 
the dest context transformation matrix
+                    surface_rect.x = (double)extents.x;
+                    surface_rect.y = (double)extents.y;
+                    surface_rect.width = 
surface_rect.x+(double)extents.width; //convert width and height to 
point coords for now
+                    surface_rect.height = 
surface_rect.y+(double)extents.height;
+
+                    //FIXME: isn't the inverse already available somewhere?
+                    matrix_invert = pattern.surface.base.matrix;
+                    cairo_matrix_invert(&matrix_invert);
+                    _cairo_matrix_transform_bounding_box (&matrix_invert,
+                            &surface_rect.x, &surface_rect.y, 
&surface_rect.width, &surface_rect.height, NULL);
+
+                    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);
+
+                    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?)?
+                    //       this is apparent in the 
filter-nearest-offset test
+//                    _cairo_gstate_user_to_backend (gstate, 
&surface_rect.x, &surface_rect.y);
+//                    _cairo_gstate_user_to_backend (gstate, 
&surface_rect.width, &surface_rect.height);
+
+                    //Go back to width and height instead of point coords
+                    surface_rect.width -= surface_rect.x;
+                    surface_rect.height -= surface_rect.y;
+
+                    x_fixed = _cairo_fixed_from_double (surface_rect.x);
+                    y_fixed = _cairo_fixed_from_double (surface_rect.y);
+
+                    dx_fixed = _cairo_fixed_from_double (surface_rect.width);
+                    dy_fixed = _cairo_fixed_from_double (surface_rect.height);
+
+                    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);
+
+                    if (status) {
+                        printf("ERROR\n");
+                       return status; //instead continue without optimization?
+                    }
+
+                    _cairo_gstate_fill(gstate, path);
+                    _cairo_path_fixed_destroy(path);
+                    return status;
+                }
+        }
+    }
+
      status = _cairo_surface_paint (gstate->target,
-                                  gstate->op,
+                                  op,
                                    &pattern.base);

      _cairo_pattern_fini (&pattern.base);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: overSource_optim.patch
Type: application/octet-stream
Size: 5760 bytes
Desc: not available
Url : http://lists.cairographics.org/archives/cairo/attachments/20080216/355bac83/attachment.obj 


More information about the cairo mailing list