[cairo] GSoC: Scan converting rasteriser update

M Joonas Pihlaja jpihlaja at cc.helsinki.fi
Thu Oct 2 02:32:08 PDT 2008


[I had some issues with my account yesterday, so I didn't
actually get mail from cairo-l and this reply won't thread
nicely.  Sorry.]

Jeff Muizelaar wrote:

> Here are some comments based on a look though some of the new
> code a while ago:

> Overall comments:
> - I don't really like the make_span_renderer interface.  It might be better if
>   instead of relying on the fallback surface, we had helpers that would be called from
>   the image surface's fill and stroke methods.

The existing rendering pipeline goes via the fallback surface
where there's a lot of code that implements drawing differently
based on the nature of the path, the compositing operation, and
the clip state, so plugging a spans rendering method alongside
the trapezoid rendering in cairo-surface-fallback.c seemed like
the right choice to me.

Reorganising the existing rendering framework was out of scope
for the project, which is what changes such as the proposed
image_surface_fill() sounds like:

>   e.g.
>   image_surface_fill() {
>     mask = span_rasterize_path_to_mask()
>     composite_surface_with_mask(mask)
>   }

Having said that, I'm not particularly happy about the state of
cairo-surface-fallback.c, especially now that there are at least
three different ways of rendering paths (regions, traps, and
spans.)

> Performance:
> - we seem to iterate over the path a number of times. It would be nice to avoid this
>  by perhaps merging passes or at least make it more clear when we are doing them.

I had a few unpushed commits in my branch at home, one of which
removes a useless round trip via cairo_polygon_t for fills.

> - on a related note: it would be nice to avoid having to convert to a polygon before calling
>  _cairo_polygon_fill_to_scan_converter. i.e. build the edge list directly
>  from the fixed path

> Style:
[snip style]

OK. Will fix.

> - _cairo_polygon_fill_to_scan_converter has an unnecessary:
>         if (status)
>		goto unwind;

It was more consistent to my eyes but sure it's unnecessary.
Gone.

> - cairo-alpha-spans.c:get_row() weird cosmetics

I'm not sure what you mean by cosmetics.  Weird gymnastics,
perhaps. :)  In any case, the cairo-alpha-spans code was written
in anticipation of integrating spans into the clipping process.
It is currently dead code.

Thanks for the review.  I'll get on the easy cleanups and dead
code elimination and make noise when they're pushed.

Cheers,

Joonas


More information about the cairo mailing list