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

[Bill Spitzak]

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.

> 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.

>> +#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.

>> +	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.

> 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.

>>   #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.

>> -    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);
>>       }