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

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?= <[email protected]>
> 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 <[email protected]>
>   */
>  
> +#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
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo