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