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

Submitted by Snir Sheriber on Aug. 26, 2019, 8:39 a.m.

Details

Message ID 20190826083948.29309-1-ssheribe@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Snir Sheriber Aug. 26, 2019, 8:39 a.m.
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 ;)

---
 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

Patch hide | download patch | download mbox

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) {
+     this->x_image = x_image;
+     this->x_shm = is_shm;
+     this->new_res = new_res;
+}
+
+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;
+}
+
+/****************** XlibCapture Class Impl *******************/
+
+static int (*previous_x_err_handler)(Display *display, XErrorEvent *event) = 0;
+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:
+        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);
+        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
+        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);
+    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
+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

Comments

> 
> 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
Hi,


Sorry for the late replay


On 8/28/19 5:44 PM, Frediano Ziglio wrote:
>> 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?

I do want them but i also want comments ;)


>
>> ---
>>   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.

What do you mean by class initialized?

XImg(XImage *x_image, bool is_shm, bool new_res): x_image(x_image), 
x_shm(is_shm),new_res(new_res)?



>
>> +}
>> +
>> +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.

xshm_image?


>
>> +        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?


Possibly should be set back to default on destruction, but we have to
have it otherwise any xerror will shut the agent.


>
>> +    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?


The caller may still use the data also after getting out of
scope, for example, gst-plugin needs to have it alive
until the GDestroyNotify is called

Snir.

>
>> +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