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

[Bill Spitzak]

On 09/23/2014 12:59 AM, Uli Schlachter wrote:
> Hi,
>
> no idea about if this is a good idea. I'll let other decide about that.

Ok latest word is that it is considered a good idea.

>> +	    break;
>>   	} else {
>>   	    /* 0.5 is enough for a bilinear filter. It's possible we
>>   	     * should defensively use more for CAIRO_FILTER_BEST, but
>
> This seems unnecessary? There is a "break" after the else branch...

Thanks. That was an error from my editing the patch to reduce the amount 
of diff.

>>   #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
>
> This could need some comment explaining things. "Cairo 1.14 changes the meaning
> of this and suddenly RENDER doesn't match" or something like that.

These flags just mean "send Good/best to xlib/xcb rather than emulate it 
using the image fallback". The assumption is that if X is fixed to 
support good/best, it will produce identical (or similar) output, so 
when these flags are turned on the results will be identical, just that 
the filtering is done in the server rather than in the image fallback.

> Also, what's the relationship between HAS_FILTERS and HAS_FILTER_GOOD/BEST?
> Could also need a comment.

HAS_FILTERS is really being treated as HAS_FILTER_BILINEAR now. May be a 
good idea to rename this.

>> -    if (! _cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL)) {
>> -	if ((flags & CAIRO_XCB_RENDER_HAS_PICTURE_TRANSFORM) == 0)
>> -	    return FALSE;
>> -    }
>
> Why are you removing this check? This seems like an unrelated change to this
> patch. And yes, you aren't moving this check around, for quite some pattern
> types you are entirely removing this.

I was under the impression that all gradient pattern types are rejected 
or accepted by other code here, but will check into this. For image 
patterns the analyze_filter has been run already and the filter changed 
to NEAREST so this test is redundant.

I will try to look into this a bit more. It is confusing and there are 
differences between the xlib and xcb versions, my assumption in general 
was that the xlib version, being used more, was better tested.

> This makes me wonder: Did you run this patch through the test suite? What were
> the results? Could you mention so in the commit message?

I do need to run the tests! Guilty as charged. Will have the results 
with my next version of the patch and hopefully this will give some 
hints that will help fix the confusing sets of if statements in these 
functions.

>> +    } else if (pattern->type == CAIRO_PATTERN_TYPE_MESH) {
>> +	return FALSE;
>>       } else { /* gradient */
>>   	if ((flags & CAIRO_XCB_RENDER_HAS_GRADIENTS) == 0)
>>   	    return FALSE;
>> @@ -435,9 +434,8 @@ _pattern_is_supported (uint32_t flags,
>>   	{
>>   	    return FALSE;
>>   	}
>> +	return TRUE;
>>       }
>> -
>> -    return pattern->type != CAIRO_PATTERN_TYPE_MESH;
>
> Should this become an ASSERT_UNREACHED? If not, what happens when the end of
> this function is reached? I'd expect some compiler warnings about a function
> without a return statement about the end.

Again I need to look into this, but the old code returned true for 
pattern types that are not mesh, and I added an if to return false for 
mesh, therefore this one should return true. But you may be right that 
this is never reached.