>
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel