[Spice-devel] [RFC spice-streaming-agent 1/3] Add Xlib capture helper files

[Frediano Ziglio]

> 
> The Xlib capture helper provides a wrapping class for XImage, Display
> information and other capturing related functionalities which allows
> to use unified and simple API for screen capturing using X.
> This also utilize the X MIT-SHM extension which improves capturing
> speed, hence, xext is required.
> ---
> 
> The original idea was just to use the X ancient MIT-SHM extension which
> reduce CPU utilization significantly when using Xlib capturing.
> Since this capturing method is currently used by two different plugins
> it was suggested to use one common class for that, this class is added
> by this patch and the 2 following patches makes the existing plugins
> to use this class.
> 
> If you have any other suggestions or comments please do ;)
> 

It's not clear why the RFC. From the comment looks like you want them.
Maybe you are not sure about the state of the patches?

> ---
>  configure.ac         |   1 +
>  src/xlib-capture.cpp | 169 +++++++++++++++++++++++++++++++++++++++++++
>  src/xlib-capture.hpp |  53 ++++++++++++++
>  3 files changed, 223 insertions(+)
>  create mode 100644 src/xlib-capture.cpp
>  create mode 100644 src/xlib-capture.hpp
> 
> diff --git a/configure.ac b/configure.ac
> index 1c7788b..3220e27 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -38,6 +38,7 @@ PKG_CHECK_MODULES(DRM, libdrm)
>  PKG_CHECK_MODULES(X11, x11)
>  PKG_CHECK_MODULES(XFIXES, xfixes)
>  PKG_CHECK_MODULES(XRANDR, xrandr)
> +PKG_CHECK_MODULES(XEXT, xext)
>  
>  PKG_CHECK_MODULES(JPEG, libjpeg, , [
>      AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> diff --git a/src/xlib-capture.cpp b/src/xlib-capture.cpp
> new file mode 100644
> index 0000000..7837ee2
> --- /dev/null
> +++ b/src/xlib-capture.cpp
> @@ -0,0 +1,169 @@
> +/* A helper classes that wraps X display information XImage capturing
> functions
> + *
> + * \copyright
> + * Copyright 2019 Red Hat Inc. All rights reserved.
> + */
> +
> +#include <sys/ipc.h>
> +#include <sys/shm.h>
> +#include "xlib-capture.hpp"
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +/******************** XImg Class Impl ***********************/
> +
> +XImg::XImg(XImage *x_image, bool is_shm, bool new_res) {

style: bracket on next line

> +     this->x_image = x_image;
> +     this->x_shm = is_shm;
> +     this->new_res = new_res;

Better to use class initialized, even if in this case won't change
much.

> +}
> +
> +XImg::~XImg() {
> +    if (!x_shm) {
> +        XDestroyImage(x_image);
> +    } // else the XlibCapture will destroy it
> +}
> +
> +char *XImg::get_data() {
> +    return x_image->data;
> +}
> +
> +int XImg::height() {
> +    return x_image->height;
> +}
> +
> +int XImg::width() {
> +    return x_image->width;
> +}
> +
> +int XImg::data_size() {
> +    return x_image->height * x_image->bytes_per_line;
> +}
> +
> +bool XImg::new_resolution() {
> +   return new_res;
> +}
> +

I would inline these in the header. Also I would add const, they don't
allow to change the object.

This "new_resolution" does not apply to the "provides a wrapping class for
XImage", are more related to capture interface. Maybe "XCapturedImg" as name
of the class?

> +/****************** XlibCapture Class Impl *******************/
> +
> +static int (*previous_x_err_handler)(Display *display, XErrorEvent *event) =
> 0;

better "= nullptr" or at least "= NULL" (although 0 for historic reason works).

> +static bool xerror = false;
> +
> +/* This is used mainly to silence x error in case of resolution change */
> +static int x_err_handler(Display *display, XErrorEvent *event) {
> +    //TODO: print some error information ?
> +    switch (event->error_code) {
> +        case BadAccess:

style: too indented.

> +        case BadMatch:
> +            xerror = true;
> +            return 0;
> +        default:
> +            return previous_x_err_handler(display, event);
> +    }
> +}
> +
> +void XlibCapture::init_xshm() {
> +    Visual *visual = DefaultVisual(display, screen);
> +    unsigned int depth = DefaultDepth(display, screen);
> +    XWindowAttributes win_info;
> +
> +    xshm_enabled = false;
> +    xerror = false;
> +    // Create Shared Memory XImage
> +    XGetWindowAttributes(display, win, &win_info); // update attributes
> +    xshm_image = XShmCreateImage(display, visual, depth, ZPixmap, nullptr,
> +                                 &shm_info, win_info.width,
> win_info.height);
> +    if (!xshm_image) {
> +        return;
> +    }
> +    // Allocate shm buffer
> +    int shm_id = shmget(IPC_PRIVATE, xshm_image->bytes_per_line *
> xshm_image->height, IPC_CREAT|0777);
> +    if (shm_id == -1) {
> +        XDestroyImage(xshm_image);

I suppose you want to reset xshm_image too to avoid dandling pointers.

> +        return;
> +    }
> +
> +    // Attach share memory bufer
> +    void *shm_addr = shmat(shm_id, nullptr, 0);
> +    if (shm_addr == (void*)-1) {
> +        XDestroyImage(xshm_image);
> +        shmctl(shm_id, IPC_RMID, nullptr);
> +        return;
> +    }
> +    shm_info.shmid = shm_id;
> +    shm_info.shmaddr = xshm_image->data = (char*)shm_addr;
> +    shm_info.readOnly = false;
> +    XShmAttach(display, &shm_info);
> +    XSync(display, false);
> +
> +    if (!xerror) {
> +        xshm_enabled = true;
> +        return; // XShm initialization succeeded
> +    }
> +}
> +
> +XImg *XlibCapture::capture() {
> +    XWindowAttributes win_info;
> +    bool changed = false;
> +    int tries = 0;
> +
> +    /* This code flow allows to overcome resolution change that occurs just
> +     * after getting the attributes and before asking for the XImage
> +     */
> +    do {
> +        XGetWindowAttributes(display, win, &win_info); // update attributes
> +
> +        if (last_width != win_info.width || last_height != win_info.height)
> {
> +            if (xshm_enabled && xshm_image) { // reinit xshm extention
> +                deinit_xshm();
> +                init_xshm();
> +            }
> +            changed = true; // resolution changed
> +            last_width = win_info.width;
> +            last_height = win_info.height;
> +        }
> +        // TODO: round down the resoultion / use custom resoultion

some typos in comment

> +        if (xshm_enabled && xshm_image) {
> +            if (XShmGetImage(display, win, xshm_image, win_info.x,
> win_info.y, AllPlanes)) {
> +                return new XImg(xshm_image, true, changed);
> +            }
> +        }
> +        XImage *image = XGetImage(display, win, win_info.x, win_info.y,
> win_info.width,
> +                                  win_info.height, AllPlanes, ZPixmap);
> +        if (image) {
> +            return new XImg(image, false, changed);
> +        }
> +    } while (++tries < 2); // there was an error to get the image, try once
> more (might be a resoultion change)
> +    return nullptr; // fail
> +}
> +
> +void XlibCapture::deinit_xshm() {
> +    if (xshm_enabled && xshm_image) {
> +        XShmDetach (display, &shm_info);
> +        xshm_image->f.destroy_image(xshm_image);
> +        shmdt(shm_info.shmaddr);
> +        shmctl(shm_info.shmid, IPC_RMID, 0);
> +        xshm_image = nullptr;
> +        xshm_enabled = false;
> +    }
> +}
> +
> +XlibCapture::XlibCapture(Display *display):
> +    display(display)
> +{
> +    XWindowAttributes win_info;
> +    this->screen = XDefaultScreen(display);
> +    this->win = RootWindow(display, screen);
> +    XGetWindowAttributes(display, win, &win_info); // update attributes
> +    last_width = -1;
> +    last_height = -1;
> +    previous_x_err_handler = XSetErrorHandler(x_err_handler);

it looks like this class is a singleton, maybe call XSetErrorHandler
only if previous_x_err_handler is nullptr ?
Are you sure you want this function set for the entire program?

> +    init_xshm();
> +}
> +
> +XlibCapture::~XlibCapture() {
> +    deinit_xshm();
> +}
> +
> +}} // namespace spice::streaming_agent
> diff --git a/src/xlib-capture.hpp b/src/xlib-capture.hpp
> new file mode 100644
> index 0000000..93200dc
> --- /dev/null
> +++ b/src/xlib-capture.hpp
> @@ -0,0 +1,53 @@
> +/*  A helper classes that wraps X display information XImage capturing
> functions
> + *
> + * \copyright
> + * Copyright 2019 Red Hat Inc. All rights reserved.
> + */
> +
> +#ifndef SPICE_STREAMING_AGENT_XLIB_CAPTURE_HPP // need header file?
> +#define SPICE_STREAMING_AGENT_XLIB_CAPTURE_HPP
> +
> +#include <X11/Xlib.h>
> +#include <X11/Xutil.h>
> +#include <X11/extensions/XShm.h>
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +class XImg
> +{
> +public:
> +    XImg(XImage *x_iamge, bool is_shm, bool new_resolution);
> +    ~XImg();
> +    char *get_data();
> +    int data_size(); // with considering stride
> +    int height();
> +    int width();
> +    bool new_resolution();
> +private:
> +    XImage *x_image;
> +    bool x_shm;
> +    bool new_res;
> +};
> +
> +class XlibCapture
> +{
> +public:
> +    XlibCapture(Display *display); // add empty constructor?
> +    ~XlibCapture();
> +    XImg *capture(); // caller should delete the return image

Use unique_ptr instead of the comment?

> +private:
> +    int last_width, last_height;
> +    Display *const display;
> +    int screen;
> +    Window win;
> +    XImage *xshm_image = nullptr;
> +    XShmSegmentInfo shm_info;
> +    bool xshm_enabled = false;
> +    void init_xshm();
> +    void deinit_xshm();
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_XLIB_CAPTURE_HPP

Frediano