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

Hi again,

On 04.09.2014 10:24, Lukáš Lalinský wrote:
> On Thu, Sep 4, 2014 at 8:47 AM, Lukáš Lalinský <[email protected]> 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?= <[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.
> ---

[...]

> +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 <[email protected]>

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