Bug 15349

Created attachment 15674 [details]
Output of test case (cairo-1.5.14, pixman-0.9.6)

Thanks for the bug report Owen.

This is fixed now along with a new test for the test suite.

See:

http://gitweb.freedesktop.org/?p=cairo;a=commit;h=731e121c802a7b1c3429d1bde7a93bc471d70880

as well as the previous two commits.

-Carl

Created attachment 15692 [details] [review]
Patch tightning bounds in some cases

Ouch - a locking failure ;-(

I took at a stab at fixing this myself this evening, since I figured
it was my buggy code to begin with. Mostly it was not different from
your patch, but I did try to handle the cases where filtering should
not have an effect (source pixels 1:1 with destination pixels) and
give them tighter bounds.

In terms of number-of-pixels composited, it's unlikely that this
will make a significant difference, but using a tight bound may
makeit easier the backend to optimize things that couldn't otherwise
be optimized since it avoids sampling outside the 

I merged this residual with the current code, feel free to use or not
use as you see fit. It's certainly debatable whether it's worthwhile
or just possibly-bug-inducing complexity.

P.S. - I'm not sure I entirely agree with the comment:

   (since currently no specific backends that could do custom
   filters are calling _cairo_pattern_get_extents)

_cairo_pattern_get_extents() gets used via the "fallback" code for
paint()/fill() for the Xlib backend, and the X server implementation
is certainly free to implement a better FilterBest.

P.P.S - as has been discussed previously:

   #define CAIRO_FILTER_DEFAULT CAIRO_FILTER_BEST

  in cairoint.h is a time-bomb waiting to happen. FILTER_DEFAULT
  should be GOOD.

(In reply to comment #3)
> Created an attachment (id=15692) [details]
> Patch tightning bounds in some cases
> 
> Ouch - a locking failure ;-(

Heh, sorry about that.

> I merged this residual with the current code, feel free to use or not
> use as you see fit. It's certainly debatable whether it's worthwhile
> or just possibly-bug-inducing complexity.

It's fine I think . But it makes me wonder if we shouldn't use a function like that to fast-path to NEAREST for the actual rendering as well.

> P.S. - I'm not sure I entirely agree with the comment:
> 
>    (since currently no specific backends that could do custom
>    filters are calling _cairo_pattern_get_extents)
> 
> _cairo_pattern_get_extents() gets used via the "fallback" code for
> paint()/fill() for the Xlib backend, and the X server implementation
> is certainly free to implement a better FilterBest.

OK, a fair point. It did still like it would be a *lot* of code complexity for zero benefit right now to add a new backend function for querying filter expansion.

> P.P.S - as has been discussed previously:
> 
>    #define CAIRO_FILTER_DEFAULT CAIRO_FILTER_BEST
> 
>   in cairoint.h is a time-bomb waiting to happen. FILTER_DEFAULT
>   should be GOOD.

Yes, thanks for the reminder.

-Carl

Comment on attachment 15692 [details] [review]
Patch tightning bounds in some cases

More extensive version of my last patch that does the optimization to nearest when drawing now filed as bug 15367

It's been pointed that what's currently in git is causing some rendering
artifacts:

 http://people.mozilla.com/~vladimir/misc/broken.c
 http://people.mozilla.com/~vladimir/misc/broken-xlib-rgb24-out.png

What's going on here is that with the current version, 
_cairo_pattern_acquire_surface() is being called:

 - with an integer translation or identity matrix
 - with a bounds bigger than the source surface

I think this could happen previously, but only when
!_cairo_operator_bounded_by_source(op).

When the above two things are true, there's an obvious bug in 
_cairo_pattern_acquire_surface_for_surface()

            /* Otherwise, we first transform the rectangle to the
             * coordinate space of the source surface so that we can
             * clone only that portion of the surface that will be
             * read. */
            [...]
            if (! _cairo_matrix_is_identity (&attr->matrix)) {
                double x1 = x;
                double y1 = y;
                double x2 = x + width;
                double y2 = y + height;
                cairo_bool_t is_tight;

                _cairo_matrix_transform_bounding_box  (&attr->matrix,
                                                       &x1, &y1, &x2, &y2,
                                                       &is_tight);

                /* The transform_bounding_box call may have resulted
                 * in a region larger than the surface, but we never
                 * want to clone more than the surface itself, (we
                 * know we're not repeating at this point due to the
                 * above.
                 *
                 * XXX: The one padding here is to account for filter
                 * radius.  It's a workaround right now, until we get a
                 * proper fix. (see bug #10508)
                 */
                x = MAX (0, floor (x1) - 1);
                y = MAX (0, floor (y1) - 1);
                width = MIN (extents.width, ceil (x2) + 1) - x;
                height = MIN (exts.height, ceil (y2) + 1) - y;
            }

So the clamp to the bounds of the surface only occurs when the matrix
is not the identity.

With the patch in bug 15367 the problem won't be apparent because we
use the tight bounds on the source surface for the identity matrix, and
there will be no regression, but I think you can still have the 
problem with other operations, so a separate tiny fix is probably more
appropriate.

I'll come up with a patch, test and attach here. There is a separate
problem with this function I noticed on inspection ... if x/y are
> 0 when we grab from the source surface, I think we'll draw
the wrong result since we make no adjustment for the partial source
surface. I'll file that separately.

Created attachment 15740 [details]
My suggested patch

This patch moves the rectangle intersection outside of the identity-matrix
optimization, and also replaces the open-coded intersection (which would
have been buggy if the rectangles didn't intersect at all say) with
cairo_rectange_interact().

I get a failure with the xlib version of filter-bilinear-extents with
this patch, but looking at the output, it's a small variation evenly
distributed over the image and I think attributable to getting hardware
acceleration of the scaling.

For a full test run, I'm not really sure what failures are "expected"
on my hardware/software and what not, so testing yourself would be
suggested. (I saw an extend reflect failure that bothered me a bit,
but I can't see how it could have anything to do with this patch 
since extend reflect doesn't even get down to the section of code
that I changed. But if that's a regression, then the setup code for
the test must be breaking somehow.)

Last mentioned issue filed as bug 15389

Proposed patch fixes the problem for me (as I wrote in #cairo).

Created attachment 15742 [details] [review]
Small revision of previous patch (regenerated, obsoletes)

The last patch had a small but crucial bug, needs to be fixed as:

-               sampled_area.width = ceil (x2) + 1 - x;
-               sampled_area.height = ceil (y2) + 1 - y;
+               sampled_area.width = ceil (x2) + 1 - sampled_area.x;
+               sampled_area.height = ceil (y2) + 1 - sampled_area.y;

I've attached a new version of the patch with that fix. (Still 
recommending reverting the whole mess for now until after 1.6)

I've reviewed the patch series between this and bug 15367. The net result is a good bit of defensive code that replaces some open-coding intersection checking, centralizes some assumptions about filter radius and corrects a bug where we missed a check for the filter radius in computing the pattern extents. So I've taken the liberty of updating the patches and pushing to master as part of the great XFAIL hunt, ending with commit 3b33d49d37a5751e7848516c468b323e19c34bea.