[cairo] [PATCH] xcb: Initialize font options from Xft resources
Uli Schlachter
psychon at znc.in
Tue Sep 9 02:10:29 PDT 2014
Hi again,
On 04.09.2014 10:24, Lukáš Lalinský wrote:
> On Thu, Sep 4, 2014 at 8:47 AM, Lukáš Lalinský <lukas at oxygene.sk> wrote:
>> > Thanks for the review. I'll update the code and send a new patch later
>> > today. Some comments bellow.
> Updated version attached. I'm also considering extracting the common
> code from cairo-xlib and cairo-xcb, but I can't find any instance of
> such code sharing between those two backends. Is there anything like
> that in already?
Nope, as far as I know there is nothing like that already.
> 0001-xcb-Initialize-font-options-from-Xft-resources.patch
>
>
> From c3f32c9beb1e7e1cd0f531a76ae53e1f2dd850a9 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.
> ---
[...]
> +static cairo_bool_t
> +resource_parse_line (char *name, cairo_xcb_resources_t *resources)
> +{
> + char *value;
> +
> + value = strchr (name, ':');
> + if (value == NULL)
> + return FALSE;
> +
> + *value++ = 0;
> +
> + name = skip_spaces (name);
> + value = skip_spaces (value);
> +
> + if (strcmp (name, "Xft.antialias") == 0)
> + parse_boolean (value, &(resources->xft_antialias));
> + else if (strcmp (name, "Xft.lcdfilter") == 0)
> + parse_integer (value, &(resources->xft_lcdfilter));
> + else if (strcmp (name, "Xft.rgba") == 0)
> + parse_integer (value, &(resources->xft_rgba));
> + else if (strcmp (name, "Xft.hinting") == 0)
> + parse_boolean (value, &(resources->xft_hinting));
> + else if (strcmp (name, "Xft.hintstyle") == 0)
> + parse_integer (value, &(resources->xft_hintstyle));
Should this also ignore trailing spaces? I have no clue about this resources
stuff...
> + return TRUE;
> +}
> +
> +static int
> +resource_parse_lines (struct resource_parser *parser)
> +{
> + char *line, *newline;
> +
> + line = parser->buffer;
> + while (1) {
> + newline = strchr (line, '\n');
> + if (newline == NULL)
> + break;
This should be intended by tabs (and "tabs + 4 spaces" for the next level).
> +
> + *newline++ = 0;
> +
> + if (! resource_parse_line (line, parser->resources))
> + break;
> +
> + line = newline;
> + }
> +
> + return line - parser->buffer;
> +}
I like it that you split up this function into two. Makes it easier to follow IMO.
> +static void
> +resource_parser_init (struct resource_parser *parser, cairo_xcb_resources_t *resources)
> +{
> + parser->buffer_size = 0;
> + parser->bytes_in_buffer = 0;
> + parser->buffer = NULL;
> + parser->resources = resources;
> +}
> +
> +static cairo_bool_t
> +resource_parser_update (struct resource_parser *parser, const char *data, int length)
> +{
> + int bytes_parsed;
> +
> + if (parser->bytes_in_buffer + length > parser->buffer_size) {
Due to the trailing null byte, this should be ">=" instead of ">" (or to make
things clearer, explicitly add a "+1" to the left side).
> + parser->buffer_size = parser->bytes_in_buffer + length + 1;
> + parser->buffer = realloc(parser->buffer, parser->buffer_size);
> + if (! parser->buffer)
> + return FALSE;
Could you set parser->buffer_size and parser->bytes_in_buffer to 0 in this case?
That would properly "destroy" the parser. In this version, continuing to use the
parser could cause issues. Also, resource_parser_done could dereference a NULL
pointer.
> + }
> +
> + 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_lines (parser);
> +
> + if (parser->bytes_in_buffer > bytes_parsed) {
> + memmove (parser->buffer, parser->buffer + bytes_parsed, parser->bytes_in_buffer - bytes_parsed);
> + parser->bytes_in_buffer -= bytes_parsed;
> + } else {
> + parser->bytes_in_buffer = 0;
> + }
> +
> + return TRUE;
> +}
> +
> +static void
> +resource_parser_done (struct resource_parser *parser)
> +{
> + if (parser->bytes_in_buffer > 0) {
> + parser->buffer[parser->bytes_in_buffer] = 0;
This zero byte is already inserted in resource_parser_update, no need to add it
here again.
> + resource_parse_line (parser->buffer, parser->resources);
> + }
> +
> + free (parser->buffer);
> +}
> +
> +static void
> +get_resources(xcb_connection_t *connection, xcb_screen_t *screen, cairo_xcb_resources_t *resources)
> +{
> + xcb_get_property_cookie_t cookie;
> + xcb_get_property_reply_t *reply;
> + struct resource_parser parser;
> + int offset;
> + cairo_bool_t has_more_data;
> +
> + resources->xft_antialias = TRUE;
> + resources->xft_lcdfilter = -1;
> + resources->xft_hinting = TRUE;
> + resources->xft_hintstyle = FC_HINT_FULL;
> + resources->xft_rgba = FC_RGBA_UNKNOWN;
> +
> + resource_parser_init (&parser, resources);
> +
> + offset = 0;
> + has_more_data = FALSE;
> + do {
> + cookie = xcb_get_property (connection, 0, screen->root, XCB_ATOM_RESOURCE_MANAGER, XCB_ATOM_STRING, offset, 1024);
> + reply = xcb_get_property_reply (connection, cookie, NULL);
> +
> + if (reply) {
> + if (reply->format == 8 && reply->type == XCB_ATOM_STRING) {
> + char *value = (char *) xcb_get_property_value (reply);
> + int length = xcb_get_property_value_length (reply);
> +
> + offset += length / 4; /* X needs the offset in 'long' units */
> + has_more_data = reply->bytes_after > 0;
> +
> + if (! resource_parser_update (&parser, value, length))
> + has_more_data = FALSE; /* early exit on error */
> + }
> +
> + free (reply);
> + }
> + } while (has_more_data);
> +
> + resource_parser_done (&parser);
> +}
> +
> +#if 0 && XCB_RENDER_MAJOR_VERSION > 99 && XCB_RENDER_MINOR_VERSION > 99
> +static void
> +get_rgba_from_render (xcb_connection_t *connection, xcb_screen_t *screen, cairo_xcb_resources_t *resources)
> +{
> + /* this is a mock-up of what the function might look like,
> + xcb_render_query_sub_pixel is not actually implemented in XCB (yet) */
XRenderQuerySubPixel() is just a wrapper in libXrender. It actually sends a
RENDER QueryPictFormats request. XRenderQuerySubpixelOrder(dpy, screen)
effectively then does just this:
return xcb_render_query_pict_formats_subpixels(reply)[screen];
However, the documentation[0] states that this requires at least render 0.6 (so
needs a xcb_render_query_version() check and return unknown if
major==0&&minor<6, or even better would be to use cairo_xcb_connection_t::flags
and add a new flag similar to CAIRO_XCB_RENDER_HAS_FILTERS) and that the list of
subpixels may be shorter than the number of screens (which you can check for
with xcb_render_query_pict_formats_subpixels_length() and which I am not
completely sure libXrender handles correctly).
Oh and this apparently requires turning an xcb_screen_t into its screen number
which requires way too much code for way too little gain.
[0]: http://cgit.freedesktop.org/xorg/proto/renderproto/tree/renderproto.txt#n518
Feel free to leave this as-is and for someone else to fix up later, but in this
case it would be nice to update the above comment.
> +
> + xcb_render_query_sub_pixel_order_cookie_t cookie;
> + xcb_render_query_sub_pixel_order_reply_t *reply;
> +
> + cookie = xcb_render_query_sub_pixel (connection, screen);
> + reply = xcb_render_query_sub_pixel_reply (connection, cookie, NULL);
> +
> + if (reply) {
> + switch (reply->sub_pixel_order) {
> + case XCB_RENDER_SUB_PIXEL_UNKNOWN:
> + resources->xft_rgba = FC_RGBA_UNKNOWN;
> + break;
> + case XCB_RENDER_SUB_PIXEL_HORIZONTAL_RGB:
> + resources->xft_rgba = FC_RGBA_RGB;
> + break;
> + case XCB_RENDER_SUB_PIXEL_HORIZONTAL_BGR:
> + resources->xft_rgba = FC_RGBA_BGR;
> + break;
> + case XCB_RENDER_SUB_PIXEL_VERTICAL_RGB:
> + resources->xft_rgba = FC_RGBA_VRGB;
> + break;
> + case XCB_RENDER_SUB_PIXEL_VERTICAL_BGR:
> + resources->xft_rgba = FC_RGBA_VBGR;
> + break;
> + case XCB_RENDER_SUB_PIXEL_NONE:
> + resources->xft_rgba = FC_RGBA_NONE;
> + break;
> + }
> +
> + free(reply);
> + }
> +}
> +#endif
[...]
> @@ -362,3 +452,20 @@ _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)
> +{
> + 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);
> +
> + _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);
Still requires a pair of _cairo_xcb_connection_acquire() /
_cairo_xcb_connection_release() somewhere. Cairo-xlib does this in
_cairo_xlib_screen_get_font_options(), but you put an assert into
_cairo_xcb_screen_get_font_options() making this the caller's responsibility.
Either place is fine with me to do the necessary locking.
(And yes, CAIRO_MUTEX_IS_LOCKED() is defined to 1 if cairo is using pthreads, so
this assert can't actually trigger. It's still nice as documentation.)
> }
>
> static cairo_status_t
> -- 1.8.3.2
Nice patch and once again thanks for working on this. Since the above are just
some minor things that can easily be addressed, this patch (with those things
fixed) is:
Reviewed-by: Uli Schlachter <psychon at znc.in>
Cheers,
Uli
--
99 little bugs in the code
99 little bugs in the code
Take one down, patch it around
117 little bugs in the code
-- @irqed
More information about the cairo
mailing list