[Spice-devel] [RFC spice-streaming-agent 1/3] Add Xlib capture helper files
Snir Sheriber
ssheribe at redhat.com
Wed Sep 11 09:56:26 UTC 2019
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
More information about the Spice-devel
mailing list