[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