On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> This makes it possible to remove X11 dependencies from the main agent file.
>
> Note: it may be unsafe to call XCloseDisplay from the destructor.
> Doing some experiments throwing exceptions, I noticed messages such as:
>
> [xcb] Unknown request in queue while dequeuing
> [xcb] Most likely this is a multi-threaded client and XInitThreads
> has not been called
> [xcb] Aborting, sorry about that.
> spice-streaming-agent: xcb_io.c:165: dequeue_pending_request:
> Assertion `!xcb_xlib_unknown_req_in_deq' failed.
> Aborted (core dumped)
Hmm, I think it is quite clear. The destructor gets called in the main
thread, meanwhile stuff is happening in the cursor thread. Apparently,
if we wanna do that, we need to either join the thread before
destroying the object (probably a good idea and I think we can do
that), or see if XInitThreads solves the issue (it might introduce some
locking?).
> The stack trace looks like this:
>
> #1 0x00007ffff65cf4da in abort () from /lib64/libc.so.6
> #2 0x00007ffff65c5d67 in __assert_fail_base () from /lib64/libc.so.6
> #3 0x00007ffff65c5e12 in __assert_fail () from /lib64/libc.so.6
> #4 0x00007ffff76b6ecc in dequeue_pending_request () from /lib64/libX11.so.6
> #5 0x00007ffff76b7d20 in _XReply () from /lib64/libX11.so.6
> #6 0x00007ffff76b36cd in XSync () from /lib64/libX11.so.6
> #7 0x00007ffff769494e in XCloseDisplay () from /lib64/libX11.so.6
>
> If we hit this problem in practice, we may need to remove the
> XCloseDisplay call from the destructor.
>
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
> src/Makefile.am | 2 +
> src/message.hpp | 2 +
> src/spice-streaming-agent.cpp | 121 +-----------------------------------------
> src/x11-cursor.cpp | 65 +++++++++++++++++++++++
> src/x11-cursor.hpp | 91 +++++++++++++++++++++++++++++++
> 5 files changed, 161 insertions(+), 120 deletions(-)
> create mode 100644 src/x11-cursor.cpp
> create mode 100644 src/x11-cursor.hpp
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 923a103..4a03e5e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -58,4 +58,6 @@ spice_streaming_agent_SOURCES = \
> stream.cpp \
> stream.hpp \
> errors.cpp \
> + x11-cursor.hpp \
> + x11-cursor.cpp \
> $(NULL)
> diff --git a/src/message.hpp b/src/message.hpp
> index 28b3e28..650bd66 100644
> --- a/src/message.hpp
> +++ b/src/message.hpp
> @@ -6,6 +6,8 @@
> #ifndef SPICE_STREAMING_AGENT_MESSAGE_HPP
> #define SPICE_STREAMING_AGENT_MESSAGE_HPP
>
> +#include "stream.hpp"
> +
> #include <spice/stream-device.h>
>
> namespace spice
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index c401a34..2264af2 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -7,6 +7,7 @@
> #include "concrete-agent.hpp"
> #include "stream.hpp"
> #include "message.hpp"
> +#include "x11-cursor.hpp"
> #include "hexdump.h"
> #include "mjpeg-fallback.hpp"
>
> @@ -36,8 +37,6 @@
> #include <vector>
> #include <string>
> #include <functional>
> -#include <X11/Xlib.h>
> -#include <X11/extensions/Xfixes.h>
>
> using namespace spice::streaming_agent;
>
> @@ -86,63 +85,6 @@ public:
> }
> };
>
> -class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage, STREAM_TYPE_CURSOR_SET>
> -{
> -public:
> - X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
> - static size_t size(XFixesCursorImage *cursor)
> - {
> - return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
> - }
> -
> - void write_message_body(Stream &stream, XFixesCursorImage *cursor)
> - {
> - StreamMsgCursorSet msg = {
> - .width = cursor->width,
> - .height = cursor->height,
> - .hot_spot_x = cursor->xhot,
> - .hot_spot_y = cursor->yhot,
> - .type = SPICE_CURSOR_TYPE_ALPHA,
> - .padding1 = { },
> - .data = { }
> - };
> -
> - size_t pixcount = pixel_count(cursor);
> - size_t pixsize = pixcount * sizeof(uint32_t);
> - std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
> - uint32_t *pixbuf = pixels.get();
> - fill_pixels(cursor, pixcount, pixbuf);
> -
> - stream.write_all("cursor message", &msg, sizeof(msg));
> - stream.write_all("cursor pixels", pixbuf, pixsize);
> - }
> -
> -private:
> - static size_t pixel_count(XFixesCursorImage *cursor)
> - {
> - return cursor->width * cursor->height;
> - }
> -
> - void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
> - {
> - for (unsigned i = 0; i < count; ++i) {
> - pixbuf[i] = cursor->pixels[i];
> - }
> - }
> -};
> -
> -class X11CursorUpdater
> -{
> -public:
> - X11CursorUpdater(Stream &stream);
> - ~X11CursorUpdater();
> - void send_cursor_changes();
> -
> -private:
> - Stream &stream;
> - Display *display;
> -};
> -
> class FrameLog
> {
> public:
> @@ -182,67 +124,6 @@ void FrameLog::dump(const void *buffer, size_t length)
> }
> }
>
> -class X11CursorThread
> -{
> -public:
> - X11CursorThread(Stream &stream);
> - static void record_cursor_changes(X11CursorThread *self) { self->updater.send_cursor_changes(); }
> -
> -private:
> - X11CursorUpdater updater;
> - std::thread thread;
> -};
> -
> -X11CursorUpdater::X11CursorUpdater(Stream &stream)
> - : stream(stream),
> - display(XOpenDisplay(NULL))
> -{
> - if (display == NULL) {
> - throw Error("failed to open display").syslog();
> - }
> -}
> -
> -X11CursorUpdater::~X11CursorUpdater()
> -{
> - XCloseDisplay(display);
> -}
> -
> -void X11CursorUpdater::send_cursor_changes()
> -{
> - unsigned long last_serial = 0;
> -
> - int event_base, error_base;
> - if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> - syslog(LOG_WARNING, "XFixesQueryExtension failed, not sending cursor changes\n");
> - return; // also terminates the X11CursorThread if that's how we were launched
> - }
> - Window rootwindow = DefaultRootWindow(display);
> - XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
> -
> - while (true) {
> - XEvent event;
> - XNextEvent(display, &event);
> - if (event.type != event_base + 1) {
> - continue;
> - }
> -
> - XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> - if (!cursor || cursor->cursor_serial == last_serial) {
> - continue;
> - }
> -
> - last_serial = cursor->cursor_serial;
> - stream.send<X11CursorMessage>(cursor);
> - }
> -}
> -
> -X11CursorThread::X11CursorThread(Stream &stream)
> - : updater(stream),
> - thread(record_cursor_changes, this)
> -{
> - thread.detach();
> -}
> -
> }} // namespace spice::streaming_agent
>
> bool quit_requested = false;
> diff --git a/src/x11-cursor.cpp b/src/x11-cursor.cpp
> new file mode 100644
> index 0000000..6abc258
> --- /dev/null
> +++ b/src/x11-cursor.cpp
> @@ -0,0 +1,65 @@
> +/* X11 cursor transmission
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#include "x11-cursor.hpp"
> +
> +#include <syslog.h>
> +
> +namespace spice
> +{
> +namespace streaming_agent
> +{
> +
> +X11CursorUpdater::X11CursorUpdater(Stream &stream)
> + : stream(stream),
> + display(XOpenDisplay(NULL))
> +{
> + if (display == NULL) {
> + throw Error("failed to open display").syslog();
> + }
> +}
> +
> +X11CursorUpdater::~X11CursorUpdater()
> +{
> + XCloseDisplay(display);
> +}
> +
> +void X11CursorUpdater::send_cursor_changes()
> +{
> + unsigned long last_serial = 0;
> +
> + int event_base, error_base;
> + if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> + syslog(LOG_WARNING, "XFixesQueryExtension failed, not sending cursor changes\n");
> + return; // also terminates the X11CursorThread if that's how we were launched
> + }
> + Window rootwindow = DefaultRootWindow(display);
> + XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
> +
> + while (true) {
> + XEvent event;
> + XNextEvent(display, &event);
> + if (event.type != event_base + 1) {
> + continue;
> + }
> +
> + XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> + if (!cursor || cursor->cursor_serial == last_serial) {
> + continue;
> + }
> +
> + last_serial = cursor->cursor_serial;
> + stream.send<X11CursorMessage>(cursor);
> + }
> +}
> +
> +X11CursorThread::X11CursorThread(Stream &stream)
> + : updater(stream),
> + thread(record_cursor_changes, this)
> +{
> + thread.detach();
> +}
> +
> +}} // namespace spic::streaming_agent
> diff --git a/src/x11-cursor.hpp b/src/x11-cursor.hpp
> new file mode 100644
> index 0000000..2038b1d
> --- /dev/null
> +++ b/src/x11-cursor.hpp
> @@ -0,0 +1,91 @@
> +/* X11 cursor transmission
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_X11_CURSOR_HPP
> +#define SPICE_STREAMING_AGENT_X11_CURSOR_HPP
> +
> +#include "message.hpp"
> +
> +#include <spice-streaming-agent/errors.hpp>
> +
> +#include <thread>
> +#include <X11/Xlib.h>
> +#include <X11/extensions/Xfixes.h>
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +class X11CursorMessage : public Message<StreamMsgCursorSet, X11CursorMessage,
> + STREAM_TYPE_CURSOR_SET>
> +{
> +public:
> + X11CursorMessage(XFixesCursorImage *cursor): Message(cursor) {}
> + static size_t size(XFixesCursorImage *cursor)
> + {
> + return sizeof(payload_t) + sizeof(uint32_t) * pixel_count(cursor);
> + }
> +
> + void write_message_body(Stream &stream, XFixesCursorImage *cursor)
> + {
> + StreamMsgCursorSet msg = {
> + .width = cursor->width,
> + .height = cursor->height,
> + .hot_spot_x = cursor->xhot,
> + .hot_spot_y = cursor->yhot,
> + .type = SPICE_CURSOR_TYPE_ALPHA,
> + .padding1 = { },
> + .data = { }
> + };
> +
> + size_t pixcount = pixel_count(cursor);
> + size_t pixsize = pixcount * sizeof(uint32_t);
> + std::unique_ptr<uint32_t[]> pixels(new uint32_t[pixcount]);
> + uint32_t *pixbuf = pixels.get();
> + fill_pixels(cursor, pixcount, pixbuf);
> +
> + stream.write_all("cursor message", &msg, sizeof(msg));
> + stream.write_all("cursor pixels", pixbuf, pixsize);
> + }
> +
> +private:
> + static size_t pixel_count(XFixesCursorImage *cursor)
> + {
> + return cursor->width * cursor->height;
> + }
> +
> + void fill_pixels(XFixesCursorImage *cursor, unsigned count, uint32_t *pixbuf)
> + {
> + for (unsigned i = 0; i < count; ++i) {
> + pixbuf[i] = cursor->pixels[i];
> + }
> + }
> +};
> +
> +class X11CursorUpdater
> +{
> +public:
> + X11CursorUpdater(Stream &stream);
> + ~X11CursorUpdater();
> + void send_cursor_changes();
> +
> +private:
> + Stream &stream;
> + Display *display;
> +};
> +
> +class X11CursorThread
> +{
> +public:
> + X11CursorThread(Stream &stream);
> + static void record_cursor_changes(X11CursorThread *self) { self->updater.send_cursor_changes(); }
> +
> +private:
> + X11CursorUpdater updater;
> + std::thread thread;
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_X11_CURSOR_HPP
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel