[PATCH xf86-video-nested 09/10] Introduce a new XCB client backend, and make it the default one.

Uli Schlachter psychon at znc.in
Thu Nov 5 12:47:43 PST 2015


Am 05.11.2015 um 10:15 schrieb Laércio de Sousa:
> This patch brings up a new XCB backend client, translated from original
> xlibclient.c, and based on latest Xephyr code. This XCB backend
> brings some improvements already present in Xephyr, like
> fullscreen and output support.
> 
> This patch will also change configure.ac to make the new XCB
> backend the default one for building the driver. For switching back
> to Xlib backend, pass configure option --with-backend=xlib.
> 
> Signed-off-by: Laércio de Sousa <laerciosousa at sme-mogidascruzes.sp.gov.br>
> ---
[...]
> +    else if (reply->major_version < major || reply->minor_version < minor)
> +    {
> +        xf86DrvMsg(scrnIndex,
> +                   X_ERROR,
> +                   "Host X server doesn't support RandR %d.%d, needed for Option \"Output\" usage.\n",
> +                   major, minor);

Why is 4.0 not better than 1.1? Shouldn't this check be replaced with the following?

  if (reply->major_version < major ||
     (reply->major_version == major && reply->minor_version < minor))

> +        free(reply);
> +        return FALSE;
> +    }
> +
> +    free(reply);
> +    return TRUE;
> +}
[...]
> +static void
> +_NestedClientSetWMClass(NestedClientPrivatePtr pPriv,
> +                        const char* wm_class)
> +{
> +    size_t class_len = strlen(wm_class) + 1;
> +    char *class_hint = malloc(class_len);
> +
> +    if (class_hint)
> +    {
> +        strcpy(class_hint, wm_class);
> +        xcb_change_property(pPriv->conn,
> +                            XCB_PROP_MODE_REPLACE,
> +                            pPriv->window,
> +                            XCB_ATOM_WM_CLASS,
> +                            XCB_ATOM_STRING,
> +                            8,
> +                            class_len,
> +                            class_hint);
> +        free(class_hint);
> +    }

Why is this strcpy needed?

> +}
> +
[...]
> +static void
> +_NestedClientCreateWindow(NestedClientPrivatePtr pPriv)
> +{
> +    xcb_size_hints_t sizeHints;
> +
> +    sizeHints.flags = XCB_ICCCM_SIZE_HINT_P_POSITION
> +                      | XCB_ICCCM_SIZE_HINT_P_SIZE
> +                      | XCB_ICCCM_SIZE_HINT_P_MIN_SIZE
> +                      | XCB_ICCCM_SIZE_HINT_P_MAX_SIZE;
> +    sizeHints.min_width = pPriv->width;
> +    sizeHints.max_width = pPriv->width;
> +    sizeHints.min_height = pPriv->height;
> +    sizeHints.max_height = pPriv->height;
> +
> +    pPriv->window = xcb_generate_id(pPriv->conn);
> +    pPriv->img = NULL;
> +
> +    xcb_create_window(pPriv->conn,
> +                      XCB_COPY_FROM_PARENT,
> +                      pPriv->window,
> +                      pPriv->rootWindow,
> +                      0, 0, 100, 100, /* will resize */

Why not creating it at the correct size and position and skip at least the first
of the following two xcb_configure_window()? (Although I don't understand the
second one either)

> +                      0,
> +                      XCB_WINDOW_CLASS_COPY_FROM_PARENT,
> +                      pPriv->visual->visual_id,
> +                      pPriv->attr_mask,
> +                      pPriv->attrs);
> +
> +    xcb_icccm_set_wm_normal_hints(pPriv->conn,
> +                                  pPriv->window,
> +                                  &sizeHints);
> +
> +    if (pPriv->usingFullscreen)
> +        _NestedClientSetFullscreenHint(pPriv);
> +
> +    _NestedClientSetDeleteWindowHint(pPriv);
> +    _NestedClientSetWindowTitle(pPriv, "");
> +    _NestedClientSetWMClass(pPriv, "Xorg");
> +
> +    {
> +        uint32_t mask = XCB_CONFIG_WINDOW_WIDTH | XCB_CONFIG_WINDOW_HEIGHT;
> +        uint32_t values[2] = { pPriv->width, pPriv->height };
> +        xcb_configure_window(pPriv->conn, pPriv->window, mask, values);
> +    }
> +
> +    xcb_map_window(pPriv->conn, pPriv->window);
> +
> +    {
> +        uint32_t mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
> +        uint32_t values[2] = { pPriv->x, pPriv->y };
> +        xcb_configure_window(pPriv->conn, pPriv->window, mask, values);
> +    }
> +}
> +
[...]
> +void
> +NestedClientUpdateScreen(NestedClientPrivatePtr pPriv,
> +                         int16_t x1, int16_t y1,
> +                         int16_t x2, int16_t y2)
> +{
> +    if (pPriv->usingShm)
> +        xcb_image_shm_put(pPriv->conn, pPriv->window,
> +                          pPriv->gc, pPriv->img,
> +                          pPriv->shminfo,
> +                          x1, y1, x1, y1, x2 - x1, y2 - y1, FALSE);
> +    else
> +        xcb_image_put(pPriv->conn, pPriv->window, pPriv->gc,
> +                      pPriv->img, x1, y1, 0);
> +
> +    xcb_aux_sync(pPriv->conn);
> +}

Does this really need a sync? Wouldn't a flush be enough? Or is the sync only
needed in the usingShm case so that the data isn't modified too early?

> +static inline void
> +_NestedClientProcessExpose(NestedClientPrivatePtr pPriv,
> +                           xcb_generic_event_t *ev)
> +{
> +    xcb_expose_event_t *xev = (xcb_expose_event_t *)ev;
> +    NestedClientUpdateScreen(pPriv,
> +                             xev->x,
> +                             xev->y,
> +                             xev->x + xev->width,
> +                             xev->y + xev->height);
> +}
> +
> +static inline void
> +_NestedClientProcessClientMessage(NestedClientPrivatePtr pPriv,
> +                                  xcb_generic_event_t *ev)
> +{
> +    xcb_client_message_event_t *cmev = (xcb_client_message_event_t *)ev;
> +
> +    if (cmev->data.data32[0] == atom_WM_DELETE_WINDOW)
> +    {
> +        /* XXX: Is there a better way to do this? */
> +        xf86DrvMsg(pPriv->scrnIndex,
> +                   X_INFO,
> +                   "Nested client window closed.\n");
> +        free(ev);
> +        exit(0);

You can as well just let the WM kill you if that's all that you do. :-)
(No, I'm not totally serious)

> +    }
> +}
> +
[...]
> +void
> +NestedClientCheckEvents(NestedClientPrivatePtr pPriv)
> +{
> +    xcb_generic_event_t *ev;
> +
> +    while (TRUE)
> +    {
> +        ev = xcb_poll_for_event(pPriv->conn);

In the name of micro-optimization I will mention xcb_poll_for_queued_event()
once and then shut up again (the idea being to poll only in the first iteration
and afterwards using poll_for_queued_event, but I don't think that this really
makes much of a difference.

> +        if (!ev)
> +        {
> +            if (_NestedClientConnectionHasError(pPriv->scrnIndex,
> +                                                pPriv->conn))
> +            {
> +                /* XXX: Is there a better way to do this? */
> +                xf86DrvMsg(pPriv->scrnIndex,
> +                           X_ERROR,
> +                           "Connection with host X server lost.\n");
> +                free(ev);
> +                NestedClientCloseScreen(pPriv);
> +                exit(1);
> +            }
> +
> +            break;
> +        }
> +
> +        switch (ev->response_type & ~0x80)
> +        {
> +        case XCB_EXPOSE:
> +            _NestedClientProcessExpose(pPriv, ev);
> +            break;
> +        case XCB_CLIENT_MESSAGE:
> +            _NestedClientProcessClientMessage(pPriv, ev);
> +            break;
> +        case XCB_MOTION_NOTIFY:
> +            _NestedClientProcessMotionNotify(pPriv, ev);
> +            break;
> +        case XCB_KEY_PRESS:
> +            _NestedClientProcessKeyPress(pPriv, ev);
> +            break;
> +        case XCB_KEY_RELEASE:
> +            _NestedClientProcessKeyRelease(pPriv, ev);
> +            break;
> +        case XCB_BUTTON_PRESS:
> +            _NestedClientProcessButtonPress(pPriv, ev);
> +            break;
> +        case XCB_BUTTON_RELEASE:
> +            _NestedClientProcessButtonRelease(pPriv, ev);
> +            break;
> +        }
> +
> +        free(ev);
> +        xcb_flush(pPriv->conn);

Why is this flushing inside of the event loop? Wouldn't a flush after the loop
be enough?

> +    }
> +}
[...]

Besides the above (minor) points, this look good to me, but I don't really have
much of a clue.

Cheers,
Uli
-- 
"Because I'm in pain," he says. "That's the only way I get your attention."
He picks up the box. "Don't worry, Katniss. It'll pass."
He leaves before I can answer.


More information about the xorg-devel mailing list