[cairo] some fixes and improvements to current cvs version of
cairo
David Reveman
davidr at novell.com
Fri Jan 28 11:34:32 PST 2005
On Fri, 2005-01-28 at 11:17 -0500, Owen Taylor wrote:
> On Fri, 2005-01-28 at 16:45 +0100, David Reveman wrote:
> > I ran some test on current cvs version of cairo today and found a number
> > of problems. Most problems where simple or obvious to fix, I've attached
> > my current diff. It includes a lot of important fixes to the pattern
> > system, some clipping fixes, some improvements to the glitz backend and
> > an up to date version of the xcb backend.
> >
> > I'll probably not be able to write changelog entries and commit this
> > until next week, but you're very welcome to pick fixes from my diff and
> > commit them if you like. Most of them are obvious fixes so you should be
> > able to fill out changelog entries yourself.
>
> if CAIRO_HAS_FT_FONT
> -libcairo_ft_headers = cairo-ft.h
> +libcairo_ft_headers = cairo-ft.h cairo-ft-private.h
> libcairo_ft_sources = cairo_ft_font.c
> endif
>
> That isn't right ... cairo-ft-private.h is a source not a header in
> automake terminology ... it will get installed if you put it in
> include_HEADERS.
you're right, it should go into libcairo_ft_sources instead.
>
> - if (gstate->pattern)
> - cairo_pattern_destroy (gstate->pattern);
> -
> - gstate->pattern = pattern;
> cairo_pattern_reference (pattern);
> + cairo_pattern_destroy (gstate->pattern);
> + gstate->pattern = pattern;
>
> That looks fine, except that that _cairo_gstate_init() doesn't
> check the result of _cairo_pattern_create_solid() so gstate->pattern
> *can* be NULL. But that's a bug in the gstate creation not here.
yes, gstate->pattern should never be NULL.
>
> if (dest->y < src->y)
> dest->y = src->y;
>
> - if (dest->x + dest->width < src->x + src->width)
> - dest->width = dest->x + dest->width - dest->x;
> - else
> + if (dest->x + dest->width > src->x + src->width)
> dest->width = src->x + src->width - dest->x;
>
> - if (dest->y + dest->height < src->y + src->height)
> - dest->height = dest->y + dest->height - dest->y;
> - else
> + if (dest->y + dest->height > src->y + src->height)
> dest->height = src->y + src->height - dest->y;
>
> Still broken with your fix here ... the problem is that
> if dest->x gets clamped dest->width doesn't get reduced.
> It's easiest to intersect as with temporary x2,y2 and
> then recompute width/height at the end.
oh, I didn't realize it was broken, I just removed some unnecessary
code. I should have noticed that.
>
> {
> + cairo_surface_t *mask = NULL;
> +
> + /* alpha as mask surface */
> + if (pattern.color.alpha != 1.0)
> + {
> + mask = _cairo_surface_create_similar_solid (gstate->surface,
> + CAIRO_FORMAT_A8,
> 1, 1,
> + &pattern.color);
> + if (!mask)
> + {
> + _cairo_pattern_fini (&pattern);
> + return CAIRO_STATUS_NO_MEMORY;
> + }
> +
> + pattern.color.alpha = 1.0;
> + cairo_surface_set_repeat (mask, 1);
> + }
>
> This pushes one of the few hw-accelerated fast paths I have in the
> win32 backend through the slow path. If the backend needs to use a mask
> for overall-alpha it can create one itself.
ok, that should all go away then. But we'll have to do something to the
xlib/xcb/image backends, as _cairo_pattern_get_surface will otherwise do
overall-alpha with an intermediate surface.
>
> (Seems you could use overall alpha for GL as well in a few cases
> 4-channel-texture * alpha over dest is possible with GL_MODULATE,
> right?)
Yes, repeating 1x1 surface in glitz are internally handled as solid
colors. So, when a repeating 1x1 surface is passed as mask to the
composite function, glitz will do GL_MODULATE with the source surface.
>
> - }
> + } else {
> + *itx = 0;
> + *ity = 0;
> + }
>
> I think that's wrong. If it's not an integer translation there is
> no meaningful value for itx/ity and the caller must avoid using
> the results.
makes sense.
>
> + /* Two round-trips to check if this fast path is possible, not
> pretty. */
> if (operator == CAIRO_OPERATOR_SRC
> && !mask
> && _cairo_matrix_is_integer_translation(&(src->base.matrix),
> - &src_x_off, &src_y_off)
> - && _cairo_matrix_is_integer_translation(&(dst->base.matrix),
> - &dst_x_off, &dst_y_off))
> {
> + &src_x_off, &src_y_off)
> &&
> + XGetGeometry (dst->dpy, dst->drawable, &dst_root,
> + &ignore, &ignore, &u_ignore, &u_ignore, &u_ignore,
> + &dst_depth) == Success &&
> + XGetGeometry (dst->dpy, src->drawable, &src_root,
> + &ignore, &ignore, &u_ignore, &u_ignore, &u_ignore,
> + &src_depth) == Success &&
> + dst_root == src_root && dst_depth == src_depth) {
> /* Fast path for copying "raw" areas. */
>
> "not pretty" doesn't begin to describe that.
:-)
> It would be
> considerably more efficient to trap errors and just try it ... one
> roundtrip instead of two. Or just assume that the server is going
> to be OK at optimizing this case. Or, since we are already have most
> of this information when creating the surface save that and use it.
>
> But I don't think the above change is better than just commenting
> out the fast path.
I didn't really want to get this ugly thing into cairo, I just got tired
of this fast path constantly crashing some of my cairo applications
after each time I did a cvs update. I think this ugly code is a good
example to why we don't want this kind of checking for fast paths in the
xlib and xcb backends, a solution that trapped errors wouldn't be very
pretty either. If someone could come up with a nice way to do this, that
would be great, but for now, I believe that just commenting out this
fast path, is definitely the best solution.
>
> Generally, the changes look good ... noticed the problem with
> prepare/restore surface and gradients last night but hadn't looked
> into fixing it yet.
>
> Regards,
> Owen
>
thanks for looking through the patch.
-David
More information about the cairo
mailing list