> On 1 Mar 2018, at 17:02, Christophe de Dinechin <christophe.de.dinechin@xxxxxxxxx> wrote: > > > >> On 1 Mar 2018, at 16:12, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: >> >> 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?). > > Re-reading the code, I think I had just not paid enough attention to the ‘thread.detach()’ call. Why is there? It is causing the problem, because as a result, destroying the thread does not wait for its completion, so the destructor of the updater can be called while it runs. > > Frediano, what was the rationale behind the ‘detach()’? I suggest we remove it. Responding to self. Actually, I will add a patch to the series that addresses this problem. It goes a bit deeper. The cursor thread also needs to terminate on quit_requested. The proposed patch is there: https://gitlab.com/c3d/spice-streaming-agent/commit/07b0e0ea9317fab3867fb29d4367be8d4ad8ba98. Totally untested ATM (following my repeated power outages today, I’m presently doing a RAID array rebuild, and since this array brand can’t rebuild from Linux, I can’t use my VMs for the moment, so doing everything “blind” from my laptop). > >> >>> 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 > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel