> 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.
>
>> 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