[cairo] [PATCH] xcb: Initialize font options from Xft resources

Uli Schlachter psychon at znc.in
Wed Sep 3 23:23:38 PDT 2014


Hi,

thanks for looking into this.

Where did you look into how these resources are implemented and how they should
be parsed? Any source that should/can be quoted?

On 03.09.2014 23:04, Lukáš Lalinský wrote:
> From 0be3f607078a4dd24273b793a07240d05f0aff1f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Lalinsk=C3=BD?= <lukas at oxygene.sk>
> Date: Wed, 3 Sep 2014 22:53:55 +0200
> Subject: [PATCH] xcb: Initialize font options from Xft resources
> 
> There is a similar code in the Xlib backend. The logic here is the same, but
> XCB doesn't support X resources directly, so there is some custom code
> to get and parse the resources from the root window.
> ---
>  src/cairo-xcb-private.h |   6 ++
>  src/cairo-xcb-screen.c  | 271 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/cairo-xcb-surface.c |   6 +-
>  3 files changed, 280 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cairo-xcb-private.h b/src/cairo-xcb-private.h
> index 134100a..b343b75 100644
> --- a/src/cairo-xcb-private.h
> +++ b/src/cairo-xcb-private.h
> @@ -199,6 +199,9 @@ struct _cairo_xcb_screen {
>      cairo_list_t link;
>      cairo_list_t surfaces;
>      cairo_list_t pictures;
> +
> +    cairo_bool_t has_font_options;
> +    cairo_font_options_t font_options;
>  };
>  
>  struct _cairo_xcb_connection {
> @@ -357,6 +360,9 @@ _cairo_xcb_screen_get_gc (cairo_xcb_screen_t *screen,
>  cairo_private void
>  _cairo_xcb_screen_put_gc (cairo_xcb_screen_t *screen, int depth, xcb_gcontext_t gc);
>  
> +cairo_private cairo_font_options_t *
> +_cairo_xcb_screen_get_font_options (cairo_xcb_screen_t *screen);
> +
>  cairo_private cairo_status_t
>  _cairo_xcb_screen_store_linear_picture (cairo_xcb_screen_t *screen,
>  					const cairo_linear_pattern_t *linear,
> diff --git a/src/cairo-xcb-screen.c b/src/cairo-xcb-screen.c
> index 2858d23..06594a7 100644
> --- a/src/cairo-xcb-screen.c
> +++ b/src/cairo-xcb-screen.c
> @@ -30,11 +30,263 @@
>   *    Chris Wilson <chris at chris-wilson.co.uk>
>   */
>  
> +#include <ctype.h>
> +
>  #include "cairoint.h"
>  
>  #include "cairo-xcb-private.h"
>  #include "cairo-list-inline.h"

I wonder if this doesn't better belong into something like a new
cairo-xcb-resources.c to isolate it better from the rest. No strong opinion on this.

> +#include "cairo-fontconfig-private.h"
> +
> +static void
> +parse_boolean (const char *v, cairo_bool_t *out)
> +{
> +    char c0, c1;
> +
> +    c0 = *v;
> +    if (c0 == 't' || c0 == 'T' || c0 == 'y' || c0 == 'Y' || c0 == '1')
> +        *out = TRUE;
> +    if (c0 == 'f' || c0 == 'F' || c0 == 'n' || c0 == 'N' || c0 == '0')
> +        *out = FALSE;
> +    if (c0 == 'o') {
> +        c1 = v[1];
> +        if (c1 == 'n' || c1 == 'N')
> +            *out = TRUE;
> +        if (c1 == 'f' || c1 == 'F')
> +            *out = FALSE;
> +    }
> +}
> +
> +static void
> +parse_integer (const char *v, int *out)
> +{
> +    char *e;
> +    int value;
> +
> +#if CAIRO_HAS_FC_FONT
> +    if (FcNameConstant ((FcChar8 *) v, out))
> +        return;
> +#endif
> +
> +    value = strtol (v, &e, 0);
> +    if (e != v)
> +        *out = value;
> +}
>
> +struct resource_state {
> +    cairo_bool_t xft_antialias;
> +    int xft_lcdfilter;
> +    cairo_bool_t xft_hinting;
> +    int xft_hintstyle;
> +    int xft_rgba;
> +};
> +
> +struct resource_parser {
> +    int buffer_size;
> +    int bytes_in_buffer;
> +    char* buffer;
> +    struct resource_state *state;
> +};

Should be renamed to resource_parse_lines(). In the caller below I wondered "why
does this only parse a single line?".

> +static int
> +resource_parse_line (struct resource_parser *parser)
> +{
> +    char *newline, *name, *value;
> +
> +    name = parser->buffer;
> +    while (1) {

I know that this break is necessary to make the parsing-in-parts work, but this
also means that the very last line of the resources property won't be parsed if
it doesn't end in a line break. Is this intended?

> +        newline = strchr (name, '\n');
> +        if (newline == NULL)
> +            break;
> +
> +        value = strchr (name, ':');
> +        if (value == NULL)
> +            break;
> +
> +        *newline++ = 0;
> +        *value++ = 0;

isspace() is locale-dependent. No idea if this actually behaves different in any
locale, but could we please avoid it anyway?

> +        while (isspace(*value))
> +            value++;
> +
> +        if (strcmp(name, "Xft.antialias") == 0)
> +            parse_boolean(value, &(parser->state->xft_antialias));
> +        else if (strcmp(name, "Xft.lcdfilter") == 0)
> +            parse_integer(value, &(parser->state->xft_lcdfilter));
> +        else if (strcmp(name, "Xft.rgba") == 0)
> +            parse_integer(value, &(parser->state->xft_rgba));
> +        else if (strcmp(name, "Xft.hinting") == 0)
> +            parse_integer(value, &(parser->state->xft_hinting));
> +        else if (strcmp(name, "Xft.hintstyle") == 0)
> +            parse_integer(value, &(parser->state->xft_hintstyle));
> +
> +        name = newline;
> +    }
> +
> +    return name - parser->buffer;
> +}
> +
> +static void
> +resource_parser_init (struct resource_parser *parser, struct resource_state *state)
> +{
> +    parser->buffer_size = 0;
> +    parser->bytes_in_buffer = 0;
> +    parser->buffer = 0;
> +    parser->state = state;
> +}
> +
> +static void
> +resource_parser_done (struct resource_parser *parser)
> +{
> +    free (parser->buffer);
> +}
> +
> +static void
> +resource_parse (struct resource_parser *parser, const char *data, int length)
> +{
> +    int bytes_parsed;
> +
> +    if (parser->bytes_in_buffer + length > parser->buffer_size) {
> +	parser->buffer_size = parser->bytes_in_buffer + length + 1;

realloc() can fail, this needs error handling.

> +	parser->buffer = realloc(parser->buffer, parser->buffer_size);
> +    }
> +
> +    memmove (parser->buffer + parser->bytes_in_buffer, data, length);
> +    parser->bytes_in_buffer += length;
> +    parser->buffer[parser->bytes_in_buffer] = 0;
> +
> +    bytes_parsed = resource_parse_line (parser);
> +
> +    if (parser->bytes_in_buffer > bytes_parsed) {
> +	memmove (parser->buffer, parser->buffer + bytes_parsed, parser->bytes_in_buffer - bytes_parsed);

Shouldn't this always subtract bytes_parsed, not just when it happens to be
smaller than bytes_in_buffer? In other words: What happens if the buffer end was
on a line break?

> +	parser->bytes_in_buffer -= bytes_parsed;
> +    }
> +}
> +
> +static void
> +get_resources(xcb_connection_t *connection, xcb_screen_t *screen, struct resource_state *state)
> +{
> +    xcb_get_property_cookie_t rm_cookie;
> +    xcb_get_property_reply_t *rm_reply;
> +    struct resource_parser parser;

has_more_data should be of type cairo_bool_t.

> +    int offset = 0, has_more_data = 0;
> +
> +    resource_parser_init (&parser, state);
> +
> +    do {

Please set has_more_data to 0 / FALSE here to make it easier to follow the
logic. I know that this doesn't actually change the behavior of the code.

How come you chose 1024?

> +        rm_cookie = xcb_get_property (connection, 0, screen->root, XCB_ATOM_RESOURCE_MANAGER, XCB_ATOM_STRING, offset, 1024);
> +        rm_reply = xcb_get_property_reply (connection, rm_cookie, NULL);
> +
> +        if (rm_reply) {
> +            if (rm_reply->format == 8 && rm_reply->type == XCB_ATOM_STRING) {
> +                char *value = (char *) xcb_get_property_value (rm_reply);
> +                int length = xcb_get_property_value_length (rm_reply);
> +
> +                resource_parse (&parser, value, length);
> +
> +                has_more_data = rm_reply->bytes_after > 0;

I love X11 for its intuitivity. Yup, this division is correct...

> +                offset += length / 4;
> +            }
> +
> +            free (rm_reply);
> +        }
> +    } while (has_more_data);
> +
> +    resource_parser_done (&parser);
> +}
> +
> +static void
> +_cairo_xcb_init_screen_font_options (cairo_xcb_screen_t *screen)
> +{
> +    struct resource_state res;
> +    cairo_antialias_t antialias;
> +    cairo_subpixel_order_t subpixel_order;
> +    cairo_lcd_filter_t lcd_filter;
> +    cairo_hint_style_t hint_style;

Would it be better to initialize "res" in get_resources() instead of here?

> +    res.xft_antialias = TRUE;
> +    res.xft_lcdfilter = -1;
> +    res.xft_hinting = TRUE;
> +    res.xft_hintstyle = FC_HINT_FULL;

I looked at cairo-xlib. If there is no xft.rgba entry, it uses RENDER to query
the subpixel order. Could you prepare the code for this and leave behind a
comment saying that this is missing? Perhaps even implement the necessary call
to xcb_render_query_subpixel_order?

> +    res.xft_rgba = FC_RGBA_UNKNOWN;
> +
> +    get_resources(screen->connection->xcb_connection, screen->xcb_screen, &res);

Hm. Could you add a comment here and in cairo-xlib-screen.c saying that the
following code was copy-pasted between the two files?

> +    if (res.xft_hinting) {
> +	switch (res.xft_hintstyle) {
> +	case FC_HINT_NONE:
> +	    hint_style = CAIRO_HINT_STYLE_NONE;
> +	    break;
> +	case FC_HINT_SLIGHT:
> +	    hint_style = CAIRO_HINT_STYLE_SLIGHT;
> +	    break;
> +	case FC_HINT_MEDIUM:
> +	    hint_style = CAIRO_HINT_STYLE_MEDIUM;
> +	    break;
> +	case FC_HINT_FULL:
> +	    hint_style = CAIRO_HINT_STYLE_FULL;
> +	    break;
> +	default:
> +	    hint_style = CAIRO_HINT_STYLE_DEFAULT;
> +	}
> +    } else {
> +	hint_style = CAIRO_HINT_STYLE_NONE;
> +    }
> +
> +    switch (res.xft_rgba) {
> +    case FC_RGBA_RGB:
> +	subpixel_order = CAIRO_SUBPIXEL_ORDER_RGB;
> +	break;
> +    case FC_RGBA_BGR:
> +	subpixel_order = CAIRO_SUBPIXEL_ORDER_BGR;
> +	break;
> +    case FC_RGBA_VRGB:
> +	subpixel_order = CAIRO_SUBPIXEL_ORDER_VRGB;
> +	break;
> +    case FC_RGBA_VBGR:
> +	subpixel_order = CAIRO_SUBPIXEL_ORDER_VBGR;
> +	break;
> +    case FC_RGBA_UNKNOWN:
> +    case FC_RGBA_NONE:
> +    default:
> +	subpixel_order = CAIRO_SUBPIXEL_ORDER_DEFAULT;
> +    }
> +
> +    switch (res.xft_lcdfilter) {
> +    case FC_LCD_NONE:
> +	lcd_filter = CAIRO_LCD_FILTER_NONE;
> +	break;
> +    case FC_LCD_DEFAULT:
> +	lcd_filter = CAIRO_LCD_FILTER_FIR5;
> +	break;
> +    case FC_LCD_LIGHT:
> +	lcd_filter = CAIRO_LCD_FILTER_FIR3;
> +	break;
> +    case FC_LCD_LEGACY:
> +	lcd_filter = CAIRO_LCD_FILTER_INTRA_PIXEL;
> +	break;
> +    default:
> +	lcd_filter = CAIRO_LCD_FILTER_DEFAULT;
> +	break;
> +    }
> +
> +    if (res.xft_antialias) {
> +	if (subpixel_order == CAIRO_SUBPIXEL_ORDER_DEFAULT)
> +	    antialias = CAIRO_ANTIALIAS_GRAY;
> +	else
> +	    antialias = CAIRO_ANTIALIAS_SUBPIXEL;
> +    } else {
> +	antialias = CAIRO_ANTIALIAS_NONE;
> +    }
> +
> +    cairo_font_options_set_hint_style (&screen->font_options, hint_style);
> +    cairo_font_options_set_antialias (&screen->font_options, antialias);
> +    cairo_font_options_set_subpixel_order (&screen->font_options, subpixel_order);
> +    _cairo_font_options_set_lcd_filter (&screen->font_options, lcd_filter);
> +    cairo_font_options_set_hint_metrics (&screen->font_options, CAIRO_HINT_METRICS_ON);
> +}
> +
>  struct pattern_cache_entry {
>      cairo_cache_entry_t key;
>      cairo_xcb_screen_t *screen;
> @@ -362,3 +614,22 @@ _cairo_xcb_screen_lookup_radial_picture (cairo_xcb_screen_t *screen,
>  
>      return picture;
>  }
> +
> +cairo_font_options_t *
> +_cairo_xcb_screen_get_font_options (cairo_xcb_screen_t *screen)
> +{

Who actually acquires this mutex? In cairo-xlib,
_cairo_xlib_screen_get_font_options() does this explicitly. In your patch,
nothing does.

> +    assert (CAIRO_MUTEX_IS_LOCKED (screen->connection->device.mutex));
> +
> +    if (! screen->has_font_options) {
> +	_cairo_font_options_init_default (&screen->font_options);
> +	_cairo_font_options_set_round_glyph_positions (&screen->font_options, CAIRO_ROUND_GLYPH_POS_ON);

Why this if? _cairo_xcb_screen_get() sets the xcb_screen member right after
allocating the screen and, as far as I can tell, nothing ever unsets it.

> +	if (screen->xcb_screen != NULL) {
> +            _cairo_xcb_init_screen_font_options (screen);
> +	}
> +
> +	screen->has_font_options = TRUE;
> +    }
> +
> +    return &screen->font_options;
> +}
> diff --git a/src/cairo-xcb-surface.c b/src/cairo-xcb-surface.c
> index 02e7a19..d7e0d73 100644
> --- a/src/cairo-xcb-surface.c
> +++ b/src/cairo-xcb-surface.c
> @@ -530,9 +530,9 @@ static void
>  _cairo_xcb_surface_get_font_options (void *abstract_surface,
>  				     cairo_font_options_t *options)
>  {
> -    /* XXX  copy from xlib */
> -    _cairo_font_options_init_default (options);
> -    _cairo_font_options_set_round_glyph_positions (options, CAIRO_ROUND_GLYPH_POS_ON);
> +    cairo_xcb_surface_t *surface = abstract_surface;
> +
> +    *options = *_cairo_xcb_screen_get_font_options (surface->screen);
>  }
>  
>  static cairo_status_t
> -- 1.8.3.2

Cheers,
Uli
-- 
"Do you know that books smell like nutmeg or some spice from a foreign land?"
                                                  -- Faber in Fahrenheit 451


More information about the cairo mailing list