[cairo] Tessellator regressions

[Carl Worth]

On Wed, 6 Dec 2006 02:38:26 +0200 (EET), M Joonas Pihlaja wrote:
> Attached are test cases and patches for fixing regressions in the
> tessellator.  They should fix most issues found so far, with the
> notable exception of the elusive 32nd bit in input coordinates.

Great work, Joonas! This is a nice, clean patch series. It's really
nice to see new test cases committed first to demonstrate bugs, and
then commits afterwards to fix them. (This is even more disciplined
a job than what I originally did during the new tessellator---which
left you wondering exactly what problem I was fixing when I threw in
those silly guard bits---so sorry about that).

A few, very minor comments on the patches below:

> + * appear in supporting documentation, and that the name of Joonas Pihlaja
> + * not be used in advertising or publicity pertaining to distribution
> + * of the software without specific, written prior permission.
> + * Joonas Pihlaja makes no representations about the suitability of this
> + * software for any purpose.  It is provided "as is" without express
> + * or implied warranty.
> + *
> + * JOONAS PIHLAJA DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
> + * NO EVENT SHALL JOONAS PIHLAJA BE LIABLE FOR ANY SPECIAL, INDIRECT OR

The blurb above is problematic in that it references the name of the
author in the disclaimers, so if we merge content from different
authors or a new author edits the file we end up duplicating the
blurbs or else having unwieldy, ever-growing blurbs.

This isn't your fault, as you are just copying existing blurbs, and I
haven't gone back and fixed all of these blurbs for old files like I
should.

But for new blurbs, I'm now using a much better version of the MIT
license, as published by OSI:

	http://www.opensource.org/licenses/mit-license.php

You can see this in a file I've recently added, for example:

	perf/box-outline.c

If you'd prefer to change them after the fact, rather than rewriting
your current git history, that's fine too.

> +    /* XXX: I wanted to be able to simply fill the nasty path to the
> +     * surface and then use a reference image to catch bugs, but the
> +     * renderer used when testing the postscript backend gets the
> +     * degeneracy wrong, thus leading to an (unfixable?) test case
> +     * failure.  Are external renderer bugs our bugs too?

No, external renderer bugs are not our bugs too. It's really hard to
get around this when using a non-cairo-based renderer like gs,
though. (Things end up being _much_ easier with svg and PDF content
where we can use cairo-based renderers such as librsvg and poppler).

Interestingly enough, there happens to be a cairo-based PostScript
rendering library hosted on freedesktop.org named hieroglyph. I just
happened to wander across it one day:

	http://hieroglyph.freedesktop.org/wiki/index

It actually looks pretty interesting, and it would be a fine little
project if someone wanted to look into integrating this into cairo's
test suite as a replacement for gs.

But in the meantime...

> +     * point sampling the tessellated path.  If there would be a way
> +     * to XFAIL only some backends we could do that for the .ps
> +     * backend only. */

We actually do have a mechanism to skip some tests for particular
backends. The mechanism is rather obscure, though, so don't feel bad
you didn't find it. A backend's create_<foo>_surface function, (which
exists in boilerplate/cairo-boilerplate.c), receives the name of the
test being performed. And if this function returns NULL then the test
just gets an UNTESTED status rather than a failure, (or even an
expected failure).

You can see in cairo-boilerplate.c that we use this mechanism to avoid
testing the non-antialiased-rendering tests on the vector backends
that have no way to request non-antialiased rendering when doing the
conversion to .png. You could (and should) use the same mechanism here
as well to avoid the rather awkward point-sampling you are doing here.

Again, if you want to fix this is a follow-up commit, that would be
fine too.

> +    "Tests that the tessellator doesn't produce obviously empty trapezoids",
> +    10, 10,
> +    draw
> +};
> +
> +static cairo_test_status_t
> +draw (cairo_t *cr_orig, int width, int height)
> +{
> +    int x,y;
> +    cairo_surface_t *surf = cairo_image_surface_create
> (CAIRO_FORMAT_ARGB32, width, height);

Same thing here. It looks really weird to be creating a new target
surface in a test, (since one of the primary purposes of the test
suite is to test the same code across all backends).

> +     * What it looke like with the bug, when the rightmost edge's end
> +     * is missed:
> +     *
> +     * |\    |\
> +     * |#\   |#\
> +     * |##\__|##\
> +     *     \#####|
> +     *      \####|

Oops. Nice catch. Thanks.

> +    /* Discrimination based on the edge pointers. */
> +    if (a->e1 < b->e1)
> +	return -1;
> +    if (a->e1 > b->e1)
> +	return +1;
> +    if (a->e2 < b->e2)
> +	return -1;
> +    if (a->e2 > b->e2)
> +	return +1;
> +    return 0;

It's amazing how many correctness fixes to the tessellator consist of
nothing more than adding yet another condition to the compare
functions to resolve some ambiguities. We've been adding these for
_years_ now. Anyway, I'm glad to have someone else helping out with
these now!

> +    /* XXX: Check that the extent of the coordinates is at most 2^31-1
> +     * units high or wide.  We're going to clamp the maximums to be
> +     * within 2^31 of the minimums if this isn't the case. XXX: This
> +     * is the wrong thing to do! */

I don't like the comment above. The last sentence "this is wrong" is a
bad thing to have in general. If you do need to put an "XXX" comment
in here, please explain what should be changed, don't just say that
something is wrong, or else we'll have little hope of ever fixing it.

The other thing that should be done in the comment is that a
description of what the code is actually doing should be cleanly
separated from the "XXX" part saying what should be fixed.

So, I don't know that the following is a technically accurate
description of the current situation, (that is, don't just copy this
text verbatim without verifying that it says what you are trying to
say). But I'm offering this as an example of the style I'd like to
see:

	/* The tessellation functions currently assume that no line
	   segment extends more than 2^31-1 in either dimension. We
	   guarantee this by simple clamping the maximum value, (and
	   yes, this generates an incorrect result).
	*/
	/* XXX: Rather than changing the input values, a better
	        approach would be to detect out-of-bounds input and
	        return a CAIRO_STATUS_OVERFLOW value to the user.
	*/

In fact, it may be that all you need to do to fix your version is to
eliminate the first "XXX". It looks like that's what happens a bit
later, (where again, the bad "this is wrong" comment shows up).

> +	     * respect ordering.  Currently it doesn't though, since
> +	     * CAIRO_BO_GUARD_BITS is zero.) */

I'm confused here. If we're making CAIRO_BO_GUARD_BITS 0, then why are
we preserving all this ugly code? Why don't we just throw away
anything that mentions the guard bits?

Anyway, I hope that's helpful. I think I feel pretty comfortable with
all of these going in as-is except for this last one. But use your
best judgment guided by my comments above.

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