[cairo] [PATCH] V3 image: xlib/xcb backends use filtering to match image backend

[Bryce Harrington]

On Fri, Oct 03, 2014 at 03:12:02PM -0700, Bill Spitzak wrote:
> On 10/03/2014 01:14 PM, Bryce Harrington wrote:
> 
> >Since we're down to the wire on 1.14.0, I'm going to hold this patch for
> >1.4.1 since there's a few things I have questions on, and it looks like
> >there's some areas where this needs further development.  Also, this
> >includes some behavioral changes, which I assume are good but perhaps
> >need some more discussion?
> 
> Okay but your previous email said you wanted this for 1.14.0. Sorry
> I did not get the cleaned up patch in fast enough.

Yeah, my original thinking was that this patch would enable us to take
advantage of using the new filters from pixman when it becomes
available.  But obviously that'll require the config logic to detect
it...  and its probably better not to rush the work.

> >You might consider breaking this patch into several patches, since this
> >is making several conceptually distinct changes.
> 
> Will try to do so and get this posted real soon. I have some further
> changes to the bounding region calculations I would like to
> contribute as well.

Thanks.

> >>+#define PIXMAN_HAS_GOOD_BEST 0 /* move this to config.h! */
> >
> >Until we have config rules supporting this, the added defines below
> >won't actually do anything.  This is probably handy for testing during
> >development, but I'd rather wait until it's actually hooked up.
> 
> Ok I will leave this out. I tested it and it works. I was trying to
> make it clear what code could be removed if pixman does the
> filtering.

If the pixman patch lands and we get a new pixman release, we could just
skip all this, up our pixman version requirement, drop the old code, and
call it good.  I don't think anyone would find it surprising that a new
cairo would need a new pixman.

> >>+	case CAIRO_FILTER_GOOD:
> >>+	    pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
> >>+	    kernel = KERNEL_BOX;
> >>+	    /* Clip the filter size to prevent extreme slowness. This
> >>+	       value could be raised if 2-pass filtering is done */
> >>+	    if (dx > 16.0) dx = 16.0;
> >>+	    if (dy > 16.0) dy = 16.0;
> >>+	    /* Match the bilinear filter for scales > .75: */
> >>+	    if (dx < 1.35) dx = 1.0;
> >>+	    if (dy < 1.35) dy = 1.0;
> >
> >This should probably be mentioned more explicitly in the changelog that
> >you're changing the bilinear filtering for scales > 75% rather than 100%
> >as before.  (And maybe this should be broken out as its own patch, to
> >ease future testing/cherrypicking/reverting needs.)
> 
> The current version (ie my version where the image backend does
> filtering) is using the same cutoff. Look at the code removed from
> _pixman_image_set_properties.

Ok
 
> >Also, you're using 1.35 here and a few other places as a constant for
> >representing a .75 scale.  I assume this is 1/75, but that's actually
> >1.3333, so I'm curious why you picked 1.35 specifically?  If it is just
> >representing 1/75, then might the code be clearer to test against
> >1./75.?
> 
> The value was arrived at using a program that toggled between BOX
> and BILINEAR at different scales. I used two images, one a photo of
> a picket fence, and another a stress test of alternating 1 and 0
> vertical lines. The size was chosen at the point where I thought the
> box started to show fewer artifacts than bilinear.
> 
> Only while working on this patch did I realize 1.35 was pretty close
> to 4/3 or 1/.75 so I started making the comments say .75 scale.
> Since my experiment was subjective it is probably ok to change the
> cutoff to exactly this value.

Ahh.  In that case another option would just be to add it as a #define
constant.

> >>  #define XCB_RENDER_HAS_PICTURE_TRANSFORM(surface)	XCB_RENDER_AT_LEAST((surface), 0, 6)
> >>  #define XCB_RENDER_HAS_FILTERS(surface)			XCB_RENDER_AT_LEAST((surface), 0, 6)
> >>+#define XCB_RENDER_HAS_FILTER_GOOD(surface) FALSE
> >>+#define XCB_RENDER_HAS_FILTER_BEST(surface) FALSE
> >
> >I'm gathering these are just stubs and would need further work to hook
> >up with pixman if/when it includes the necessary functionality?
> 
> Yes, I just wanted to make sure they are there so they are easy to
> find and in the code where they need to be checked.
> 
> >>      if (pattern->type == CAIRO_PATTERN_TYPE_SURFACE) {
> >>-	cairo_filter_t filter;
> >>-
> >>-	filter = pattern->filter;
> >>-	if (_cairo_matrix_is_pixel_exact (&pattern->matrix))
> >>-	{
> >>-	    filter = CAIRO_FILTER_NEAREST;
> >>-	}
> >>-
> >>-	if (! (filter == CAIRO_FILTER_NEAREST || filter == CAIRO_FILTER_FAST)) {
> >>-	    if ((flags & CAIRO_XCB_RENDER_HAS_FILTERS) == 0)
> >>-		return FALSE;
> >>+	switch (pattern->filter) {
> >>+	case CAIRO_FILTER_FAST:
> >>+	case CAIRO_FILTER_NEAREST:
> >>+	    return (flags & CAIRO_XCB_RENDER_HAS_PICTURE_TRANSFORM) ||
> >>+		_cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL);
> >>+	case CAIRO_FILTER_GOOD:
> >>+	    return flags & CAIRO_XCB_RENDER_HAS_FILTER_GOOD;
> >>+	case CAIRO_FILTER_BEST:
> >>+	    return flags & CAIRO_XCB_RENDER_HAS_FILTER_BEST;
> >>+	case CAIRO_FILTER_BILINEAR:
> >>+	case CAIRO_FILTER_GAUSSIAN:
> >>+	default:
> >>+	    return flags & CAIRO_XCB_RENDER_HAS_FILTERS;
> >
> >Could you explain the above refactoring?  I don't grok how you get to
> >the new code from the old.
> 
> As analyze_filter has been called by this point, it will have
> already changed pixel_exact transforms to use NEAREST filter so that
> test is not needed.
> 
> I copied the test for integer translation from the xlib code as it
> seems to apply here too. But in fact any translation will work when
> the filter is NEAREST, should support that as well.
> 
> Otherwise I rearranged the code into a case statement to return the
> same value as the if (the GOOD/BEST options are false currently).
> 
> XLib version of same patch for comparison, note it does not check
> for pixel_exact and has the integer_translate test.

Ah gotcha.  Might help to have each of those changes as their own patch,
so the steps in the transition are clearer.

> >>-    if (! CAIRO_RENDER_HAS_PICTURE_TRANSFORM (display)) {
> >>-	if (!_cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL))
> >>-	    return FALSE;
> >>-    }
> >>-
> >>-    if (! CAIRO_RENDER_HAS_FILTERS (display)) {
> >>-	    /* No filters implies no transforms, so we optimise away BILINEAR */
> >>+    switch (pattern->filter) {
> >>+    case CAIRO_FILTER_FAST:
> >>+    case CAIRO_FILTER_NEAREST:
> >>+	return CAIRO_RENDER_HAS_PICTURE_TRANSFORM (display) ||
> >>+	    _cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL);
> >>+    case CAIRO_FILTER_GOOD:
> >>+	return CAIRO_RENDER_HAS_FILTER_GOOD (display);
> >>+    case CAIRO_FILTER_BEST:
> >>+	return CAIRO_RENDER_HAS_FILTER_BEST (display);
> >>+    case CAIRO_FILTER_BILINEAR:
> >>+    case CAIRO_FILTER_GAUSSIAN:
> >>+    default:
> >>+	return CAIRO_RENDER_HAS_FILTERS (display);
> >>      }

Thanks again,
Bryce