[cairo] Quartz backend revamp

[Carl Worth]

On Wed, Aug 12, 2009 at 06:33:27PM +0200, Andrea Canciani wrote:
> I'm working on improving the quartz backend (first of all make it
> reasonably pass most/all the test suite).

Yes, and this is very appreciated. Thank you!

> I already posted some patches (
> http://lists.cairographics.org/archives/cairo/2009-August/017925.html
> ), here is one more.
> This patch makes the backend PASS a1-traps-sample and causes no regressions.

I really appreciate your attention to detail, (I love seeing people
running the test suite and using it to improve cairo). A couple of
minor comments on the patch:

> Subject: [PATCH] [quartz] Fix stroking/filling when antialias is disabled

The one-line description is great here. But you're missing the
following description of the change in the commit message. For
example, what was the bug? Different rounding needed for
non-antialiased rendering? Quartz has some funny rounding? Or cairo
does and we have to play games to make Quartz match?

You clearly did some investigation to figure things out here, and it's
important to capture that in the commit message.

> +static inline double
> +_cairo_fixed_round (cairo_fixed_t f)
> +{
> +    return _cairo_fixed_integer_floor (f + (CAIRO_FIXED_ONE >> 1));
> +}

It looks like this should live in cairo-fixed-private.h with the other
inline _cairo_fixed_* functions.

> -    if (CGContextIsPathEmpty (stroke->cgContext))
> -	CGContextMoveToPoint (stroke->cgContext, x, y);
> -    else
> -	CGContextAddLineToPoint (stroke->cgContext, x, y);
> +    CGContextAddLineToPoint (stroke->cgContext, x, y);

What's this change about? It doesn't seem obviously related given the
commit message.

>  static cairo_status_t
>  _cairo_quartz_cairo_path_to_quartz_context (cairo_path_fixed_t *path,
> -					    quartz_stroke_t *stroke)
> +					    cairo_quartz_surface_t *surface,
> +					    cairo_matrix_t *ctm_inverse,
> +					    cairo_antialias_t antialias)
>  {
> +    quartz_stroke_t stroke;
> +
> +    stroke.cgContext = surface->cgContext;
> +    stroke.ctm_inverse = ctm_inverse;
> +    stroke.antialias = antialias;

Why are some of the fields of quartz_stroke_t now being passed as
function arguments and then immediately assigned into the structure
again?

That makes the function have more arguments that I really like, but
worse, it opens a window during which their is inconsistent (or at the
very least redundant) data being passed around. Is there some reason
for this that I'm missing? Is this just to save one local variable
from the callers or something?

> -#if 0
> +#ifdef QUARTZ_DEBUG

I'm not sure what QUARTZ_DEBUG is all about[*], but whatever this
change is, it seems unrelated to the current patch, so should be
separate.

[*] Are we abusing "QUARTZ_" and "quartz_" as our own prefixes in this
file without putting something like "CAIRO_" or at the very least "_"
in front of them? We should fix that.

> -    if (cg_advances != &cg_advances_static[0]) {
> +    if (cg_advances != cg_advances_static)
>  	free (cg_advances);
> -    }
>  
> -    if (cg_glyphs != &glyphs_static[0]) {
> +    if (cg_glyphs != glyphs_static)
>  	free (cg_glyphs);
> -    }

These also look unrelated, right?

So just those few things and it looks great to me.

Andrea, do you have you a freedesktop.org account yet? With patches
like this, you should soon be pushing them yourself after getting
review on the list. See:

http://freedesktop.org/wiki/AccountRequests

Have fun!

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.cairographics.org/archives/cairo/attachments/20090812/aa26a625/attachment.pgp